Repository navigation
fix(client): send the API key as the bearer token - #118
RamanKharchee wants to merge 2 commits into
Conversation
Current Nudgebee servers no longer serve POST /api/auth/token. They accept an `sk-nb-` API key directly as `Authorization: Bearer`, and the old endpoint answers 400. nbctl exchanged the key there before every request, so against such a server every command, including `configure`, failed with "token endpoint returned non-2xx status: 400 Bad Request". Send the key itself. Servers that predate direct API-key auth refuse it with a 401 but still run the exchange. So on the first refusal the transport tries the exchange, and if that works it keeps using exchanged tokens. nbctl therefore works against both, including self-hosted installs that upgrade later. Drop the fallback once no supported server predates direct auth. Every re-send now gets its own copy of the request body via GetBody. A plain clone shares the reader the first attempt drained, and concurrent first requests against an older server failed with "ContentLength=N with Body length 0". The 401 refresh-retry had the same latent bug. A 401 is now an error. The app's 401 body carries neither `data` nor `errors`, so decoding it alone would have reported a rejected key as an empty result. The message says why when it can tell: no key, a key from before the `sk-nb-` format (must be recreated), or an AI Gateway key. Co-Authored-By: Claude Opus 5.5 <[email protected]>
There was a problem hiding this comment.
Code Review
This pull request transitions the client authentication from legacy token exchange to direct API-key authentication, sending the API key as a bearer token. It includes a fallback mechanism to exchange the key for a session token on older servers that do not support direct API-key auth, along with corresponding updates to tests and documentation. Feedback on the changes highlights a potential issue in replayableRequest where the request body might be drained and not replayed if req.GetBody is nil but req.Body is non-nil, suggesting defensively reading the body into memory to ensure replayability.
| func replayableRequest(req *http.Request) (*http.Request, error) { | ||
| r := cloneRequest(req) | ||
| if req.GetBody != nil { | ||
| body, err := req.GetBody() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| r.Body = body | ||
| } | ||
| return r, nil | ||
| } |
There was a problem hiding this comment.
If req.Body is non-nil but req.GetBody is nil (which can happen if a client created via NewHTTPClient is used with a custom io.Reader), the request body will be drained during the first attempt. Any subsequent retry attempt (such as the fallback attempt) will fail with an empty body or a closed body error. To make the request fully replayable, we should defensively read the body into memory and populate req.GetBody when it is missing.
func replayableRequest(req *http.Request) (*http.Request, error) {
if req.Body != nil && req.GetBody == nil {
buf, err := io.ReadAll(req.Body)
if err != nil {
return nil, err
}
_ = req.Body.Close()
req.Body = io.NopCloser(bytes.NewReader(buf))
req.GetBody = func() (io.ReadCloser, error) {
return io.NopCloser(bytes.NewReader(buf)), nil
}
}
r := cloneRequest(req)
if req.GetBody != nil {
body, err := req.GetBody()
if err != nil {
return nil, err
}
r.Body = body
}
return r, nil
}There was a problem hiding this comment.
The gap you describe is real, but nothing reaches it today, so I documented the requirement instead of buffering (44f3b72).
Client.Runalways builds its body withbytes.NewReader, sohttp.NewRequestWithContextsetsGetBody.NewHTTPClienthas no callers in this repo.
I didn't take the suggested code, for two reasons:
- It assigns
req.Bodyandreq.GetBodyon the caller's request. Thehttp.RoundTrippercontract says a transport must not modify the request it is given. - It reads every body without
GetBodyfully into memory, including streaming uploads, to cover a fallback that only runs against older servers.
NewHTTPClient's doc comment now states that a request with a body must be replayable. http.NewRequest makes it so for bytes and strings readers.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Description
Current Nudgebee servers no longer serve
POST /api/auth/token. They accept ansk-nb-…API key directly asAuthorization: Bearer. nbctl exchanged the key there before every request, so against such a server every command,nbctl configureincluded, fails with:nbctl now sends the key itself. Servers that predate direct API-key auth (most deployments today, and self-hosted installs until they upgrade) refuse that with a 401. On that first 401, nbctl falls back to the old exchange, and if the exchange works it keeps using it. So this build works against both, and can ship before the server change rolls out.
A 401 is now an error. Before, a rejected key decoded as an empty result. The error message names the likely cause:
sk-nb-format and has to be recreated;Type of change
How Has This Been Tested?
make test(all packages),go vet ./...andgolangci-lint runpass.I ran the old and new builds against one server with the change and one without, using fake keys:
mainbuild/api/auth/tokentoken endpoint returned non-2xx status: 400not authenticated: the API key was rejected…/api/auth/tokentoken endpoint returned non-2xx status: 401not authenticated: the API key was rejected…Not run yet: a real key, against both kinds of server. To check, on each server:
Expect your accounts on both.
New tests
ContentLength=N with Body length 0, which the 401 refresh-retry onmaincould also hit;--verboselogging never writes the key.Notes for reviewers
authTransport.RoundTrip.legacylatches only after the exchange succeeds, which a server without/api/auth/tokencan't do.roundTripExchangedis the previous exchange-and-refresh code. The only change is that each re-send now takes its own copy of the body viaGetBody.--verboselog wraps the auth transport, so it records each request before theAuthorizationheader is set.TestVerboseLogOmitsApiKeypins that, since the long-lived key now goes out on every request.main: in a long-lived process (nbctl mcp) that has already switched to the exchange, revoking the key mid-session surfaces astoken endpoint returned non-2xx statusrather than the "not authenticated" message. It is still an error./api/auth/tokenmocks are gone from the command tests. With the server answering 200, nbctl never reaches the exchange.roundTripExchanged,fetchTokenand the token cache.WithUsernameis then unused for auth (the nubi commands still read the username).🤖 Generated with Claude Code