From f8ba38779bf874694e38ab6aebc5b346f168a9e5 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Fri, 17 Apr 2026 03:53:08 +0100 Subject: [PATCH] fix(api): require explicit ?site= in matchSiteID middleware MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit matchSiteID guarded most authenticated and admin routes with `if siteID != "" && user.SiteID != siteID`. Dropping the ?site= query parameter made the check no-op and any authenticated user passed the middleware. Downstream handlers fell back to reading site from the JSON body or just used the empty string, so on email/telegram subscribe endpoints (which read site from body) a user authenticated to siteA could perform actions targeting siteB without the cross-site guard ever firing. Require ?site= to be present and to match user.SiteID. Body-only site flows are still supported provided the URL also carries the matching ?site= — both must agree, which removes the bypass and keeps the declared site visible to the middleware. Reproduction TestRest_matchSiteID enumerates four cases (matching, mismatched, missing, empty). Existing test calls that relied on the implicit pass had to add ?site=remark42 to the URL: the addComment helper now derives the param from c.Locator.SiteID, picture upload URL gets the param explicitly, and the email/telegram subscribe table adds it to every endpoint. The negative cases that previously asserted StatusBadRequest from the handler now correctly assert StatusForbidden from the middleware. --- backend/app/cmd/server_test.go | 16 ++--- backend/app/rest/api/admin_test.go | 4 +- backend/app/rest/api/rest.go | 4 +- backend/app/rest/api/rest_private_test.go | 76 +++++++++++------------ backend/app/rest/api/rest_test.go | 46 +++++++++++++- 5 files changed, 96 insertions(+), 50 deletions(-) diff --git a/backend/app/cmd/server_test.go b/backend/app/cmd/server_test.go index 9aa7ef20..eb16b501 100644 --- a/backend/app/cmd/server_test.go +++ b/backend/app/cmd/server_test.go @@ -48,7 +48,7 @@ func TestServerApp(t *testing.T) { // add comment client := http.Client{Timeout: 10 * time.Second} defer client.CloseIdleConnections() - req, err := http.NewRequest("POST", fmt.Sprintf("http://localhost:%d/api/v1/comment", port), + req, err := http.NewRequest("POST", fmt.Sprintf("http://localhost:%d/api/v1/comment?site=remark", port), strings.NewReader(`{"text": "test 123", "locator":{"url": "https://radio-t.com/blah1", "site": "remark"}}`)) require.NoError(t, err) req.SetBasicAuth("admin", "password") @@ -154,7 +154,7 @@ func TestServerApp_AnonMode(t *testing.T) { assert.Equal(t, http.StatusOK, resp.StatusCode) // try to add a comment as good anonymous - req, err := http.NewRequest("POST", fmt.Sprintf("http://localhost:%d/api/v1/comment", port), + req, err := http.NewRequest("POST", fmt.Sprintf("http://localhost:%d/api/v1/comment?site=remark", port), strings.NewReader(`{"text": "test 123", "locator":{"url": "https://radio-t.com/blah1", "site": "remark"}}`)) require.NoError(t, err) @@ -219,7 +219,7 @@ func TestServerApp_AnonMode(t *testing.T) { // try to add a comment as anonymous with admin name time.Sleep(time.Second) - req, err = http.NewRequest("POST", fmt.Sprintf("http://localhost:%d/api/v1/comment", port), + req, err = http.NewRequest("POST", fmt.Sprintf("http://localhost:%d/api/v1/comment?site=remark", port), strings.NewReader(`{"text": "test 123", "locator":{"url": "https://radio-t.com/blah1", "site": "remark"}}`)) require.NoError(t, err) @@ -742,7 +742,7 @@ func TestServerAuthHooks(t *testing.T) { defer client.CloseIdleConnections() // add comment - req, err := http.NewRequest("POST", fmt.Sprintf("http://localhost:%d/api/v1/comment", port), + req, err := http.NewRequest("POST", fmt.Sprintf("http://localhost:%d/api/v1/comment?site=remark", port), strings.NewReader(`{"text": "test 123", "locator":{"url": "https://radio-t.com/p/2018/12/29/podcast-630/", "site": "remark"}}`)) require.NoError(t, err) req.Header.Set("X-JWT", tk) @@ -757,7 +757,7 @@ func TestServerAuthHooks(t *testing.T) { tkNoAud, err := tkService.Token(badClaimsNoAud) require.NoError(t, err) t.Logf("no-aud claims: %s", tkNoAud) - req, err = http.NewRequest("POST", fmt.Sprintf("http://localhost:%d/api/v1/comment", port), + req, err = http.NewRequest("POST", fmt.Sprintf("http://localhost:%d/api/v1/comment?site=remark", port), strings.NewReader(`{"text": "test 123", "locator":{"url": "https://radio-t.com/p/2018/12/29/podcast-631/", "site": "remark"}}`)) require.NoError(t, err) @@ -775,7 +775,7 @@ func TestServerAuthHooks(t *testing.T) { tkMultipleAuds, err := tkService.Token(badClaimsMultipleAud) require.NoError(t, err) t.Logf("multiple aud claims: %s", tkMultipleAuds) - req, err = http.NewRequest("POST", fmt.Sprintf("http://localhost:%d/api/v1/comment", port), + req, err = http.NewRequest("POST", fmt.Sprintf("http://localhost:%d/api/v1/comment?site=remark", port), strings.NewReader(`{"text": "test 123", "locator":{"url": "https://radio-t.com/p/2018/12/29/podcast-631/", "site": "remark"}}`)) require.NoError(t, err) @@ -794,7 +794,7 @@ func TestServerAuthHooks(t *testing.T) { tkNoUser, err := tkService.Token(badClaimsNoUser) require.NoError(t, err) t.Logf("no user claims: %s", tkNoUser) - req, err = http.NewRequest("POST", fmt.Sprintf("http://localhost:%d/api/v1/comment", port), + req, err = http.NewRequest("POST", fmt.Sprintf("http://localhost:%d/api/v1/comment?site=remark", port), strings.NewReader(`{"text": "test 123", "locator":{"url": "https://radio-t.com/p/2018/12/29/podcast-631/", "site": "remark"}}`)) require.NoError(t, err) @@ -820,7 +820,7 @@ func TestServerAuthHooks(t *testing.T) { t.Log(string(b)) // try add a comment with blocked user - req, err = http.NewRequest("POST", fmt.Sprintf("http://localhost:%d/api/v1/comment", port), + req, err = http.NewRequest("POST", fmt.Sprintf("http://localhost:%d/api/v1/comment?site=remark", port), strings.NewReader(`{"text": "test 123 blah", "locator":{"url": "https://radio-t.com/blah1", "site": "remark"}}`)) require.NoError(t, err) req.Header.Set("X-JWT", tk) diff --git a/backend/app/rest/api/admin_test.go b/backend/app/rest/api/admin_test.go index 663efd88..5afe2bb8 100644 --- a/backend/app/rest/api/admin_test.go +++ b/backend/app/rest/api/admin_test.go @@ -465,7 +465,7 @@ func TestAdmin_ReadOnly(t *testing.T) { Locator: store.Locator{SiteID: "remark42", URL: "https://radio-t.com/blah"}} b, err := json.Marshal(c) assert.NoError(t, err, "can't marshal comment %+v", c) - req, err = http.NewRequest("POST", ts.URL+"/api/v1/comment", bytes.NewBuffer(b)) + req, err = http.NewRequest("POST", ts.URL+"/api/v1/comment?site=remark42", bytes.NewBuffer(b)) require.NoError(t, err) resp, err = sendReq(t, req, adminUmputunToken) require.NoError(t, err) @@ -489,7 +489,7 @@ func TestAdmin_ReadOnly(t *testing.T) { Locator: store.Locator{SiteID: "remark42", URL: "https://radio-t.com/blah"}} b, err = json.Marshal(c) assert.NoError(t, err, "can't marshal comment %+v", c) - req, err = http.NewRequest("POST", ts.URL+"/api/v1/comment", bytes.NewBuffer(b)) + req, err = http.NewRequest("POST", ts.URL+"/api/v1/comment?site="+c.Locator.SiteID, bytes.NewBuffer(b)) require.NoError(t, err) resp, err = sendReq(t, req, adminUmputunToken) require.NoError(t, err) diff --git a/backend/app/rest/api/rest.go b/backend/app/rest/api/rest.go index 785b7d71..964fd00a 100644 --- a/backend/app/rest/api/rest.go +++ b/backend/app/rest/api/rest.go @@ -584,7 +584,9 @@ func matchSiteID(next http.Handler) http.Handler { } siteID := r.URL.Query().Get("site") - if siteID != "" && user.SiteID != siteID { + // require an explicit site so the user.SiteID check below cannot be bypassed + // by simply omitting the query parameter + if siteID == "" || user.SiteID != siteID { http.Error(w, "Access denied", http.StatusForbidden) return } diff --git a/backend/app/rest/api/rest_private_test.go b/backend/app/rest/api/rest_private_test.go index 4cfc3331..56dd3a89 100644 --- a/backend/app/rest/api/rest_private_test.go +++ b/backend/app/rest/api/rest_private_test.go @@ -38,7 +38,7 @@ func TestRest_Create(t *testing.T) { ts, _, teardown := startupT(t) defer teardown() - resp, err := post(t, ts.URL+"/api/v1/comment", + resp, err := post(t, ts.URL+"/api/v1/comment?site=remark42", `{"text": "test 123", "locator":{"url": "https://radio-t.com/blah1", "site": "remark42"}}`) assert.NoError(t, err) b, err := io.ReadAll(resp.Body) @@ -60,7 +60,7 @@ func TestRest_CreateFilteredCode(t *testing.T) { ts, _, teardown := startupT(t) defer teardown() - resp, err := post(t, ts.URL+"/api/v1/comment", + resp, err := post(t, ts.URL+"/api/v1/comment?site=remark42", `{"text": "`+"`foo`"+`", "locator":{"url": "https://radio-t.com/blah1", "site": "remark42"}}`) assert.NoError(t, err) b, err := io.ReadAll(resp.Body) @@ -110,7 +110,7 @@ func TestRest_CreateAndPreviewWithImage(t *testing.T) { defer pngServer.Close() t.Run("create", func(t *testing.T) { - resp, err := post(t, ts.URL+"/api/v1/comment", + resp, err := post(t, ts.URL+"/api/v1/comment?site=remark42", `{"text": "![](`+pngServer.URL+`/gopher.png)", "locator":{"url": "https://radio-t.com/blah1", "site": "remark42"}}`) assert.NoError(t, err) b, err := io.ReadAll(resp.Body) @@ -176,7 +176,7 @@ func TestRest_CreateOldPost(t *testing.T) { assert.Equal(t, 1, len(comments)) // try to add new comment to the same old post - resp, err := post(t, ts.URL+"/api/v1/comment", + resp, err := post(t, ts.URL+"/api/v1/comment?site=remark42", `{"text": "test 123", "locator":{"site": "remark42","url": "https://radio-t.com/blah1"}}`) assert.NoError(t, err) assert.NoError(t, resp.Body.Close()) @@ -189,7 +189,7 @@ func TestRest_CreateOldPost(t *testing.T) { _, err = srv.DataService.Create(old) assert.NoError(t, err) - resp, err = post(t, ts.URL+"/api/v1/comment", + resp, err = post(t, ts.URL+"/api/v1/comment?site=remark42", `{"text": "test 123", "locator":{"site": "remark42","url": "https://radio-t.com/blah1"}}`) assert.NoError(t, err) assert.NoError(t, resp.Body.Close()) @@ -202,7 +202,7 @@ func TestRest_CreateTooBig(t *testing.T) { longComment := fmt.Sprintf(`{"text": "%4001s", "locator":{"url": "https://radio-t.com/blah1", "site": "remark42"}}`, "Щ") - resp, err := post(t, ts.URL+"/api/v1/comment", longComment) + resp, err := post(t, ts.URL+"/api/v1/comment?site=remark42", longComment) assert.NoError(t, err) assert.Equal(t, http.StatusBadRequest, resp.StatusCode) b, err := io.ReadAll(resp.Body) @@ -215,7 +215,7 @@ func TestRest_CreateTooBig(t *testing.T) { assert.Equal(t, "invalid comment", c["details"]) veryLongComment := fmt.Sprintf(`{"text": "%70000s", "locator":{"url": "https://radio-t.com/blah1", "site": "remark42"}}`, "Щ") - resp, err = post(t, ts.URL+"/api/v1/comment", veryLongComment) + resp, err = post(t, ts.URL+"/api/v1/comment?site=remark42", veryLongComment) assert.NoError(t, err) assert.Equal(t, http.StatusBadRequest, resp.StatusCode) b, err = io.ReadAll(resp.Body) @@ -235,7 +235,7 @@ func TestRest_CreateWithRestrictedWord(t *testing.T) { badComment := `{"text": "What the duck is that?", "locator":{"url": "https://radio-t.com/blah1", "site": "remark42"}}` - resp, err := post(t, ts.URL+"/api/v1/comment", badComment) + resp, err := post(t, ts.URL+"/api/v1/comment?site=remark42", badComment) assert.NoError(t, err) assert.Equal(t, http.StatusBadRequest, resp.StatusCode) b, err := io.ReadAll(resp.Body) @@ -254,7 +254,7 @@ func TestRest_CreateRelativeURL(t *testing.T) { // 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) + resp, err := post(t, ts.URL+"/api/v1/comment?site=remark42", relativeURLText) assert.NoError(t, err) assert.Equal(t, http.StatusBadRequest, resp.StatusCode) b, err := io.ReadAll(resp.Body) @@ -273,7 +273,7 @@ func TestRest_CreateRejected(t *testing.T) { body := `{"text": "test 123", "locator":{"url": "https://radio-t.com/blah1", "site": "remark42"}}` // try to create without auth - resp, err := http.Post(ts.URL+"/api/v1/comment", "", strings.NewReader(body)) + resp, err := http.Post(ts.URL+"/api/v1/comment?site=remark42", "", strings.NewReader(body)) require.NoError(t, err) require.NoError(t, resp.Body.Close()) assert.Equal(t, http.StatusUnauthorized, resp.StatusCode) @@ -281,7 +281,7 @@ func TestRest_CreateRejected(t *testing.T) { // try with wrong aud client := &http.Client{Timeout: 5 * time.Second} defer client.CloseIdleConnections() - req, err := http.NewRequest("POST", ts.URL+"/api/v1/comment", strings.NewReader(body)) + req, err := http.NewRequest("POST", ts.URL+"/api/v1/comment?site=remark42", strings.NewReader(body)) require.NoError(t, err) req.Header.Add("X-JWT", devTokenBadAud) resp, err = client.Do(req) @@ -295,7 +295,7 @@ func TestRest_CreateWithWrongImage(t *testing.T) { defer teardown() // create comment - resp, err := post(t, ts.URL+"/api/v1/comment", fmt.Sprintf(`{"text": "![non-existent.jpg](%s/api/v1/picture/dev_user/bad_picture)", "locator":{"url": "https://radio-t.com/blah1", "site": "radio-t"}}`, srv.RemarkURL)) + resp, err := post(t, ts.URL+"/api/v1/comment?site=remark42", fmt.Sprintf(`{"text": "![non-existent.jpg](%s/api/v1/picture/dev_user/bad_picture)", "locator":{"url": "https://radio-t.com/blah1", "site": "radio-t"}}`, srv.RemarkURL)) assert.NoError(t, err) assert.Equal(t, http.StatusBadRequest, resp.StatusCode) b, err := io.ReadAll(resp.Body) @@ -317,7 +317,7 @@ func TestRest_CreateWithLazyImage(t *testing.T) { defer teardown() body := `{"text": "test 123 ![](http://example.com/image.png)", "locator":{"url": "https://radio-t.com/blah1", "site": "remark42"}}` // create comment - resp, err := post(t, ts.URL+"/api/v1/comment", body) + resp, err := post(t, ts.URL+"/api/v1/comment?site=remark42", body) require.NoError(t, err) require.Equal(t, http.StatusCreated, resp.StatusCode) b, err := io.ReadAll(resp.Body) @@ -334,7 +334,7 @@ func TestRest_CreateAndGet(t *testing.T) { defer teardown() // create comment - resp, err := post(t, ts.URL+"/api/v1/comment", + resp, err := post(t, ts.URL+"/api/v1/comment?site=remark42", `{"text": "**test** *123*\n\n http://radio-t.com", "locator":{"url": "https://radio-t.com/blah1", "site": "remark42"}}`) require.NoError(t, err) require.Equal(t, http.StatusCreated, resp.StatusCode) @@ -373,7 +373,7 @@ func TestRest_CreateWithQuotes(t *testing.T) { defer teardown() // create comment with quotes with smartypants - resp, err := post(t, ts.URL+"/api/v1/comment", + resp, err := post(t, ts.URL+"/api/v1/comment?site=remark42", `{"text": "smartpants \"quoted\" text", "locator":{"url": "https://radio-t.com/blah1", "site": "remark42"}}`) require.NoError(t, err) require.Equal(t, http.StatusCreated, resp.StatusCode) @@ -396,7 +396,7 @@ func TestRest_CreateWithQuotes(t *testing.T) { // create comment with quotes without smartypants srv.privRest.disableFancyTextFormatting = true - resp, err = post(t, ts.URL+"/api/v1/comment", + resp, err = post(t, ts.URL+"/api/v1/comment?site=remark42", `{"text": "no_smartpants \"quoted\" text", "locator":{"url": "https://radio-t.com/blah1", "site": "remark42"}}`) require.NoError(t, err) require.Equal(t, http.StatusCreated, resp.StatusCode) @@ -882,30 +882,30 @@ func TestRest_EmailAndTelegram(t *testing.T) { body string }{ {description: "issue delete request without auth", url: "/api/v1/email", method: http.MethodDelete, responseCode: http.StatusUnauthorized, noAuth: true}, - {description: "issue delete request without site_id", url: "/api/v1/email", method: http.MethodDelete, responseCode: http.StatusBadRequest}, + {description: "issue delete request without site_id", url: "/api/v1/email", method: http.MethodDelete, responseCode: http.StatusForbidden}, {description: "delete non-existent user email", url: "/api/v1/email?site=remark42", method: http.MethodDelete, responseCode: http.StatusOK}, - {description: "set user email, token not set", url: "/api/v1/email/confirm", method: http.MethodPost, responseCode: http.StatusBadRequest, body: `{"site":"remark42"}`}, + {description: "set user email, token not set", url: "/api/v1/email/confirm?site=remark42", method: http.MethodPost, responseCode: http.StatusBadRequest, body: `{"site":"remark42"}`}, {description: "set user email, token not set, old query param", url: "/api/v1/email/confirm?site=remark42", method: http.MethodPost, responseCode: http.StatusBadRequest}, - {description: "send email confirmation without address", url: "/api/v1/email/subscribe", method: http.MethodPost, responseCode: http.StatusBadRequest, body: `{"site":"remark42"}`}, + {description: "send email confirmation without address", url: "/api/v1/email/subscribe?site=remark42", method: http.MethodPost, responseCode: http.StatusBadRequest, body: `{"site":"remark42"}`}, {description: "send email confirmation without address, old query param", url: "/api/v1/email/subscribe?site=remark42", method: http.MethodPost, responseCode: http.StatusBadRequest}, - {description: "send email confirmation", url: "/api/v1/email/subscribe", method: http.MethodPost, responseCode: http.StatusOK, body: `{"site":"remark42","address":"good@example.com"}`}, + {description: "send email confirmation", url: "/api/v1/email/subscribe?site=remark42", method: http.MethodPost, responseCode: http.StatusOK, body: `{"site":"remark42","address":"good@example.com"}`}, {description: "send email confirmation, old query param", url: "/api/v1/email/subscribe?site=remark42&address=good@example.com", method: http.MethodPost, responseCode: http.StatusOK}, - {description: "set user email, token is good", url: "/api/v1/email/confirm", method: http.MethodPost, responseCode: http.StatusOK, cookieEmail: "good@example.com", body: fmt.Sprintf(`{"site":"remark42","token":%q}`, goodToken)}, + {description: "set user email, token is good", url: "/api/v1/email/confirm?site=remark42", method: http.MethodPost, responseCode: http.StatusOK, cookieEmail: "good@example.com", body: fmt.Sprintf(`{"site":"remark42","token":%q}`, goodToken)}, {description: "set user email, token is good, old query param", url: fmt.Sprintf("/api/v1/email/confirm?site=remark42&tkn=%s", goodToken), method: http.MethodPost, responseCode: http.StatusOK, cookieEmail: "good@example.com"}, {description: "send confirmation with same address", url: "/api/v1/email/subscribe?site=remark42&address=good@example.com", method: http.MethodPost, responseCode: http.StatusConflict}, {description: "get user email", url: "/api/v1/email?site=remark42", method: http.MethodGet, responseCode: http.StatusOK}, {description: "delete user email", url: "/api/v1/email?site=remark42", method: http.MethodDelete, responseCode: http.StatusOK}, {description: "send another confirmation", url: "/api/v1/email/subscribe?site=remark42&address=good@example.com", method: http.MethodPost, responseCode: http.StatusOK}, - {description: "set user email, token is good", url: "/api/v1/email/confirm", method: http.MethodPost, responseCode: http.StatusOK, cookieEmail: "good@example.com", body: fmt.Sprintf(`{"site":"remark42","token":%q}`, goodToken)}, + {description: "set user email, token is good", url: "/api/v1/email/confirm?site=remark42", method: http.MethodPost, responseCode: http.StatusOK, cookieEmail: "good@example.com", body: fmt.Sprintf(`{"site":"remark42","token":%q}`, goodToken)}, {description: "set user email, token is good, old query param", url: fmt.Sprintf("/api/v1/email/confirm?site=remark42&tkn=%s", goodToken), method: http.MethodPost, responseCode: http.StatusOK, cookieEmail: "good@example.com"}, {description: "unsubscribe user, no token", url: "/email/unsubscribe.html?site=remark42", method: http.MethodPost, responseCode: http.StatusBadRequest}, {description: "unsubscribe user, wrong token", url: "/email/unsubscribe.html?site=remark42&tkn=jwt", method: http.MethodGet, responseCode: http.StatusForbidden}, {description: "unsubscribe user, good token", url: fmt.Sprintf("/email/unsubscribe.html?site=remark42&tkn=%s", goodToken), method: http.MethodPost, responseCode: http.StatusOK}, {description: "unsubscribe user second time, good token", url: fmt.Sprintf("/email/unsubscribe.html?site=remark42&tkn=%s", goodToken), method: http.MethodPost, responseCode: http.StatusConflict}, {description: "issue delete request without auth", url: "/api/v1/telegram", method: http.MethodDelete, responseCode: http.StatusUnauthorized, noAuth: true}, - {description: "issue delete request without site_id", url: "/api/v1/telegram", method: http.MethodDelete, responseCode: http.StatusBadRequest}, + {description: "issue delete request without site_id", url: "/api/v1/telegram", method: http.MethodDelete, responseCode: http.StatusForbidden}, {description: "delete non-existent user telegram", url: "/api/v1/telegram?site=remark42", method: http.MethodDelete, responseCode: http.StatusOK}, - {description: "send telegram confirmation, no siteID", url: "/api/v1/telegram/subscribe", method: http.MethodGet, responseCode: http.StatusBadRequest}, + {description: "send telegram confirmation, no siteID", url: "/api/v1/telegram/subscribe", method: http.MethodGet, responseCode: http.StatusForbidden}, {description: "send telegram confirmation", url: "/api/v1/telegram/subscribe?site=remark42", method: http.MethodGet, responseCode: http.StatusOK}, {description: "set user telegram, token is good", url: "/api/v1/telegram/subscribe?site=remark42&tkn=good_token", method: http.MethodGet, responseCode: http.StatusOK}, {description: "send confirmation with same address", url: "/api/v1/telegram/subscribe?site=remark42", method: http.MethodGet, responseCode: http.StatusConflict}, @@ -956,7 +956,7 @@ func TestRest_EmailNotification(t *testing.T) { defer client.CloseIdleConnections() // create new comment from dev user - req, err := http.NewRequest("POST", ts.URL+"/api/v1/comment", strings.NewReader( + req, err := http.NewRequest("POST", ts.URL+"/api/v1/comment?site=remark42", strings.NewReader( `{"text": "test 123", "user": {"name": "provider1_dev::good@example.com"}, "locator":{"url": "https://radio-t.com/blah1", @@ -977,7 +977,7 @@ func TestRest_EmailNotification(t *testing.T) { assert.Empty(t, mockDestination.Get()[0].Emails) // create child comment from another user, email notification only to admin expected - req, err = http.NewRequest("POST", ts.URL+"/api/v1/comment", strings.NewReader(fmt.Sprintf( + req, err = http.NewRequest("POST", ts.URL+"/api/v1/comment?site=remark42", strings.NewReader(fmt.Sprintf( `{"text": "test 456", "pid": %q, "user": {"name": "other_user"}, @@ -999,7 +999,7 @@ func TestRest_EmailNotification(t *testing.T) { // send confirmation token for email req, err = http.NewRequest( http.MethodPost, - ts.URL+"/api/v1/email/subscribe", + ts.URL+"/api/v1/email/subscribe?site=remark42", io.NopCloser(strings.NewReader(`{"site": "remark42", "address": "good@example.com"}`)), ) require.NoError(t, err) @@ -1038,7 +1038,7 @@ func TestRest_EmailNotification(t *testing.T) { // verify email req, err = http.NewRequest( http.MethodPost, - ts.URL+"/api/v1/email/confirm", + ts.URL+"/api/v1/email/confirm?site=remark42", io.NopCloser(strings.NewReader(fmt.Sprintf(`{"site": "remark42", "token": %q}`, verificationToken))), ) require.NoError(t, err) @@ -1070,7 +1070,7 @@ func TestRest_EmailNotification(t *testing.T) { Picture: "http://example.com/pic.png", IP: "127.0.0.1", SiteID: "remark42"}, subscribedUser) // create child comment from another user, email notification expected - req, err = http.NewRequest("POST", ts.URL+"/api/v1/comment", strings.NewReader(fmt.Sprintf( + req, err = http.NewRequest("POST", ts.URL+"/api/v1/comment?site=remark42", strings.NewReader(fmt.Sprintf( `{"text": "test 789", "pid": %q, "user": {"name": "other_user"}, @@ -1101,7 +1101,7 @@ func TestRest_EmailNotification(t *testing.T) { assert.Equal(t, http.StatusOK, resp.StatusCode, string(body)) // create child comment from another user, no email notification - req, err = http.NewRequest("POST", ts.URL+"/api/v1/comment", strings.NewReader( + req, err = http.NewRequest("POST", ts.URL+"/api/v1/comment?site=remark42", strings.NewReader( `{"text": "test 321", "user": {"name": "other_user"}, "locator":{"url": "https://radio-t.com/blah1", @@ -1159,7 +1159,7 @@ func TestRest_EmailNotification(t *testing.T) { // confirm email via subscribe call, no email notification is expected req, err = http.NewRequest( http.MethodPost, - ts.URL+"/api/v1/email/subscribe", + ts.URL+"/api/v1/email/subscribe?site=remark42", io.NopCloser(strings.NewReader(`{"site": "remark42", "address": "good@example.com"}`)), ) require.NoError(t, err) @@ -1206,7 +1206,7 @@ func TestRest_TelegramNotification(t *testing.T) { defer client.CloseIdleConnections() // create new comment from dev user - req, err := http.NewRequest("POST", ts.URL+"/api/v1/comment", strings.NewReader( + req, err := http.NewRequest("POST", ts.URL+"/api/v1/comment?site=remark42", strings.NewReader( `{"text": "test 123", "user": {"name": "provider1_dev::good@example.com"}, "locator":{"url": "https://radio-t.com/blah1", @@ -1227,7 +1227,7 @@ func TestRest_TelegramNotification(t *testing.T) { assert.Empty(t, mockDestination.Get()[0].Telegrams) // create child comment from another user, telegram notification only to admin expected - req, err = http.NewRequest("POST", ts.URL+"/api/v1/comment", strings.NewReader(fmt.Sprintf( + req, err = http.NewRequest("POST", ts.URL+"/api/v1/comment?site=remark42", strings.NewReader(fmt.Sprintf( `{"text": "test 456", "pid": %q, "user": {"name": "other_user"}, @@ -1340,7 +1340,7 @@ func TestRest_TelegramNotification(t *testing.T) { Picture: "http://example.com/pic.png", IP: "127.0.0.1", SiteID: "remark42"}, user) // create child comment from another user, telegram notification expected - req, err = http.NewRequest("POST", ts.URL+"/api/v1/comment", strings.NewReader(fmt.Sprintf( + req, err = http.NewRequest("POST", ts.URL+"/api/v1/comment?site=remark42", strings.NewReader(fmt.Sprintf( `{"text": "test 789", "pid": %q, "user": {"name": "other_user"}, @@ -1371,7 +1371,7 @@ func TestRest_TelegramNotification(t *testing.T) { assert.Equal(t, http.StatusOK, resp.StatusCode, string(body)) // create child comment from another user, no telegram notification - req, err = http.NewRequest("POST", ts.URL+"/api/v1/comment", strings.NewReader( + req, err = http.NewRequest("POST", ts.URL+"/api/v1/comment?site=remark42", strings.NewReader( `{"text": "test 321", "user": {"name": "other_user"}, "locator":{"url": "https://radio-t.com/blah1", @@ -1536,7 +1536,7 @@ func TestRest_SavePictureCtrl(t *testing.T) { client := http.Client{} defer client.CloseIdleConnections() - req, err := http.NewRequest(http.MethodPost, fmt.Sprintf("%s/api/v1/picture", ts.URL), bodyBuf) + req, err := http.NewRequest(http.MethodPost, fmt.Sprintf("%s/api/v1/picture?site=remark42", ts.URL), bodyBuf) require.NoError(t, err) req.Header.Add("Content-Type", contentType) req.Header.Add("X-JWT", devToken) @@ -1628,7 +1628,7 @@ func TestRest_CreateWithPictures(t *testing.T) { require.NoError(t, bodyWriter.Close()) client := http.Client{} defer client.CloseIdleConnections() - req, err := http.NewRequest(http.MethodPost, fmt.Sprintf("%s/api/v1/picture", ts.URL), bodyBuf) + req, err := http.NewRequest(http.MethodPost, fmt.Sprintf("%s/api/v1/picture?site=remark42", ts.URL), bodyBuf) require.NoError(t, err) req.Header.Add("Content-Type", contentType) req.Header.Add("X-JWT", devToken) @@ -1654,7 +1654,7 @@ func TestRest_CreateWithPictures(t *testing.T) { text := fmt.Sprintf(`text 123 ![](%s/api/v1/picture/%s) *xxx* ![](%s/api/v1/picture/%s) ![](%s/api/v1/picture/%s)`, svc.RemarkURL, ids[0], svc.RemarkURL, ids[1], svc.RemarkURL, ids[2]) body := fmt.Sprintf(`{"text": %q, "locator":{"url": "https://radio-t.com/blah1", "site": "remark42"}}`, text) - resp, err := post(t, ts.URL+"/api/v1/comment", body) + resp, err := post(t, ts.URL+"/api/v1/comment?site=remark42", body) assert.NoError(t, err) b, err := io.ReadAll(resp.Body) assert.NoError(t, err) diff --git a/backend/app/rest/api/rest_test.go b/backend/app/rest/api/rest_test.go index 1cc848c1..ba4e0c23 100644 --- a/backend/app/rest/api/rest_test.go +++ b/backend/app/rest/api/rest_test.go @@ -630,7 +630,11 @@ func addCommentGetCreatedTime(t *testing.T, c store.Comment, ts *httptest.Server client := &http.Client{Timeout: 5 * time.Second} defer client.CloseIdleConnections() - req, err := http.NewRequest("POST", ts.URL+"/api/v1/comment", bytes.NewBuffer(b)) + postURL := ts.URL + "/api/v1/comment" + if c.Locator.SiteID != "" { + postURL += "?site=" + c.Locator.SiteID + } + req, err := http.NewRequest("POST", postURL, bytes.NewBuffer(b)) require.NoError(t, err) req.Header.Add("X-JWT", devToken) resp, err := client.Do(req) @@ -696,3 +700,43 @@ func TestMain(m *testing.M) { goleak.IgnoreTopFunction("github.com/hashicorp/golang-lru/v2/expirable.NewLRU[...].func1"), ) } + +// TestRest_matchSiteID reproduces the multi-tenant isolation gap in the matchSiteID +// middleware. Before the fix, the check `if siteID != "" && user.SiteID != siteID` +// silently allowed any authenticated request that omitted the ?site= query param. +// On admin and user-mutation routes this meant the cross-site check was bypassable +// just by dropping the parameter. The fix requires ?site= to be present and to match +// the user's bound site. +func TestRest_matchSiteID(t *testing.T) { + wrapped := matchSiteID(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("ok")) + })) + + cases := []struct { + name string + userSite string + query string + want int + }{ + {name: "matching site allowed", userSite: "site-a", query: "?site=site-a", want: http.StatusOK}, + {name: "mismatched site forbidden", userSite: "site-a", query: "?site=site-b", want: http.StatusForbidden}, + {name: "missing site param rejected", userSite: "site-a", query: "", want: http.StatusForbidden}, + {name: "empty site param rejected", userSite: "site-a", query: "?site=", want: http.StatusForbidden}, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + r = rest.SetUserInfo(r, store.User{ID: "u", Name: "u", SiteID: c.userSite}) + wrapped.ServeHTTP(w, r) + }) + ts := httptest.NewServer(h) + defer ts.Close() + resp, err := http.Get(ts.URL + c.query) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + assert.Equal(t, c.want, resp.StatusCode) + }) + } +}