Conversation
- POST /oauth2/tokens, /oauth2/revoke, and /oauth2/register had no rate limit - They now share the login limit of 60 requests per minute per IP address - The limiter runs before the app lookup so refused requests never reach the database - DELETE /oauth2/tokens is authenticated and keeps its own per-user bucket - Add a test for each endpoint and a note in the provider docs
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
|
/coder-agents-review |
|
Chat: Review in progress (2/2 reviewers complete) | View chat deep-review v0.9.0 | Round 2 | Last posted: Round 2, 12 findings (2 P2, 3 P3, 3 Nit, 4 Note), COMMENT. Review Finding inventoryFinding inventoryPR #29510. Coder review findings (CRF). Findings
Contested and acknowledgedNone. Law analysisNot run. Effective additions 125 (R1) and 212 (R2), below the 1000 threshold. Round logRound 1Netero-only first pass (P2 present, panel gated). 1 P2, 3 P3, 2 Note, 1 P4. Round 2Churn guard: PROCEED. All 7 prior findings classified addressed (author claims, About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
First-pass review only. These are mechanical findings from a single reviewer; the full review panel has not looked at this PR yet and will review once these are addressed.
The wiring is right where it is applied: the limiter sits before the app lookup on both /tokens POST and /revoke, so a refused request never reaches the database, and DELETE /tokens keeps its own middleware group. Reusing LoginRateLimit instead of adding a flag is the correct call, and the test helpers (doRequest, formContentType, sessionToken) are reused rather than redefined. Three of the four new subtests fail against the base commit, so they test the change.
1 P2, 3 P3, 2 Notes, 1 P4.
The P2 is the one to look at first: the same defect this PR closes is still open one route over. From the reviewer: "This is the same defect the PR set out to close, one route over."
P4 [CRF-7] coderd/database/migrations/000596_aibridge_workspace_context.up.sql: two migrations share version 000596 on origin/main (000596_aibridge_workspace_context.* from #29235 and 000596_template_usage_stats_session_usage.* from #29109). Every Postgres-backed test fails with failed to init driver with path .: duplicate migration file before it starts; renumbering the aibridge pair in a scratch worktree was required to run the new test at all. Present at the base commit, so not this PR's defect and not this PR's to fix, but it needs a ticket or a fix on main.
🤖 This review was automatically generated with Coder Agents.
- The per-endpoint limiter missed GET, PUT, and DELETE
/oauth2/clients/{client_id}. They take a bearer token from a caller
with no API key, and the middleware hits the database before it
checks that token.
- Mount the limiter on the tree so a new route is covered by default.
- Key the bucket on a fixed name, not the path. The client ID is in
the path, so a caller could vary it to get a fresh bucket.
- /authorize and DELETE /tokens are now counted too. They key on the
user.
- The docs say all /oauth2 endpoints draw on one limit, but no test checked that across two endpoints. - Spend the budget on /register, then assert /revoke returns 429 instead of its usual 401.
- A refused request under /oauth2 returned {"message": ...}, which a
client decoding OAuth2 errors cannot read. It now returns
temporarily_unavailable, the closest code RFC 6749 offers for
throttling.
- Name the flag that disables the limit in the docs, and say the 60 is
fixed rather than configurable.
|
/coder-agents-review |
There was a problem hiding this comment.
Second first-pass round. These are mechanical findings from a single reviewer; the full panel still has not reviewed this PR. It reviews next round regardless of what lands, since two first-pass rounds is the limit.
Every round 1 finding is fixed, and the fixes were verified rather than taken on trust: reverse-patching the two production files makes all six subtests fail, so the new tests hold the behavior down. Two of the fixes are better than what was asked for. RateLimitOAuth2 keying on a fixed name instead of the path is the detail that actually closes the RFC 7592 hole, since a caller cycling client IDs would have got a fresh bucket per request, and ClientConfigurationVaryingClientID pins it. Extracting rateLimitMessage and threading onLimited keeps the other two limiters byte-identical while the OAuth2 tree gets an RFC 6749 body.
New this round: 2 P2, 1 P3, 3 Nits, 3 Notes.
The headline P2 is a consequence of the broad fix. Mounting on the tree means chi runs the limiter before it decides whether a route matches, so /oauth2/<anything> spends the same bucket as the real endpoints. From the reviewer: "Before this PR that traffic could not deny service to anyone; the limiter is what makes it possible."
Two items need a decision from you rather than a code change from the author. CRF-9 (the discovery endpoint) is outside this diff but is the same defect class the PR closes, with the endpoint's own comment naming rate limiting as its mitigation: fix it here, file an issue, or say why it stays open. CRF-10 is a consequence the author named and accepted in a reply; an agent cannot accept a user-visible regression as permanent, so it needs a human call.
P2 [CRF-9] coderd/coderd.go:1242: the RFC 8414 discovery endpoint is still unauthenticated, unlimited, and runs a database query per request. (Netero)
/.well-known/oauth-authorization-serveris mounted on the root router with onlyRequireOAuth2Provider, and the rootr.Usechain contains no rate limiter;apiRateLimiteris applied per group, not at the root. Its handler queries the database on every request, and its own comment names rate limiting as the mitigation: "a flood of requests should be mitigated with rate limiting or firewalling, not a cache" (coderd/oauth2provider/metadata.go:19-24).
I confirmed the wiring at coderd/coderd.go:1241-1245 and the unconditional GetOAuth2DCREnabled call. The route also registers Get("/*"), so every path under the prefix reaches that query. /.well-known/oauth-protected-resource needs nothing; it touches no database. The PR's stated rationale is the unbounded database work that came with the missing control, and that work is still reachable here with no credential and no limit.
Note [CRF-16] coderd/httpmw/ratelimit_test.go: RateLimitOAuth2 has no unit test beside its two siblings, which both have one. (Netero)
TestRateLimitandTestRateLimitByAPICompatibilityEndpointlive incoderd/httpmw/ratelimit_test.go; the new exported function is exercised only through theoauth2providerintegration test. That test covers the 429 status, the RFC 6749 body, the shared bucket, and per-user keying, which is the whole observable contract, so a unit test would add location rather than coverage.
Recorded, not requested. Worth knowing that deleting or moving the oauth2provider test leaves the exported function uncovered.
Process note: make lint/go cannot run in a review worktree here. /tmp/coder-script-data/bin/golangci-lint is a shim that refuses to run and shadows the mise-installed binary on PATH, so the Makefile target invokes the shim. Running the pinned 2.0.2 binary directly reports 0 issues on both changed packages. Not a PR defect; noted so the clean-lint claim is checkable.
🤖 This review was automatically generated with Coder Agents.
The limiter was mounted on the /oauth2 subrouter, so requests to paths that match no route spent the budget too. Junk traffic from one address could refuse OAuth2 service to every caller behind that address, and all endpoints drew on the same budget. Mount the limiter on each route instead. Every endpoint now counts on its own, and a path that reaches no handler counts for nothing. Extend it to the two .well-known discovery endpoints, which carry no credential and so are limited per address.
|
/coder-agents-review re-review the P2 and P3 reported in the last review. Only report if there are more P0 to P3. |
|
/coder-agents-review re-review the P2 and P3 reported in the last review. Only report if there are more P0 to P3. |
The endpoints under
/oauth2had no rate limit. The whole tree is mounted outside the API rate limiter groups, so an unauthenticated caller could send unlimited requests. RFC 6749 section 2.3.1 and OAuth 2.1 section 2.4.1 require the server to protect any endpoint that takes a client secret against brute force. This closes finding D6-01 in the spec compliance matrix.The login rate limit now applies to
/authorize,/tokens,/revoke,/register, the RFC 7592 client configuration routes, and the two/.well-knowndiscovery endpoints. Requests with no Coder session are counted per IP address and the rest are counted per user. An Owner can bypass the limit on a single request withX-Coder-Bypass-Ratelimit, and the limit is off when--dangerous-disable-rate-limitsis set, same as login.The limiter is mounted on each route rather than on the
/oauth2tree. A tree-wide mount also counts requests to paths that match no route, which would let junk traffic from one address spend the budget that real OAuth2 traffic needs, and it would put every endpoint on a single budget. Each route now carries its own counter, so an endpoint drained by one caller leaves the others alone. The cost is that a route added under/oauth2later has to mount the limiter itself.The bucket keys on a fixed name rather than the request path, because two of these routes vary their path.
/oauth2/clients/{client_id}carries the client ID, and/.well-known/oauth-protected-resourceanswers on any suffix per RFC 9728. Keying by path would let a caller vary either one and get a fresh bucket on every request. The client configuration route also takes a bearer token from a caller with no API key and queries the database before it checks the token.Guessing a secret was already infeasible, since secrets are 40 random characters compared in constant time. The gap was the missing control on the endpoints and the unbounded database work that came with it.
🤖 Generated with Claude Code