Skip to content

Commit

Permalink
don't allow relative links in comments
Browse files Browse the repository at this point in the history
(url) is a text inserted by default and never an intended URL.

That additional validation will ensure that users won't post relative
links because they are rarely intended.
  • Loading branch information
paskal committed Apr 1, 2023
1 parent cc84290 commit 270ff85
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 22 deletions.
19 changes: 19 additions & 0 deletions backend/app/rest/api/rest_private_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,25 @@ func TestRest_CreateWithRestrictedWord(t *testing.T) {
assert.Equal(t, "invalid comment", c["details"])
}

func TestRest_CreateRelativeURL(t *testing.T) {
ts, _, teardown := startupT(t)
defer teardown()

// check that it's not possible to click insert URL button and not alter the URL in it (which is `url` by default)
relativeURLText := `{"text": "here is a link with relative URL: [google.com](url)", "locator":{"url": "https://radio-t.com/blah1", "site": "remark42"}}`
resp, err := post(t, ts.URL+"/api/v1/comment", relativeURLText)
assert.NoError(t, err)
assert.Equal(t, http.StatusBadRequest, resp.StatusCode)
b, err := io.ReadAll(resp.Body)
assert.NoError(t, err)
assert.NoError(t, resp.Body.Close())
c := R.JSON{}
err = json.Unmarshal(b, &c)
assert.NoError(t, err)
assert.Equal(t, "links should start with mailto:, http:// or https://", c["error"])
assert.Equal(t, "invalid comment", c["details"])
}

