[Security] HTTP redirect target not validated (open redirect risk)

[pathosDev]

Severity / Size

• Severity: MEDIUM
• Size: S
• Threat model: closed-group cluster; HTTP layer faces public/internet traffic. An attacker controls a next= / returnUrl= style query parameter via a phishing link.

Affected files

• src/http/Route.ts:74-77 — redirect(url, status) sets Location: <url> verbatim, no validation.

Background

The redirect() helper in Route.ts is the recommended way for handler code to emit a 302 Found (or any 3xx) response:

export function redirect(url: string, status: number = Status.Found): HttpResponse {
  return { status, headers: { location: url }, body: null };
}

The url is passed through to the Location header without any check. That's fine when the handler computes the URL itself (redirect('/dashboard')), but the common pattern applications end up writing is:

get(async (req) => {
  const next = queryParam(req, 'next') ?? '/';
  if (!isLoggedIn(req)) return redirect('/login?next=' + encodeURIComponent(req.path));
  return redirect(next);   // <-- attacker-controlled
});

next is attacker-controlled — they craft https://app.example.com/login?next=https://evil.example.com/phish. After successful login the browser navigates to evil.example.com, which mimics the login page and harvests credentials a second time. This is the classic "open redirect" pattern (OWASP A01:2021 → Broken Access Control sub-category).

The framework can't force user code to validate, but it can pick a safer default: reject absolute URLs in redirect() and require an explicit opt-in for them. That puts the trust boundary on the framework side where the cost of getting it wrong is highest.

Exploit walkthrough

Step 1 — App author wires up "redirect after login" pattern:

// User's login route, taken straight from a tutorial:
const login = post(async (req) => {
  const creds = entity<{ user: string; pass: string }>(req);
  if (!authenticate(creds)) throw new HttpError(401, 'bad creds');
  const next = queryParam(req, 'next') ?? '/';
  setSessionCookie(req, creds.user);
  return redirect(next);
});

Step 2 — Attacker phishes a victim:

https://app.example.com/login?next=https%3A%2F%2Fapp-example.evil.com%2Flogin

The hostname looks fine — app.example.com is the real app.

Step 3 — Victim logs in. Real backend authenticates them successfully. redirect(next) returns 302 Found with Location: https://app-example.evil.com/login to the victim's browser.

Step 4 — Victim's browser follows the redirect to app-example.evil.com, which renders a pixel-perfect clone of the login page with "Session expired, please log in again". Victim re-enters credentials. Attacker captures them.

Realistic worst case: plus a same-origin OAuth flow (?response_type=token&redirect_uri=…), the attacker can steal access tokens — not just session credentials, since the OAuth response is appended to the redirect target.

How the 8 already-landed security fixes inform this

Three of the prior fixes apply the same shape — reject obviously dangerous input at the boundary, fail closed, require explicit opt-in for the unsafe path:

• FS path-traversal (assertSafeKey in FilesystemObjectStorageBackend.ts:92-113) — rejects absolute paths and .. segments syntactically. No "allow but with warning" mode; if you really need an absolute path, you build a separate backend.
• Memcached CRLF guard — rejects \r/\n in cache keys at the wrapper layer. No flag to disable.
• WebSocket frame cap — fails closed at the configured byte limit. Override requires a constructor option, not a per-call argument.

For redirect() the analogous shape is: reject anything that looks like an external URL by default, accept an explicit allowAbsolute: boolean option on the call site for the rare case where the caller really means it.

Fix design

Track 1 — Boundary check in redirect() (primary). Reject:

• URLs starting with // (protocol-relative, e.g. //evil.com/x).
• URLs containing a scheme + :// (e.g. https://evil.com, javascript:alert(1)).
• URLs containing \r / \n / \0 (header injection — same shape as the Memcached CRLF fix).
• Everything else (relative paths like /foo, dashboard, ?q=1) is allowed.

