Conversation
|
/coder-agents-review |
|
Chat: Review in progress (17/17 reviewers complete) | View chat deep-review v0.9.0 | Round 1 | Last posted: Round 1, 7 findings (3 P3, 2 Nit, 2 Note), COMMENT. Review Finding inventoryFinding inventory, PR #29384Findings
Round logRound 1Netero-only pre-pass: P3 doc-comment displacement + Nit typo. Panel (16): bisky, hisoka, mafu-san, mafuuu, pariston, kurapika, knov, razor, chopper, kite, ging-go, komugi, gon, leorio, meruem, zoro. Production change verified sound: the three ErrNoSuchToken sites close the status-code existence oracle, all three covered distinctly by TestOAuth2ProviderRevokeInvalidToken (per-block coverage confirmed by multiple reviewers). No P0-P2 confirmed. 3 P3, 2 Nit posted inline, 1 Nit in body, 2 Notes. Reviewed against 800629c..fc314ae. 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.
The production change is small and does what it claims: the three in-tx validation failures (wrong refresh secret, wrong API-key secret, valid non-OAuth2 API key) that previously fell through to a 500 now return ErrNoSuchToken -> 200, matching the unknown-token and cross-app paths. That closes the status-code existence oracle described in the PR body, per RFC 7009 §2.2, and revokes nothing on any refused path. Multiple reviewers ran TestOAuth2ProviderRevokeInvalidToken with per-block coverage and confirmed all three new returns (revoke.go:151, :191, :196) and the handler's 200 branch each execute, with the staysValid case proving a real non-OAuth2 session survives the refused revoke. The PR description is accurate against the code and was singled out for praise.
No P0-P2 confirmed. Findings: 3 P3 (a displaced doc comment, the sentinel's misleading name, and an inaccurate debug log), 2 Nits, and 2 Notes.
The doc-comment displacement (CRF-1) was independently flagged by nine reviewers; Leorio rated it P2, but the consequence is documentation-only in a test file, so I settled it at P3. The naming and log findings (CRF-2, CRF-3) are the same underlying fact from two angles: ErrNoSuchToken is returned only when a token record IS found but fails validation (the genuinely-absent sql.ErrNoRows paths return nil), and for the non-OAuth2 case a token exists and its secret matched, so both the identifier and the log line "no token matches the one presented" misdescribe that path.
Process: the commit subject is 73 characters, one over the 72-char limit (Leorio). Trimming a word keeps the scenario intact.
Hisoka, after trying to break the endpoint: "I came to fight the revocation endpoint and it held its ground."
🤖 This review was automatically generated with Coder Agents.
BobbyHo
changed the title
5923f7e to
ef17afe
Compare
…voke - A wrong token secret, or a valid API key that is not an OAuth2 token, returned 500. That told the caller the token existed. - These cases now return 200, as RFC 7009 asks. A malformed token still returns 400, and a database failure still returns 500. - Add tests for a wrong refresh secret, a wrong access token secret, and a session token from another login.
ef17afe to
f7116aa
Compare
1 participant
POST /oauth2/revokereturned 500 when the token's secret did not match a stored token, and when the token was a valid API key that OAuth2 did not issue. An unknown token returned 200. The difference told the caller which tokens exist.Found in review of #29208 and deferred from it.