fix(auth): keep percent-escapes in the login callbackUrl for #794 - #824
Open
hdimer wants to merge 1 commit into
Open
fix(auth): keep percent-escapes in the login callbackUrl for #794#824hdimer wants to merge 1 commit into
hdimer wants to merge 1 commit into
Conversation
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.
Fixes #794.
What's wrong
A DID name can contain a literal
/, so the UI builds DID links with the name escaped into a single[name]segment —href={`/did/${encodeURIComponent(scope)}/${encodeURIComponent(name)}`}in seven places (ListDIDMeta.tsx:19,DetailsRuleLocks.tsx:32,ListRuleTable.tsx:34, and others) — andsrc/app/(rucio)/did/[scope]/[name]/page.tsx:9decodes it back when the page renders. Copying such a link and opening it logged out is the reporter's case, and it passes through two separate places that drop the escape.1.
src/proxy.ts:36—initiateLogin(), the branch taken when there is no session at all, interpolates the pathname into the query string raw:reLogin()two lines above already wraps the same expression inencodeURIComponent(), as do/api/auth/login(route.ts:27) and the three call sites insession-monitor.tsx.initiateLoginis the only producer that doesn't, so the%reaches the query unescaped andURLSearchParamsreads%2Fback as/.2.
src/app/auth/login/page.tsx:446— the login page reads the value withuseSearchParams().get('callbackUrl'), which has already percent-decoded it once, and then callsdecodeURIComponent()on it a second time. That decode destroys the escape whichever producer sent it, so fixing the proxy alone doesn't clear the 404 — the login page undoes it. This half also affects the?expired=truere-login path, which was encoding correctly all along.Either way the redirect target gains a path segment (
/did/ddmadmin/foo%2Fbar→/did/ddmadmin/foo/bar) and 404s.Two things fall out of the same root cause, both worth knowing when weighing this:
%crashed rather than 404'd./did/ddmadmin/a%25bbecame/did/ddmadmin/a%b, anddid/[scope]/[name]/page.tsx:9then calleddecodeURIComponent('a%b'), which throwsURIError.page.tsx:23readsexpired, so a link whose path contained&expired=trueproduced/auth/login?callbackUrl=/foo&expired=trueand the login page told the user their session had expired.The fix
Two lines: encode in
initiateLogin, and drop the second decode on the read side.redirectURLthen comes out byte-identical torequest.nextUrl.pathname, which is the form bothrouter.push()and the[name]route expect.I did not factor the two
proxy.tsredirect literals into a shared helper. They now differ only byexpired=true, but that is two call sites of a one-line template, and the same shape appears four more times elsewhere in the codebase — extracting it would grow this diff more than it shrinks it. Happy to do it separately if you'd rather.Tests
Four cases, all four red on
main:test/api/auth/proxy-callback-url.test.tsdrives the realproxy()with a null token and asserts thecallbackUrlon the redirect it returns.test/component/auth/login-page-callback-url.test.tsxrenders the real login page with the story component stubbed to capture its props, drives the userpass success handler, and asserts the URLrouter.push()receives. It feeds in an already-correctly-encodedcallbackUrl— what a fixed proxy emits — and still fails onmain, which is how I established that the second half was needed rather than assuming it.One case is there specifically to rule out a near-miss:
encodeURIalso makes the%2Ftest pass, because it escapes%but leaves/alone. The&-in-a-DID-name case fails underencodeURI(the parameter gets truncated at the&) and passes only underencodeURIComponent. I checked that by actually running the suite against anencodeURIvariant.npm testis green (109 suites, 459 tests).tsc --noEmitclean,npm run buildsucceeds, andeslinton the two touched source files reports the same 7 pre-existing warnings asmain, none new. Both files have pre-existing Prettier violations on lines this diff doesn't touch (proxy.ts:25-28,page.tsx:482); I left them alone rather than mix a reformat into a two-line fix.Adjacent, deliberately not touched
callbackUrlisn't origin-validated today, atpage.tsx:179/:372andapi/auth/logout/route.ts:22. This change doesn't widen that, and it makes one double-encoded protocol-relative payload inert, but it isn't a fix for it. Happy to open a separate issue.proxy.tsredirects usenextUrl.pathnameonly, so any query string on the original link is lost across login. Pre-existing in both functions and out of scope here.@maany — you said on the issue you'd try to recreate this. If you already have something in flight, say the word and I'll close this. I mainly wanted to write down that it's two bugs rather than one, since the proxy half on its own doesn't clear the 404.
Used AI assistance on this; I reviewed and tested the change myself.