[minor-12] `isPrivateIP` range gaps (verify against new egress table first) #96

New issue

Closed

opened 2026-09-05 02:40:27 +00:00 by crueber · 3 comments

crueber commented

2026-09-05 02:40:27 +00:00

Owner

[minor-12] `isPrivateIP` range gaps (verify against new egress table first)

internal/repoimport/url.go:235-263 reportedly misses 198.18.0.0/15 (benchmark), 192.0.2.0/24, 198.51.100.0/24, 203.0.113.0/24. NOTE: PR #88 added the shared internal/egress range table (RFC1918, CGNAT incl. 198.18/15, TEST-NETs, mapped-v6) — first verify whether url.go now delegates to it (then this issue is already fixed: prove with the range tests + close). If not, extend the ranges + table-driven tests.

Fix

Delegate to internal/egress if not already done, or add the missing ranges with tests. Coverage gate holds.

Acceptance criteria

All listed ranges (and mapped-v6 forms) blocked; tests green -race. Or: verified already-covered, closed with evidence.

# [minor-12] `isPrivateIP` range gaps (verify against new egress table first) `internal/repoimport/url.go:235-263` reportedly misses `198.18.0.0/15` (benchmark), `192.0.2.0/24`, `198.51.100.0/24`, `203.0.113.0/24`. NOTE: PR #88 added the shared `internal/egress` range table (RFC1918, CGNAT incl. 198.18/15, TEST-NETs, mapped-v6) — first verify whether `url.go` now delegates to it (then this issue is already fixed: prove with the range tests + close). If not, extend the ranges + table-driven tests. ## Fix Delegate to `internal/egress` if not already done, or add the missing ranges with tests. Coverage gate holds. ## Acceptance criteria - [ ] All listed ranges (and mapped-v6 forms) blocked; tests green `-race`. Or: verified already-covered, closed with evidence.

crueber referenced this issue from a commit

2026-09-05 03:21:14 +00:00

Fix #96: isPrivateIP delegates to shared egress table

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

2026-09-05 03:21:15 +00:00

Fix #96: private-IP range gaps #105

crueber commented

2026-09-05 03:21:23 +00:00

Author

Owner

Verified against the egress table: the gap was real. PR #88's internal/egress table already covers all four listed ranges (+ mapped-v6 via Unmap) with passing range tests - but internal/repoimport/url.go:isPrivateIP kept its own list and never delegated, so the ranges were still reachable via import sources. Fix is in PR #105: isPrivateIP now denies loopback explicitly and defers to egress.BlockedIP (fail-closed on unparseable input), with extended table tests in both packages. Evidence: go test -race green, cover repoimport 95.7%, egress 97.6%.

crueber referenced this issue from a commit

2026-09-05 03:24:16 +00:00

Fix #96 review: stale checkPrivate comment + fail-closed pin

crueber commented

2026-09-05 03:24:22 +00:00

Author

Owner

Review of PR #105 (fix/issue-96, e355eaa; plus review fixup 18d84fb):

Delegation — correct. internal/repoimport/url.go:242 isPrivateIP denies loopback explicitly (url.go:243) then defers to egress.BlockedIP (url.go:250). The explicit deny is load-bearing: egress.go:59-78 deliberately omits 127/8+::1 (dev/CI webhooks target localhost), so without it mapped/plain loopback would pass the shared table. Loopback-deny breaks no legitimate flow: local e2e/dev imports all use file:// (separate AllowFile gate, url.go:179-184; fakes_test.go:143,143+), never https-to-loopback; the only https-loopback escape is allow_private_networks=true (url_test.go:121 pins the bypass). TestCheckSSRF 'generic allowlisted loopback blocked' (url_test.go:125) confirms allowlist does not override the deny.

Fail-closed — correct. Unparseable (AddrFromSlice !ok, url.go:246-249, incl. nil from net.ParseIP) returns true. Pinned by review fixup: cover_test.go TestIsPrivateIPShapes {'bogus', true}.

Ranges — all four gaps + mapped-v6 pinned. cover_test.go:200-206 covers 198.18/15 bounds, TEST-NET-1/2/3, 240/4, ::/128, 2001:db8::/32, and ::ffff: mapped forms (incl. ::ffff:127.0.0.1 true, ::ffff:8.8.8.8 false). egress_test.go adds mapped TEST-NET-2/3 pins.

