amadan
fix: safeReturn rejects the backslash spelling of scheme-relative
Issue oficina/sheets #4, the second of the five Fable findings left open on core. `safeReturn` rejected a leading "//" and not a leading "/\". Browsers normalise the backslash to a forward slash before resolving — the URL Standard requires it — so the two spellings mean the same thing to the only consumer that matters. `url.Parse` does not: it reports an empty Scheme and an empty Host for "/\evil.example.com", so every check in the function passed and the value came back intact. **What the test run showed, and it is worth writing down rather than claiming the scarier thing.** The value that came back was `/%5Cevil.example.com/` — `u.RequestURI()` percent-encodes the backslash, so the Location header was a same-origin path and no browser was ever sent off-site. The hole was real and the defence was accidental: the encoder, not any check in this function, is what stood between a hostile return value and a redirect. That is one refactor away from not being true, and a caller who used the raw input rather than the parsed output would have found nothing in the way. So this is not "an open redirect was live and is now closed". It is a check that was doing less than it appeared to, made to do what it says. Exploitability was low before — POST-only, behind the origin-checking middleware, `return` server-populated at every current call site — and it is nil now for a reason the code states. The check moves ahead of the parse, because the parse is structurally incapable of making it: both spellings are tested on the raw bytes. Three cases added to the existing hostile table, one of which — a bare `\\evil.example.com` — passed before the change too. It stays as a control: it documents that a value with no leading slash was never the problem, so the table cannot be misread as three new bugs. make test green, including -race. 🤖 Authored by Claude Opus 5, directed by Paul.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0114wZuXnD6aGcDCzL39NpXD
2 files changed,
+28
−3
internal/sheets/theme.go+20 −3internal/sheets/theme_test.go+8 −0
CI passed — run details
diff --git a/internal/sheets/theme.go b/internal/sheets/theme.go| index ac11d0c..3d855d4 100644 |
| --- a/internal/sheets/theme.go |
| +++ b/internal/sheets/theme.go |
| @@ -101,10 +101,27 @@ func (a *App) handleMode(w http.ResponseWriter, r *http.Request) { |
| // |
| // A form field that becomes a Location header is an open redirect |
| // unless it is constrained to a path on this origin. Anything with a |
| -// scheme, a host, or a leading "//" — which a browser reads as |
| -// scheme-relative and therefore off-site — is discarded for the root. |
| +// scheme, a host, or a scheme-relative opening is discarded for the |
| +// root. |
| +// |
| +// Scheme-relative has two spellings and only one of them is obvious. |
| +// "//host" is read off-site by every browser. So is "/\host": browsers |
| +// normalise the backslash to a forward slash before resolving, which |
| +// the URL Standard requires of them. url.Parse does neither — it |
| +// reports an empty Scheme and an empty Host for the second — so the |
| +// parse below cannot be what rejects it, and the check has to happen |
| +// before it. |
| +// |
| +// Today `u.RequestURI()` happens to percent-encode that backslash, so |
| +// the header comes out as a same-origin path and no browser is actually |
| +// sent anywhere. That is an accident of the encoder, not a decision by |
| +// this function, and it is one refactor away from not being true. The |
| +// explicit check is what turns it into a decision. |
| func safeReturn(raw string) string { |
| - if raw == "" || !strings.HasPrefix(raw, "/") || strings.HasPrefix(raw, "//") { |
| + if raw == "" || raw[0] != '/' { |
| + return "/" |
| + } |
| + if len(raw) > 1 && (raw[1] == '/' || raw[1] == '\\') { |
| return "/" |
| } |
| u, err := url.Parse(raw) |
diff --git a/internal/sheets/theme_test.go b/internal/sheets/theme_test.go| index 9a39ab7..e4d6f7d 100644 |
| --- a/internal/sheets/theme_test.go |
| +++ b/internal/sheets/theme_test.go |
| @@ -156,6 +156,14 @@ func TestModeReturnIsNotAnOpenRedirect(t *testing.T) { |
| "//evil.example.com/", |
| "http://evil.example.com", |
| "javascript:alert(1)", |
| + // The backslash spelling of the same attack. Browsers |
| + // normalise "\" to "/" and read these as scheme-relative, |
| + // exactly as they read "//" — but url.Parse does not, so it |
| + // reports an empty Scheme and an empty Host and every check |
| + // below passes. The check has to be made before the parse. |
| + `/\evil.example.com/`, |
| + `/\/evil.example.com`, |
| + `\\evil.example.com`, |
| } { |
| if got := safeReturn(hostile); got != "/" { |
| t.Errorf("safeReturn(%q) = %q, want %q", hostile, got, "/") |