fix: let scoped tokens list and mark inbox notifications read

[EhabY]

Problem

inbox_notification:read and inbox_notification:update exist, but aren't in the public scope catalog. So no scoped API key or OAuth token (short of coder:all) can list, watch, or mark inbox notifications read.

The inbox endpoints also hide a missing scope:

• GET /notifications/inbox/watch upgrades the socket without authorizing, then each message counts unread notifications, which needs inbox_notification:read. When that fails the server closes the socket with 1000, so clients see a clean close instead of a refusal.
• List, read-status and mark-all return 500 when the scope is missing.
• PUT /notifications/inbox/{id}/read-status reads the notification and unread count back after updating. An update-only token marks the notification read, then gets a 500.
• PUT /notifications/inbox/{id}/read-status acts on any notification RBAC allows, so an owner could mark another user's notification read and get its content back. A member passing another user's or an unknown ID got a 500.

Found while auditing the VS Code extension's OAuth scopes in coder/vscode-coder#1128.

Fix

• Add inbox_notification:read and inbox_notification:update to externalLowLevel (and the generated PublicAPIKeyScopes). create stays internal, so there's no inbox_notification:*. The DB enum already has both values, so no migration.

• Every inbox endpoint authorizes before doing anything and returns 403, with a detail naming the missing scope:

  endpoint	needs
  GET /notifications/inbox	read
  GET /notifications/inbox/watch	read (before upgrading)
  PUT /notifications/inbox/{id}/read-status	read + update
  PUT /notifications/inbox/mark-all-as-read	update

• read-status fetches the notification first and returns 404 unless it belongs to the caller.

Verification

TestInboxNotifications_Scopes covers each endpoint with too few and enough scopes, and checks a refused request leaves the notification unread. Its four refusal cases fail without the handler change. TestInboxNotification_Watch/NOK - missing scope dials the watch socket with a workspace:read token and gets 403.

[CalvinTjoaquinn]

Traced the catalog change against every handler that touches this resource, and read + update is exactly the set these endpoints need, no more:

handler	store method	action required
listInboxNotifications	GetFilteredInboxNotificationsByUserID, GetInboxNotificationsByUserID	ActionRead (dbauthz.go:4055, :4165)
watchInboxNotifications	CountUnreadInboxNotificationsByUserID	ActionRead (dbauthz.go:2081)
updateInboxNotificationReadStatus	UpdateInboxNotificationReadStatus	ActionUpdate, via update() → fetchAndExec(..., policy.ActionUpdate, ...) (dbauthz.go:1174, :7891)
markAllInboxNotificationsAsRead	MarkAllInboxNotificationsAsRead	ActionUpdate (dbauthz.go:7141)

Nothing on the read or write paths needs ActionCreate; only InsertInboxNotification does and that is server-side, so keeping create internal costs callers nothing.

That also answers a question the diff invites: the file block immediately above adds file:* alongside file:create, and this block adds no inbox_notification:*. The asymmetry is right, since the wildcard would hand out create as well. Might be worth a one-line comment next to the entries so the next person editing this map does not "fix" it by adding the wildcard for consistency.

One gap: update becomes public but nothing exercises it

Both tests cover the read half. nok - missing scope proves a workspace:read token is refused, and OK proves an inbox_notification:read token can watch. Neither drives inbox_notification:update.

Since the PR's own premise is that these scopes exist in the enum but no token can reach them, the symmetric proof for the other half would be a token scoped to inbox_notification:update against one of:

• PUT /notifications/inbox/{id}/read-status
• the mark-all endpoint

Both resolve to ActionUpdate per the table above, so one case is enough to pin that update is actually reachable and that it is not silently satisfied by read. As it stands, half of what the catalog now advertises is covered only by inspection.

One thing I suspected and it is fine

The new gate authorizes ResourceInboxNotification.WithOwner(apikey.UserID.String()), while the work it is gating happens per message further down, so I expected the two could disagree about whose inbox is being read. They do not: the per-message count is CountUnreadInboxNotificationsByUserID(ctx, apikey.UserID) at inboxnotifications.go:247 and the subscription channel is InboxNotificationForOwnerEventChannel(apikey.UserID) at :157. The gate covers exactly the work that follows it.

Placing the check before the upgrade also changes the failure from a 1000 close to a 403, which is the part a client can actually act on.

