crueber/walhub

Watch

Fork 0

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

New issue

Closed

opened 2026-09-09 15:28:45 +00:00 by crueber · 3 comments

crueber commented

2026-09-09 15:28:45 +00:00

Owner

Summary

The import feature's SSRF/allowlist gate (internal/repoimport/url.go CheckSSRF) has a bypass: the URL is normalized before the allowlist comparison, and the normalization path rewrites/derives the host in ways that let source URLs which should NOT match the allowlist pass the check. On hub.packden.us the allowlist is configured (non-empty — example.com/gitlab.com/github.com are all rejected with source host … is not in import.url_allowlist), yet several source URL forms targeting git.packden.us are accepted and cloned successfully.

Reproduced on the live dev instance (hub.packden.us, anonymous, no token)

Allowlist is active (confirmed):

source_url	result
`https://example.com/x/y.git`	400 `not in import.url_allowlist` ✅
`https://github.com/octocat/Hello-World.git`	400 `not in import.url_allowlist` ✅
`https://gitlab.com/gitlab-org/gitlab-test.git`	400 `not in import.url_allowlist` ✅

Bypassed (should have been rejected if git.packden.us is not itself the intended sole allowlist entry — or at minimum these must resolve to the SAME host check as the canonical form):

source_url	result
`https://git.packden.us/crueber/dotfiles.git`	202 ✅ (expected — assuming this host IS the allowlist entry)
`https://git.packden.us:443/crueber/dotfiles.git`	202 — accepted with an explicit port
`https://git.packden.us/crueber/dotfiles/`	202 — trailing slash
`https://git.packden.us/crueber/dotfiles.git/`	202 — `.git/` trailing slash
`http://git.packden.us/crueber/dotfiles.git`	202 — plaintext http scheme (not https)
`https://GIT.PACKDEN.US/crueber/dotfiles.git`	202 — uppercase host

The :443 variant is the sharpest one: NormalizeSource keeps u.Hostname() (which strips the port) for the allowlist comparison (url.go:92, url.go:186-192) but keeps the full URL (u.String(), port intact) as the clone target (url.go:94 canon). So git.packden.us:443 matches the git.packden.us allowlist entry on host comparison, but the actual outbound clone URL retains :443 — the checked string and the fetched string are not the same string. The same check-vs-fetch divergence applies to scheme: http://git.packden.us/… passed the gate and cloned (task i8b16772e2804fe016377730ef061b9, ok: true, log shows clone start http://git.packden.us/crueber/dotfiles), so a plaintext http URL is gated on the same host entry as its https twin with no scheme distinction.

Two more gate-integrity findings, same area:

