Skip to content

test(hutch): guard return-URL round-trip through rendered auth forms#948

Open
FagnerMartinsBrack wants to merge 1 commit into
mainfrom
claude/auth-return-url-encoding-23a8b1
Open

test(hutch): guard return-URL round-trip through rendered auth forms#948
FagnerMartinsBrack wants to merge 1 commit into
mainfrom
claude/auth-return-url-encoding-23a8b1

Conversation

@FagnerMartinsBrack

Copy link
Copy Markdown
Member

What & why

The signup and login form actions carry the return URL in their query string (action="/signup?return=…", action="/login?…&return=…"). For that to survive a POST, the value must stay percent-encoded end to end — otherwise a return URL whose own query string contains & (e.g. /import/abc123?source=pocket&page=2) merges into the action's query string and the browser drops every param after the first when it submits.

While investigating, I confirmed the encoding is already in place: SignupPage/LoginPage in auth.component.ts pass encodeURIComponent(data.returnUrl) into the template, so the current round-trip is correct. What was missing was coverage — the existing return-URL tests only use single-param URLs (/oauth/authorize?client_id=test), which cannot detect a trailing-param drop, and none of them assert the value survives an actual POST.

This PR adds that missing regression coverage.

Changes

Two route tests in projects/hutch/src/runtime/web/auth/auth.route.test.ts, one per form:

  • Render the form (GET /signup / GET /login) with a multi-param return URL.
  • Parse the action off the rendered form (exactly what the browser would submit to) and assert it carries the fully percent-encoded return value.
  • Replay that action via POST and assert the full return URL — trailing params included — reaches the post-auth 303 redirect Location.

The login case additionally guards the trickier action that already carries &-joined UTM params.

Verification

  • Both tests pass on main as-is (encoding present).
  • Temporarily removing encodeURIComponent from the component reproduces the described failure and makes both tests fail (rendered action becomes /signup?return=/import/abc123?source=pocket&page=2; the server parses only /import/abc123?source=pocket), confirming the tests are a genuine regression guard rather than passing vacuously.
  • Full pnpm nx run hutch:check (lint + tests + coverage) passes; coverage thresholds met.

🤖 Generated with Claude Code

https://claude.ai/code/session_01RmAwU3aMYqao58fvjBWAQ3


Generated by Claude Code

The signup and login form actions carry the return URL in their query
string, so the value must stay percent-encoded end to end or a return URL
whose own query string contains `&` loses every param after the first when
the browser POSTs the form. Add route tests that render each form, replay
the exact parsed action, and assert the full return URL — trailing params
included — reaches the post-auth redirect. These fail if the encoding is
dropped, locking in the round-trip.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RmAwU3aMYqao58fvjBWAQ3
@FagnerMartinsBrack

Copy link
Copy Markdown
Member Author

@claude Review this PR. Use your Read tool to read the local file .github/workflows/claude-PR-code-reviewer.md for instructions. The file is in the checked-out repository.

Context

@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @FagnerMartinsBrack's task in 5m 14s —— View job


Summary of what I checked

I traced the full round-trip the two new tests exercise and independently confirmed the regression mechanism they guard:

  • Encoding sourceauth.component.ts:53 (LoginPage) and auth.component.ts:109 (SignupPage) both pass data.returnUrl ? encodeURIComponent(data.returnUrl) : undefined into the template. The templates interpolate it as {{returnUrl}} (HTML-escaped), and since the value is percent-encoded ASCII, the escape is a no-op. ✔
  • Template actionslogin.template.html:7 renders action="/login?utm_source=…&utm_medium=…&utm_content=…{{#if returnUrl}}&return={{returnUrl}}{{/if}}" (the tricky &-joined case), and signup.template.html:8 renders action="/signup{{#if returnUrl}}?return={{returnUrl}}{{/if}}". ✔
  • Server parse/redirectPOST /login redirects via parseReturnUrl(req.query) (auth.page.ts:228) and free-tier POST /signup via parseReturnUrl({ return: returnUrl }) (auth.page.ts:323). Express splits the query on literal & first, so the %26 inside the encoded value stays inside return, decoding to /import/abc123?source=pocket&page=2. ✔
  • Regression reproduction — I confirmed the author's claim by reasoning through the un-encoded path: without encodeURIComponent, the signup expect(action).toBe(...) fails immediately, and the POST redirect drops &page=2 (Express parses page=2 as a sibling param, leaving return=/import/abc123?source=pocket). Both tests fail. So they are genuine guards, not vacuous. ✔
  • The existing single-param POST test (auth.route.test.ts:156) already proves res.redirect preserves ?/= in the Location without re-encoding; the new tests extend that to the &-carrying multi-param case that single-param URLs structurally cannot catch. ✔

The change is test-only (+62/−0), self-contained per-test with fresh fixtures, unique emails (login-round-trip@, signup-round-trip@), and freshLoadedAt() clears the bot-defense window. assert(action, …) and the ?.getAttribute pattern match existing conventions in the file.

High Priority Issues (Must Fix)

None found

Medium Priority Issues

None found

Low Priority Suggestions

1. Sibling return-URL carriers share the same encode but lack multi-param coverage
The same encodeURIComponent(data.returnUrl) also feeds the Google/Apple buttons (href="/auth/google?return={{returnUrl}}", login.template.html:22/28, signup.template.html:34/40) and the login↔signup footer cross-links (login.template.html:37, signup.template.html:46). This PR's guard doesn't cover those hrefs surviving a multi-param return URL — they're only exercised with single-param values today (e.g. auth.route.test.ts:72, :1291). Because they draw from the same encode call, the risk is low, so this is optional. Fix this →

2. The signup expect(action).toBe(...) couples to the exact encodeURIComponent representation
That exact-string assertion is the deliberate regression target, so it's correct as written. Worth noting only that swapping to an equivalent encoder later (e.g. building the URL via URLSearchParams) would trip it even though behavior is unchanged — the behavioral guarantee is already fully covered by the subsequent POST expect(submitted.headers.location).toBe(returnUrl). No change needed; just a note on where the brittleness lives.

cc @FagnerMartinsBrack - No high or medium priority issues found. This PR is ready for human review.
· claude/auth-return-url-encoding-23a8b1

@FagnerMartinsBrack

Copy link
Copy Markdown
Member Author

⚠️ Auto-fix skipped: the review run produced no parseable review block (missing CLAUDE_REVIEW_START/END): no CLAUDE_REVIEW block found in the run output. The full review is in the run's tracking comment — address any high/medium findings manually.

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