Close the admin takeover through the delete-me link

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,11 @@ 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
+			}
 			return !claims.User.BoolAttr("blocked")
 		}),
 		JWTQuery:          "jwt", // change default from "token" as it used for deleteme
@@ -1522,6 +1528,18 @@ 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 })
+}
+
 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,22 @@ 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)
+
 	// try to add comment with multiple auds
 	badClaimsMultipleAud := claims
 	badClaimsMultipleAud.Audience = jwt.ClaimStrings{"remark", "second_aud"}
@@ -955,6 +974,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 +1254,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 +1271,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 +1290,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 +1447,21 @@ 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")
+}

backend/app/rest/api/rest_private.go

@@ -12,6 +12,7 @@ import (
 	"io"
 	"net"
 	"net/http"
+	"net/url"
 	"strings"
 	"time"
 
@@ -720,7 +721,8 @@ func (s *private) deleteMeCtrl(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	link := fmt.Sprintf("%s/web/deleteme.html?token=%s", s.remarkURL, tokenStr)
+	// the page signs the admin's requests for the site it is given, so the link names the user's site
+	link := fmt.Sprintf("%s/web/deleteme.html?site_id=%s&token=%s", s.remarkURL, url.QueryEscape(siteID), tokenStr)
 	R.RenderJSON(w, R.JSON{"site": siteID, "user_id": user.ID, "token": tokenStr, "link": link})
 }
 

backend/app/rest/api/rest_private_test.go

@@ -1493,7 +1493,7 @@ func TestRest_DeleteMe(t *testing.T) {
 	assert.Equal(t, "provider1_dev", claims.User.ID)
 	assert.Equal(t, "http://example.com/pic.png", claims.User.Picture,
 		"delete_me token must carry the user's picture so the avatar can be removed when the request is processed")
-	assert.Equal(t, "https://demo.remark42.com/web/deleteme.html?token="+tkn, m["link"])
+	assert.Equal(t, "https://demo.remark42.com/web/deleteme.html?site_id=remark42&token="+tkn, m["link"])
 
 	req, err = http.NewRequest(http.MethodPost, fmt.Sprintf("%s/api/v1/deleteme?site=remark42", ts.URL), http.NoBody)
 	assert.NoError(t, err)

frontend/apps/remark42/app/deleteme.test.ts

@@ -0,0 +1,71 @@
+import type * as Api from 'common/api';
+
+jest.mock('common/api', () => ({
+  getUser: jest.fn(),
+  approveDeleteMe: jest.fn(),
+}));
+
+const payload = '<img src=x onerror=window.__deletemeInjected=1>';
+
+/** loads the entry script against a fresh root node and waits for it to finish rendering */
+async function runPage(): Promise<HTMLElement> {
+  process.env.REMARK_NODE = 'remark42';
+  document.body.innerHTML = '<div id="remark42"></div>';
+  const root = document.getElementById('remark42')!;
+
+  await jest.isolateModulesAsync(async () => {
+    await import('./deleteme');
+  });
+  // getUser and approveDeleteMe resolve on separate turns before the page renders
+  for (let i = 0; i < 5; i++) {
+    await Promise.resolve();
+  }
+
+  return root;
+}
+
+function mockedApi(): jest.Mocked<typeof Api> {
+  return jest.requireMock('common/api');
+}
+
+describe('deleteme page', () => {
+  beforeEach(() => {
+    jest.resetModules();
+    jest.clearAllMocks();
+    jest.spyOn(console, 'error').mockImplementation(() => {});
+    window.history.replaceState(null, '', '/web/deleteme.html?token=t');
+    mockedApi().getUser.mockResolvedValue({ admin: true } as Awaited<ReturnType<typeof Api.getUser>>);
+  });
+
+  it('shows an error from the server as text', async () => {
+    // the shape the backend returns when the token names a site it does not serve
+    mockedApi().approveDeleteMe.mockRejectedValue({ code: 0, error: `site "${payload}" not found` });
+
+    const root = await runPage();
+
+    expect(root.querySelector('img')).toBeNull();
+    expect(root.querySelector('pre')?.textContent).toBe(`site "${payload}" not found`);
+  });
+
+  it('asks a signed-out admin to sign in to the site the request came from', async () => {
+    mockedApi().getUser.mockResolvedValue(null);
+    window.history.replaceState(null, '', `/web/deleteme.html?site_id=${encodeURIComponent(payload)}&token=t`);
+
+    const root = await runPage();
+
+    expect(root.querySelector('img')).toBeNull();
+    expect(root.querySelector('h3')?.textContent).toBe('You are not logged in');
+    expect(root.querySelector('pre')?.textContent).toContain(`site "${payload}"`);
+    expect(mockedApi().approveDeleteMe).not.toHaveBeenCalled();
+  });
+
+  it('shows the deletion result as text', async () => {
+    mockedApi().approveDeleteMe.mockResolvedValue({ user_id: 'dev_user', site_id: [payload] } as never);
+
+    const root = await runPage();
+
+    expect(root.querySelector('img')).toBeNull();
+    expect(root.querySelector('h3')?.textContent).toBe('User deleted successfully');
+    expect(root.querySelector('pre')?.textContent).toContain(payload);
+  });
+});

frontend/apps/remark42/app/deleteme.ts

@@ -1,6 +1,6 @@
 import { NODE_ID } from 'common/constants';
 import { approveDeleteMe, getUser } from 'common/api';
-import { token } from 'common/settings';
+import { siteId, token } from 'common/settings';
 import type { ApiError } from 'common/types';
 
 if (document.readyState === 'loading') {
@@ -23,29 +23,38 @@ async function init(): Promise<void> {
       return;
     }
 
+    // everything below reaches the page as text. the token is supplied by whoever sent the link,
+    // and both the response and the error echo values it carries, its site id among them
     approveDeleteMe(token).then(
       (data) => {
-        node.innerHTML = `
-            <h3>User deleted successfully</h3>
-            <pre>${JSON.stringify(data, null, 4)}</pre>
-          `;
+        render(node, 'User deleted successfully', JSON.stringify(data, null, 4));
       },
       (err: Error | ApiError | string) => {
         const message =
           err instanceof Error ? err.message : typeof err === 'object' && err !== null && err.error ? err.error : err;
         console.error(err);
-        node.innerHTML = `
-          <h3>Something went wrong</h3>
-          <pre>${message}</pre>
-        `;
+        render(node, 'Something went wrong', String(message));
       }
     );
   });
 }
 