dangerous comes from the request body, unchecked against auth. ParseRequest (internal/repoimport/service.go:136-143) feeds in.Dangerous straight into SSRFConfig.Dangerous. With a non-empty allowlist this is masked (the allowlist branch returns before the dangerous check, url.go:186-193), but if the operator ever empties the allowlist, any caller — including the anonymous POST that succeeded in testing — can set dangerous: true and reach arbitrary public hosts. The flag exists precisely to require an operator-side confirm (§9.6); accepting it from the untrusted request body defeats that.
Trailing-slash URLs skip the .git canonicalization but still hit the same host check (url.go:94 stripDotGit only strips the suffix; …dotfiles/ and …dotfiles.git/ both 202). Not a security gap on its own (same host), but it means three different canonical URLs can land for one source, which breaks the same-source resume/no-op idempotency that import.json provenance matching relies on (source_url is compared for the B3 join/no-op — task i02ac… 409'd on :443 vs plain as "different source", proving the stored form is the raw input).

Expected

The host string compared against import.url_allowlist must be derived identically to (or stricter than) the URL actually cloned: no port-stripping asymmetry, scheme-restricted (https only for remote sources unless explicitly enabled), case-insensitive on both sides, trailing-slash/tail canonicalized so one logical source has exactly one canonical form.
dangerous: true must never be honored from the request body when the effective policy is "no allowlist" — it should require an authenticated admin principal (or be CLI-only via walhub import --dangerous).
Testable example: https://git.packden.us/crueber/dotfiles.git (tiny repo) is a good allowlist-match fixture; https://git.packden.us:443/…, http://git.packden.us/…, https://GIT.PACKDEN.US/…, and …/dotfiles/ are the current-bypass fixtures.

Acceptance criteria

Unit tests pin the normalization: :443, uppercase host, trailing /, and .git/ variants all produce the SAME Normalized.Host and canonical URL as the plain form — one canonical source, one gate decision, one import.json provenance match.
The clone URL equals the canonical URL that passed the gate (no check-vs-fetch divergence). Add a test asserting the args handed to git.CloneMirror match the gated canonical form exactly.
Decide and document the scheme rule: http:// to a remote host is rejected (or requires the same dangerous confirm) — currently it clones happily. Same for any non-default port (:9418, :8080 etc.) on an allowlisted host.
dangerous from the API body is ignored or rejected unless the principal is an admin (or the allowlist is empty AND auth mode is dev); the flag's authority stays with the operator/CLI.
The three variant URLs above fail identically to their canonical twin when the host is not allowlisted, and no-op identically when it is (B3 join/no-op sees one source).

## Summary The import feature's SSRF/allowlist gate (`internal/repoimport/url.go` `CheckSSRF`) has a bypass: the URL is normalized **before** the allowlist comparison, and the normalization path rewrites/derives the host in ways that let source URLs which should NOT match the allowlist pass the check. On hub.packden.us the allowlist is configured (non-empty — `example.com`/`gitlab.com`/`github.com` are all rejected with `source host … is not in import.url_allowlist`), yet several source URL forms targeting git.packden.us are accepted and cloned successfully. ## Reproduced on the live dev instance (hub.packden.us, anonymous, no token) Allowlist is active (confirmed): | source_url | result | |---|---| | `https://example.com/x/y.git` | **400** `not in import.url_allowlist` ✅ | | `https://github.com/octocat/Hello-World.git` | **400** `not in import.url_allowlist` ✅ | | `https://gitlab.com/gitlab-org/gitlab-test.git` | **400** `not in import.url_allowlist` ✅ | Bypassed (should have been rejected if `git.packden.us` is not itself the intended sole allowlist entry — or at minimum these must resolve to the SAME host check as the canonical form): | source_url | result | |---|---| | `https://git.packden.us/crueber/dotfiles.git` | 202 ✅ (expected — assuming this host IS the allowlist entry) | | `https://git.packden.us:443/crueber/dotfiles.git` | **202** — accepted with an explicit port | | `https://git.packden.us/crueber/dotfiles/` | **202** — trailing slash | | `https://git.packden.us/crueber/dotfiles.git/` | **202** — `.git/` trailing slash | | `http://git.packden.us/crueber/dotfiles.git` | **202** — plaintext http scheme (not https) | | `https://GIT.PACKDEN.US/crueber/dotfiles.git` | **202** — uppercase host | The `:443` variant is the sharpest one: `NormalizeSource` keeps `u.Hostname()` (which strips the port) for the allowlist comparison (`url.go:92`, `url.go:186-192`) but keeps the full URL (`u.String()`, port intact) as the clone target (`url.go:94` `canon`). So `git.packden.us:443` matches the `git.packden.us` allowlist entry on host comparison, but the actual outbound clone URL retains `:443` — the checked string and the fetched string are not the same string. The same check-vs-fetch divergence applies to scheme: `http://git.packden.us/…` passed the gate and cloned (task `i8b16772e2804fe016377730ef061b9`, `ok: true`, log shows `clone start http://git.packden.us/crueber/dotfiles`), so a plaintext http URL is gated on the same host entry as its https twin with no scheme distinction. Two more gate-integrity findings, same area: 1. **`dangerous` comes from the request body, unchecked against auth.** `ParseRequest` (`internal/repoimport/service.go:136-143`) feeds `in.Dangerous` straight into `SSRFConfig.Dangerous`. With a non-empty allowlist this is masked (the allowlist branch returns before the dangerous check, `url.go:186-193`), but if the operator ever empties the allowlist, any caller — including the anonymous POST that succeeded in testing — can set `dangerous: true` and reach arbitrary public hosts. The flag exists precisely to require an operator-side confirm (§9.6); accepting it from the untrusted request body defeats that. 2. **Trailing-slash URLs skip the `.git` canonicalization but still hit the same host check** (`url.go:94` `stripDotGit` only strips the suffix; `…dotfiles/` and `…dotfiles.git/` both 202). Not a security gap on its own (same host), but it means three different canonical URLs can land for one source, which breaks the same-source resume/no-op idempotency that `import.json` provenance matching relies on (`source_url` is compared for the B3 join/no-op — task `i02ac…` 409'd on `:443` vs plain as "different source", proving the stored form is the raw input). ## Expected - The host string compared against `import.url_allowlist` must be derived identically to (or stricter than) the URL actually cloned: no port-stripping asymmetry, scheme-restricted (https only for remote sources unless explicitly enabled), case-insensitive on both sides, trailing-slash/tail canonicalized so one logical source has exactly one canonical form. - `dangerous: true` must never be honored from the request body when the effective policy is "no allowlist" — it should require an authenticated admin principal (or be CLI-only via `walhub import --dangerous`). - Testable example: `https://git.packden.us/crueber/dotfiles.git` (tiny repo) is a good allowlist-match fixture; `https://git.packden.us:443/…`, `http://git.packden.us/…`, `https://GIT.PACKDEN.US/…`, and `…/dotfiles/` are the current-bypass fixtures. ## Acceptance criteria - [ ] Unit tests pin the normalization: `:443`, uppercase host, trailing `/`, and `.git/` variants all produce the SAME `Normalized.Host` and canonical URL as the plain form — one canonical source, one gate decision, one `import.json` provenance match. - [ ] The clone URL equals the canonical URL that passed the gate (no check-vs-fetch divergence). Add a test asserting the args handed to `git.CloneMirror` match the gated canonical form exactly. - [ ] Decide and document the scheme rule: `http://` to a remote host is rejected (or requires the same dangerous confirm) — currently it clones happily. Same for any non-default port (`:9418`, `:8080` etc.) on an allowlisted host. - [ ] `dangerous` from the API body is ignored or rejected unless the principal is an admin (or the allowlist is empty AND auth mode is dev); the flag's authority stays with the operator/CLI. - [ ] The three variant URLs above fail identically to their canonical twin when the host is not allowlisted, and no-op identically when it is (B3 join/no-op sees one source).

crueber referenced this issue

2026-09-09 15:55:23 +00:00

Mirror repos (pull-only) with scheduled upstream syncs: create-from-URL flow, cron-ish schedule presets, next-sync display, and push rejection #240

crueber referenced this issue from a commit

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

Fix #237: SSRF allowlist bypass + anon dangerous

crueber referenced this issue from a pull request that will close it,

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

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

crueber commented

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

Author

Owner

Fix is up: PR #241 (branch fix/issue-237) — canonical-URL rebuild (default-port strip, non-default-port 400, case/slash/.git folding, clone URL == gated URL) + dangerous:true now requires an authenticated admin under auth.mode token/oidc (403 otherwise, CLI-only on auth-none). Every table row pinned by regression tests (incl. clone-argv identity through Begin→drive→CloneMirror); full package -race green, coverage 95.8%. hub.packden.us was not probed — verified on a scratch httptest harness. Do NOT merge per task.

crueber referenced this issue from a commit

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

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

crueber commented

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

Author

Owner

SECURITY REVIEW — PR #241 (fix/issue-237), verified in scratch worktree against 9db8d7a. Main worktree untouched (still clean on main).

CHECKED-STRING == FETCHED-STRING: PASS. Single derivation path confirmed: NormalizeSource → Params.Source → drive (task.go:77 CloneMirror(ctx, params.Source.URL, …, params.Source.Scheme, params.Source.Host, …)). scrubbedMap (service.go:47), B3 probe + no-op compare (service.go:225,228,289), and import.json provenance (task.go:137,379) all read the same Normalized. TestCloneReceivesCanonicalURL237 proves it through the real Begin→drive→git-argv path: all 5 variants hand git exactly 'https://localhost/x/y'. No second derivation path exists.

CANONICALIZATION: PASS. Lowercase host (url.go:102), default-port strip via Host rebuild (canonicalGenericURL url.go:196 — port dropped by reconstruction, not string-surgery), slash trim + single .git strip (url.go:204-205), userinfo refused 400 (url.go:93-95), non-default ports refused 400 before any gate (url.go:108). Residual variants all fail SAFE, not open: trailing-dot host → allowlist miss → 400; encoded dots (%2Egit)/double .git/.GIT/query/fragment → distinct strings under the SAME host gate (provenance 409 at worst, never a gate bypass); scp-like → ValidateTransport 400 (url.go:126) + Host '' can never match a non-empty allowlist. GitHub :443 folds (test-pinned); http github.com URL upgrades to https canonical — safe direction, gated==cloned.

PORTS: non-default → 400, no legitimate use broken — ssh/git schemes are refused wholesale in v1 anyway (ValidateTransport), and allowlist entries are plain hosts by config validation (validate.go:395), so a ported URL could never match. SSH-scheme default-port folding is moot (refused later) — harmless.

SCHEME RULE: http-without-token allowed, token-over-http still refused 400 (ValidateTransport url.go:131, pinned url_test.go:81 + service_test.go:291). Walk: allowlisted-host-over-http gates on the same host entry as its https twin, but the twins are DISTINCT canonical strings with distinct provenance (test-pinned) — no SSRF bypass (scheme doesn't change which host is fetched; MITM-on-plaintext is the documented residual, https-only knob explicitly deferred in 10_git_import.md). Acceptable per documented decision.

DANGEROUS GATE: PASS. Order in Begin is checkCreate (401 anon, service.go:198) BEFORE dangerous gate (403, service.go:204) — anon→401 first ✓. writer/non-admin→403 ✓, token/oidc admin→202 with real fixture import ✓ (TestBeginDangerousGate237), auth-none (Admin-but-unauthenticated)→403 naming CLI ✓, unknown/empty modes fail closed ✓, nil cfg fails closed ✓. CLI RunHeadless bypasses Begin (service.go:559) — operator --dangerous unaffected ✓. ParseRequest evaluates the flag for SSRF shape only (no principal there) — authority enforced in Begin ✓. UI (Import.jsx:92,225): checkbox posts dangerous verbatim; 401/403 bodies surface in the error box + anon banner — sensible, no UI change needed.

TABLE COVERAGE: every issue row pinned — :443, trailing /, .git/, http twin, uppercase (TestNormalizeCanonical237 + TestCheckSSRFVariantParity237 + TestCloneReceivesCanonicalURL237 + TestVariantGateParityHTTP237), anon/writer/none/admin dangerous (TestDangerousAllowed237 + TestBeginDangerousGate237), canonical-legit 202 (TestLegitimateCanonicalAllowlisted237).

REVIEWER ADDITION (pushed 9db8d7a): TestPortRefusedDespiteAllowlistMatch237 — the one missing variant: non-default port (:8080/:8443 https, :8080 http) refused 400 even when the host IS allowlisted (port rule precedes the gate), plus handler-level userinfo 400 on an allowlisted host.

RESULTS: go test -race ./internal/repoimport/... ok (28s); coverage 96.0% total, new fns 96.8–100% (≥95% gate holds); gofmt clean; go vet clean. No new non-stdlib imports (test adds only internal auth + stdlib). Doc Decisions entries in 10_git_import.md accurate (canonical rule, port rule, scheme decision + deferred knob, dangerous authority, both Concurrency subsections).

MERGE RECOMMENDATION: ready to merge (after CI confirms; close #237 on merge).

SECURITY REVIEW — PR #241 (fix/issue-237), verified in scratch worktree against 9db8d7a. Main worktree untouched (still clean on main). CHECKED-STRING == FETCHED-STRING: PASS. Single derivation path confirmed: NormalizeSource → Params.Source → drive (task.go:77 CloneMirror(ctx, params.Source.URL, …, params.Source.Scheme, params.Source.Host, …)). scrubbedMap (service.go:47), B3 probe + no-op compare (service.go:225,228,289), and import.json provenance (task.go:137,379) all read the same Normalized. TestCloneReceivesCanonicalURL237 proves it through the real Begin→drive→git-argv path: all 5 variants hand git exactly 'https://localhost/x/y'. No second derivation path exists. CANONICALIZATION: PASS. Lowercase host (url.go:102), default-port strip via Host rebuild (canonicalGenericURL url.go:196 — port dropped by reconstruction, not string-surgery), slash trim + single .git strip (url.go:204-205), userinfo refused 400 (url.go:93-95), non-default ports refused 400 before any gate (url.go:108). Residual variants all fail SAFE, not open: trailing-dot host → allowlist miss → 400; encoded dots (%2Egit)/double .git/.GIT/query/fragment → distinct strings under the SAME host gate (provenance 409 at worst, never a gate bypass); scp-like → ValidateTransport 400 (url.go:126) + Host '' can never match a non-empty allowlist. GitHub :443 folds (test-pinned); http github.com URL upgrades to https canonical — safe direction, gated==cloned. PORTS: non-default → 400, no legitimate use broken — ssh/git schemes are refused wholesale in v1 anyway (ValidateTransport), and allowlist entries are plain hosts by config validation (validate.go:395), so a ported URL could never match. SSH-scheme default-port folding is moot (refused later) — harmless. SCHEME RULE: http-without-token allowed, token-over-http still refused 400 (ValidateTransport url.go:131, pinned url_test.go:81 + service_test.go:291). Walk: allowlisted-host-over-http gates on the same host entry as its https twin, but the twins are DISTINCT canonical strings with distinct provenance (test-pinned) — no SSRF bypass (scheme doesn't change which host is fetched; MITM-on-plaintext is the documented residual, https-only knob explicitly deferred in 10_git_import.md). Acceptable per documented decision. DANGEROUS GATE: PASS. Order in Begin is checkCreate (401 anon, service.go:198) BEFORE dangerous gate (403, service.go:204) — anon→401 first ✓. writer/non-admin→403 ✓, token/oidc admin→202 with real fixture import ✓ (TestBeginDangerousGate237), auth-none (Admin-but-unauthenticated)→403 naming CLI ✓, unknown/empty modes fail closed ✓, nil cfg fails closed ✓. CLI RunHeadless bypasses Begin (service.go:559) — operator --dangerous unaffected ✓. ParseRequest evaluates the flag for SSRF shape only (no principal there) — authority enforced in Begin ✓. UI (Import.jsx:92,225): checkbox posts dangerous verbatim; 401/403 bodies surface in the error box + anon banner — sensible, no UI change needed. TABLE COVERAGE: every issue row pinned — :443, trailing /, .git/, http twin, uppercase (TestNormalizeCanonical237 + TestCheckSSRFVariantParity237 + TestCloneReceivesCanonicalURL237 + TestVariantGateParityHTTP237), anon/writer/none/admin dangerous (TestDangerousAllowed237 + TestBeginDangerousGate237), canonical-legit 202 (TestLegitimateCanonicalAllowlisted237). REVIEWER ADDITION (pushed 9db8d7a): TestPortRefusedDespiteAllowlistMatch237 — the one missing variant: non-default port (:8080/:8443 https, :8080 http) refused 400 even when the host IS allowlisted (port rule precedes the gate), plus handler-level userinfo 400 on an allowlisted host. RESULTS: go test -race ./internal/repoimport/... ok (28s); coverage 96.0% total, new fns 96.8–100% (≥95% gate holds); gofmt clean; go vet clean. No new non-stdlib imports (test adds only internal auth + stdlib). Doc Decisions entries in 10_git_import.md accurate (canonical rule, port rule, scheme decision + deferred knob, dangerous authority, both Concurrency subsections). MERGE RECOMMENDATION: ready to merge (after CI confirms; close #237 on merge).

crueber closed this issue

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

crueber commented

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

Author

Owner

Fixed by PR #241 incl. review port/userinfo tests (canonical gated URL is the fetched URL; dangerous admin-gated; 96.0% coverage), merged. Closing.

crueber referenced this issue from a commit

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

Merge pull request 'Fix #237: SSRF allowlist bypass + anon dangerous' (#241) from fix/issue-237 into main

crueber referenced this issue

2026-09-09 17:42:57 +00:00