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
65b23e2 to
af7f02b
Compare
Add `pnpm test:scopes`, which signs in and refreshes the token through the extension's OAuth flow against a throwaway deployment (test/scopes/compose.yaml). It checks that login and refresh both grant exactly the scopes the extension requests from that server, then runs a probe for every Coder API method the extension calls and for the requests `coder ssh` and `coder start` make, using the refreshed token. `scopeProbes.test.ts` runs in `pnpm test` and uses the TypeScript checker to find every CoderApi method src/ references, failing when one has no probe or listed reason. The workflow runs the suite on relevant PRs against the v2.38.0 release, the first that enforces requested scopes, and nightly against `coder-preview:latest`. A failed nightly run comments on an open tracking issue, or opens one.
af7f02b to
e49159f
Compare
This branch has not been deployed
1 participant
Part of VSC-24. Follows #1138, which changed 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 that the extension requests enough. Missing scopes show up only against a server that enforces them, and sometimes only as an empty list, a closed socket, or a CLI warning. This suite runs every request the extension makes against such a server, using a real OAuth token.
What catches what
pnpm test, every PR, about 1.5 seconds:test/unit/oauth/scopeProbes.test.tsuses the TypeScript checker to find everyCoderApirequest or stream method thatsrc/references, including through destructuring and types likePick<CoderApi, ...>. It fails when a method has no probe and no listed reason inunprobedMethods, or when a probe or reason is stale.pnpm test:scopes, on PRs that touch the code the probes run, about 40 seconds:OAuthSessionManager. Login and refresh must both grant exactly the scopes the extension requests from that server.coder sshsends the agent lookup, the usage report, and the coordination socket, for an owned and a shared workspace. The CLI only logs a failed usage report, so a gap there would let a workspace autostop while in use.coder startrun through the extension'supdateWorkspaceandstartWorkspacewith the CLI the server serves.coder startruns on an outdated workspace with automatic updates on, so the CLI dry-runs first.coder-preview:latest: the same suite catches server changes before a release. A failed run comments on an open "Nightly OAuth scope test failed" issue, or opens one.PRs use the v2.38.0 release, pinned by tag and digest, so a PR's result doesn't change from one day to the next and the image never disappears. It's the first release that enforces requested scopes: v2.37.1 returns no
scopeand grants everything, so every probe passes against it. v2.38.0 doesn't offerinbox_notification:read, so the extension leaves it out, and the suite expects only the optional scopes the server lists inscopes_supported.pnpm testand the CI unit job leave the live suite out.Not covered
src/oauth/constants.tsfor that.coder starton a shared workspace fails withget owning member: Resource not found, which needs a server-side fix: Shared workspaces: scoped tokens can't runcoder start, anduse-role users can't report usage coder#30423.startWorkspaceandstopWorkspaceare listed inunprobedMethods. Only the tasks panel and CLIs older than 2.24 use them, and servers that enforce scopes update in one build.The API types stay on v2.37.1: v2.38.0 removes the Tasks API from the SDK client, so moving the types is its own change.
Size
+1022/-15 in 14 files. Most of it is
test/scopes/(setup, probes, and test).src/changes only by exportingrawDataToString, so the probes decode socket messages the way the extension does.