amadan
console: the callback path validator, before anything can store one
Task 11's other half that does not depend on the door. Spec §6.2.
The plan's draft 1 accepted this field in the enqueue task and validated
it four commits later. That is four commits during which the console
stores whatever an app sends, and a validator that arrives after the
field has already been bypassed — every item written in between is still
in the bucket when it lands. So the rule comes first and the door will
call it.
WHY IT IS A PATH. The console is an authenticated request generator: it
holds credentials no tenant has, sits inside the deployment's network,
and will make an outbound request because an app asked it to. An app
naming the HOST is the whole of SSRF handed over in a JSON field. It
names a path; the console supplies the host.
The table is the spec's, and every entry is a way of smuggling a host, a
header or a different resource through a field that is supposed to be a
path: absolute URLs, protocol-relative //evil/x and /\evil/x, an
authority hidden as userinfo (/x@evil/y parses as a path and re-parses
as an authority), traversal raw and percent-encoded, raw and encoded
CR/LF, fragments, NUL, over 1KB.
callbackTarget builds url.URL{Scheme, Host, Path, RawQuery} and never a
string. Concatenation is how a validated path still ends up naming
another host; a url.URL with Host set cannot express one whatever the
path holds. It re-validates rather than trusting its caller, because the
validator runs at the door and this runs at delivery, and those are far
enough apart that only one of them will be looked at.
ON THE MUTATION RESULTS, HONESTLY. Nine mutations; four "survived" the
first run and three of those were a lie — the perl had not matched, and
an unchanged file passing its tests reports as a surviving mutant. The
harness now diffs before believing anything, which is the same mistake
in the same direction as the one this branch already paid for once.
With that fixed, three genuine survivors remain, and they are redundancy
rather than gaps: the raw control-byte check is covered by url.Parse's
own refusal, and the protocol-relative check and the post-parse
authority invariant each cover the other. Removed in PAIRS they are
caught, so the property is pinned even though the individual guards are
not. Which is worth saying plainly rather than claiming nine for nine.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEkJh1ESqiA84quAHJV4K6
2 files changed,
+258
−0
internal/console/scanhook.go+129 −0internal/console/scanhook_test.go+129 −0
CI passed — run details
diff --git a/internal/console/scanhook.go b/internal/console/scanhook.go| new file mode 100644 |
| index 000000000..af27c8d1f |
| --- /dev/null |
| +++ b/internal/console/scanhook.go |
| @@ -0,0 +1,129 @@ |
| +package console |
| + |
| +import ( |
| + "errors" |
| + "fmt" |
| + "net/url" |
| + "strings" |
| +) |
| + |
| +// The callback path: what an app may ask the console to POST to, and why it is |
| +// a path rather than a URL. |
| +// |
| +// The console is an authenticated request generator. It holds credentials no |
| +// tenant has, it sits inside the deployment's network, and it will make an |
| +// outbound request because an app asked it to. If an app could name the HOST, |
| +// that is the whole of SSRF handed over in a JSON field — so it names a path, |
| +// and the console supplies the host from the app's platform-assigned name. |
| +// |
| +// Everything refused below is a way of smuggling a host, a header, or a |
| +// different resource through a field that is supposed to be a path. Spec §6.2. |
| + |
| +// maxCallbackPath is 1KB. Not a security boundary on its own — every hostile |
| +// shape below is refused at any length — but a durable field an app controls |
| +// wants a bound, and a callback path longer than a kilobyte is a mistake |
| +// rather than a requirement. |
| +const maxCallbackPath = 1024 |
| + |
| +var errCallbackPath = errors.New("console: callback path") |
| + |
| +// validCallbackPath is the rule. It runs at the DOOR, on the way in, so that |
| +// nothing hostile is ever stored — a validator that runs at delivery time |
| +// leaves every already-accepted item in the bucket to be found later. |
| +func validCallbackPath(p string) error { |
| + switch { |
| + case p == "": |
| + return fmt.Errorf("%w: empty", errCallbackPath) |
| + case len(p) > maxCallbackPath: |
| + return fmt.Errorf("%w: longer than %d bytes", errCallbackPath, maxCallbackPath) |
| + } |
| + |
| + // Control bytes first, before anything tries to interpret the string. A |
| + // raw CR or LF is header injection into the request the console is about |
| + // to build; a NUL truncates in anything that later hands this to C. |
| + if i := strings.IndexAny(p, "\r\n\x00"); i >= 0 { |
| + return fmt.Errorf("%w: control byte at %d", errCallbackPath, i) |
| + } |
| + // And the percent-encoded forms, which survive to whatever decodes them. |
| + // Checked on the raw string because url.Parse will happily keep them in |
| + // RawQuery, and the comparison is case-insensitive because %0D and %0d are |
| + // the same byte. |
| + lower := strings.ToLower(p) |
| + for _, enc := range []string{"%0d", "%0a", "%00"} { |
| + if strings.Contains(lower, enc) { |
| + return fmt.Errorf("%w: encoded control byte %s", errCallbackPath, enc) |
| + } |
| + } |
| + |
| + // Exactly one leading slash, and no backslash anywhere. "//evil/x" is |
| + // protocol-relative and "/\evil/x" is treated as protocol-relative by |
| + // enough parsers to count; a backslash has no legitimate place in a path |
| + // we are about to build a URL from, so it is refused outright rather than |
| + // enumerated. |
| + switch { |
| + case !strings.HasPrefix(p, "/"): |
| + return fmt.Errorf("%w: must begin with /", errCallbackPath) |
| + case strings.HasPrefix(p, "//"): |
| + return fmt.Errorf("%w: protocol-relative", errCallbackPath) |
| + case strings.ContainsRune(p, '\\'): |
| + return fmt.Errorf("%w: backslash", errCallbackPath) |
| + case strings.Contains(p, "#"): |
| + return fmt.Errorf("%w: fragment", errCallbackPath) |
| + } |
| + |
| + u, err := url.Parse(p) |
| + if err != nil { |
| + return fmt.Errorf("%w: %v", errCallbackPath, err) |
| + } |
| + // The spec's post-parse invariant, stated as an invariant. If any of these |
| + // is non-empty the string named something other than a path on this host, |
| + // whatever route it took to get there. |
| + if u.Scheme != "" || u.Host != "" || u.User != nil || u.Opaque != "" { |
| + return fmt.Errorf("%w: names a host or scheme", errCallbackPath) |
| + } |
| + // An '@' in the first segment is how an authority is smuggled past a |
| + // check that only looks at u.Host: "/x@evil/y" parses as a path, but |
| + // anything that later re-parses a concatenated form can read it as |
| + // userinfo. We build structurally and would survive it — this refuses it |
| + // anyway, because a callback path has no reason to hold one and the next |
| + // consumer of this field may not build structurally. |
| + if strings.Contains(u.Path, "@") { |
| + return fmt.Errorf("%w: '@' in the path", errCallbackPath) |
| + } |
| + |
| + // Traversal, on the DECODED path, which is the whole point: "%2e%2e" is |
| + // ".." to every server that will receive it. u.Path is already decoded, so |
| + // this catches both spellings with one check. |
| + for _, seg := range strings.Split(u.Path, "/") { |
| + if seg == ".." { |
| + return fmt.Errorf("%w: '..' segment", errCallbackPath) |
| + } |
| + } |
| + return nil |
| +} |
| + |
| +// callbackTarget builds the URL the console will POST to: https, the |
| +// platform-assigned host, and the app's path — assembled as a url.URL and |
| +// never as a string. |
| +// |
| +// Structural assembly is the point (§6.2's last line). A concatenated |
| +// "https://" + host + path is one unvalidated path away from naming a |
| +// different host; a url.URL with Host set cannot express one no matter what |
| +// the path holds. It re-validates rather than trusting its caller because the |
| +// validator runs at the door and this runs at delivery, and those are far |
| +// enough apart that only one of them will be looked at. |
| +func callbackTarget(appHost, p string) (*url.URL, error) { |
| + if err := validCallbackPath(p); err != nil { |
| + return nil, err |
| + } |
| + u, err := url.Parse(p) |
| + if err != nil { |
| + return nil, fmt.Errorf("%w: %v", errCallbackPath, err) |
| + } |
| + return &url.URL{ |
| + Scheme: "https", |
| + Host: appHost, |
| + Path: u.Path, |
| + RawQuery: u.RawQuery, |
| + }, nil |
| +} |
diff --git a/internal/console/scanhook_test.go b/internal/console/scanhook_test.go| new file mode 100644 |
| index 000000000..051b203b5 |
| --- /dev/null |
| +++ b/internal/console/scanhook_test.go |
| @@ -0,0 +1,129 @@ |
| +package console |
| + |
| +import ( |
| + "net/url" |
| + "strings" |
| + "testing" |
| +) |
| + |
| +// THE HOSTILE TABLE, AND IT LANDS WITH THE DOOR THAT ACCEPTS THE FIELD. |
| +// |
| +// Draft 1 of the plan took the callback path in the enqueue task and validated |
| +// it four commits later, which means four commits during which the console |
| +// stored whatever an app sent. A validator that arrives after the field is a |
| +// validator that has already been bypassed — every item written in between is |
| +// still in the bucket when it lands. |
| +// |
| +// Spec §6.2. The reason this is a path and not a URL is that an app must not |
| +// choose the HOST: the console posts to the platform-assigned name and nowhere |
| +// else, because the alternative is an authenticated request generator pointed |
| +// at anything an app can name. Everything below is a way of smuggling a host, |
| +// a header, or a different resource past a field that is supposed to be a |
| +// path. |
| +func TestCallbackPathValidation(t *testing.T) { |
| + refused := []struct { |
| + name, path string |
| + }{ |
| + {"empty", ""}, |
| + {"not a path at all", "callback"}, |
| + {"absolute http URL", "http://evil.example/x"}, |
| + {"absolute https URL", "https://evil.example/x"}, |
| + {"scheme-only", "https:"}, |
| + {"protocol-relative", "//evil.example/x"}, |
| + {"protocol-relative, backslash", "/\\evil.example/x"}, |
| + {"backslash-backslash", "\\\\evil.example\\x"}, |
| + // url.Parse is happy to hand back an authority from this shape, and a |
| + // naive Scheme==""/Host=="" check can be walked past with it. |
| + {"authority via userinfo", "/x@evil.example/y"}, |
| + {"traversal", "/hooks/../../admin"}, |
| + {"traversal, trailing", "/hooks/.."}, |
| + {"traversal, encoded", "/hooks/%2e%2e/admin"}, |
| + {"raw CR", "/hooks\rX-Evil: 1"}, |
| + {"raw LF", "/hooks\nX-Evil: 1"}, |
| + {"raw CRLF", "/hooks\r\nX-Evil: 1"}, |
| + {"encoded CRLF", "/hooks%0d%0aX-Evil:%201"}, |
| + {"encoded CRLF, upper", "/hooks%0D%0AX-Evil:%201"}, |
| + {"fragment", "/hooks#frag"}, |
| + {"NUL", "/hooks\x00"}, |
| + {"over 1KB", "/" + strings.Repeat("a", 1024)}, |
| + } |
| + |
| + for _, tc := range refused { |
| + t.Run("refused: "+tc.name, func(t *testing.T) { |
| + if err := validCallbackPath(tc.path); err == nil { |
| + t.Fatalf("validCallbackPath(%q) = nil, want a refusal", tc.path) |
| + } |
| + }) |
| + } |
| + |
| + accepted := []struct { |
| + name, path string |
| + }{ |
| + {"plain", "/hooks/scan"}, |
| + {"root", "/"}, |
| + {"with a query", "/hooks/scan?token=abc"}, |
| + {"query with an encoded value", "/hooks/scan?k=a%20b"}, |
| + {"dots that are not traversal", "/hooks/v1.2/scan"}, |
| + {"a dotted segment that is not ..", "/hooks/.well-known/scan"}, |
| + {"exactly 1KB", "/" + strings.Repeat("a", 1022)}, |
| + } |
| + |
| + for _, tc := range accepted { |
| + t.Run("accepted: "+tc.name, func(t *testing.T) { |
| + if err := validCallbackPath(tc.path); err != nil { |
| + t.Fatalf("validCallbackPath(%q) = %v, want it accepted", tc.path, err) |
| + } |
| + }) |
| + } |
| +} |
| + |
| +// The target is BUILT, never concatenated — §6.2's last line, which exists |
| +// because concatenation is how a validated path still ends up naming another |
| +// host. url.URL{} with the host fixed cannot express a different one no matter |
| +// what the path holds. |
| +func TestCallbackTargetIsBuiltStructurally(t *testing.T) { |
| + const host = "titogo.bab.oncarlos.com" |
| + |
| + got, err := callbackTarget(host, "/hooks/scan?a=b%20c") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + want := "https://titogo.bab.oncarlos.com/hooks/scan?a=b%20c" |
| + if got.String() != want { |
| + t.Fatalf("target = %q, want %q", got, want) |
| + } |
| + if got.Scheme != "https" { |
| + t.Fatalf("scheme = %q — a callback carries a signature and a verdict; plaintext is not an option", got.Scheme) |
| + } |
| + if got.Host != host { |
| + t.Fatalf("host = %q, want the platform-assigned %q", got.Host, host) |
| + } |
| +} |
| + |
| +// And the validator is the only way in. A path that got past validation must |
| +// still not be able to move the host, so the builder refuses too rather than |
| +// trusting its caller — the two are used from different places (the door |
| +// stores, the drain delivers) and only one of them will be looking. |
| +func TestCallbackTargetRefusesWhatTheValidatorRefuses(t *testing.T) { |
| + for _, p := range []string{"https://evil.example/x", "//evil.example/x", "/x\r\nX: 1"} { |
| + if _, err := callbackTarget("titogo.bab.oncarlos.com", p); err == nil { |
| + t.Fatalf("callbackTarget(%q) built a target, want a refusal", p) |
| + } |
| + } |
| +} |
| + |
| +// url.Parse's own view, pinned. Spec §6.2 states the post-parse invariant as |
| +// Scheme=="" Host=="" User==nil Opaque=="" — this asserts our validator agrees |
| +// with it on every accepted path, so a future rewrite of the checks cannot |
| +// quietly admit a shape the spec rules out. |
| +func TestAcceptedPathsCarryNoAuthority(t *testing.T) { |
| + for _, p := range []string{"/hooks/scan", "/", "/hooks/scan?token=abc", "/hooks/v1.2/scan"} { |
| + u, err := url.Parse(p) |
| + if err != nil { |
| + t.Fatalf("url.Parse(%q): %v", p, err) |
| + } |
| + if u.Scheme != "" || u.Host != "" || u.User != nil || u.Opaque != "" { |
| + t.Fatalf("accepted path %q parses with an authority: %+v", p, u) |
| + } |
| + } |
| +} |