+function render(node: HTMLElement, heading: string, details: string): void {
+  const title = document.createElement('h3');
+  title.textContent = heading;
+
+  const body = document.createElement('pre');
+  body.textContent = details;
+
+  node.replaceChildren(title, body);
+}
+
 function handleNotAuthorizedError(node: HTMLElement): void {
-  node.innerHTML = `
-    <h3>You are not logged in</h3>
-    <p><a href='/web' target='_blank'>Sign in</a> as admin to delete user information</p>
-  `;
+  // the session has to belong to the site the request came from, so the admin signs in through the
+  // comments on that site rather than the demo page, which is fixed to a site of its own
+  render(
+    node,
+    'You are not logged in',
+    `Sign in as admin to the comments on site "${siteId}", then open this link again to delete user information.`
+  );
 }

site/content/docs/contributing/api/index.md

@@ -231,6 +231,6 @@ http://oldsite.com/from-old-page/1 https://newsite.com/to-new-page/1
 - `DELETE /api/v1/admin/user/{userid}?site=site-id` - delete the user's comments and stored details; succeeds even if the user has no comments or is already absent
 - `PUT /api/v1/admin/readonly?site=site-id&url=post-url&ro=1` - set read-only status
 - `PUT /api/v1/admin/verify/{userid}?site=site-id&verified=1` - set verified status
-- `GET /api/v1/admin/deleteme?token=token` - process a user's deleteme request; already-deleted or dataless users return success (idempotent)
+- `GET /api/v1/admin/deleteme?site=site-id&token=token` - process a user's deleteme request; already-deleted or dataless users return success (idempotent)
 
 _all admin calls require auth and admin privilege_