From 8b544b0a435f0e12dcaedd3ba5519b830b82c4a2 Mon Sep 17 00:00:00 2001 From: Umputun Date: Sat, 12 May 2018 11:42:05 -0500 Subject: [PATCH 01/25] move comment size validation to raw comment #3 --- app/rest/api/rest.go | 7 ++++++- app/store/service.go | 7 ------- app/store/service_test.go | 4 ++-- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/app/rest/api/rest.go b/app/rest/api/rest.go index 70684735..1460c450 100644 --- a/app/rest/api/rest.go +++ b/app/rest/api/rest.go @@ -137,9 +137,14 @@ func (s *Rest) createCommentCtrl(w http.ResponseWriter, r *http.Request) { return } - comment.PrepareUntrusted() // clean all fields user not suppoed to set + comment.PrepareUntrusted() // clean all fields user not supposed to set comment.User = user comment.User.IP = strings.Split(r.RemoteAddr, ":")[0] + if err = s.DataService.ValidateComment(&comment); err != nil { + rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "invalid comment") + return + } + comment.Text = string(blackfriday.Run([]byte(comment.Text), blackfriday.WithExtensions(mdExt))) log.Printf("[DEBUG] create comment %+v", comment) diff --git a/app/store/service.go b/app/store/service.go index c373f5ef..7d621ff0 100644 --- a/app/store/service.go +++ b/app/store/service.go @@ -31,9 +31,6 @@ func (s *Service) Create(comment Comment) (commentID string, err error) { comment.Votes = make(map[string]bool) } - if err = s.ValidateComment(&comment); err != nil { - return "", err - } comment.Sanitize() // clear potentially dangerous js from all parts of comment comment.User.hashIP(s.Secret) // replace ip by hash @@ -111,10 +108,6 @@ func (s *Service) EditComment(locator Locator, commentID string, text string, ed comment.Edit = &edit comment.Edit.Timestamp = time.Now() - if err = s.ValidateComment(&comment); err != nil { - return comment, err - } - comment.Sanitize() err = s.Put(locator, comment) return comment, err diff --git a/app/store/service_test.go b/app/store/service_test.go index 84ec4fb6..fab06b62 100644 --- a/app/store/service_test.go +++ b/app/store/service_test.go @@ -85,7 +85,7 @@ func TestService_Vote(t *testing.T) { assert.Equal(t, map[string]bool{"user1": true}, c.Votes, "user voted +") c, err = b.Vote(Locator{URL: "https://radio-t.com", SiteID: "radio-t"}, res[0].ID, "user", true) - assert.NotNil(t, "self-voting not allowed") + assert.NotNil(t, err, "self-voting not allowed") _, err = b.Vote(Locator{URL: "https://radio-t.com", SiteID: "radio-t"}, res[0].ID, "user1", true) assert.NotNil(t, err, "double-voting rejected") @@ -178,7 +178,7 @@ func TestService_EditCommentDurationFailed(t *testing.T) { time.Sleep(time.Second) - comment, err = b.EditComment(Locator{URL: "https://radio-t.com", SiteID: "radio-t"}, res[0].ID, "xxx", + _, err = b.EditComment(Locator{URL: "https://radio-t.com", SiteID: "radio-t"}, res[0].ID, "xxx", Edit{Summary: "my edit"}) assert.NotNil(t, err) } From 5d8a3ea78b27f9fe2ae8fd6f63c9173a4136d29f Mon Sep 17 00:00:00 2001 From: Umputun Date: Sat, 12 May 2018 11:42:34 -0500 Subject: [PATCH 02/25] lint: test shadowing --- app/rest/api/admin_test.go | 12 ++++++------ app/rest/api/rest_test.go | 2 ++ app/rest/auth/provider_test.go | 1 + 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/app/rest/api/admin_test.go b/app/rest/api/admin_test.go index 3ee4fb18..e874a3a7 100644 --- a/app/rest/api/admin_test.go +++ b/app/rest/api/admin_test.go @@ -105,13 +105,13 @@ func TestAdmin_Block(t *testing.T) { block := func(val int) (code int, body []byte) { client := http.Client{} - req, err := http.NewRequest(http.MethodPut, + req, e := http.NewRequest(http.MethodPut, fmt.Sprintf("http://dev:password@127.0.0.1:%d/api/v1/admin/user/%s?site=radio-t&block=%d", port, "user1", val), nil) - assert.Nil(t, err) - resp, err := client.Do(req) - require.Nil(t, err) - body, err = ioutil.ReadAll(resp.Body) - assert.Nil(t, err) + assert.Nil(t, e) + resp, e := client.Do(req) + require.Nil(t, e) + body, e = ioutil.ReadAll(resp.Body) + assert.Nil(t, e) resp.Body.Close() return resp.StatusCode, body } diff --git a/app/rest/api/rest_test.go b/app/rest/api/rest_test.go index 493a3d73..78f4fdbd 100644 --- a/app/rest/api/rest_test.go +++ b/app/rest/api/rest_test.go @@ -84,6 +84,7 @@ func TestServer_Preview(t *testing.T) { r := strings.NewReader(`{"text": "test 123", "locator":{"url": "https://radio-t.com/blah1", "site": "radio-t"}}`) resp, err := http.Post(fmt.Sprintf("http://dev:password@127.0.0.1:%d/api/v1/preview", port), "application/json", r) + assert.Nil(t, err) assert.Equal(t, http.StatusOK, resp.StatusCode) b, err := ioutil.ReadAll(resp.Body) assert.Nil(t, err) @@ -111,6 +112,7 @@ BKT t.Log(j) r := strings.NewReader(j) resp, err := http.Post(fmt.Sprintf("http://dev:password@127.0.0.1:%d/api/v1/preview", port), "application/json", r) + assert.Nil(t, err) assert.Equal(t, http.StatusOK, resp.StatusCode) b, err := ioutil.ReadAll(resp.Body) assert.Nil(t, err) diff --git a/app/rest/auth/provider_test.go b/app/rest/auth/provider_test.go index f7ac15d7..b5c522b7 100644 --- a/app/rest/auth/provider_test.go +++ b/app/rest/auth/provider_test.go @@ -30,6 +30,7 @@ func TestLogin(t *testing.T) { assert.Nil(t, err) assert.Equal(t, 200, resp.StatusCode) body, err := ioutil.ReadAll(resp.Body) + assert.Nil(t, err) t.Logf("resp %s", string(body)) u := store.User{} err = json.Unmarshal(body, &u) From 75a2cac9994c620794c8c16c11d6b100804b16a9 Mon Sep 17 00:00:00 2001 From: Umputun Date: Sat, 12 May 2018 11:49:46 -0500 Subject: [PATCH 03/25] adjust test for large comments #3 --- app/rest/api/rest_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/rest/api/rest_test.go b/app/rest/api/rest_test.go index 78f4fdbd..be4456b5 100644 --- a/app/rest/api/rest_test.go +++ b/app/rest/api/rest_test.go @@ -62,11 +62,11 @@ func TestServer_CreateTooBig(t *testing.T) { require.NotNil(t, srv) defer cleanup(srv) - longComment := fmt.Sprintf(`{"text": "%6000s", "locator":{"url": "https://radio-t.com/blah1", "site": "radio-t"}}`, "blah") + longComment := fmt.Sprintf(`{"text": "%4001s", "locator":{"url": "https://radio-t.com/blah1", "site": "radio-t"}}`, "X") r := strings.NewReader(longComment) resp, err := http.Post(fmt.Sprintf("http://dev:password@127.0.0.1:%d/api/v1/comment", port), "application/json", r) assert.Nil(t, err) - assert.Equal(t, http.StatusInternalServerError, resp.StatusCode) + assert.Equal(t, http.StatusBadRequest, resp.StatusCode) b, err := ioutil.ReadAll(resp.Body) assert.Nil(t, err) c := JSON{} @@ -74,7 +74,7 @@ func TestServer_CreateTooBig(t *testing.T) { assert.Nil(t, err) assert.Equal(t, "comment text exceeded max allowed size", c["error"]) - assert.Equal(t, "can't save comment", c["details"]) + assert.Equal(t, "invalid comment", c["details"]) } func TestServer_Preview(t *testing.T) { @@ -444,6 +444,7 @@ func TestServer_Config(t *testing.T) { assert.Nil(t, err) assert.Equal(t, 300., j["edit_duration"]) assert.EqualValues(t, []interface{}([]interface{}{"a1", "a2"}), j["admins"]) + assert.Equal(t, 4000., j["max_comment_size"]) t.Logf("%+v", j) } From 70c910b21af37ffcbd2318da6fab7db29c539ef1 Mon Sep 17 00:00:00 2001 From: Umputun Date: Sat, 12 May 2018 12:02:49 -0500 Subject: [PATCH 04/25] count as rune --- app/rest/api/rest_test.go | 4 ++-- app/store/service.go | 4 ++-- app/store/service_test.go | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/rest/api/rest_test.go b/app/rest/api/rest_test.go index be4456b5..c6cd145c 100644 --- a/app/rest/api/rest_test.go +++ b/app/rest/api/rest_test.go @@ -62,7 +62,7 @@ func TestServer_CreateTooBig(t *testing.T) { require.NotNil(t, srv) defer cleanup(srv) - longComment := fmt.Sprintf(`{"text": "%4001s", "locator":{"url": "https://radio-t.com/blah1", "site": "radio-t"}}`, "X") + longComment := fmt.Sprintf(`{"text": "%4001s", "locator":{"url": "https://radio-t.com/blah1", "site": "radio-t"}}`, "Щ") r := strings.NewReader(longComment) resp, err := http.Post(fmt.Sprintf("http://dev:password@127.0.0.1:%d/api/v1/comment", port), "application/json", r) assert.Nil(t, err) @@ -73,7 +73,7 @@ func TestServer_CreateTooBig(t *testing.T) { err = json.Unmarshal(b, &c) assert.Nil(t, err) - assert.Equal(t, "comment text exceeded max allowed size", c["error"]) + assert.Equal(t, "comment text exceeded max allowed size 4000 (4001)", c["error"]) assert.Equal(t, "invalid comment", c["details"]) } diff --git a/app/store/service.go b/app/store/service.go index 7d621ff0..98564305 100644 --- a/app/store/service.go +++ b/app/store/service.go @@ -133,8 +133,8 @@ func (s *Service) ValidateComment(c *Comment) error { if c.Text == "" { return errors.New("empty comment text") } - if len(c.Text) > maxSize { - return errors.New("comment text exceeded max allowed size") + if len([]rune(c.Text)) > maxSize { + return errors.Errorf("comment text exceeded max allowed size %d (%d)", maxSize, len([]rune(c.Text))) } if c.User.ID == "" || c.User.Name == "" { return errors.Errorf("empty user info") diff --git a/app/store/service_test.go b/app/store/service_test.go index fab06b62..414c977b 100644 --- a/app/store/service_test.go +++ b/app/store/service_test.go @@ -195,7 +195,7 @@ func TestService_ValidateComment(t *testing.T) { {inp: Comment{}, err: errors.New("empty comment text")}, {inp: Comment{Text: "something blah", User: User{ID: "myid", Name: "name"}}, err: nil}, {inp: Comment{Text: "something blah", User: User{ID: "myid"}}, err: errors.New("empty user info")}, - {inp: Comment{Text: longText, User: User{ID: "myid", Name: "name"}}, err: errors.New("comment text exceeded max allowed size")}, + {inp: Comment{Text: longText, User: User{ID: "myid", Name: "name"}}, err: errors.New("comment text exceeded max allowed size 2000 (4000)")}, } for n, tt := range tbl { From 9cacdb1497dd63eb8707f608eb453498bbdf900b Mon Sep 17 00:00:00 2001 From: Umputun Date: Sat, 12 May 2018 13:03:01 -0500 Subject: [PATCH 05/25] client-side caching for avatars --- app/rest/auth/avatar.go | 19 +++++++++++++++++++ app/rest/auth/avatar_test.go | 2 +- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/app/rest/auth/avatar.go b/app/rest/auth/avatar.go index fdaf8f35..f78fcb8b 100644 --- a/app/rest/auth/avatar.go +++ b/app/rest/auth/avatar.go @@ -8,6 +8,7 @@ import ( "net/http" "os" "path" + "strconv" "strings" "sync" "time" @@ -87,7 +88,20 @@ func (p *AvatarProxy) Routes() (string, chi.Router) { // GET /123456789.image router.Get("/{avatar}", func(w http.ResponseWriter, r *http.Request) { + avatar := chi.URLParam(r, "avatar") + + // client-side caching + etag := `"` + avatar + `"` + w.Header().Set("Etag", etag) + w.Header().Set("Cache-Control", "max-age=2592000") // 30 days + if match := r.Header.Get("If-None-Match"); match != "" { + if strings.Contains(match, etag) { + w.WriteHeader(http.StatusNotModified) + return + } + } + location := p.location(strings.TrimSuffix(avatar, imgSfx)) avFile := path.Join(location, avatar) fh, err := os.Open(avFile) @@ -103,6 +117,11 @@ func (p *AvatarProxy) Routes() (string, chi.Router) { }() w.Header().Set("Content-Type", "image/*") + if fi, err := fh.Stat(); err == nil { + w.Header().Set("Content-Length", strconv.Itoa(int(fi.Size()))) + } + + // write all headers if status, ok := r.Context().Value(render.StatusCtxKey).(int); ok { w.WriteHeader(status) } diff --git a/app/rest/auth/avatar_test.go b/app/rest/auth/avatar_test.go index 71741cd8..9389aa64 100644 --- a/app/rest/auth/avatar_test.go +++ b/app/rest/auth/avatar_test.go @@ -85,7 +85,7 @@ func TestRoutes(t *testing.T) { handler.ServeHTTP(rr, req) assert.Equal(t, http.StatusOK, rr.Code) - assert.Equal(t, http.Header{"Content-Type": []string{"image/*"}}, rr.HeaderMap) + assert.EqualValues(t, http.Header{"Content-Type": []string{"image/*"}, "Content-Length": []string{"21"}}, rr.HeaderMap) bb := bytes.Buffer{} sz, err := io.Copy(&bb, rr.Body) assert.NoError(t, err) From 9c498ea4c44225676222e4a6891eacabd878809c Mon Sep 17 00:00:00 2001 From: Umputun Date: Sat, 12 May 2018 13:09:40 -0500 Subject: [PATCH 06/25] Etag to avatar test --- app/rest/auth/avatar_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/rest/auth/avatar_test.go b/app/rest/auth/avatar_test.go index 9389aa64..9b22790c 100644 --- a/app/rest/auth/avatar_test.go +++ b/app/rest/auth/avatar_test.go @@ -85,7 +85,11 @@ func TestRoutes(t *testing.T) { handler.ServeHTTP(rr, req) assert.Equal(t, http.StatusOK, rr.Code) - assert.EqualValues(t, http.Header{"Content-Type": []string{"image/*"}, "Content-Length": []string{"21"}}, rr.HeaderMap) + + assert.Equal(t, []string{"image/*"}, rr.HeaderMap["Content-Type"]) + assert.Equal(t, []string{"21"}, rr.HeaderMap["Content-Length"]) + assert.NotNil(t, rr.HeaderMap["Etag"]) + bb := bytes.Buffer{} sz, err := io.Copy(&bb, rr.Body) assert.NoError(t, err) From 70a32695898db1a7fbd70a34d418c57a8f77498d Mon Sep 17 00:00:00 2001 From: Umputun Date: Sat, 12 May 2018 13:14:51 -0500 Subject: [PATCH 07/25] use login in github name is --- app/rest/auth/providers.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/rest/auth/providers.go b/app/rest/auth/providers.go index 5e0d2c6e..f92489d5 100644 --- a/app/rest/auth/providers.go +++ b/app/rest/auth/providers.go @@ -51,6 +51,10 @@ func NewGithub(p Params) Provider { Name: data.value("name"), Picture: data.value("avatar_url"), } + // github may have no user name, use login in this case + if userInfo.Name == "" { + userInfo.Name = data.value("login") + } if userInfo.Name == "" { userInfo.Name = userInfo.ID } From f587af37bd5ac23dfdfa926b45a7b589504e5610 Mon Sep 17 00:00:00 2001 From: Umputun Date: Sat, 12 May 2018 13:17:34 -0500 Subject: [PATCH 08/25] lint: avatar shadow err --- app/rest/auth/avatar.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/rest/auth/avatar.go b/app/rest/auth/avatar.go index f78fcb8b..67323660 100644 --- a/app/rest/auth/avatar.go +++ b/app/rest/auth/avatar.go @@ -117,7 +117,7 @@ func (p *AvatarProxy) Routes() (string, chi.Router) { }() w.Header().Set("Content-Type", "image/*") - if fi, err := fh.Stat(); err == nil { + if fi, e := fh.Stat(); e == nil { w.Header().Set("Content-Length", strconv.Itoa(int(fi.Size()))) } From 89e4e75b00f57d55314566f1593d49a5f5827ba3 Mon Sep 17 00:00:00 2001 From: igoradamenko Date: Sat, 12 May 2018 21:21:40 +0300 Subject: [PATCH 09/25] dont show actions for deleted comments --- web/app/components/comment/comment.jsx | 131 +++++++++++++------------ 1 file changed, 67 insertions(+), 64 deletions(-) diff --git a/web/app/components/comment/comment.jsx b/web/app/components/comment/comment.jsx index 180d7dd6..1318d0c6 100644 --- a/web/app/components/comment/comment.jsx +++ b/web/app/components/comment/comment.jsx @@ -195,7 +195,7 @@ export default class Comment extends Component { } render(props, { guest, isUserIdVisible, userBlocked, pinned, score, scoreIncreased, scoreDecreased, deleted, isInputVisible }) { - const { data, mix, mods = {} } = props; + const { data, mods = {} } = props; const isAdmin = !guest && store.get('user').admin; const isGuest = guest || !Object.keys(store.get('user')).length; const isCurrentUser = (data.user && data.user.id) === (store.get('user') && store.get('user').id); @@ -321,74 +321,77 @@ export default class Comment extends Component { dangerouslySetInnerHTML={{ __html: o.text }} /> -
- { - !mods.disabled && !isGuest && ( - {isInputVisible ? 'Cancel' : 'Reply'} - ) - } + { + !deleted && ( +
+ { + !mods.disabled && !isGuest && ( + {isInputVisible ? 'Cancel' : 'Reply'} + ) + } - { - isAdmin && - ( - - { - !pinned && ( - Pin - ) - } + { + isAdmin && ( + + { + !pinned && ( + Pin + ) + } - { - pinned && ( - Unpin - ) - } + { + pinned && ( + Unpin + ) + } - { - userBlocked && ( - Unblock - ) - } + { + userBlocked && ( + Unblock + ) + } - { - !userBlocked && ( - Block - ) - } + { + !userBlocked && ( + Block + ) + } - { - !deleted && ( - Delete - ) - } - - ) - } -
+ { + !deleted && ( + Delete + ) + } + + ) + } +
+ ) + } { From cc1894caa4e38d821f52155fa646f334a9308cd1 Mon Sep 17 00:00:00 2001 From: igoradamenko Date: Sat, 12 May 2018 21:42:15 +0300 Subject: [PATCH 10/25] fix default sort --- web/app/common/constants.js | 2 ++ web/app/components/root/root.jsx | 6 +++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/web/app/common/constants.js b/web/app/common/constants.js index cc22d65d..a0d7d159 100644 --- a/web/app/common/constants.js +++ b/web/app/common/constants.js @@ -11,6 +11,7 @@ const PROVIDER_NAMES = { facebook: 'Facebook', github: 'GitHub', }; +const DEFAULT_SORT = '-score'; module.exports = { BASE_URL, @@ -22,4 +23,5 @@ module.exports = { DEFAULT_LAST_COMMENTS_MAX, DEFAULT_MAX_COMMENT_SIZE, PROVIDER_NAMES, + DEFAULT_SORT, }; diff --git a/web/app/components/root/root.jsx b/web/app/components/root/root.jsx index c8b37e2e..660f12bd 100644 --- a/web/app/components/root/root.jsx +++ b/web/app/components/root/root.jsx @@ -1,7 +1,7 @@ import { h, Component } from 'preact'; import api from 'common/api'; -import { BASE_URL, NODE_ID, COMMENT_NODE_CLASSNAME_PREFIX } from 'common/constants'; +import { BASE_URL, NODE_ID, COMMENT_NODE_CLASSNAME_PREFIX, DEFAULT_SORT } from 'common/constants'; import { url } from 'common/settings'; import store from 'common/store'; @@ -21,9 +21,9 @@ export default class Root extends Component { let sort; try { - sort = localStorage.getItem(LS_SORT_KEY); + sort = localStorage.getItem(LS_SORT_KEY) || DEFAULT_SORT; } catch(e) { - sort = '-score'; + sort = DEFAULT_SORT; } this.state = { From 0b683e7c9a63235d5e9beccd983323af866d0ef4 Mon Sep 17 00:00:00 2001 From: igoradamenko Date: Sat, 12 May 2018 21:44:21 +0300 Subject: [PATCH 11/25] fix typo --- web/index.ejs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/index.ejs b/web/index.ejs index 01a85b57..38b8edd7 100644 --- a/web/index.ejs +++ b/web/index.ejs @@ -24,7 +24,7 @@