feat: pass opaque caller data to auth hooks - #2689
Open
ddurham2 wants to merge 6 commits into
Open
Conversation
The before-user-created hook can reject a signup, but sees only the user record
and the request metadata -- nothing the caller sent that is specific to the
application. So a hook can decide on an address or a provider, and nothing else.
That rules out any policy where the right to create an account is carried by
something the client presents: a code, a ticket, an allowlist entry, a signed
grant. The nearest workaround, `data` on /signup, exists only on the password
path and lands in user_metadata, where it is persisted on the user forever and
shares a namespace with claims derived from the provider.
This adds `hook_data`: one opaque value, accepted at the entry points that can
create a user, carried to every hook invoked while serving that request, and
never written to the user record.
POST /signup body
GET /authorize query, stored on the flow state and read
back when the provider returns
POST /otp body
POST /token?grant_type=id_token body
It rides the request context, so `NewMetadata` picks it up and no hook signature
changes. Absent, the field is omitted and payloads serialise exactly as before.
The value is opaque here and is never parsed, so the only validation is a 4 KiB
bound -- it is written to a flow state row and copied into hook payloads. It
originates with the client and is therefore untrusted: a hook that acts on it
must validate it, which is documented on the field.
Note that anything left in the /authorize query is forwarded to the provider, so
hook_data is removed there along with scopes, provider and the PKCE parameters.
The first commit tested hook_data on the path where it is easiest to test --
/signup, where the value never leaves the request. The path that actually needed
covering is the other one: on OAuth the value has to outlive the request that
carried it, written when the flow begins and read back after a redirect.
Both ends now, plus the thing that would leak it:
- /authorize stores it on the flow state
- /authorize does NOT forward it to the identity provider. Everything left in
the query is appended to the provider's authorize URL, so without the
deletion an application's private data would be sent to Google on every
sign-in
- an oversize value is refused rather than truncated
- the callback restores it into the request context, which is where
NewMetadata reads it when building a hook payload
- absent, the flow state column stays null and the context stays empty
That closes the chain end to end: query -> flow state -> context -> metadata ->
hook payload, with the /signup e2e test covering the last two links and the
v0hooks unit tests covering serialisation.
Also documents the field in openapi.yaml for all four entry points, noting on
each that the value is opaque, unstored, and must be validated by any hook that
acts on it.
The swagger wrappers for /signup and /otp reference api.SignupParams and api.OtpParams by type, so the new body field is already described there. The /authorize route is the exception -- its query parameters are written out by hand -- so the one place that needed a manual edit was this file. Notes the two properties a caller cannot infer from the name: it is not forwarded to the identity provider, and it is not stored on the user record.
ddurham2
marked this pull request as ready for review
August 11, 2026 06:40
Review raised two ways a caller's value could be recovered: from the flow state row, and from the URL it arrived in. Both come down to somebody putting a replayable bearer secret in a field that is designed to be untrusted. Cleared once consumed. The value is dead the moment the callback puts it in the request context -- the hook has run and nothing reads it again -- so it now goes out of the row in the same update that claims the flow state. The implicit flow already destroys the whole row. Done there rather than where the value is restored, because that happens before the user exists, and a callback failing after it would have thrown away something it still needed. Documented in the three places different people read: the struct field, the OpenAPI description on all four entry points, and the /authorize parameter. Each now says that it travels in a URL and so reaches history, referrer headers and proxy logs; that a hook must validate what arrives rather than treat its presence as proof; and that the intended shape is a reference the integrator checks against their own records, not a grant that authorises by existing. Encryption at rest was suggested and is not here. It defends a stolen backup, not anything holding the application's own key, and it would advertise safety for precisely the use these comments discourage -- people would believe the encryption over the warning. Clearing on consume shrinks the window to one round trip, which is the proportionate answer. Glad to add it if maintainers disagree. The new test fails if the clearing line is removed.
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.
The proposed problem
before-user-createdexists so an operator can reject a signup. It receives the user record and ametadataobject describing the call — but nothing the caller sent that is specific to the application. So a hook can decide on an address or a provider, and nothing else.That rules out a whole class of policy: any rule where the right to create an account is carried by something the client presents — an invitation code, a ticket, an allowlist entry, a signed grant. Invite-only and waitlisted apps are the obvious cases.
There is a partial workaround on one path only.
POST /signupacceptsdata, which becomesuser_metadataand is visible to the hook. It does not exist on/authorize, so the same policy cannot be applied to OAuth sign-ups at all.What this adds
One opaque value,
hook_data, accepted where a user can be created and carried to the hooks invoked while serving that request:POST /signupGET /authorizePOST /otpPOST /token?grant_type=id_tokenIt rides the request context, so
NewMetadatapicks it up and no hook signature changes. It is never parsed, and never written to the user record.{ "metadata": { "uuid": "...", "time": "...", "name": "before-user-created", "ip_address": "...", "hook_data": "<whatever the caller sent>" }, "user": { "...": "..." } }Notes for review
omitempty, so a request that carries nothing produces a byte-identical payload. There is a test asserting exactly that.user_metadata, notapp_metadata. On the OAuth leg it lives on the flow state for the duration of the round trip and dies with it.validateHookData./authorizequery before the provider redirect. Everything left in that query is appended to the provider's authorize URL, so without the deletion this would be sent to Google on every sign-in. Worth noting thatinvite_tokenis not currently deleted and is forwarded today — I left that alone rather than widen the diff.Deliberately out of scope
SAML/SSO sign-up, and
POST /admin/users— which does not invoke this hook at all today. Both felt like separate arguments.Alternative, if you would rather
The narrower framing is "
dataworks on/signupbut not/authorize; close the gap." That is a smaller idea and I would be glad to build it instead. I did not lead with it for one reason: on the OAuth leg that field is already occupied —so parity means merging caller-supplied keys into provider-derived ones (
email_verified,given_name,picture, …), and something has to decide precedence. RFC 7519 §4.2 pushes toward collision-resistant names for exactly this, and this repo already answers it once — provider-specific extras are nested undercustom_claimsrather than flat-merged. A separate, unpersisted field seemed the cleaner primitive for gating, as opposed to recording. But that is a judgement call and it is yours.Tests
internal/hooks/v0hooks/metadata_test.go— populated from context; absent from the payload when unset; reaches every hook on the request.internal/api/external_hook_data_test.go—/authorizestores it on the flow state; it is not forwarded to the provider; oversize is refused; the callback restores it into the context; absent stays null.internal/api/e2e_test.go— end to end over/signup: the value arrives in the hook payload, and a request without it produces a payload containing nohook_datakey.go build ./...,go vet ./internal/...,gofmt, and the full suites forinternal/api/...,internal/models/...,internal/hooks/...,internal/utilities/...pass against a local Postgres.I have not added tests for
/otpand theid_tokengrant beyond the shared validator — both are the same two lines as/signup, and the latter needs a mocked OIDC provider. Glad to add them if you would like them.