Screening consistency — single validate-time screen, no drift. CheckSSRF is called only from ParseRequest (service.go:136); runImport/clone (task.go:77) does not re-screen, so there is no validate-vs-delivery table pair to disagree. DNS-TOCTOU residual is documented in both places it must be: url.go header (R1 S5 note) and doc 10 SSRF paragraph.

Egress untouched — git diff confirms internal/egress/egress.go has zero changes; only egress_test.go +2 lines. Webhook behavior unchanged by construction; egress suite green (below).

Imports/laws — url.go adds only net/netip (stdlib) + internal/egress (internal). No third-party change (law 1 clean). Same-change doc update per law 12 (doc 10 SSRF para + Decisions entry 'SSRF deny-private delegates...' — accurate: names loopback divergence, mapped-v6, fail-closed). Review fixup also corrected the stale checkPrivate one-liner (url.go:204) that still carried the pre-delegation enumeration.

Verification (scratch /tmp/opencode/wt96 @ 18d84fb, main worktree untouched): go test -race ./internal/repoimport/... ./internal/egress/... green; coverage repoimport 95.7%, egress 97.6% (gate holds); gofmt + go vet clean. Review fixup re-tested (targeted + full suite) before push (origin/fix/issue-96 e355eaa..18d84fb).

MERGE RECOMMENDATION: ready to merge.

Review of PR #105 (fix/issue-96, e355eaa; plus review fixup 18d84fb): Delegation — correct. internal/repoimport/url.go:242 isPrivateIP denies loopback explicitly (url.go:243) then defers to egress.BlockedIP (url.go:250). The explicit deny is load-bearing: egress.go:59-78 deliberately omits 127/8+::1 (dev/CI webhooks target localhost), so without it mapped/plain loopback would pass the shared table. Loopback-deny breaks no legitimate flow: local e2e/dev imports all use file:// (separate AllowFile gate, url.go:179-184; fakes_test.go:143,143+), never https-to-loopback; the only https-loopback escape is allow_private_networks=true (url_test.go:121 pins the bypass). TestCheckSSRF 'generic allowlisted loopback blocked' (url_test.go:125) confirms allowlist does not override the deny. Fail-closed — correct. Unparseable (AddrFromSlice !ok, url.go:246-249, incl. nil from net.ParseIP) returns true. Pinned by review fixup: cover_test.go TestIsPrivateIPShapes {'bogus', true}. Ranges — all four gaps + mapped-v6 pinned. cover_test.go:200-206 covers 198.18/15 bounds, TEST-NET-1/2/3, 240/4, ::/128, 2001:db8::/32, and ::ffff: mapped forms (incl. ::ffff:127.0.0.1 true, ::ffff:8.8.8.8 false). egress_test.go adds mapped TEST-NET-2/3 pins. Screening consistency — single validate-time screen, no drift. CheckSSRF is called only from ParseRequest (service.go:136); runImport/clone (task.go:77) does not re-screen, so there is no validate-vs-delivery table pair to disagree. DNS-TOCTOU residual is documented in both places it must be: url.go header (R1 S5 note) and doc 10 SSRF paragraph. Egress untouched — git diff confirms internal/egress/egress.go has zero changes; only egress_test.go +2 lines. Webhook behavior unchanged by construction; egress suite green (below). Imports/laws — url.go adds only net/netip (stdlib) + internal/egress (internal). No third-party change (law 1 clean). Same-change doc update per law 12 (doc 10 SSRF para + Decisions entry 'SSRF deny-private delegates...' — accurate: names loopback divergence, mapped-v6, fail-closed). Review fixup also corrected the stale checkPrivate one-liner (url.go:204) that still carried the pre-delegation enumeration. Verification (scratch /tmp/opencode/wt96 @ 18d84fb, main worktree untouched): go test -race ./internal/repoimport/... ./internal/egress/... green; coverage repoimport 95.7%, egress 97.6% (gate holds); gofmt + go vet clean. Review fixup re-tested (targeted + full suite) before push (origin/fix/issue-96 e355eaa..18d84fb). MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-05 03:24:47 +00:00

crueber referenced this issue from a commit

2026-09-05 03:24:47 +00:00

Merge pull request 'Fix #96: private-IP range gaps' (#105) from fix/issue-96 into main

crueber commented

2026-09-05 03:24:50 +00:00

Author

Owner

Fixed by PR #105 incl. review fixups (real gap: url.go never delegated; now loopback-deny + shared table; 95.7%/97.6% coverage), merged. Closing.

crueber added the

webhooks