oficina / sheets Public

safeReturn misses the backslash spelling of a scheme-relative URL Issue closed

claude agent, operated by paul@keymail.dev driven by paul@keymail.dev session a066678d · claude-code/2.1.252 · DOCS-T1

Found by the Fable review pass on core, recorded in discussion #2 and not actioned there. Re-verified present on c4a3954.

safeReturn constrains a form-supplied return path to this origin:

internal/sheets/theme.go:106

It rejects a leading //, which a browser reads as scheme-relative. It does not reject /\. Browsers normalise the backslash to a forward slash and read /\evil.com as scheme-relative too, so it is the same attack with a different spelling — but Go's url.Parse leaves both Scheme and Host empty for it, so every check in the function passes and the value is returned intact.

Exploitability is genuinely low and worth stating rather than inflating: the affected handlers are POST-only, they sit behind the origin-checking middleware, and in current call sites return is server-populated rather than attacker-supplied. This is a latent hole that a future caller would fall into, not a live open redirect.

Fix: reject a second byte of \ alongside /.

Test that must fail first: a table of hostile return values — /\evil.com, /\/evil.com — asserting each resolves to /, alongside the existing //evil.com case and a legitimate path that must survive.

claude agent, operated by paul@keymail.dev driven by paul@keymail.dev session a066678d · claude-code/2.1.252 · DOCS-T1

Fixed on branch fix/safe-return-backslash, commit 9920d91, deployed to edge and verified on the process: GET /api/version returns 9920d91.

The check moved ahead of the parse, because the parse is structurally incapable of making it — both spellings are now tested on the raw bytes.

One correction to the issue as I filed it, from watching the test fail. I wrote that url.Parse "leaves both Scheme and Host empty so it passes", which is true, and implied the value then reached the browser as written, which is not. The failure printed:

safeReturn("/\\evil.example.com/") = "/%5Cevil.example.com/"

u.RequestURI() percent-encodes the backslash, so the Location header was a same-origin path all along and no browser was ever sent off-site.

That makes this smaller than "an open redirect, now closed" and worth saying plainly rather than letting the fix imply the scarier version. What was actually wrong: the thing standing between a hostile return value and a redirect was the encoder, not any check in this function. Accidental defence, one refactor from evaporating, and no help at all to a caller who used the raw input instead of the parsed output. The check turns an accident into a decision.

Three cases went into the existing hostile table. One of them — a bare \evil.example.com with no leading slash — passed before the change as well, and it stays as a control so the table cannot be read as three new bugs.

make test green including -race. Closing; not reviewed, per AGENTS.md §Review.