func TestRest_CreateRejected(t *testing.T) {
ts, _, teardown := startupT(t)
defer teardown()
Expand Down
4 changes: 2 additions & 2 deletions backend/app/store/comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,8 @@ func (c *Comment) Sanitize() {
// special case for embedding the quotes from Twitter
p.AllowAttrs("class").Matching(regexp.MustCompile("^twitter-tweet$")).OnElements("blockquote")
// this is list of <span> tag classes which could be produced by chroma code renderer
// source: https://github.com/alecthomas/chroma/blob/cc2dd5b/types.go#L211-L307
const codeSpanClassRegex = "^(bg|chroma|line|ln|lnt|hl|lntable|lntd|cl|w|err|x|k|kc" +
// source: https://github.com/alecthomas/chroma/blob/c263f6f/types.go#L209-L306
const codeSpanClassRegex = "^(bg|chroma|line|ln|lnt|hl|lntable|lntd|lnlinks|cl|w|err|x|k|kc" +
"|kd|kn|kp|kr|kt|n|na|nb|bp|nc|no|nd|ni|ne|nf|fm|py|nl|nn|nx|nt|nv|vc|vg" +
"|vi|vm|l|ld|s|sa|sb|sc|dl|sd|s2|se|sh|si|sx|sr|s1|ss|m|mb|mf|mh|mi|il" +
"|mo|o|ow|p|c|ch|cm|cp|cpf|c1|cs|g|gd|ge|gr|gh|gi|go|gp|gs|gu|gt|gl)$"
Expand Down
28 changes: 17 additions & 11 deletions backend/app/store/formatter.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,17 +42,8 @@ func (f *CommentFormatter) Format(c Comment) Comment {

// FormatText converts text with markdown processor, applies external converters and shortens links
func (f *CommentFormatter) FormatText(txt string) (res string) {
mdExt := bf.NoIntraEmphasis | bf.Tables | bf.FencedCode |
bf.Strikethrough | bf.SpaceHeadings | bf.HardLineBreak |
bf.BackslashLineBreak | bf.Autolink

rend := bf.NewHTMLRenderer(bf.HTMLRendererParameters{
Flags: bf.Smartypants | bf.SmartypantsFractions | bf.SmartypantsDashes | bf.SmartypantsAngledQuotes,
})

extRend := bfchroma.NewRenderer(bfchroma.Extend(rend), bfchroma.ChromaOptions(html.WithClasses(true)))

res = string(bf.Run([]byte(txt), bf.WithExtensions(mdExt), bf.WithRenderer(extRend)))
mdExt, rend := GetMdExtensionsAndRenderer()
res = string(bf.Run([]byte(txt), bf.WithExtensions(mdExt), bf.WithRenderer(rend)))
res = f.unEscape(res)

for _, conv := range f.converters {
Expand Down Expand Up @@ -126,3 +117,18 @@ func (f *CommentFormatter) lazyImage(commentHTML string) (resHTML string) {
}
return resHTML
}

// GetMdExtensionsAndRenderer returns blackfriday extensions and renderer used for rendering markdown
// within store module.
func GetMdExtensionsAndRenderer() (bf.Extensions, *bfchroma.Renderer) {
mdExt := bf.NoIntraEmphasis | bf.Tables | bf.FencedCode |
bf.Strikethrough | bf.SpaceHeadings | bf.HardLineBreak |
bf.BackslashLineBreak | bf.Autolink

rend := bf.NewHTMLRenderer(bf.HTMLRendererParameters{
Flags: bf.Smartypants | bf.SmartypantsFractions | bf.SmartypantsDashes | bf.SmartypantsAngledQuotes,
})

extRend := bfchroma.NewRenderer(bfchroma.Extend(rend), bfchroma.ChromaOptions(html.WithClasses(true)))
return mdExt, extRend
}
21 changes: 19 additions & 2 deletions backend/app/store/service/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
log "github.com/go-pkgz/lgr"
"github.com/google/uuid"
"github.com/hashicorp/go-multierror"
bf "github.com/russross/blackfriday/v2"

"github.com/umputun/remark42/backend/app/store"
"github.com/umputun/remark42/backend/app/store/admin"
Expand Down Expand Up @@ -627,7 +628,9 @@ func (s *DataStore) Counts(siteID string, postIDs []string) ([]store.PostInfo, e
return res, nil
}

// ValidateComment checks if comment size below max and user fields set
// ValidateComment checks if comment size below max and user fields set.
// It also validates the absence of relative links as they are almost never the intention of the commenter,
// usually added by mistakes and only create confusion.
func (s *DataStore) ValidateComment(c *store.Comment) error {
maxSize := s.MaxCommentSize
if s.MaxCommentSize <= 0 {
Expand All @@ -642,7 +645,21 @@ func (s *DataStore) ValidateComment(c *store.Comment) error {
if c.User.ID == "" || c.User.Name == "" {
return fmt.Errorf("empty user info")
}
return nil

mdExt, rend := store.GetMdExtensionsAndRenderer()
parser := bf.New(bf.WithRenderer(rend), bf.WithExtensions(bf.CommonExtensions), bf.WithExtensions(mdExt))
var wrongLinkError error
parser.Parse([]byte(c.Orig)).Walk(func(node *bf.Node, entering bool) bf.WalkStatus {
if len(node.LinkData.Destination) != 0 &&
!(strings.HasPrefix(string(node.LinkData.Destination), "http://") ||
strings.HasPrefix(string(node.LinkData.Destination), "https://") ||
strings.HasPrefix(string(node.LinkData.Destination), "mailto:")) {
wrongLinkError = fmt.Errorf("links should start with mailto:, http:// or https://")
return bf.Terminate
}
return bf.GoToNext
})
return wrongLinkError
}

// IsAdmin checks if usesID in the list of admins
Expand Down
17 changes: 10 additions & 7 deletions backend/app/store/service/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -780,22 +780,25 @@ func TestService_ValidateComment(t *testing.T) {

tbl := []struct {
inp store.Comment
err error
err string
}{
{inp: store.Comment{}, err: fmt.Errorf("empty comment text")},
{inp: store.Comment{Orig: "something blah", User: store.User{ID: "myid", Name: "name"}}, err: nil},
{inp: store.Comment{Orig: "something blah", User: store.User{ID: "myid"}}, err: fmt.Errorf("empty user info")},
{inp: store.Comment{Orig: longText, User: store.User{ID: "myid", Name: "name"}}, err: fmt.Errorf("comment text exceeded max allowed size 2000 (4000)")},
{inp: store.Comment{}, err: "empty comment text"},
{inp: store.Comment{Orig: "something blah", User: store.User{ID: "myid", Name: "name"}}, err: ""},
{inp: store.Comment{Orig: "something blah", User: store.User{ID: "myid"}}, err: "empty user info"},
{inp: store.Comment{Orig: longText, User: store.User{ID: "myid", Name: "name"}}, err: "comment text exceeded max allowed size 2000 (4000)"},
{inp: store.Comment{Orig: "here is a link with relative URL: [google.com](url)", User: store.User{ID: "myid", Name: "name"}}, err: "links should start with mailto:, http:// or https://"},
{inp: store.Comment{Orig: "here is a link with relative URL: [google.com](url)", User: store.User{ID: "myid", Name: "name"}}, err: "links should start with mailto:, http:// or https://"},
{inp: store.Comment{Orig: "multiple links, one is bad: [test](http://test) [test2](bad_url) [test3](https://test3)", User: store.User{ID: "myid", Name: "name"}}, err: "links should start with mailto:, http:// or https://"},
}

for n, tt := range tbl {
err := b.ValidateComment(&tt.inp)
if tt.err == nil {
if tt.err == "" {
assert.NoError(t, err, "check #%d", n)
continue
}
require.Error(t, err)
assert.EqualError(t, tt.err, err.Error(), "check #%d", n)
assert.EqualError(t, err, tt.err, "check #%d", n)
}
}

Expand Down

0 comments on commit 270ff85

Please sign in to comment.