Skip to content

[Bug] isLocalPath treats protocol-relative URLs as local (open redirect in cart redirectTo) #474

Description

@hta218

Found while reviewing Weaverse#36. The same code exists in Aspen, but it originates here, so it should be fixed upstream and synced down rather than patched per-theme.

Problem

app/routes/cart/cart-page.tsx:244

function isLocalPath(url: string) {
  try {
    new URL(url);
  } catch (e) {
    return true;
  }
  return false;
}

The function decides a URL is local when new URL() throws. Without a base argument, new URL() throws for protocol-relative URLs as well as for real paths, so //evil.com is classified as local.

It is used at app/routes/cart/cart-page.tsx:142 to gate the redirectTo form field:

if (typeof redirectTo === "string" && isLocalPath(redirectTo)) {
  return redirect(redirectTo, { headers });
}

redirect("//evil.com") sets Location: //evil.com, which browsers resolve scheme-relative against the current protocol — the customer lands on https://evil.com. Since redirectTo is an attacker-controllable form field on the cart action, this is an open redirect on a commerce flow, where it is most useful for phishing.

Verified:

node -e 'const f=(u)=>{try{new URL(u)}catch(e){return true}return false};
  ["//evil.com","/\\evil.com","https://evil.com","/collections"].forEach(u=>console.log(u,f(u)))'

//evil.com       true    <-- should be false
/\evil.com       true    <-- should be false
https://evil.com false
/collections     true

Backslash variants (/\evil.com) are normalised to // by several browsers and are treated as local by this function too.

Suggested fix

Resolve against a base and compare origins, rather than relying on a thrown exception:

function isLocalPath(request: Request, url: string) {
  try {
    return new URL(url, request.url).origin === new URL(request.url).origin;
  } catch {
    return false;
  }
}

This rejects //evil.com, https://evil.com, and the backslash variants, while still accepting /collections and /collections?foo=bar.

Note the failure direction should be inverted as well: the current version returns true (allow) when parsing fails, which fails open. It should fail closed.

Affected

  • Weaverse/pilotapp/routes/cart/cart-page.tsx:142,244
  • Weaverse/aspenapp/routes/($locale).cart.tsx:213,278 (inherited verbatim; will be picked up when the fix is synced)

Worth grepping the other themes for the same helper.

Acceptance criteria

  • isLocalPath("//evil.com") returns false
  • Parse failures fail closed rather than open
  • A test covers //host, /\host, absolute URLs, and ordinary paths

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bugSomething isn't working

Type

No type

Projects

  • Status
    Todo

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions