Repository navigation
Conversation
36cea00 to
b799b1a
Compare
b799b1a to
329435a
Compare
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.13.0 | Round 3 | Last posted: Round 3, 65 findings (2 P1, 8 P2, 19 P3, 24 P4, 12 Nit), COMMENT. Review Finding inventoryFinding inventory: PR #1128Findings
Contested and acknowledgedCRF-7 (P2, test/scopes/oauthScopes.test.ts:15) - no leave-one-out test
CRF-8 (P2, src/oauth/constants.ts:14) - no probe needs user:read_personal
CRF-24 (P3, src/oauth/constants.ts:11) - workspace:create only for the dry-run
CRF-44 (P3, src/oauth/utils.ts:62) - pre-upgrade sessions lack the optional inbox scope
CRF-49 (P3, test/scopes/deployment.ts:186) - awaitFollowUpBuild can hang
CRF-51 (P4, test/scopes/oauthScopes.test.ts:26) - grant test requires the optional scope
Law analysis
Round logRound 1Full panel (round 1). Netero: 2 P2, 1 Nit, no P0, so panel proceeded. Law not run (729 effective additions). Panel: ging-ts, kurapika (auth, OAuth tokens), pariston, mafuuu, bisky, komugi, hisoka, gon, leorio, mafu-san, knov, wildcard kite. Multi-domain (auth scopes plus CI and test infra), size 12. Result: 2 P1, 9 P2, 11 P3, 6 P4, 8 Nit; 7 out of scope; 1 body note. Gon's comment-restatement P2s consolidated into one Nit (CRF-28): keep-argument was that redundant comments drift, but they are accurate today and cause no behavior risk. Gon naming P3s downgraded to Nit per the Nit definition. CRF-25 downgraded from P2 to P3: keep-argument was that it misleads contributors, but the metatest enforces the real contract. Reviewed against 7ff5019..329435a. Round 2Churn guard PROCEED: 31 addressed, 3 contested (CRF-7, CRF-8, CRF-24). Classified as a restructure round (files added to the PR; new production code in authorizer.ts, sessionManager.ts, utils.ts). Law ran (1178 effective additions, never run before): Split, Mandatory. Per the Law decision gate the panel was skipped, so the contested findings and the round-2 auth code were not panel-reviewed. Netero: 1 P3, 1 P4 new, both verified against the code; Netero also verified fixes for CRF-1, CRF-2, CRF-3, CRF-23 and CRF-36/37/38 with mutation checks. Round 1 post receipt was written by hand after the CLI got a 502 from GitHub although the review was created (review 5391464079). Reviewed against f852fdd..427ac73. Round 3PR split after round 2: #1128 now holds only the live suite, metatest, CI and docs, stacked on #1138 (c9eff9e). Churn guard PROCEED: 17 addressed, 3 contested. Restructure round (goal changed, src/oauth files left the diff). Law not rerun (effective additions 1123, down from 1178). Netero (reused agent): 2 P4 new; accepted the CRF-44 defense (1/1), which is now in #1138. Full panel: ging-ts, pariston, mafuuu, bisky, gon, leorio, komugi, wildcard kite (size 8), plus hisoka for the disputes. Hisoka, Leorio and Bisky reused their prior agents. CRF-49 closed 1/1, CRF-51 closed 2/2, CRF-7 fix verified live by Bisky. Several reviewers ran the live suite: 39/39 passed. New: 1 P3, 9 P4, 1 Nit; CRF-83 dropped; 1 out of scope. Gon's P2 on the promoteNewTemplateVersion doc was merged with the versionId doc P3 into CRF-78 at P3: the keep-argument was that the hidden side effect can mislead a later probe, but no current probe relies on the stale value (Hisoka checked). Gon's TEMPLATE comment P3 was downgraded to P4 (CRF-79): the keep-argument was that the agent attachment is load-bearing, but Gon's reason for it is unverified. Reviewed against c9eff9e..d0009b8. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR replaces the OAuth scope list with the two coder:workspaces.* composites plus workspace:create, user:read and user:read_personal, and adds a live-server scope suite and a source-scan metatest. Findings: 2 P1, 9 P2, 11 P3, 6 P4, 8 Nit.
Out of scope (needs a ticket or explicit acceptance by a human):
- coder/coder
GET /organizations/{org}/members/{user}: returns 404 for scoped tokens even for the caller's own membership (500 withorganization:read), socoder startandcoder updateon another user's workspace fail for every scoped token. src/oauth/sessionManager.ts:482:revokeTokensreads tokens through the scope check, so logging out with a pre-upgrade session leaves its refresh token valid on the server. This PR makes every stored session hit that path.src/oauth/sessionManager.ts:102: thegetStoredTokensdoc says invalid tokens are cleared; the function returnsundefinedand leaves them in storage.src/oauth/sessionManager.ts:264:hasRequiredScopesacceptscoder:*as a wildcard but notcoder:all, so a session grantedcoder:allis rejected.src/inbox.ts:44: on scope-enforcing servers the inbox socket closes on the first notification and reconnects, so out-of-disk and out-of-memory notifications are never shown (server side: coder/coder#30176).- coder/coder
/users/meandscopes_catalog.go: reading your own user needsuser:read, which reads every visible user, andorganization_member:readis only grantable through composites. - coder/coder
templateVersionExternalAuth:coder starton a template with external auth reads external auth links, which needuser:read_personal; no scoped-token test in either repository covers it.
Notes:
src/oauth/constants.ts:8: the server defines what the two composites contain, so the extension's grant widens whenever the server adds actions to them, and the suite only detects missing permissions.
CHANGELOG.md:7
P3 [CRF-10] The PR widens the consent screen (workspace:create, user:read) and forces existing OAuth users to sign in again, but adds no CHANGELOG entry. (Leorio)
Recent fix PRs (#1129, #1115, #1110, #1092) each add one. Add an Unreleased "Fixed" entry that names both user-visible effects.
🤖
🤖 This review was automatically generated with Coder Agents.
d5e8ad0 to
427ac73
Compare
|
CRF-10: added an Unreleased "Fixed" entry covering the wider consent screen and the one-time re-authentication. Out of scope, fixed here anyway: logout now revokes tokens with outdated scopes, |
|
/coder-agents-review |
There was a problem hiding this comment.
The new commit addresses 31 of the 34 round 1 findings, and it also fixes revocation for outdated sessions, accepts coder:all, and requests inbox_notification:read when the server offers it. This round: the request to split the PR, 1 P2, 8 P3, 9 P4 and 3 Nit, of which 1 P3 re-raises a disputed finding. Of the three disputed findings, the user:read_personal and workspace:create defenses are accepted, and the leave-one-out check is re-raised at P3 in its thread.
Please split the PR. The diff is now +1178 -70 across 20 files, and most of the roughly 126 lines of new src/oauth code arrived this round beside about 500 lines of harness rewrite:
fix(oauth): request the scopes the extension and CLI actually need: everysrc/oauthchange (scope list,OPTIONAL_OAUTH_SCOPES, revocation throughreadStoredTokens,coder:alland thehasRequiredScopesmove) with its unit tests and the CHANGELOG lines.test(oauth): check OAuth scopes against a live server:test/scopes/,scopeProbes.test.ts, the vitest project, the--project !scopesexclusions, the workflow, thepnpm-workspace.yamlcomment, CONTRIBUTING.md and AGENTS.md. It imports PR 1's exports and merges after it.
The revocation change must not merge after the scope list change, or logging out with a pre-upgrade session skips/oauth2/revoke.
Out of scope (needs a ticket or explicit acceptance by a human):
- coder/coder
GET /organizations/{org}/members/{user}: returns 404 for scoped tokens, socoder starton another user's workspace fails for every scoped token (tracked here as thecoder start (shared)known gap). src/api/workspace.ts:82:runCliCommandspawns the CLI with the extension host's full environment, so aCODER_SESSION_TOKENthere overrides the stored session, andcoder start/coder updatefail with 401 or run as another user.- OAuth on 2.38 with dynamic client registration off (the default): discovery still succeeds, so the extension offers OAuth and sign-in then fails at registration. The PR description calls this a follow-up without a ticket.
src/oauth/authorizer.ts:216: the warning "Server may still accept them" is wrong on scope-enforcing servers, which reject the whole authorize request over an unknown name.- coder/coder
coderd/workspaces.go:1801: a member a workspace is shared with under theuserole lacks workspaceupdate, socoder sshusage posts on shared workspaces fail whatever the token's scopes. src/oauth/constants.ts:6:DEFAULT_OAUTH_SCOPESis the required list thathasRequiredScopesenforces, but "default" reads as overridable.src/oauth/sessionManager.ts:44: theStoredTokensdoc says it is used bygetStoredTokens;readStoredTokensbuilds it now.
src/oauth/utils.ts:101
P3 [CRF-48] The forced sign-in on servers that ignore scopes is avoidable: there the stored scope is the extension's own request list, while the server granted coder:all. (Pariston)
coder/coder release/2.37 records every authorization code as coder:all (authorize.go:278) and returns no scope, so this line stores the old DEFAULT_OAUTH_SCOPES, which the new hasRequiredScopes rejects (verified), and the user signs in again to get a token with the same permissions. A stored session with the old list cannot come from a scope-enforcing server, because the old extension's login fails at GET /users/me there, and release/2.38 returns scope on every exchange and refresh. Store a missing server scope as unrestricted instead of the request list, and accept the exact legacy list in getStoredTokens, so neither this change nor later edits to DEFAULT_OAUTH_SCOPES sign out every user of a server that ignores scopes.
🤖
🤖 This review was automatically generated with Coder Agents.
427ac73 to
d0009b8
Compare
EhabY
changed the title
|
Split as suggested. #1138 has every CRF-48: partly done. A missing server Out of scope: fixed the |
|
Follow-ups filed:
|
|
/coder-agents-review |
There was a problem hiding this comment.
After the split, this PR holds only the live scope suite, the metatest, CI and docs, stacked on #1138. Every round 2 finding is addressed or settled. The three disputed ones are accepted by the reviewers who raised them: the hang in awaitFollowUpBuild, the grant test on servers without the optional scope, and pre-upgrade sessions lacking the inbox scope (now in #1138). Reviewers ran the suite against the pinned image several times, and all 39 tests passed each time. New this round: 1 P3, 9 P4 and 1 Nit, none of which changes what the suite checks except the two leave-one-out and grant-test assertions.
Out of scope (needs a ticket or explicit acceptance by a human):
src/websocket/reconnectingWebSocket.ts:379:connectadds itsmessagelistener only afterawait this.#socketFactory()resolves, so a message the server sends right after the handshake is dropped (20 of 20 in awsreproduction). The agent metadata watcher may then show nothing until the next metadata message after each connect.
🤖 This review was automatically generated with Coder Agents.
37af2eb to
e3334c4
Compare
|
Out of scope (round 3): filed #1141 for the dropped first message. A plain |
e3334c4 to
6e5a414
Compare
6e5a414 to
ed8e384
Compare
Add `pnpm test:scopes`, which signs in through the extension's OAuth flow against a throwaway deployment (test/scopes/compose.yaml) and runs a probe for every Coder API method and CLI request the extension makes with the granted token. It also checks that each requested scope is needed by a named probe, and asserts the exact error of known gaps. `scopeProbes.test.ts` runs on every PR and uses the TypeScript checker to find every CoderApi method src/ references, failing when one has no probe. The workflow runs the suite on relevant changes against a pinned coder-preview image, and nightly against `latest`. `pnpm test` and the CI unit job leave it out.
ed8e384 to
c92c0ec
Compare
This branch has not been deployed
1 participant
Part of VSC-24. Stacked on #1138, which changes the scopes the extension requests. This PR checks them against a live server.
Why
Released servers ignore requested scopes, so unit tests can't show the extension requests enough. Missing scopes show up only against a server that enforces them, and sometimes only as an empty list or a closed socket. This suite runs every request the extension makes against such a server, using a real OAuth token.
Size
+1204/−47 in 15 files:
test/scopes/: +940 (probes, deployment setup, test, compose file).test/unit/oauth/scopeProbes.test.ts: +141.--project !scopesexclusions, CONTRIBUTING.md, AGENTS.md).src/oauth: +42/−35.hasRequiredScopesmoves unchanged from a private method insessionManager.tstoutils.ts, so the suite can check the granted scopes with it.constants.tsgains a pointer toscopeConsumersfor why each scope is requested.Tests
test/unit/oauth/scopeProbes.test.ts(every PR, ~2s): uses the TypeScript checker to find everyCoderApirequest or stream method thatsrc/references, including through destructuring,Pick<CoderApi, …>and structural interfaces. It fails if a method has no probe, or if a probe or exemption is stale.test/scopes/(pnpm test:scopes, ~85s):hasRequiredScopesaccepts them.coder startand the one-build update run through the extension'sstartWorkspaceandupdateWorkspace, using the CLI the server serves.coder startruns on an outdated workspace with automatic updates on, so the CLI dry-runs first.scopeConsumersnames, for each requested scope, a probe that fails without it and the error it fails with. Each of those probes is run with a token missing that scope, so a scope nothing needs can't be added unnoticed..github/workflows/oauth-scopes.yaml: runs when code the probes run or import changes, against a coder-preview image pinned by digest, plus nightly againstlatest.pnpm testand the CI unit job leave the suite out.Each scope #1138 requests, and a probe that fails without it:
getAuthenticatedUserGET /users/me: 404user:readgetWorkspaceByOwnerAndNameGET /users/member/workspace/own: 404user:readgetWorkspaceByOwnerAndName (shared)GET /users/admin/workspace/shared: 404organization_member:read)stopWorkspacePOST …/builds: 403 You do not have permission to stop this workspacecoder:workspaces.operatestartWorkspacePOST …/builds: 400 Failed to fetch workspace owneruser:readcoder startworkspace:createcoder ssh coordinateGET …/coordinate: 404coder:workspaces.accesswatchInboxNotificationsGET /notifications/inbox/watch: 403inbox_notification:readKnown gaps
coder starton a shared workspace fails withget owning member: Resource not found. The CLI looks up the owner's org membership, which no scope grants, then the owner, whichuser:readcovers only for yourself. This needs a server-side fix: Shared workspaces: scoped tokens can't runcoder start, anduse-role users can't report usage coder#30423.user:read_personal.coder startreads external auth links with it, but only once a user has linked a provider, which the test deployment can't do.🤖 Generated with Claude Code