fix: stop leaking raw internal errors to clients in RefreshTokenGrant - #2813
Open
Rakshit-gen wants to merge 1 commit into
Open
Rakshit-gen wants to merge 1 commit into
Rakshit-gen wants to merge 1 commit into
Conversation
RefreshTokenGrant had five places that built the response error as
apierrors.NewInternalServerError("%s", err.Error()), which puts the
raw Go error text directly into the msg field of the response sent
back to the client, and leaves InternalError nil so the real cause
is also lost from server side logs.
Every other call site in the codebase (298 of them) follows the
convention of a generic message plus WithInternalError(err), which
keeps the client facing message safe and the real error available
for logging through Cause(). Bring these five in line with that.
Fixes supabase#2812
Author
|
cc @cemalkilic @hf, tagging you both since you have the most recent commits touching internal/tokens/service.go. Would appreciate a look when you have time. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce?
Bug fix.
What is the current behavior?
RefreshTokenGrantininternal/tokens/service.gohas five places where an internal error is built like this:This puts the raw Go error text directly into the
msgfield of the JSON response sent back to the API caller, sinceHTTPError.Messageis taggedjson:"msg". It also leavesInternalErrornil, so the real cause never makes it into server side logs either,Cause()only returnsInternalErrorwhen it is set, otherwise it falls back to the same generic looking error.I checked the rest of the codebase, there are 303 calls to
NewInternalServerError, and only these five do this. Everywhere else follows the convention of a generic client facing message plus.WithInternalError(err).Fixes #2812
What is the new behavior?
All five call sites now use a generic, descriptive message plus
.WithInternalError(err), matching the convention used everywhere else in the codebase:SinglePerUseris enabledThe client still gets a plain, generic 500 message. The real error is now available through
InternalErrorfor logging, the same way it already is for every other internal error in this codebase.I verified this against a real, freshly migrated Postgres database, not just a mock. I corrupted a session's
refresh_token_hmac_keyso it can no longer be decoded, then called the refresh grant with that token. Before the fix, the response'smsgfield is literally"illegal base64 data at input byte 3". After the fix,msgis a generic message and the real decode error is available throughInternalErrorinstead.Additional context
internal/tokens/service.go: five call sites updated to stop leaking raw error text into the client facing message.internal/tokens/service_test.go: new regression testTestCorruptHmacKeyDoesNotLeakInternalErrorToClientthat corrupts a session's refresh token HMAC key and asserts the client facing message does not contain the raw decode error, and that the real error is available throughInternalError.Test plan
go vet ./...passesgofmt -lreports no filesgo test ./internal/tokens/... -run TestRefreshTokenV2 -vpasses, including the new regression testgo test ./internal/api/... -run "TestToken|TestRefresh" -vpasses with no regressions