Close the admin takeover through the delete-me link

.github/workflows/ci-backend.yml

@@ -66,13 +66,13 @@ jobs:
       - name: golangci-lint
         uses: golangci/golangci-lint-action@v9
         with:
-          version: "v2.13.2"
+          version: "v2.14.0"
           working-directory: backend/app
 
       - name: golangci-lint on example directory
         uses: golangci/golangci-lint-action@v9
         with:
-          version: "v2.13.2"
+          version: "v2.14.0"
           args: --config ../../.golangci.yml
           working-directory: backend/_example/memory_store
 

backend/app/cmd/server.go

@@ -1469,6 +1469,7 @@ func (s *ServerCommand) getAuthenticator(ds *service.DataStore, avas avatar.Stor
 		SecretReader: token.SecretFunc(func(aud string) (string, error) { // get secret per site
 			return admns.Key(aud)
 		}),
+		AudienceReader: s.tokenAudiences(),
 		ClaimsUpd: token.ClaimsUpdFunc(func(c token.Claims) token.Claims { // set attributes, on new token or refresh
 			if c.User == nil {
 				return c
@@ -1509,6 +1510,14 @@ func (s *ServerCommand) getAuthenticator(ds *service.DataStore, avas avatar.Stor
 			if claims.User.Audience == "" { // reject empty aud, made with old (pre 0.8.x) version of auth package
 				return false
 			}
+			// a delete-me token names the user and is signed like a session, but it is handed to the
+			// admin, so it must only ever work as the deletion request it is
+			if claims.User.BoolAttr("delete_me") {
+				return false
+			}
+			if !s.servesAudience(claims.Audience) {
+				return false
+			}
 			return !claims.User.BoolAttr("blocked")
 		}),
 		JWTQuery:          "jwt", // change default from "token" as it used for deleteme
@@ -1522,6 +1531,33 @@ func (s *ServerCommand) getAuthenticator(ds *service.DataStore, avas avatar.Stor
 	})
 }
 
+// tokenAudiences limits the audience of issued and accepted tokens to the configured sites when
+// remark42 keeps the data itself. The audience is the site a user names at login, and it is echoed
+// back in responses and error messages, so a value nothing serves must not be accepted at all. A
+// remote store decides which sites exist on its side, so with one any audience is let through.
+func (s *ServerCommand) tokenAudiences() token.Audience {
+	if s.Store.Type != "bolt" {
+		return nil
+	}
+	sites := slices.Clone(s.Sites)
+	return token.AudienceFunc(func() ([]string, error) { return sites, nil })
+}
+
+// servesAudience reports whether every audience in a token is a configured site, compared exactly.
+// tokenAudiences restricts the auth package's own check, which ignores case, while the bolt store
+// keys sites by the exact name
+func (s *ServerCommand) servesAudience(auds []string) bool {
+	if s.Store.Type != "bolt" {
+		return true
+	}
+	for _, aud := range auds {
+		if !slices.Contains(s.Sites, aud) {
+			return false
+		}
+	}
+	return true
+}
+
 func (s *ServerCommand) parseSameSite(ss string) http.SameSite {
 	switch strings.ToLower(ss) {
 	case "default":

backend/app/cmd/server_test.go

@@ -10,6 +10,7 @@ import (
 	"net"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"os"
 	"strconv"
 	"strings"
@@ -901,10 +902,12 @@ func TestServerAuthHooks(t *testing.T) {
 	assert.Equal(t, http.StatusCreated, resp.StatusCode, "non-blocked user able to post")
 
 	// try to add comment with no-aud claim
+	// the token service refuses to issue it, so sign it the way a holder of the secret could
 	badClaimsNoAud := claims
 	badClaimsNoAud.Audience = jwt.ClaimStrings{""}
-	tkNoAud, err := tkService.Token(badClaimsNoAud)
-	require.NoError(t, err)
+	_, err = tkService.Token(badClaimsNoAud)
+	require.Error(t, err, "token service must not issue a token with an empty audience")
+	tkNoAud := signRawToken(t, badClaimsNoAud)
 	t.Logf("no-aud claims: %s", tkNoAud)
 	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/",
@@ -918,6 +921,36 @@ func TestServerAuthHooks(t *testing.T) {
 	require.NoError(t, resp.Body.Close())
 	assert.Equal(t, http.StatusUnauthorized, resp.StatusCode, "user without aud claim rejected, \n"+tkNoAud+"\n"+string(body))
 
+	// a token for a site this instance does not serve is neither issued nor accepted
+	badClaimsOtherSite := claims
+	badClaimsOtherSite.Audience = jwt.ClaimStrings{"<b>other</b>"}
+	_, err = tkService.Token(badClaimsOtherSite)
+	require.Error(t, err, "token service must not issue a token for a site it does not serve")
+	tkOtherSite := signRawToken(t, badClaimsOtherSite)
+	req, err = http.NewRequest("GET", fmt.Sprintf("http://localhost:%d/api/v1/user?site=%s", port, url.QueryEscape("<b>other</b>")), http.NoBody)
+	require.NoError(t, err)
+	req.Header.Set("X-JWT", tkOtherSite)
+	resp, err = client.Do(req)
+	require.NoError(t, err)
+	body, err = io.ReadAll(resp.Body)
+	require.NoError(t, err)
+	require.NoError(t, resp.Body.Close())
+	assert.Equal(t, http.StatusUnauthorized, resp.StatusCode, "token for a site not served rejected, %s", body)
+
+	// the auth package matches audiences ignoring case, so it issues this token, but the bolt store
+	// keys sites by the exact name and the session must not be accepted
+	tkOtherCase, err := tkService.Token(func() token.Claims { c := claims; c.Audience = jwt.ClaimStrings{"REMARK"}; return c }())
+	require.NoError(t, err)
+	req, err = http.NewRequest("GET", fmt.Sprintf("http://localhost:%d/api/v1/user?site=REMARK", port), http.NoBody)
+	require.NoError(t, err)
+	req.Header.Set("X-JWT", tkOtherCase)
+	resp, err = client.Do(req)
+	require.NoError(t, err)
+	body, err = io.ReadAll(resp.Body)
+	require.NoError(t, err)
+	require.NoError(t, resp.Body.Close())
+	assert.Equal(t, http.StatusUnauthorized, resp.StatusCode, "token for a differently cased site rejected, %s", body)
+
 	// try to add comment with multiple auds
 	badClaimsMultipleAud := claims
 	badClaimsMultipleAud.Audience = jwt.ClaimStrings{"remark", "second_aud"}
@@ -955,6 +988,37 @@ func TestServerAuthHooks(t *testing.T) {
 	require.NoError(t, resp.Body.Close())
 	assert.Equal(t, http.StatusUnauthorized, resp.StatusCode, "user without user information rejected, \n"+tkNoUser+"\n"+string(body))
 
+	// a delete-me token, as the endpoint issues it, is a deletion request for the admin and not a session
+	req, err = http.NewRequest(http.MethodPost, fmt.Sprintf("http://localhost:%d/api/v1/deleteme?site=remark", port), http.NoBody)
+	require.NoError(t, err)
+	req.Header.Set("X-JWT", tk)
+	resp, err = client.Do(req)
+	require.NoError(t, err)
+	body, err = io.ReadAll(resp.Body)
+	require.NoError(t, err)
+	require.NoError(t, resp.Body.Close())
+	require.Equal(t, http.StatusOK, resp.StatusCode, string(body))
+	deleteMe := struct {
+		Token string `json:"token"`
+	}{}
+	require.NoError(t, json.Unmarshal(body, &deleteMe))
+	require.NotEmpty(t, deleteMe.Token)
+	for _, probe := range []struct{ method, path, body string }{
+		{http.MethodGet, "/api/v1/user?site=remark", ""},
+		{http.MethodPost, "/api/v1/comment?site=remark",
+			`{"text": "test 123", "locator":{"url": "https://radio-t.com/p/2018/12/29/podcast-632/", "site": "remark"}}`},
+	} {
+		req, err = http.NewRequest(probe.method, fmt.Sprintf("http://localhost:%d%s", port, probe.path), strings.NewReader(probe.body))
+		require.NoError(t, err)
+		req.Header.Set("X-JWT", deleteMe.Token)
+		resp, err = client.Do(req)
+		require.NoError(t, err)
+		body, err = io.ReadAll(resp.Body)
+		require.NoError(t, err)
+		require.NoError(t, resp.Body.Close())
+		assert.Equal(t, http.StatusUnauthorized, resp.StatusCode, "delete-me token rejected on %s %s, %s", probe.method, probe.path, body)
+	}
+
 	// block user github_dev as admin
 	req, err = http.NewRequest(http.MethodPut,
 		fmt.Sprintf("http://localhost:%d/api/v1/admin/user/github_dev?site=remark&block=1&ttl=10d", port), http.NoBody)
@@ -1204,15 +1268,15 @@ func waitForServerStart(t *testing.T, ports ...int) {
 // getRetryThrottled issues a GET and retries while the auth routes answer 429, since the /auth/
 // group is limited to 2 req/s and this test logs in more often than that. a transport error is
 // retried a couple of times and then reported as itself, so a dead server is not read as throttling
-func getRetryThrottled(t *testing.T, client *http.Client, url string) *http.Response {
+func getRetryThrottled(t *testing.T, client *http.Client, reqURL string) *http.Response {
 	t.Helper()
 	const transportRetries = 2
 	errCount := 0
 	for deadline := time.Now().Add(serverStartTimeout); time.Now().Before(deadline); time.Sleep(authRetryPoll) {
-		r, err := client.Get(url)
+		r, err := client.Get(reqURL)
 		if err != nil {
 			errCount++
-			require.LessOrEqual(t, errCount, transportRetries, "request to %s failed: %v", url, err)
+			require.LessOrEqual(t, errCount, transportRetries, "request to %s failed: %v", reqURL, err)
 			continue
 		}
 		if r.StatusCode == http.StatusTooManyRequests {
@@ -1221,7 +1285,7 @@ func getRetryThrottled(t *testing.T, client *http.Client, url string) *http.Resp
 		}
 		return r
 	}
-	t.Fatalf("request to %s kept being rate limited", url)
+	t.Fatalf("request to %s kept being rate limited", reqURL)
 	return nil
 }
 
@@ -1240,6 +1304,52 @@ func waitForServerPort(port int, timeout time.Duration) bool {
 	return false
 }
 
+// signRawToken signs claims with the shared secret prepServerApp configures, bypassing the token
+// service and the checks it makes before issuing
+func signRawToken(t *testing.T, claims token.Claims) string {
+	t.Helper()
+	tkn, err := jwt.NewWithClaims(jwt.SigningMethodHS256, claims).SignedString([]byte("secret"))
+	require.NoError(t, err)
+	return tkn
+}
+
+// TestServerApp_LoginRejectsUnknownSite covers the audience a user names at login: it ends up in
+// the session token and is echoed back in responses, so a site the instance does not serve gets
+// no session at all.
+func TestServerApp_LoginRejectsUnknownSite(t *testing.T) {
+	port := chooseUnusedPort(t)
+	app, ctx, cancel := prepServerApp(t, func(o ServerCommand) ServerCommand {
+		o.Port = port
+		o.Auth.Anonymous = true
+		return o
+	})
+	defer cancel()
+
+	go func() { _ = app.run(ctx) }()
+	waitForHTTPServerStart(t, port)
+
+	client := http.Client{Timeout: 10 * time.Second}
+	defer client.CloseIdleConnections()
+
+	resp := getRetryThrottled(t, &client, fmt.Sprintf("http://localhost:%d/auth/anonymous/login?user=blah123&aud=%s",
+		port, url.QueryEscape("<b>other</b>")))
+	body, err := io.ReadAll(resp.Body)
+	require.NoError(t, err)
+	require.NoError(t, resp.Body.Close())
+	assert.NotEqual(t, http.StatusOK, resp.StatusCode, "login to a site not served must fail, %s", body)
+	for _, c := range resp.Cookies() {
+		assert.NotEqual(t, "JWT", c.Name, "no session cookie for a site not served")
+	}
+
+	// the configured site still signs in
+	resp = getRetryThrottled(t, &client, fmt.Sprintf("http://localhost:%d/auth/anonymous/login?user=blah123&aud=remark", port))
+	require.NoError(t, resp.Body.Close())
+	assert.Equal(t, http.StatusOK, resp.StatusCode)
+
+	cancel()
+	app.Wait()
+}
+
 func prepServerApp(t *testing.T, fn func(o ServerCommand) ServerCommand) (*serverApp, context.Context, context.CancelFunc) {
 	cmd := ServerCommand{}
 	cmd.SetCommon(CommonOpts{RemarkURL: "https://demo.remark42.com", SharedSecret: "secret"})
@@ -1351,3 +1461,33 @@ func TestServerApp_MakeCacheKeepsLargeValuesByDefault(t *testing.T) {
 	}
 	assert.Equal(t, 1, loads, "a 300 KB value must be served from cache with default limits")
 }
+
+func TestServerCommand_tokenAudiences(t *testing.T) {
+	cmd := ServerCommand{Sites: []string{"remark", "other"}}
+	cmd.Store.Type = "bolt"
+	auds := cmd.tokenAudiences()
+	require.NotNil(t, auds, "the bolt store knows its sites, so the audience is limited to them")
+	got, err := auds.Get()
+	require.NoError(t, err)
+	assert.Equal(t, []string{"remark", "other"}, got)
+
+	cmd.Sites[0] = "changed"
+	got, err = auds.Get()
+	require.NoError(t, err)
+	assert.Equal(t, []string{"remark", "other"}, got, "the list is fixed when the authenticator is built")
+
+	cmd.Store.Type = "rpc"
+	assert.Nil(t, cmd.tokenAudiences(), "a remote store decides which sites exist, so any audience passes")
+}
+
+func TestServerCommand_servesAudience(t *testing.T) {
+	cmd := ServerCommand{Sites: []string{"remark", "other"}}
+	cmd.Store.Type = "bolt"
+	assert.True(t, cmd.servesAudience([]string{"remark"}))
+	assert.True(t, cmd.servesAudience([]string{"remark", "other"}))
+	assert.False(t, cmd.servesAudience([]string{"Remark"}), "sites are compared exactly")
+	assert.False(t, cmd.servesAudience([]string{"remark", "third"}), "every audience must be served")
+
+	cmd.Store.Type = "rpc"
+	assert.True(t, cmd.servesAudience([]string{"Remark"}), "a remote store decides which sites exist")
+}

backend/app/rest/api/admin.go

@@ -113,6 +113,14 @@ func (a *admin) deleteMeRequestCtrl(w http.ResponseWriter, r *http.Request) {
 
 	audience := claims.Audience[0]
 
+	// admin rights hold for the site the session belongs to, which matchSiteID has already matched
+	// against this query value. Only the basic-auth admin, who administers every site, may omit it
+	if site := r.URL.Query().Get("site"); site != "" && site != audience {
+		rest.SendErrorJSON(w, r, http.StatusForbidden, fmt.Errorf("token site %q, request site %q", audience, site),
+			"can't use provided token for this site", rest.ErrNoAccess)
+		return
+	}
+
 	if err = a.dataService.DeleteUserDetail(audience, claims.User.ID, engine.AllUserDetails); err != nil {
 		code := parseError(err, rest.ErrInternal)
 		rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't delete user details for user", code)

backend/app/rest/api/admin_test.go

@@ -9,6 +9,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 	"time"
@@ -19,8 +20,11 @@ import (
 	"github.com/golang-jwt/jwt/v5"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	bolt "go.etcd.io/bbolt"
 
 	"github.com/umputun/remark42/backend/app/store"
+	adminstore "github.com/umputun/remark42/backend/app/store/admin"
+	"github.com/umputun/remark42/backend/app/store/engine"
 	"github.com/umputun/remark42/backend/app/store/service"
 )
 
@@ -782,6 +786,63 @@ func TestAdmin_DeleteMeRequest(t *testing.T) {
 	assert.NoFileExists(t, os.TempDir()+"/ava-remark42/42/pic.image", "user's avatar should be removed on deleteme")
 }
 
+// an admin is an admin of the site the session belongs to, so a deletion token for another site
+// served by the same instance must not be processed under that session
+func TestAdmin_DeleteMeRequestRejectsOtherSite(t *testing.T) {
+	ts, srv, teardown := startupT(t, func(srv *Rest) {
+		require.NoError(t, srv.DataService.Engine.Close())
+		b, err := engine.NewBoltDB(bolt.Options{},
+			engine.BoltSite{FileName: filepath.Join(t.TempDir(), "remark42.db"), SiteID: "remark42"},
+			engine.BoltSite{FileName: filepath.Join(t.TempDir(), "other.db"), SiteID: "other"})
+		require.NoError(t, err)
+		srv.DataService.Engine = b
+		srv.DataService.AdminStore = adminstore.NewStaticStore("123456", []string{"remark42", "other"}, []string{"a1", "a2"}, "admin@remark-42.com")
+	})
+	defer teardown()
+
+	c := store.Comment{Text: "test test #1", Locator: store.Locator{SiteID: "other", URL: "https://radio-t.com/blah"},
+		User: store.User{Name: "user1 name", ID: "user1"}}
+	_, err := srv.DataService.Create(c)
+	require.NoError(t, err)
+
+	tkn, err := srv.Authenticator.TokenService().Token(token.Claims{
+		SessionOnly: true,
+		RegisteredClaims: jwt.RegisteredClaims{
+			Audience:  jwt.ClaimStrings{"other"},
+			ID:        "3456789",
+			Issuer:    "remark42",
+			NotBefore: jwt.NewNumericDate(time.Now().Add(-1 * time.Minute)),
+			ExpiresAt: jwt.NewNumericDate(time.Now().Add(30 * time.Minute)),
+		},
+		User: &token.User{ID: "user1", Attributes: map[string]any{"delete_me": true}},
+	})
+	require.NoError(t, err)
+
+	// adminUmputunToken is a session for remark42
+	req, err := http.NewRequest(http.MethodGet, fmt.Sprintf("%s/api/v1/admin/deleteme?site=remark42&token=%s", ts.URL, tkn), http.NoBody)
+	require.NoError(t, err)
+	resp, err := sendReq(req, adminUmputunToken)
+	require.NoError(t, err)
+	require.NoError(t, resp.Body.Close())
+	assert.Equal(t, http.StatusForbidden, resp.StatusCode)
+	comments, err := srv.DataService.User("other", "user1", 0, 0, store.User{})
+	require.NoError(t, err)
+	assert.Len(t, comments, 1, "the other site's data is untouched")
+
+	// the basic-auth admin administers every site and processes it when naming that site
+	client := http.Client{}
+	defer client.CloseIdleConnections()
+	req, err = http.NewRequest(http.MethodGet, fmt.Sprintf("%s/api/v1/admin/deleteme?site=other&token=%s", ts.URL, tkn), http.NoBody)
+	require.NoError(t, err)
+	req.SetBasicAuth("admin", "password")
+	resp, err = client.Do(req)
+	require.NoError(t, err)
+	require.NoError(t, resp.Body.Close())
+	assert.Equal(t, http.StatusOK, resp.StatusCode)
+	_, err = srv.DataService.User("other", "user1", 0, 0, store.User{})
+	assert.EqualError(t, err, "no comments for user user1 in store")
+}
+
 // a delete_me request whose token carries a picture must still succeed when the avatar is
 // already gone from the store: the user data is deleted and a missing avatar is tolerated
 func TestAdmin_DeleteMeRequestMissingAvatar(t *testing.T) {
@@ -949,7 +1010,7 @@ func TestAdmin_DeleteMeRequestFailed(t *testing.T) {
 	resp, err = client.Do(req)
 	assert.NoError(t, err)
 	assert.NoError(t, resp.Body.Close())
-	assert.Equal(t, http.StatusForbidden, resp.StatusCode)
+	assert.Equal(t, http.StatusUnauthorized, resp.StatusCode, "the delete-me token in the query is not a session")
 
 	// unknown user: deletion is idempotent, so a valid (signed) delete_me token for a user with
 	// no stored data is a no-op success rather than an error