export function redirect(
  url: string,
  status: number = Status.Found,
  opts?: { readonly allowAbsolute?: boolean },
): HttpResponse {
  assertSafeRedirectTarget(url, opts?.allowAbsolute === true);
  return { status, headers: { location: url }, body: null };
}

function assertSafeRedirectTarget(url: string, allowAbsolute: boolean): void {
  if (typeof url !== 'string' || url.length === 0) {
    throw new HttpError(Status.BadRequest, 'redirect: target URL must be a non-empty string');
  }
  if (url.includes('\r') || url.includes('\n') || url.includes('\0')) {
    throw new HttpError(Status.BadRequest, 'redirect: control characters in target URL not allowed');
  }
  if (allowAbsolute) return;
  // Protocol-relative (//host/...) or absolute (scheme://host/...).
  if (url.startsWith('//')) {
    throw new HttpError(Status.BadRequest, 'redirect: protocol-relative URLs not allowed (pass {allowAbsolute: true} to override)');
  }
  if (/^[a-zA-Z][a-zA-Z0-9+.-]*:/.test(url)) {
    throw new HttpError(Status.BadRequest, 'redirect: absolute URLs not allowed (pass {allowAbsolute: true} to override)');
  }
}

Track 2 — Defence-in-depth: same check at backend write time. All three backends (Fastify / Hono / Express) setHeader('location', value) for whatever the handler returned. They can re-run assertSafeRedirectTarget on outbound location headers when status is in [301, 302, 303, 307, 308], with allowAbsolute: false always — but only as a console.warn if the handler didn't go through redirect() (we don't want to break legitimate native-Express middleware that emits absolute redirects).

The clean version: leave the warning at debug-log level, default-off via a new HonoBackendOptions.warnOnAbsoluteRedirect: boolean flag, default false. Track 1 is the real defence; Track 2 only catches the case where the user constructed an HttpResponse literal themselves.

Track 3 — Diagnostic counter. Track http_redirect_rejected_total{reason="absolute"|"control_char"|"protocol_relative"} in the metrics extension. Lets ops detect "are we rejecting any real traffic?" before deploying the change in strict mode.

API surface

// Route.ts
export function redirect(
  url: string,
  status?: number,
  opts?: { readonly allowAbsolute?: boolean },
): HttpResponse;

The third argument is opt-in; existing calls redirect('/foo') and redirect('/foo', 301) work unchanged.

Backward compatibility

Breaking for users who do redirect(externalUrl) today. The fix is to either:

1. Add { allowAbsolute: true } (audited opt-in).
2. Compute a relative path instead.

Document under examples/http/README.md and add a CHANGELOG entry. Since the threat-model section in README.md already flags open-redirect as a known caveat we're closing, this is a desirable break.

Test plan

1. Exploit test — handler that does redirect(queryParam(req, 'next') ?? '/'), request with ?next=https://evil.com → backend returns 400 Bad Request (instead of 302 Found with Location: https://evil.com).
2. Defence test — exhaustive: https://x, //x, javascript:alert(1), file:///etc/passwd, data:text/html,<script>, /foo\r\nSet-Cookie: x=y all rejected with appropriate error messages.
3. Happy-path test — redirect('/foo'), redirect('/foo?q=1'), redirect('?q=1'), redirect('') (empty is rejected with a separate message), redirect('https://x.com', undefined, { allowAbsolute: true }) all behave correctly.
4. Header-injection test — redirect('/foo\r\nLocation: https://evil') rejected even with allowAbsolute: true (control-char check fires first).
5. Regression — existing Route.test.ts redirect tests (relative paths) still pass without modification.

Acceptance criteria

• redirect('https://evil.com') throws HttpError(400, ...).
• redirect('//evil.com') throws HttpError(400, ...).
• redirect('javascript:alert(1)') throws HttpError(400, ...).
• redirect('/foo') and redirect('?q=1') continue to work.
• redirect('https://x.com', 302, { allowAbsolute: true }) returns the 302 Found response unchanged.
• redirect('/foo\r\n…') throws regardless of allowAbsolute.
• CHANGELOG entry under "Security" + README "Known security caveats" updated.