fix(api): require explicit ?site= in matchSiteID middleware
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.
This commit is contained in:
Dmitry Verkhoturov
committed by
Umputun
Umputun
parent
94d1f6e224
commit
f8ba38779b
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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<bar>`"+`", "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": "", "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": "", "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": "", "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 ", "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  *xxx*  `, 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)
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user