[BobbyHo]

/coder-agents-review

[coder-agents-review]

Chat: Review posted | View chat
Requested: 2026-10-01 15:13 UTC by @BobbyHo

Review history

• R1 (2026-10-01): 12 reviewers, 4 Nit, 2 P3, 3 P4, COMMENT. Review

deep-review v0.13.0 | Round 1 | cc73f32..35684e5

Last posted: Round 1, 9 findings (2 P3, 3 P4, 4 Nit), COMMENT. Review

Finding inventory

Findings

#	Sev	Status	Location	Summary	Round	Reviewer	Posted
CRF-1	P3	Open	inboxnotifications.go:408	read-status pre-check authorizes caller's inbox, not the {id} notification; owner-scoped token acts on other users' notifications, cross-user/missing IDs return 500	R1	Kurapika	Yes
CRF-2	P3	Open	inboxnotifications_test.go:965	list - ok discards scoped response; empty post-filtered list would pass	R1	Bisky P3, Kite P4	Yes
CRF-3	P4	Open	scopes_catalog.go:49	"create is server-only" comment: RBAC grants members create; only the absence of an endpoint keeps it server-side	R1	Gon	Yes
CRF-4	Nit	Open	inboxnotifications_test.go:986	wantRead field reads as "want read scope"	R1	Gon	Yes
CRF-5	Nit	Open	inboxnotifications_test.go:86	Subtest names use lowercase ok/nok; file uses OK/NOK	R1	Gon	Yes
CRF-6	P4	Open	inboxnotifications.go:159	New 403s send scoped-token users to an administrator instead of naming the missing scope	R1	Leorio P3	Yes
CRF-7	P4	Open	commit/PR title	Title says "read" but the PR also exposes inbox_notification:update	R1	Leorio	Yes
CRF-8	Nit	Open	inboxnotifications.go:157	Comment describes old behavior instead of why the check precedes websocket.Accept	R1	Leorio	Yes
CRF-9	Nit	Open	PR description	Verification section with pasted go test output; PR style guide excludes test plans	R1	Leorio	Yes
CRF-10	Note	Note (body)	inboxnotifications_test.go:100	Watch refusal test sends a plain GET (base returns 426), not a websocket handshake	R1	Netero	No
CRF-11	Note	Note (body)	inboxnotifications.go:408	allow_list naming one inbox_notification ID yields a token every inbox endpoint refuses	R1	Mafuuu	No
CRF-12	Note	Note (body)	scopes_catalog.go:50	Scope names become persisted in tokens and OAuth metadata once released	R1	Ryosuke	No
CRF-13	OOS	Out of scope (body)	inboxnotifications.go:340	Invalid or foreign starting_before cursor silently returns the first page	R1	Kurapika, Ryosuke, Kite, Razor	No
CRF-14	OOS	Out of scope (body)	inboxnotifications.go:214	SubscribeWithErr failure returns an empty 200	R1	Mafuuu, Leorio, Hisoka	No
CRF-15	OOS	Out of scope (body)	inboxnotifications.go:249	Count error in watch loop closes the socket as a normal close	R1	Zoro	No
CRF-16	OOS	Out of scope (body)	notifications.go:255	Other user notification handlers lack pre-checks; scoped refusals return 500	R1	Mafuuu	No
CRF-17	OOS	Out of scope (body)	httpapi.go:170	ResourceForbiddenResponse never names a missing scope (85 call sites)	R1	Leorio	No
CRF-18	OOS	Out of scope (body)	inboxnotifications.go:152	read_status validation error names starting_before (also :328)	R1	Leorio	No
CRF-19	Note	Dropped by orchestrator (merged into CRF-1; same fix)	inboxnotifications.go:427	Cross-user or nonexistent ID on read-status returns 500	R1	Ryosuke, Mafuuu, Kite, Razor, Hisoka, Zoro, Bisky, Netero	No
CRF-20	Note	Dropped by orchestrator (Storybook CI failure, PR changes no site/ files; log unreadable, 401)	CI	Storybook job failure	R1	Netero, Kite, Razor, Hisoka	No

Contested and acknowledged

Round log

Round 1

