Fix #237: SSRF allowlist bypass + anon dangerous #241

Merged

crueber merged 2 commits from fix/issue-237 into main

2026-09-09 16:44:33 +00:00

crueber commented

2026-09-09 16:37:59 +00:00

Owner

Closes #237. LIVE SECURITY BYPASS fix, fail-closed throughout.

What was wrong

NormalizeSource derived the allowlist-compared host with u.Hostname() (port-stripping) but kept the full raw URL (u.String(), port/case/slashes intact) as the clone target: the checked string and the fetched string diverged. :443, uppercase host, trailing /, .git/, and plaintext-http variants of an allowlisted host all passed the gate.
dangerous:true flowed straight from the request body into SSRFConfig. Under auth-none the caller presents as auth.None() (Admin:true, unauthenticated), so any network caller could arm the empty-allowlist escape hatch.

Fix

url.go: every non-GitHub source is rebuilt as one canonical scheme://host/path (lowercased host, default ports stripped, trailing slashes + one .git folded; explicit non-default ports refused 400). The allowlist compares the canonical host; CloneMirror receives exactly the gated string.
Scheme decision (documented in 10 §7 + Decisions): http:// without token stays allowed (§1 scope stands); token still requires https; allowlist is host-only and the scheme is part of the canonical identity.
service.go Begin: dangerous:true honored only for authenticated admins under server.auth.mode token/oidc (403 otherwise, naming walhub import --dangerous); auth-none is CLI-only; anonymous still 401s via the unchanged S6 order. CLI --dangerous (headless runner) unaffected.
No web/ change: the SDK normalizeSource is prefill-only by documented contract; the server is the source of truth.

Tests (all in `internal/repoimport/issue237_test.go`, no network)

Every issue-table row collapses to one canonical URL/Host; gate parity (pass allowlisted / identical 400 foreign); non-default ports 400.
Clone-argv identity proven through the real Begin→drive→CloneMirror path with a capture-script git binary: all 5 variants hand git the identical canonical URL.
dangerous: anon→401, writer→403, auth-none→403 naming the CLI, token-mode admin→202 + real fixture import succeeds (legitimate flow intact); canonical allowlisted form→202.

Verification

gofmt/ clean; go test -race full package green; single-flight stress -count=20 green; coverage 95.8% (≥95% gate holds; touched fns: NormalizeSource 96.8%, isDefaultPort/CheckSSRF/dangerousAllowed/canonicalGenericURL 100%).
### Concurrency subsections in code + doc entries: no new goroutines/locks/channels (pure predicates).
hub.packden.us was NOT probed (read-only issue read); verified against httptest scratch harness instead.

Do NOT merge (per task).

Closes #237. LIVE SECURITY BYPASS fix, fail-closed throughout. ## What was wrong - `NormalizeSource` derived the allowlist-compared host with `u.Hostname()` (port-stripping) but kept the full raw URL (`u.String()`, port/case/slashes intact) as the clone target: the checked string and the fetched string diverged. `:443`, uppercase host, trailing `/`, `.git/`, and plaintext-http variants of an allowlisted host all passed the gate. - `dangerous:true` flowed straight from the request body into `SSRFConfig`. Under auth-none the caller presents as `auth.None()` (Admin:true, unauthenticated), so any network caller could arm the empty-allowlist escape hatch. ## Fix - `url.go`: every non-GitHub source is rebuilt as one canonical `scheme://host/path` (lowercased host, default ports stripped, trailing slashes + one `.git` folded; explicit non-default ports refused 400). The allowlist compares the canonical host; `CloneMirror` receives exactly the gated string. - Scheme decision (documented in 10 §7 + Decisions): `http://` without token stays allowed (§1 scope stands); token still requires `https`; allowlist is host-only and the scheme is part of the canonical identity. - `service.go` `Begin`: `dangerous:true` honored only for authenticated admins under `server.auth.mode` token/oidc (403 otherwise, naming `walhub import --dangerous`); auth-none is CLI-only; anonymous still 401s via the unchanged S6 order. CLI `--dangerous` (headless runner) unaffected. - No web/ change: the SDK `normalizeSource` is prefill-only by documented contract; the server is the source of truth. ## Tests (all in `internal/repoimport/issue237_test.go`, no network) - Every issue-table row collapses to one canonical URL/Host; gate parity (pass allowlisted / identical 400 foreign); non-default ports 400. - Clone-argv identity proven through the real Begin→drive→CloneMirror path with a capture-script git binary: all 5 variants hand git the identical canonical URL. - dangerous: anon→401, writer→403, auth-none→403 naming the CLI, token-mode admin→202 + real fixture import succeeds (legitimate flow intact); canonical allowlisted form→202. ## Verification - `gofmt`/ clean; `go test -race` full package green; single-flight stress `-count=20` green; coverage 95.8% (≥95% gate holds; touched fns: NormalizeSource 96.8%, isDefaultPort/CheckSSRF/dangerousAllowed/canonicalGenericURL 100%). - `### Concurrency` subsections in code + doc entries: no new goroutines/locks/channels (pure predicates). - hub.packden.us was NOT probed (read-only issue read); verified against httptest scratch harness instead. Do NOT merge (per task).

crueber added 1 commit

2026-09-09 16:38:00 +00:00

Fix #237 : SSRF allowlist bypass + anon dangerous 6d9a99fe1b

NormalizeSource now rebuilds every non-GitHub source as one canonical
scheme://host/path string (lowercased host, default ports stripped,
trailing slashes + one .git folded; explicit non-default ports refused
400), so the allowlist compares the canonical host and CloneMirror
receives exactly the gated string (no check-vs-fetch divergence).
Begin honors dangerous:true only for authenticated admins under
server.auth.mode token/oidc (403 otherwise, CLI-only on auth-none);
anonymous still 401s via the S6 order. Regression tests pin every
issue-table row + the clone-argv identity through Begin->drive->
CloneMirror. Docs: 10_git_import.md SSRF contract + Decisions.

crueber referenced this pull request

2026-09-09 16:38:04 +00:00

Import: source URL variants bypass the strict allowlist check — case/trailing-slash host normalization lets any URL through, and dangerous flag is accepted from anonymous/unauthenticated requests #237

crueber referenced this pull request from a commit

2026-09-09 16:43:48 +00:00

Review #241: pin port-refusal precedence over allowlist match + userinfo (fix #237)

crueber added 1 commit

2026-09-09 16:43:48 +00:00