docs: make redirectUri a first-class setup step, not a reference footnote - #197
Merged
Conversation
…note
Every on-ramp told readers to register a callback URL with their OAuth
provider, and none told them to set `redirectUri` so the plugin actually
sends it. Follow the README, getting-started, or any provider section
verbatim and a deployed app silently keeps the local-dev default
(`http://localhost:9926/oauth/callback`) — the provider then redirects
users to their own machine and login never completes, with no
configuration error raised anywhere. An internal adopter lost an
afternoon to exactly this.
- Add `redirectUri` to every config example: README quick start,
getting-started, the configuration reference's own basic example, and
all six provider sections in providers.md
- New "Callback URLs: Both Sides Are Required" section in providers.md —
the plugin-level option, the provider-name append, and why the value
you configure has no provider name in it but the URL you register does
- Correct the configuration reference: `redirectUri`'s default was listed
as "(auto-gen)", which implies it is derived from the request host. It
is a hardcoded localhost fallback
- New "Verifying the Authorization Request" section: curl the login route
and read the `Location` header to confirm `redirect_uri` and `client_id`
before trying a login
- New Common Issues entries for the two failures that surface no error —
a deployed app redirecting to localhost, and unexpanded `${OAUTH_...}`
placeholders being shipped to the provider verbatim
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Reviewed; no blockers found. Prior 1 finding resolved by fce005c. |
There was a problem hiding this comment.
Code Review
This pull request updates the documentation across several files to clarify the configuration and importance of the redirectUri option for the @harperfast/oauth plugin, especially for deployed applications. The feedback suggests improving the local development examples in README.md and docs/getting-started.md by ensuring the local development environment variables use localhost by default and clarifying comments to prevent confusion when transitioning to production.
Contributor
|
Reviewed; no blockers found. |
Gemini review: both quick-start export blocks are explicitly labeled local-development-only, but OAUTH_REDIRECT_URI was set to a production placeholder there — copy-pasting it would break the local flow. Use the localhost value in both, and carry the deploy requirement in the inline comment (README also points at step 3, which covers both sides). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ample Gemini review: the Custom OIDC Provider example hardcoded its endpoint URLs while the accompanying export block defined OAUTH_CUSTOM_AUTHORIZATION_URL and friends, so those exports had no effect. buildProviderConfig runs expandEnvVar over every provider key, so the placeholders work — use them, matching every other provider section. Found while verifying that: the same example documented `jwksUrl`, but the code only ever reads `jwksUri` (src/types.ts, consumed in OAuthProvider.ts) and there is no alias — so a custom provider configured per these docs silently had no JWKS and could not verify ID tokens. Corrected in providers.md, configuration.md, and token-refresh-and-sessions.md. Same failure class as the redirectUri gap this PR started from: config the docs tell you to set that the plugin never receives. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Summary
redirectUriwas documented as a reference footnote, so every on-ramp in this repo taught readers only half of the callback setup. Follow any of them verbatim and a deployed app keeps the local-dev default — after which the OAuth provider redirects your users tolocalhostand login never completes, with no configuration error raised anywhere. An internal adopter lost an afternoon to exactly this; the docs are the reason it wasn't obvious.The gap
Each on-ramp tells you to register
https://yourdomain.com/oauth/<provider>/callbackwith your provider. None of them tell you to setredirectUriso the plugin actually sends that URL:redirectUriREADME.md§ 3 "Configure OAuth Callback"docs/getting-started.md§ 1 (+ § 5 tests againstlocalhost)docs/providers.md— all six provider sectionsOnly
docs/configuration.md§ Understanding Redirects covered it properly — and its options table listed the default as(auto-gen), which reads as "the plugin derives it from the request host." It doesn't: it's a hardcoded fallback (config.yamlredirectUri, andsrc/lib/config.tsbuildProviderConfig).Why it fails silently
Two independent behaviors combine into a failure with no error message on either side:
buildProviderConfigfalls back tohttp://localhost:9926/oauth, rewrites it to/oauth/<provider>/callback, and sends it. The provider does not reject it wheneverlocalhostis also registered on the client — a near-universal leftover from local development — so the flow reaches the consent screen, the user approves, and only then does the browser get sent to their own machine. There is noredirect_uri_mismatchto search for.${VAR}placeholders are passed through as literals.expandEnvVarreturns the literal${VAR}string when the variable is undefined, so a deployment missing its env vars shipsclient_id=${OAUTH_..._CLIENT_ID}to the provider verbatim. The plugin logs a healthy startup either way.Neither is visible in the provider's logs, because both are decided before the provider is involved.
What changed
redirectUriin every config example — README quick start, getting-started, the configuration reference's own basic example, and all six provider sections inproviders.md, each with the matchingOAUTH_REDIRECT_URIexport.providers.md§ "Callback URLs: Both Sides Are Required" — states the two-sided requirement once, explains that the option is plugin-level (a sibling ofproviders), and explains the provider-name append, i.e. why the value you configure has no provider name in it but the URL you register does.redirectUri's default is now the actual value instead of(auto-gen), with a note that omitting it breaks login for everyone but you.302, socurl -D -on it and reading theLocationheader confirms bothredirect_uriandclient_idwithout completing a login. This is the fastest way to catch either failure and it was documented nowhere.localhost" and "Provider Rejects an Unexpanded${OAUTH_...}Value" — each with the symptom as experienced, the cause, and the fix. Also extended the existing "Redirect URI Mismatch" entry, whose advice was previously provider-side only.Two more silently-ignored keys, found in review
Review flagged that the Custom OIDC Provider example hardcoded its endpoint URLs while the accompanying export block defined
OAUTH_CUSTOM_AUTHORIZATION_URLand friends — so those exports had no effect.buildProviderConfigrunsexpandEnvVarover every provider key, so the placeholders do work; the example now uses them, matching every other provider section.Verifying that surfaced a second one in the same block: the docs documented
jwksUrl, but the code only ever readsjwksUri(src/types.ts, consumed inOAuthProvider.ts), with no alias. A custom OIDC provider configured per these docs therefore had no JWKS at all and could not verify ID tokens. Corrected inproviders.md,configuration.md, andtoken-refresh-and-sessions.md, with a spelling note on the option so it doesn't get re-introduced.Both are the same failure class as the
redirectUrigap this PR started from: config the docs tell you to set that the plugin never receives.Follow-ups (not in this PR)
This PR documents around two behaviors that arguably shouldn't be silent. Both are small, and would make the failure self-diagnosing instead:
redirectUrito localhost on a deployed app. Warn at startup (or fail closed) when it's unset, rather than silently emitting a loopback callback.${VAR}placeholder survives expansion. Shipping a literal${OAUTH_..._CLIENT_ID}to a provider is never intentional.Happy to file both as issues if wanted.
Verification
npm run format:check(prettier) passes on all changed files.#understanding-redirects,#common-issues,#verifying-the-authorization-request,#callback-urls-both-sides-are-required,#provider-rejects-an-unexpanded-oauth_-value.🤖 Generated with Claude Code