Repository navigation
feat(api)!: send route_not_found and authentication_unavailable - #672
Conversation
Two of upstream's specific codes needed a status or shape change: - An /api/ path that no route matches answers 404 route_not_found as a problem body, and a path with routes for other methods answers 405 method_not_allowed with Allow, instead of ServeMux's plain text. - A key store failure while verifying a token answers 403 authentication_unavailable instead of 401 invalid token. The verifier now returns a KeySourceError and refuses an iss with a control character before the key lookup. repo_claim_missing stays unsent: since #666 a token without a repo claim is org-wide, and a per-repo route refuses it with 403, as it refuses repo "org". _Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high_ Change-Id: Id04ba9ebfd98048895dff2aca9070fbeb5b36120 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
Validation evidence for this head (
Dogfood ran on |
|
@codex review |
|
@codex security review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8e05ad483
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var keysErr *auth.KeySourceError | ||
| if errors.As(err, &keysErr) && !errors.Is(err, meta.ErrNotFound) { |
There was a problem hiding this comment.
Map key-source failures on every token surface
When the key store starts failing after the outer create-repository authorization succeeds, forkAwareCreateRepo calls Verifier.Verify directly in internal/api/handlers_fork.go and still converts the resulting KeySourceError to a generic 403; Git and LFS authentication likewise converts it to 401 in internal/githttp/server.go. Consequently the new authentication_unavailable retry signal applies only to REST bearer middleware even though docs/reference/authentication.md includes fork-source and Git transports in the same failure map. Route these direct verification consumers through the same key-source-error classification.
Useful? React with 👍 / 👎.
…entication_unavailable Codex round 1 on #672: forkAwareCreateRepo verified base_repo.auth.token itself and answered every failure 403 forbidden. A key store failure there is now 403 authentication_unavailable, as in requireAuth, through one keyStoreFailed helper. The failure map says the Git and LFS transports keep 401: they send no problem bodies. Change-Id: I7d39a65ad8a370f0f2ebee4eb7416cdbf7d51dac Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
Codex round 1 had one P2 finding (medium severity): the key store mapping covered only the REST bearer middleware. I fixed it in f35e2ad, a new commit (no force-push):
|
|
@codex review |
|
@codex security review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f35e2ad02f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if strings.ContainsFunc(unverified.Iss, unicode.IsControl) { | ||
| return nil, errors.New("jwt iss claim has a control character") |
There was a problem hiding this comment.
Reject control-character orgs at configuration time instead
When OPENSTORAGE_ORG contains a control character other than PostgreSQL's problematic NUL, the server currently accepts and persists that value—resolveServeConfig passes it through verbatim and the metadata backends impose no such restriction—but this new check rejects every correctly signed token for the configured issuer before lookup. Because the verifier is shared by REST, Git, and LFS, such a deployment boots successfully yet cannot authenticate any request; either validate the configured org at startup or narrow this token-side rejection to values that can actually make the lookup fail.
Useful? React with 👍 / 👎.
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Codex round 2 on #672: refusing every control character would lock out a configured org that holds one. Only a NUL fails the PostgreSQL lookup, and no configured org can hold one. Change-Id: I16997fe2a074968d1d81345a1ad70202027096af Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
Codex round 2 had one P2 finding (medium severity): refusing every control character in |
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
open.storage now sends two more of upstream's specific error codes. Each needed a status or shape change, which is why the earlier #352 PRs left them out. This is the rest of item 1 of #352, and the last PR for that issue.
What changes
/api/path that no route matchestext/plain"404 page not found" (ServeMux)route_not_found/api/path that has routes, but not for this methodtext/plain, withAllowmethod_not_allowed, with the sameAllowunauthorized"invalid token"authentication_unavailable"cannot verify the token now, retry"base_repo.auth.tokenforbidden"base_repo.auth.token: invalid token"authentication_unavailableHow:
unmatchedAPIRoute(api.go) is registered for/api/. Registering it stops ServeMux from writing its own 405s, so it asks the mux which pattern each other method would match and buildsAllowfrom that.auth.OrgKeyVerifier.Verifyreturns a*KeySourceErrorwhen theKeySourcefails. The middleware answers 403authentication_unavailableunless the error ismeta.ErrNotFound(an unknown issuer stays 401). Verify now refuses anisswith a NUL before the key lookup, because a NUL fails the PostgreSQL query, so a craftedisscannot turn into "retry". Such tokens were 401 before and stay 401. Other control characters still reach the key lookup, because a configured org may hold them (Codex round 2).Codex round 1: the fork handler verifies
base_repo.auth.tokenitself, so it now uses the same check (keyStoreFailed). The Git and LFS transports send no problem bodies, so they keep 401, andauthentication.mdsays so.http-api.md(Errors, status table),authentication.md(failure map), and the RFC 9457 row ofwire-compatibility.md.Upstream evidence: the vendored spec's "Specific codes" list. The live server answers
GET /api/nopewith 404route_not_foundandDELETE /api/metawith 405method_not_allowedandAllow: GET, HEAD, bothapplication/problem+json.Codes that stay unsent, with the reason
repo_claim_missing(400): since fix(auth): accept any non-empty sub and treat a token without repo as org-wide #666, a token without arepoclaim is org-wide (the desk decision on Accept the tokens that SDK 0.1.0 signs: SDK sub claims and org-wide tokens without a repo claim #658). A per-repo route refuses it with 403forbidden, as it refusesrepo: "org", andauthentication.mddocuments 403 for an empty or nullrepotoo. No token is left for which "the token has norepoclaim" holds. Earlier drafts of this PR sent the 400. I dropped it when fix(auth): accept any non-empty sub and treat a token without repo as org-wide #666 landed.merge_conflict(409): upstream documents the merge file-conflict 409 asMergeConflict, which has nocodemember. open.storage sends that body since the previous PR.sync_in_progress,upstream_already_configured(409): no upstream operation documents them, and no open.storage answer matches them.repository_thawing,github_*,hosting_*,deployment_*,idempotency_key_reused: features open.storage does not have.While checking item 1 I found that upstream answers an invalid or expired token with 403
forbidden, not 401. The vendored spec does not say this, so I filed it as #664 instead of changing it here.Validation
go build ./...,golangci-lint run(2.15.0): 0 issues. Docs gate: 0 broken links.go test ./internal/api ./internal/auth ./cmd/...pass. New tests:TestUnmatchedAPIRoutes(both lanes),TestAuthenticationUnavailable,TestVerify_KeySourceError.Dogfood
I used the verify-openstorage harness on a node built from the item 3 part 3 head (
0244c633) and on this change (4343a41a; the head adds only a comment and a doc note from part 3). Each run passed 8 checks. The token without arepoclaim was signed with PyJWT and the run's org key, becauseopenstorage mintrequires--repo.GET /api/nope,GET /api/repos/demo/nopetext/plainroute_not_foundDELETE /api/metatext/plain,Allow: GET, HEADmethod_not_allowed,Allow: GET, HEADGET /api/v1/repos/commit-packtext/plain,Allow: POSTmethod_not_allowed,Allow: POSTGET /api/v1/repowith norepoclaimforbiddenforbidden(unchanged, seerepo_claim_missingabove)GET /api/v1/repowith a valid token, and with noneSDK 0.1.0 Go:
repos.listsucceeds on both nodes, because #662 servesGET /api/repos. No SDK 0.1.0 call I tried reaches an unmatched route. I could not produce a key store failure on a real node, so the unit test coversauthentication_unavailable.Fixes #352
BREAKING CHANGE: a key store failure during REST token verification answers 403
authentication_unavailableinstead of 401 (or instead of 403forbiddenfor a fork source token; the Git and LFS transports keep 401; during a key store outage a caller with any well-formed token sees this 403 instead of 401), and unmatched/api/paths answer problem bodies (404route_not_found, or 405method_not_allowedwithAllow) instead of ServeMux's plain text.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high