Netero: no findings, two notes. Full panel (round 1, multi-domain auth surface): ging-go, kurapika, ryosuke, pariston, mafuuu, bisky, gon, leorio, kite, razor, hisoka, wildcard zoro. 2 P3, 2 P4 inline, 3 Nit inline, 2 body items (CRF-7, CRF-9, no file location), 3 Notes, 6 OOS. Leorio's CRF-6 downgraded P3 to P4: keep-argument is that the PR's goal is an actionable refusal and the body misdirects users to an admin; downgrade reason is that the 403 status itself is the actionable signal and the message is the shared httpapi.Forbidden convention whose root fix is OOS (CRF-17). Kite's note that cross-user IDs stay refused is contradicted for owner tokens by Kurapika's reproduction; empirical result wins. Reviewed against cc73f32..35684e5.

About deep-review

CRF = Coder Review Finding (P0-P4, Nit, Note)

Reviewer	Focus
Bisky	tests
Chopper	ops/errors
Churn-guard	change verification
Ging	language modernization
Gon	naming
Hisoka	edge cases
Killua	perf
Kite	change integrity
Knov	contracts
Knuckle	SQL
Komugi	flake/determinism
Kurapika	security
Law	decomposition
Leorio	docs
Luffy	product
Mafu-san	process
Mafuuu	contracts
Melody	dispatch/pairing
Meruem	structural
Nami	frontend
Netero	mechanical checks
Pariston	premise testing
Pen-botter	product gaps
Razor	verification
Robin	duplication
Ryosuke	Go arch
Takumi	concurrency
Zoro	shape

🤖 Managed by Coder Agents.

[coder-agents-review]

This PR adds inbox_notification:read and inbox_notification:update to the public scope catalog and adds handler pre-checks so the four inbox endpoints return 403 to under-scoped tokens. Findings: 2 P3, 2 P4, 3 Nit inline, plus a P4 and a Nit on the PR text.

The failing Storybook job does not touch this diff (no files under site/ change, and typesGenerated.ts already listed both scopes). The job log was not readable from the review environment, so the cause is unverified.

Out of scope (needs a ticket or explicit acceptance by a human):

• coderd/inboxnotifications.go:340: an unknown or unreadable starting_before ID is ignored and the first page is returned, so a client paginating with that cursor gets page one again.
• coderd/inboxnotifications.go:214: when SubscribeWithErr fails, the watch handler returns without writing a response, so the client gets an empty 200.
• coderd/inboxnotifications.go:249: any CountUnreadInboxNotificationsByUserID error in the watch loop closes the socket as a normal close, so a DB failure looks like a clean shutdown.
• coderd/notifications.go:255: other user-owned notification handlers have no pre-check, so an under-scoped token gets a 500 there.
• coderd/httpapi/httpapi.go:170: ResourceForbiddenResponse (85 call sites) never names a missing scope, so every scoped-token refusal tells the user to contact an administrator.
• coderd/inboxnotifications.go:152: the read_status validation error names starting_before (same text at :328).

Notes:

• coderd/inboxnotifications_test.go:100: NOK - missing scope sends a plain GET; at the base SHA it returns 426, not the 1000 close a real websocket client sees. A websocket.Dial asserting 403 would cover the client's path.
• coderd/inboxnotifications.go:408: a token whose allow list names one inbox_notification:<uuid> is accepted at creation but refused by every inbox endpoint, because each check authorizes an object with no ID. The same was true before this PR via the unread count.
• coderd/rbac/scopes_catalog.go:50: once released, these names are stored in tokens and published in OAuth2 scopes_supported, so they cannot be renamed or narrowed without breaking existing tokens.

---

In reply to IC_kwDOGkVX1s8AAAABYbckcA:

P4 [CRF-7] The commit subject and PR title say "read", but the PR also makes inbox_notification:update public and opens both mark-as-read endpoints to scoped tokens. Suggested: fix: let scoped tokens list and mark inbox notifications read. (Leorio)

🤖

In reply to IC_kwDOGkVX1s8AAAABYbckcA:

Nit [CRF-9] The PR description's Verification section pastes go test output, which .claude/docs/PR_STYLE_GUIDE.md lists under "Never Include". Keep the sentence "Its four refusal cases fail without the handler change", which CI does not show, and drop the rest. (Leorio)

🤖

🤖 This review was automatically generated with Coder Agents.