feat: add POST /{domain}/auth endpoint for token handoff#224
Conversation
Elektra currently passes Keystone tokens via URL query parameters when
linking to the Maia dashboard. Tokens in URLs leak through browser
history, server logs, proxy caches, and Referrer headers.
Add a POST endpoint that accepts the token in the request body instead.
After the existing authorize middleware validates and sets the HttpOnly
cookie, the handler 303-redirects to /{domain}/graph. The old
?x-auth-token= path remains for backward compatibility but now logs a
deprecation notice.
- MaxBytesReader (16KB) prevents memory exhaustion via large POST bodies
- Method guard ensures PostFormValue only runs on POST requests
- observeDuration wrapper maintains metrics parity with other endpoints
notque
requested review from
Kuckkuck,
LeoZhangZXZ,
ashifnihalb,
ibakshay,
joluc,
notandy,
olandr,
richardtief,
timojohlo,
trouaux and
viennaa
as code owners
- Replace PostFormValue with explicit ParseForm + error handling to surface MaxBytesReader failures instead of silently swallowing them - Fix deprecation log format (was producing double-slash path) - Add debug logging in POST body path and tokenLogin handler - Add CHANGELOG.md with Added/Deprecated entries
- Only parse POST body when Content-Type is application/x-www-form-urlencoded (prevents body consumption for other POST endpoints, gives clear failure for wrong Content-Type) - Differentiate MaxBytesError (logged at Error) from generic parse failures (logged at Info) for operator visibility into potential abuse vs client bugs - Add debug log for invalid domain in tokenLogin (consistent with other handlers) - Add TestTokenLogin_bodyTooLarge: verifies >16KB body → 401 - Add TestTokenLogin_wrongContentType: verifies JSON body → 401
There was a problem hiding this comment.
Pull request overview
Adds a POST-based token handoff flow so clients can submit auth tokens in a request body, authenticate via existing middleware, receive the existing auth cookie, and redirect to the graph UI.
Changes:
- Adds
POST /{domain}/authroute and redirect handler. - Extends Keystone auth extraction to read
x-auth-tokenfrom form-encoded POST bodies with a 16KB limit. - Adds API tests and changelog entries for the new endpoint and query-token deprecation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
pkg/api/server.go |
Registers the new auth route and redirects successful logins to /graph. |
pkg/keystone/keystone.go |
Adds POST form token extraction and deprecation logging for URL query tokens. |
pkg/api/api_test.go |
Adds tests around token login success and failure cases. |
CHANGELOG.md |
Documents the new endpoint and deprecated query-token path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Four review-driven fixes plus the test coverage that exposes them:
- tokenLogin redirect now preserves the global-region selector (URL param or
X-Global-Region header) so the follow-up dashboard GET re-binds to the same
Keystone backend instead of silently falling back to regional.
- Content-Type recognition uses mime.ParseMediaType, accepting case variants
per RFC 9110 §8.3.1 and tolerating parameters such as "; charset=utf-8".
- authorizeRules skips the cookie -> X-Auth-Token header promotion when the
request is a POST form-token handoff (POST /{domain}/auth with form-encoded
body). A stale session cookie no longer shadows a fresh token in the body.
This is the path-specific Approach A; non-handoff POSTs are unaffected.
- New keystone-level tests exercise authOptionsFromRequest directly (the
api-package tests previously mocked AuthenticateRequest, leaving the POST
form branch uncovered): success, case-insensitive Content-Type, missing
field, non-POST method, wrong Content-Type, header-beats-body precedence,
oversize body, and the isFormURLEncoded helper.
- New api-level tests cover the redirect-with-global propagation (param and
header), the case-insensitive Content-Type wired end-to-end, and the
stale-cookie + fresh-form-token scenario asserting the body token wins.
| } | ||
| mediaType, _, err := mime.ParseMediaType(contentType) | ||
| if err != nil { | ||
| return false |
There was a problem hiding this comment.
The silent false is intentional. mime.ParseMediaType only errors on malformed Content-Type headers (e.g. ";; charset=utf-8"); legitimate-but-different types like application/json parse fine and fall through the equality check. Treating a malformed header as "not form-encoded" is the safe default — the alternative is parsing a body whose framing we don't trust.
The error doesn't get swallowed in practice: when this returns false, control falls through the else if chain in authOptionsFromRequest, and a request with no other auth signal fails with a normal AuthenticationError (401). The user-visible signal is the 401, not a debug log.
This matches net/http convention — r.ParseForm() and r.FormValue() use the same mime.ParseMediaType pattern and silently treat malformed headers as non-form. Logging here would mostly capture probes and scanners sending garbage headers, which buries the signal we actually want.
Happy to add a logg.Debug if you'd prefer it for ops grepping, but I'd lean against — let me know.
| } | ||
| mediaType, _, err := mime.ParseMediaType(req.Header.Get("Content-Type")) | ||
| if err != nil { | ||
| return false |
There was a problem hiding this comment.
Same answer as the matching helper in pkg/keystone — silent false is intentional. mime.ParseMediaType only errors on malformed Content-Type; well-formed-but-different types parse fine and fall through the equality check. Treating a malformed header as "not a form handoff" is the safe default.
Errors aren't lost: a request that fails this predicate falls through to the regular authorize path. If the user expected a form handoff and the header was malformed, they'll see a 401 — the actionable signal — rather than a silently-promoted stale cookie.
Matches net/http convention (ParseForm/FormValue use the same idiom). Whatever we decide for the matching helper in pkg/keystone should land here too — the two are symmetric.
authOptionsFromRequest called query.Del on the copy returned by r.URL.Query() but never wrote the result back to r.URL.RawQuery. As a result, downstream consumers that read r.URL — request logging, metrics labelers, reverse-proxy paths — observed the original query string with x-auth-token, project_id, and domain_id still present, even though the function had logically consumed and removed them. Track a queryDirty flag at each Del site and flush via r.URL.RawQuery = query.Encode() before the success return. The flush is gated on queryDirty so URLs with no consumed params keep their original RawQuery byte-for-byte (url.Values.Encode sorts keys, which would otherwise reorder unrelated params on every request). Adds TestAuthOptionsFromRequest_ScrubsRawQuery covering all three deletion sites plus a no-op fast-path assertion that pins original key ordering. Addresses PR #224 review feedback.
| if token := r.Header.Get("X-Auth-Token"); token != "" { | ||
| // perfect: we have a token and thus a authorization scope | ||
| ba.TokenID = token | ||
| } else if token := query.Get("x-auth-token"); token != "" { | ||
| // perfect: we have a token and thus a authorization scope (albeit in lower-case) | ||
| logg.Info("DEPRECATION: token passed via URL query parameter on %s; migrate to POST /{domain}/auth or X-Auth-Token header", r.URL.Path) | ||
| ba.TokenID = token | ||
| // relocate to header | ||
| query.Del("x-auth-token") | ||
| queryDirty = true |
|
|
||
| ### Changed | ||
|
|
||
| - POST /{domain}/auth now accepts Content-Type values per RFC 9110 §8.3.1, so | ||
| case variants such as `Application/X-WWW-Form-Urlencoded; charset=utf-8` are | ||
| recognised as form-encoded. | ||
| - POST /{domain}/auth redirects now preserve `?global=true` (and the | ||
| `X-Global-Region` header equivalent) so the dashboard load binds to the same | ||
| Keystone backend that authenticated the handoff. | ||
| - POST /{domain}/auth no longer promotes a pre-existing X-Auth-Token cookie | ||
| into the request header before the form body is parsed, so a fresh token in | ||
| the body takes precedence over a stale session cookie. | ||
|
|
Hoist the ?x-auth-token= scrub above the credential dispatch and flush r.URL.RawQuery via defer so all five early-return paths (app-cred header, basic-auth *appcred, guessScope failure, missing Authorization, missing scope) drop the deprecated query token consistently. Add subtests covering the two CRITICAL leak paths (app-cred header + query token; basic-auth *appcred + query token). Collapse CHANGELOG entries to one-line bullets under Added. Untrack build/coverprofile.out and add to .gitignore.
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
Summary
POST /{domain}/authendpoint that accepts tokens in the request body instead of URL query parametersauthorizemiddleware), sets HttpOnly cookie and 303-redirects to the dashboard?x-auth-token=query parameter path (still functional, no breaking change)Motivation
URL query parameters are not appropriate for passing authentication tokens — they persist in browser history, server access logs, proxy caches, and Referrer headers. This moves token handoff to a POST body, which is the standard approach for compliance with credential handling best practices.
The old
?x-auth-token=path continues to work unchanged. A follow-up PR will remove it once downstream consumers (Elektra) have migrated to the new endpoint.Changes
pkg/api/server.gotokenLoginhandlerpkg/keystone/keystone.gopkg/api/api_test.goBackward Compatibility
?x-auth-token=query parameter path still works identically.DEPRECATION: token passed via URL query parameter) tracks migration progress.Follow-up
?x-auth-token=query parameter support once deprecation logs confirm zero usageTest plan
make checkpasses (tests + golangci-lint + typos)