Skip to content

fix: encode parameter values in the authorization url - #178

Merged
mmktomato merged 1 commit into
nulab:masterfrom
masaru87:encode-authorization-url-params
Jul 28, 2026
Merged

fix: encode parameter values in the authorization url#178
mmktomato merged 1 commit into
nulab:masterfrom
masaru87:encode-authorization-url-params

Conversation

@masaru87

Copy link
Copy Markdown
Contributor

OAuth2.getAuthorizationURL builds the query string by interpolating the values directly, so redirect_uri and state are sent unencoded:

.map((key) => (params[key] ? `${key}=${params[key]}` : ""))

Two ways this breaks, both reproduced against master:

1. A base64 state loses its +. crypto.randomBytes(n).toString("base64") is a common way to generate one:

state passed in : ab+cd/ef==
state the server receives : ab cd/ef==

+ in a query string means space, so the value that comes back on the callback no longer matches what was sent and the CSRF check fails — intermittently, since it only happens when the random bytes encode to a +.

2. A redirect_uri that has its own query string is truncated.

redirectUri passed in : https://app.example.com/callback?tenant=acme&lang=ja
redirect_uri the server receives : https://app.example.com/callback?tenant=acme
authorization request params : client_id, response_type, redirect_uri, lang, state

The &lang=ja is parsed as a parameter of the authorization request itself, and the truncated redirect_uri no longer matches the registered one.

Applying encodeURIComponent to the values fixes both. Existing behaviour is unchanged for values without reserved characters, so the current test still passes as-is.

Verified on Node 24 (the version used in CI):

  • added a regression test that round-trips redirect_uri and state through URL.searchParams and asserts no extra parameters leak in — it fails before this change and passes after
  • npm test — 36 tests pass
  • npm run lint, npm run fmt:check, npm run typecheck, npm run build — all clean

@mmktomato
mmktomato self-requested a review July 28, 2026 02:09

@mmktomato mmktomato left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@masaru87
Thank you for your pull request. LGTM!!!

@mmktomato
mmktomato merged commit 25c11a8 into nulab:master Jul 28, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants