Security quick fixes

CLAUDE.md

@@ -136,6 +136,7 @@ must meet, and how it is enforced), [`docs/feed.md`](docs/feed.md),
 [`docs/profile-fields.md`](docs/profile-fields.md), [`docs/testing.md`](docs/testing.md),
 [`docs/load-testing.md`](docs/load-testing.md) (JMeter load tests),
 [`docs/logging-monitoring.md`](docs/logging-monitoring.md),
+[`docs/websocket-auth.md`](docs/websocket-auth.md) (who may subscribe to what, and turning enforcement on),
 [`docs/troubleshooting.md`](docs/troubleshooting.md).
 Mobile: [`android/README.md`](android/README.md), [`ios/README.md`](ios/README.md),
 [`docs/ios.md`](docs/ios.md) (iOS plan and remaining manual steps),

backend/api/package.json

@@ -1,6 +1,6 @@
 {
   "name": "@compass/api",
-  "version": "1.93.1",
+  "version": "1.94.0",
   "private": true,
   "description": "Backend API endpoints",
   "main": "src/serve.ts",
@@ -34,7 +34,7 @@
     "compression": "^1.8.0",
     "cors": "2.8.5",
     "dayjs": "1.11.19",
-    "express": "5.0.0",
+    "express": "5.2.1",
     "firebase-admin": "13.5.0",
     "gcp-metadata": "6.1.0",
     "jsdom": "29.0.1",

backend/api/src/app.ts

@@ -209,19 +209,17 @@ const requestMonitoring: RequestHandler = (req, _res, next) => {
 }
 
 const apiErrorHandler: ErrorRequestHandler = (error, _req, res, _next) => {
-  if (error instanceof APIError) {
+  if (error instanceof APIError && error.code !== 500) {
     log.info(error)
     if (!res.headersSent) {
       res.status(error.code).json(error.toJSON())
     }
   } else {
     log.error(error)
     if (!res.headersSent) {
-      const apiError = APIErrors.internalServerError(error.message || 'Internal server error', {
-        originalError: error.toString(),
-        context: 'Unhandled exception in request processing',
-      })
-      res.status(500).json(apiError.toJSON())
+      // Generic on purpose, including for a thrown APIError 500: many handlers build those from
+      // `error.message`, which can carry Postgres constraint and column names. The detail is in the log.
+      res.status(500).json(APIErrors.internalServerError('Internal server error').toJSON())
     }
   }
 }

backend/api/src/create-comment.ts

@@ -3,7 +3,6 @@ import {assertNotBlocked} from 'api/helpers/blocks'
 import {MAX_COMMENT_JSON_LENGTH, validateCommentAuthor} from 'api/helpers/comment'
 import {APIErrors, APIHandler} from 'api/helpers/endpoint'
 import {Notification} from 'common/notifications'
-import {convertComment} from 'common/supabase/comment'
 import {type Row} from 'common/supabase/utils'
 import {User} from 'common/user'
 import {getNotificationDestinationsForUser} from 'common/user-notification-preferences'
@@ -13,7 +12,7 @@ import {sendNewEndorsementEmail} from 'email/functions/helpers'
 import {createSupabaseDirectClient} from 'shared/supabase/init'
 import {insertNotificationToSupabase} from 'shared/supabase/notifications'
 import {getPrivateUser, getUser} from 'shared/utils'
-import {broadcastUpdatedComment} from 'shared/websockets/helpers'
+import {broadcastProfileCommentsChanged} from 'shared/websockets/helpers'
 
 export {MAX_COMMENT_JSON_LENGTH}
 
@@ -50,7 +49,7 @@ export const createComment: APIHandler<'create-comment'> = async (
       comment.id,
     )
 
-  broadcastUpdatedComment(convertComment(comment))
+  broadcastProfileCommentsChanged(onUser.id)
 
   return {status: 'success'}
 }

backend/api/src/helpers/endpoint.ts

@@ -12,6 +12,7 @@ import {bannedFromWritingError, type BanReason} from 'common/moderation/ban'
 import {PrivateUser} from 'common/user'
 import {NextFunction, Request, Response} from 'express'
 import * as admin from 'firebase-admin'
+import {getIp} from 'shared/analytics'
 import {getPrivateUserByKey, log} from 'shared/utils'
 import {z} from 'zod'
 
@@ -90,7 +91,6 @@ export const parseCredentials = async (req: Request): Promise<Credentials> => {
         console.error('Error verifying Firebase JWT: ', err, scheme, {
           jwtHeader: _header,
         })
-        debug('Failing Firebase JWT payload: ', payload)
         Sentry.captureException(err, {
           extra: {jwtHeader: _header},
         })
@@ -115,7 +115,7 @@ export const lookupUser = async (creds: Credentials): Promise<AuthedUser> => {
       const key = creds.data
       const privateUser = await getPrivateUserByKey(key)
       if (!privateUser) {
-        debug('No private user exists with API key', key)
+        debug('No private user exists with the provided API key')
         throw APIErrors.unauthorized('No private user exists with the provided API key.')
       }
       return {uid: privateUser.id, creds: {privateUser, ...creds}}
@@ -202,6 +202,7 @@ export type APIHandler<N extends APIPath> = (
 //   API_RATE_LIMIT_PER_MIN_UNAUTHED
 // Endpoints can be exempted by adding their name to RATE_LIMIT_EXEMPT (comma-separated)
 const __rateLimitState: Map<string, {windowStart: number; count: number}> = new Map()
+let __rateLimitWindow = 0
 
 function getRateLimitConfig() {
   const authed = Number(process.env.API_RATE_LIMIT_PER_MIN_AUTHED ?? 120)
@@ -211,8 +212,9 @@ function getRateLimitConfig() {
 
 function rateLimitKey(name: string, req: Request, auth?: AuthedUser) {
   if (auth) return `uid:${auth.uid}`
-  // fallback to IP for unauthenticated requests
-  return `ip:${req.ip}`
+  // fallback to IP for unauthenticated requests. Not `req.ip`: there is no `trust proxy`, so behind
+  // Cloud Run that is Google's front end, and every visitor would share a single bucket.
+  return `ip:${getIp(req)}`
 }
 
 function checkRateLimit(name: string, req: Request, res: Response, auth?: AuthedUser) {
@@ -224,6 +226,13 @@ function checkRateLimit(name: string, req: Request, res: Response, auth?: Authed
   const windowMs = 60_000
   const windowStart = Math.floor(now / windowMs) * windowMs
 
+  // Entries from a past window are never read again, and one key per client IP would otherwise grow
+  // for as long as the instance lives.
+  if (windowStart !== __rateLimitWindow) {
+    __rateLimitState.clear()
+    __rateLimitWindow = windowStart
+  }
+
   let state = __rateLimitState.get(key)
   if (!state || state.windowStart !== windowStart) {
     state = {windowStart, count: 0}

backend/api/src/hide-comment.ts

@@ -1,14 +1,23 @@
 import {APIErrors, APIHandler} from 'api/helpers/endpoint'
 import {isAdminId} from 'common/envs/constants'
-import {convertComment, convertVoteComment} from 'common/supabase/comment'
+import {convertVoteComment} from 'common/supabase/comment'
 import {Row} from 'common/supabase/utils'
 import {createSupabaseDirectClient} from 'shared/supabase/init'
-import {broadcastUpdatedComment, broadcastUpdatedVoteComment} from 'shared/websockets/helpers'
+import {
+  broadcastProfileCommentsChanged,
+  broadcastUpdatedVoteComment,
+} from 'shared/websockets/helpers'
 
 export const hideComment: APIHandler<'hide-comment'> = async (
   {commentId, hide, commentType},
   auth,
 ) => {
+  // Nothing records who hid a comment, so an author who could un-hide their own comment could also
+  // reverse a removal by a moderator or by the profile owner.
+  if (!hide && !isAdminId(auth.uid)) {
+    throw APIErrors.forbidden('Only admins can restore a hidden comment')
+  }
+
   const pg = createSupabaseDirectClient()
 
   if (commentType === 'vote') {
@@ -44,5 +53,5 @@ export const hideComment: APIHandler<'hide-comment'> = async (
 
   await pg.none(`update profile_comments set hidden = $2 where id = $1`, [commentId, hide])
 
-  broadcastUpdatedComment(convertComment(comment))
+  broadcastProfileCommentsChanged(comment.on_user_id)
 }

backend/api/src/react-to-message.ts

@@ -3,6 +3,16 @@ import {createSupabaseDirectClient} from 'shared/supabase/init'
 
 import {APIErrors, APIHandler} from './helpers/endpoint'
 
+// `reactions` maps each emoji to the ids of the members who used it. This takes one member ($2) off
+// one emoji ($1), and drops the emoji once they were the last on it. The scalar comparison covers old
+// rows holding a bare id rather than a list, which `MessageReactions` also tolerates.
+const WITHOUT_REACTOR = `
+  CASE
+    WHEN reactions -> $1 IN (to_jsonb($2::text), to_jsonb(ARRAY [$2::text]))
+      THEN reactions - $1
+    ELSE jsonb_set(reactions, ARRAY [$1], (reactions -> $1) - $2::text)
+  END`
+
 export const reactToMessage: APIHandler<'react-to-message'> = async (
   {messageId, reaction, toDelete},
   auth,
@@ -28,19 +38,19 @@ export const reactToMessage: APIHandler<'react-to-message'> = async (
     // Remove the reaction
     await pg.none(
       `UPDATE private_user_messages
-       SET reactions = reactions - $1
-       WHERE id = $2
-       AND reactions -> $1 ? $3`,
-      [reaction, messageId, auth.uid],
+       SET reactions = ${WITHOUT_REACTOR}
+       WHERE id = $3
+       AND reactions -> $1 ? $2`,
+      [reaction, auth.uid, messageId],
     )
   } else {
     // Toggle reaction
     await pg.none(
       `UPDATE private_user_messages
        SET reactions =
                CASE
-                   WHEN reactions -> $1 IS NOT NULL
-                       THEN reactions - $1
+                   WHEN reactions -> $1 ? $2
+                       THEN ${WITHOUT_REACTOR}
                    ELSE jsonb_set(
                            COALESCE(reactions, '{}'::jsonb),
                            ARRAY [$1],

backend/api/tests/unit/create-comment.unit.test.ts

@@ -1,6 +1,5 @@
 import {createComment} from 'api/create-comment'
 import {AuthedUser} from 'api/helpers/endpoint'
-import {convertComment} from 'common/supabase/comment'
 import {sqlMatch} from 'common/test-utils'
 import * as notificationPrefs from 'common/user-notification-preferences'
 import {richTextToString} from 'common/util/parse'
@@ -78,7 +77,6 @@ describe('createComment', () => {
         content: mockContent.content,
         replyToCommentId: mockReplyToCommentId,
       }
-      const mockConvertCommentReturn = 'mockConverComment'
 
       ;(sharedUtils.getUser as jest.Mock)
         .mockResolvedValueOnce(mockCreator)
@@ -91,7 +89,6 @@ describe('createComment', () => {
       ;(notificationPrefs.getNotificationDestinationsForUser as jest.Mock).mockReturnValue(
         mockNotificationDestination,
       )
-      ;(convertComment as jest.Mock).mockReturnValue(mockConvertCommentReturn)
 
       const results = await createComment(mockProps, mockAuth, mockReq)
 
@@ -128,8 +125,8 @@ describe('createComment', () => {
         mockOnUser,
         richTextToString(mockProps.content),
       )
-      expect(websocketHelpers.broadcastUpdatedComment).toBeCalledTimes(1)
-      expect(websocketHelpers.broadcastUpdatedComment).toBeCalledWith(mockConvertCommentReturn)
+      expect(websocketHelpers.broadcastProfileCommentsChanged).toBeCalledTimes(1)
+      expect(websocketHelpers.broadcastProfileCommentsChanged).toBeCalledWith(mockOnUser.id)
     })
   })
 

backend/api/tests/unit/hide-comment.unit.test.ts

@@ -1,7 +1,6 @@
 import {AuthedUser} from 'api/helpers/endpoint'
 import {hideComment} from 'api/hide-comment'
 import * as envConsts from 'common/envs/constants'
-import {convertComment} from 'common/supabase/comment'
 import {sqlMatch} from 'common/test-utils'
 import * as supabaseInit from 'shared/supabase/init'
 import * as websocketHelpers from 'shared/websockets/helpers'
@@ -44,11 +43,9 @@ describe('hideComment', () => {
         user_name: 'mockUserName',
         user_username: 'mockUserUsername',
       }
-      const mockConvertedComment = 'mockConvertedCommentValue'
 
       ;(mockPg.oneOrNone as jest.Mock).mockResolvedValue(mockComment)
       jest.spyOn(envConsts, 'isAdminId').mockReturnValue(true)
-      ;(convertComment as jest.Mock).mockReturnValue(mockConvertedComment)
 
       await hideComment(mockProps, mockAuth, mockReq)
 
@@ -59,10 +56,10 @@ describe('hideComment', () => {
       )
       expect(envConsts.isAdminId).toBeCalledTimes(1)
       expect(envConsts.isAdminId).toBeCalledWith(mockAuth.uid)
-      expect(convertComment).toBeCalledTimes(1)
-      expect(convertComment).toBeCalledWith(mockComment)
-      expect(websocketHelpers.broadcastUpdatedComment).toBeCalledTimes(1)
-      expect(websocketHelpers.broadcastUpdatedComment).toBeCalledWith(mockConvertedComment)
+      expect(websocketHelpers.broadcastProfileCommentsChanged).toBeCalledTimes(1)
+      expect(websocketHelpers.broadcastProfileCommentsChanged).toBeCalledWith(
+        mockComment.on_user_id,
+      )
     })
 
     it('should successfully hide the comment if the user is the one who made the comment', async () => {
@@ -117,6 +114,38 @@ describe('hideComment', () => {
       await hideComment(mockProps, mockAuth, mockReq)
     })
   })
+  describe('when restoring a hidden comment', () => {
+    const mockAuth = {uid: '321'} as AuthedUser
+    const mockReq = {} as any
+    const mockComment = {hidden: true, id: 123, on_user_id: '321', user_id: '321'}
+
+    it.each([undefined, 'vote'] as const)(
+      'should refuse the author and the profile owner (commentType %s)',
+      async (commentType) => {
+        ;(mockPg.oneOrNone as jest.Mock).mockResolvedValue(mockComment)
+        jest.spyOn(envConsts, 'isAdminId').mockReturnValue(false)
+
+        await expect(
+          hideComment({commentId: 'mockCommentId', hide: false, commentType}, mockAuth, mockReq),
+        ).rejects.toThrow('Only admins can restore a hidden comment')
+
+        expect(mockPg.none).not.toBeCalled()
+      },
+    )
+
+    it('should let an admin restore it', async () => {
+      ;(mockPg.oneOrNone as jest.Mock).mockResolvedValue(mockComment)
+      jest.spyOn(envConsts, 'isAdminId').mockReturnValue(true)
+
+      await hideComment({commentId: 'mockCommentId', hide: false}, mockAuth, mockReq)
+
+      expect(mockPg.none).toBeCalledWith(sqlMatch('update profile_comments set hidden = $2'), [
+        'mockCommentId',
+        false,
+      ])
+    })
+  })
+
   describe('when an error occurs', () => {
     it('should throw if the comment was not found', async () => {
       const mockProps = {

backend/api/tests/unit/react-to-message.unit.test.ts

@@ -78,9 +78,11 @@ describe('reactToMessage', () => {
       expect(result.success).toBeTruthy()
       expect(mockPg.oneOrNone).toBeCalledTimes(1)
       expect(mockPg.none).toBeCalledTimes(1)
-      expect(params1).toEqual([mockProps.reaction, mockProps.messageId, mockAuth.uid])
+      expect(params1).toEqual([mockProps.reaction, mockAuth.uid, mockProps.messageId])
       expect(sql1).toEqual(sqlMatch('UPDATE private_user_messages'))
-      expect(sql1).toEqual(sqlMatch('SET reactions = reactions - $1'))
+      // Takes this member off the emoji; the emoji key itself goes only with its last reactor.
+      expect(sql1).toEqual(sqlMatch('(reactions -> $1) - $2::text'))
+      expect(sql1).toEqual(sqlMatch('AND reactions -> $1 ? $2'))
     })
   })
   describe('when an error occurs', () => {

backend/shared/CLAUDE.md

@@ -23,6 +23,7 @@ backend/shared/src/
 │   ├── users.ts, messages.ts, notifications.ts, options.ts   Domain queries
 ├── websockets/
 │   ├── server.ts, switchboard.ts
+│   ├── topics.ts           who may subscribe to what (closed by default)
 │   └── helpers.ts          broadcastUpdatedUser, broadcastUpdatedPrivateUser, ...
 │                           (topics here are what the web `useApiSubscription` listens to)
 ├── monitoring/
@@ -40,7 +41,8 @@ backend/shared/src/
 - **SQL**: never string-concatenate. Use the helpers in `supabase/utils` for inserts/updates, or compose with
   `sql-builder` for reads. Lowercase SQL by convention.
 - **New websocket topic**: add a `broadcastX` function in `websockets/helpers.ts` so the frontend has a typed
-  topic to subscribe to.
+  topic to subscribe to, and list the topic in `websockets/topics.ts`, or subscribers are refused. See
+  [`docs/websocket-auth.md`](../../docs/websocket-auth.md).
 - **Logging**: `log` from `monitoring/log`. `debug()` from `common/logger` is fine for verbose dev logs but
   `log` carries structured context to Cloud Logging.
 - **Adding a domain helper**: if a query is reused by more than one API endpoint, lift it into

backend/shared/src/monitoring/metrics.ts

@@ -35,6 +35,10 @@ export const CUSTOM_METRICS = {
     metricKind: 'CUMULATIVE',
     valueKind: 'int64Value',
   },
+  'ws/unauthed_private_sessions': {
+    metricKind: 'CUMULATIVE',
+    valueKind: 'int64Value',
+  },
   'http/request_count': {
     metricKind: 'CUMULATIVE',
     valueKind: 'int64Value',

backend/shared/src/websockets/helpers.ts

@@ -1,4 +1,4 @@
-import {type ProfileComment, type VoteComment} from 'common/comment'
+import {type VoteComment} from 'common/comment'
 import {type User} from 'common/user'
 
 import {broadcast, broadcastMulti} from './server'
@@ -12,8 +12,10 @@ export function broadcastUpdatedUser(user: Partial<User> & {id: string}) {
   broadcast(`user/${user.id}`, {user})
 }
 
-export function broadcastUpdatedComment(comment: ProfileComment) {
-  broadcast(`user/${comment.onUserId}/comment`, {comment})
+// Carries no comment on purpose: this topic is open to anyone, while who may read a profile's comments
+// depends on the viewer. Subscribers refetch, and `get-profile-comments` applies those rules.
+export function broadcastProfileCommentsChanged(onUserId: string) {
+  broadcast(`user/${onUserId}/comment`, {})
 }
 
 export function broadcastUpdatedVoteComment(comment: VoteComment) {