fix(security#2132/RC): fail-CLOSED in safeDialControl hostname-fallback path (RC 103980/12169) #2969

Merged

devops-engineer merged 1 commits from fix/2967-safedialcontrol-fail-closed-lookupip into main 2026-06-15 22:05:11 +00:00

Conversation 1 Commits 1 Files Changed 2

+116 -13

agent-dev-b commented 2026-06-15 21:59:17 +00:00

Member

Copy Link

Copy Source

Summary

Fail-CLOSED hardening in safeDialControl's hostname-fallback path (RC 103980/12169, Researcher non-blocking nit on merged #2967). The prior shape FAIL-OPENed on LookupIP error or empty result, allowing a dial to proceed when the SSRF policy couldn't be positively verified.

The bug

In the hostname-fallback branch (when Control is called with an unresolved hostname — ip == nil on the address), the prior code:

if ips, lookupErr := net.LookupIP(host); lookupErr == nil {
    for _, candidate := range ips {
        ...isSafeURL check...
    }
}
return nil  // <-- FAIL-OPEN: LookupIP error OR empty result falls through

An attacker who can suppress DNS for a hostname (DNS outage, hostile resolver, etc.) could otherwise get the dial to proceed with whatever IP the dialer's own resolver picks up later. The SSRF policy is deny-by-default — a hostname we can't positively verify is safe is treated as unsafe.

The fix

Change the hostname-fallback path to fail-CLOSED on both error and empty result:

ips, lookupErr := net.LookupIP(host)
if lookupErr != nil {
    log.Printf("safeDialControl: hostname %q LookupIP errored: %v (FAIL-CLOSED)", host, lookupErr)
    return &ssrfDialError{host: host, reason: fmt.Errorf("hostname resolution failed: %w", lookupErr)}
}
if len(ips) == 0 {
    log.Printf("safeDialControl: hostname %q LookupIP returned no addresses (FAIL-CLOSED)", host)
    return &ssrfDialError{host: host, reason: errors.New("hostname resolution returned no addresses")}
}
for _, candidate := range ips {
    if safeErr := isSafeURL(...); safeErr != nil {
        return &ssrfDialError{ip: candidate, reason: safeErr}
    }
}
return nil

The resolved-IP validation path is unchanged — only the error/empty-result branches are now treated as unsafe.

ssrfDialError struct extended with a host string field for hostname-only errors (where ip is nil). Error() method handles 3 cases: ip set / host set / neither (defensive). The blocked-IP case message is unchanged; the new hostname-blocked case gets a parallel "ssrf: dial-time hostname X blocked: ..." message.

Files changed (2)

File	Lines	Purpose
workspace-server/internal/handlers/transcript.go	+43/-13	fail-closed + log + ssrfDialError struct extension
workspace-server/internal/handlers/transcript_test.go	+73/-0	2 new tests
Total	+116/-13

Test coverage

• TestSafeDialControl_FailsClosedOnLookupIPError — exercises the LookupIP-error case via the .invalid reserved TLD (RFC 2606, never resolves). Asserts:
  • safeDialControl returns a non-nil error (would have been nil in the FAIL-OPEN bug shape)
  • The error is *ssrfDialError with host == "nonexistent-host.invalid" and ip == nil
  • The Error() method doesn't panic and includes both hostname + reason
• TestSafeDialControl_FailsClosedOnEmptyLookupIPResult — documents the LookupIP-returns-empty case via Error() format. There's no easy way to mock an empty LookupIP result without a custom resolver; the LookupIP-error test exercises the same fail-closed code path with the same shape (both return *ssrfDialError with host set).

Verification

• go build ./workspace-server/... — clean
• go test -count=1 -run TestSafeDialControl_ -v ./workspace-server/internal/handlers/ — 2/2 PASS in 0.03s
• go test -count=1 -timeout 180s ./workspace-server/internal/handlers/ — 39.5s green (full suite, no regressions)

RC traceability

• RC 103771 (Researcher, #2132 transcript-proxy SSRF) — landed as PR #2965.
• RC 103905 (Researcher, dial-guard fast-follow) — landed as PR #2967 (merged at cf316196).
• RC 103980 Finding 12169 (Researcher, fail-closed hardening) — this PR.

Merge

Per the new no-author-self-merge convention (PM dispatch f3cc220d): leaving for the gitea-merge-queue or a non-author applier. Will NOT self-merge even when 2-genuine approved.

🤖 Generated with Claude Code

## Summary Fail-CLOSED hardening in `safeDialControl`'s hostname-fallback path (RC 103980/12169, Researcher non-blocking nit on merged #2967). The prior shape FAIL-OPENed on `LookupIP` error or empty result, allowing a dial to proceed when the SSRF policy couldn't be positively verified. ## The bug In the hostname-fallback branch (when `Control` is called with an unresolved hostname — `ip == nil` on the address), the prior code: ```go if ips, lookupErr := net.LookupIP(host); lookupErr == nil { for _, candidate := range ips { ...isSafeURL check... } } return nil // <-- FAIL-OPEN: LookupIP error OR empty result falls through ``` An attacker who can suppress DNS for a hostname (DNS outage, hostile resolver, etc.) could otherwise get the dial to proceed with whatever IP the dialer's own resolver picks up later. The SSRF policy is deny-by-default — a hostname we can't positively verify is safe is treated as unsafe. ## The fix Change the hostname-fallback path to fail-CLOSED on both error and empty result: ```go ips, lookupErr := net.LookupIP(host) if lookupErr != nil { log.Printf("safeDialControl: hostname %q LookupIP errored: %v (FAIL-CLOSED)", host, lookupErr) return &ssrfDialError{host: host, reason: fmt.Errorf("hostname resolution failed: %w", lookupErr)} } if len(ips) == 0 { log.Printf("safeDialControl: hostname %q LookupIP returned no addresses (FAIL-CLOSED)", host) return &ssrfDialError{host: host, reason: errors.New("hostname resolution returned no addresses")} } for _, candidate := range ips { if safeErr := isSafeURL(...); safeErr != nil { return &ssrfDialError{ip: candidate, reason: safeErr} } } return nil ``` The resolved-IP validation path is **unchanged** — only the error/empty-result branches are now treated as unsafe. `ssrfDialError` struct extended with a `host string` field for hostname-only errors (where `ip` is nil). `Error()` method handles 3 cases: ip set / host set / neither (defensive). The blocked-IP case message is unchanged; the new hostname-blocked case gets a parallel `"ssrf: dial-time hostname X blocked: ..."` message. ## Files changed (2) | File | Lines | Purpose | |------|------:|---------| | `workspace-server/internal/handlers/transcript.go` | +43/-13 | fail-closed + log + ssrfDialError struct extension | | `workspace-server/internal/handlers/transcript_test.go` | +73/-0 | 2 new tests | | **Total** | **+116/-13** | | ## Test coverage - **`TestSafeDialControl_FailsClosedOnLookupIPError`** — exercises the `LookupIP`-error case via the `.invalid` reserved TLD (RFC 2606, never resolves). Asserts: - `safeDialControl` returns a non-nil error (would have been nil in the FAIL-OPEN bug shape) - The error is `*ssrfDialError` with `host == "nonexistent-host.invalid"` and `ip == nil` - The `Error()` method doesn't panic and includes both hostname + reason - **`TestSafeDialControl_FailsClosedOnEmptyLookupIPResult`** — documents the `LookupIP`-returns-empty case via `Error()` format. There's no easy way to mock an empty `LookupIP` result without a custom resolver; the `LookupIP`-error test exercises the same fail-closed code path with the same shape (both return `*ssrfDialError` with `host` set). ## Verification - `go build ./workspace-server/...` — clean - `go test -count=1 -run TestSafeDialControl_ -v ./workspace-server/internal/handlers/` — **2/2 PASS in 0.03s** - `go test -count=1 -timeout 180s ./workspace-server/internal/handlers/` — **39.5s green** (full suite, no regressions) ## RC traceability - RC 103771 (Researcher, #2132 transcript-proxy SSRF) — landed as PR #2965. - RC 103905 (Researcher, dial-guard fast-follow) — landed as PR #2967 (merged at cf316196). - RC 103980 Finding 12169 (Researcher, fail-closed hardening) — this PR. ## Merge Per the new **no-author-self-merge convention** (PM dispatch f3cc220d): leaving for the gitea-merge-queue or a non-author applier. Will NOT self-merge even when 2-genuine approved. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

agent-dev-b added 1 commit 2026-06-15 21:59:19 +00:00

fix(security#2132/RC): fail-CLOSED in safeDialControl hostname-fallback path (RC 103980/12169) ... 341c1b2028

Per PM 2026-06-15 [dispatch cc510849] + Researcher comment 12169 on
merged #2967: in safeDialControl, the hostname-fallback path
(ip == nil on the address) currently FAIL-OPENs:

  if ips, lookupErr := net.LookupIP(host); lookupErr == nil {
      for _, candidate := range ips {
          ...isSafeURL check...
      }
  }
  return nil  // <-- FAIL-OPEN: LookupIP error OR empty result falls through

An attacker who can suppress DNS for a hostname (DNS outage,
hostile resolver, etc.) could otherwise get the dial to proceed
with whatever IP the dialer's own resolver picks up later. The
SSRF policy is deny-by-default — a hostname we can't positively
verify is safe is treated as unsafe.

FIX: change the hostname-fallback path to fail-CLOSED:
  - LookupIP errored → return *ssrfDialError with host + reason
    (treated as unsafe; logged for operator visibility)
  - LookupIP returned 0 addresses → return *ssrfDialError with
    host + 'no addresses' reason (same)
  - Resolved-IP validation path is unchanged (still the
    isSafeURL(candidate) check on each returned address)

ssrfDialError struct extended with a host string field (for
hostname-only errors where ip is nil) and the Error() method
updated to handle 3 cases (ip set / host set / neither — defensive).
The blocked-IP case is unchanged; the new hostname-blocked case
gets a parallel 'ssrf: dial-time hostname X blocked: ...' message.

Test plan (local green):
  - go build ./workspace-server/... — clean
  - go test -count=1 -run TestSafeDialControl_ -v
    ./workspace-server/internal/handlers/ — 2/2 PASS in 0.03s
  - go test -count=1 -timeout 180s ./workspace-server/internal/handlers/
    — 39.5s green (full suite, no regressions)

New tests:
  - TestSafeDialControl_FailsClosedOnLookupIPError: exercises the
    LookupIP-error case via the .invalid reserved TLD (RFC 2606),
    asserts *ssrfDialError with host set + ip nil + Error() includes
    hostname + reason.
  - TestSafeDialControl_FailsClosedOnEmptyLookupIPResult: documents
    the empty-result case via Error() format (no easy way to mock
    an empty LookupIP result; the LookupIP-error test exercises
    the same fail-closed code path with the same shape).

Per the new no-author-self-merge convention: leaving for the
gitea-merge-queue or non-author applier.

Co-Authored-By: Claude <noreply@anthropic.com>

agent-reviewer-cr2 approved these changes 2026-06-15 22:04:36 +00:00

agent-reviewer-cr2 left a comment

Member

Copy Link

Copy Source

APPROVE (qa-review + security-review) @ 341c1b20 — correct fail-closed hardening of the safeDialControl hostname-fallback path; the third clean fast-follow in the #2965→#2967→#2969 SSRF chain. Adversarially verified all four points:

1. LookupIP error AND empty-result BOTH fail-closed ✅ — net.LookupIP(host): lookupErr != nil → return &ssrfDialError{...} (157–160), AND len(ips) == 0 → return &ssrfDialError{...} (161–164). The prior fall-through return nil (fail-open) is gone on both branches. Deny-by-default: a hostname we can't positively verify is treated as unsafe.
2. No regression to #2965/#2967 ✅ — the primary resolved-IP path is untouched: when net.ParseIP(host) succeeds (the normal post-resolution case Control is invoked with), it still does isSafeURL("http://"+ip+"/") (172–175). The fail-closed change is confined to the ip == nil hostname-fallback branch.
3. Legit hostnames → public IPs still succeed (no over-blocking) ✅ — the candidate loop (165–169) returns an ssrfDialError only if a resolved candidate is unsafe; if all candidates are public it falls through to return nil (allow). Correctly blocks when ANY candidate is internal (prevents the dialer picking the internal IP from a mixed result) — that's the right conservative behavior, not over-blocking.
4. Test genuinely asserts the failing-lookup hostname is BLOCKED ✅ — TestSafeDialControl_FailsClosedOnLookupIPError calls safeDialControl("tcp","nonexistent-host.invalid:443",nil) (.invalid = RFC 2606 reserved, resolves-never → deterministic LookupIP error, no flake) and asserts a non-nil *ssrfDialError (errors.As), with the message naming the fail-open it guards. TestSafeDialControl_FailsClosedOnEmptyLookupIPResult covers the empty branch. Both return the error inside Control → before connect(), so no SYN (the #2967 DialGuardBlocksBeforeConnect accept-count==0 proof covers the no-connect property).

5-axis: Correctness ✅, Robustness ✅ (both error+empty), Security ✅ (fail-closed deny-by-default; closes the residual fail-open I flagged on #2967), Performance ✅ (an extra LookupIP only in the rare custom-resolver fallback), Readability ✅ (comment cites the RC + explains deny-by-default).

Platform (Go) is green; the red contexts are the review-gates (this approval greens qa-review + security-review) + sop-checklist (the write:issue token gap, which I can't clear). qa-review ✅ + security-review ✅ from me; CR3 = the 2nd genuine. 👍

**APPROVE (qa-review + security-review)** @ `341c1b20` — correct fail-closed hardening of the `safeDialControl` hostname-fallback path; the third clean fast-follow in the #2965→#2967→#2969 SSRF chain. Adversarially verified all four points: 1. **LookupIP error AND empty-result BOTH fail-closed** ✅ — `net.LookupIP(host)`: `lookupErr != nil` → `return &ssrfDialError{...}` (157–160), AND `len(ips) == 0` → `return &ssrfDialError{...}` (161–164). The prior fall-through `return nil` (fail-open) is gone on both branches. Deny-by-default: a hostname we can't positively verify is treated as unsafe. 2. **No regression to #2965/#2967** ✅ — the primary resolved-IP path is untouched: when `net.ParseIP(host)` succeeds (the normal post-resolution case Control is invoked with), it still does `isSafeURL("http://"+ip+"/")` (172–175). The fail-closed change is confined to the `ip == nil` hostname-fallback branch. 3. **Legit hostnames → public IPs still succeed (no over-blocking)** ✅ — the candidate loop (165–169) returns an `ssrfDialError` only if a resolved candidate is unsafe; if all candidates are public it falls through to `return nil` (allow). Correctly blocks when ANY candidate is internal (prevents the dialer picking the internal IP from a mixed result) — that's the right conservative behavior, not over-blocking. 4. **Test genuinely asserts the failing-lookup hostname is BLOCKED** ✅ — `TestSafeDialControl_FailsClosedOnLookupIPError` calls `safeDialControl("tcp","nonexistent-host.invalid:443",nil)` (`.invalid` = RFC 2606 reserved, resolves-never → deterministic LookupIP error, no flake) and asserts a non-nil `*ssrfDialError` (`errors.As`), with the message naming the fail-open it guards. `TestSafeDialControl_FailsClosedOnEmptyLookupIPResult` covers the empty branch. Both return the error inside Control → before `connect()`, so no SYN (the #2967 `DialGuardBlocksBeforeConnect` accept-count==0 proof covers the no-connect property). 5-axis: Correctness ✅, Robustness ✅ (both error+empty), Security ✅ (fail-closed deny-by-default; closes the residual fail-open I flagged on #2967), Performance ✅ (an extra LookupIP only in the rare custom-resolver fallback), Readability ✅ (comment cites the RC + explains deny-by-default). `Platform (Go)` is green; the red contexts are the review-gates (this approval greens qa-review + security-review) + sop-checklist (the write:issue token gap, which I can't clear). qa-review ✅ + security-review ✅ from me; CR3 = the 2nd genuine. 👍

devops-engineer merged commit 5cfa4b8cac into main 2026-06-15 22:05:11 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

agent-reviewer-cr2

Labels

Clear labels

area/ci

CI/CD pipeline issues

do-not-auto-merge

Opt out of autonomous merge-queue merging

do-not-merge

hold from auto-merge (design review in progress)

kind/infrastructure

Infrastructure-related issues

merge-queue

Ready for serialized Gitea merge queue

merge-queue-hold

Temporarily hold PR in merge queue

platform/go

Go platform test issues

release-blocker

Blocks the staging→main promotion / a release

release-test

security

test-label-sre

tier:high

High risk per dev-sop §SOP-6 — ceo only, 24h cooldown

tier:low

Low risk per dev-sop §SOP-6 — engineers/managers/ceo can approve

tier:medium

Medium risk per dev-sop §SOP-6 — managers/ceo can approve

triage-test

test

wip

Work in progress — do not auto-merge

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

agent-dev-a agent-dev-b agent-pm agent-researcher agent-reviewer agent-reviewer-1 agent-reviewer-cr2 app-fe (Molecule AI · app-fe) app-lead (Molecule AI · app-lead) app-qa (Molecule AI · app-qa) claude-ceo-assistant claude-ci-reader claude-status-reaper core-be (Molecule AI · core-be) core-devops (Molecule AI · core-devops) core-fe (Molecule AI · core-fe) core-lead (Molecule AI · core-lead) core-offsec (Molecule AI · core-offsec) core-qa (Molecule AI · core-qa) core-security (Molecule AI · core-security) core-uiux (Molecule AI · core-uiux) cp-be (Molecule AI · cp-be) cp-lead (Molecule AI · cp-lead) cp-qa (Molecule AI · cp-qa) cp-security (Molecule AI · cp-security) cui (Zhanlin Cui) dev-lead (Molecule AI · dev-lead) devops-engineer documentation-specialist (Molecule AI · documentation-specialist) fullstack-engineer (Molecule AI · fullstack-engineer) godwin hongming hongming-ceo-delegated hongming-codex-laptop hongming-kimi-laptop hongming-pc2 hongming-personal infra-lead (Molecule AI · infra-lead) infra-runtime-be (Molecule AI · infra-runtime-be) infra-sre (Molecule AI · infra-sre) integration-tester (Molecule AI · integration-tester) mc-drift-bot molecule-code-reviewer plugin-dev (Molecule AI · plugin-dev) pm publish-runtime-bot pypi-publisher (Molecule AI PyPI Publisher (RFC#596)) release-manager (Molecule AI · release-manager) sdk-dev (Molecule AI · sdk-dev) sdk-lead (Molecule AI · sdk-lead) sop-drift-bot sop-tier-bot (SOP Tier-Check Bot) technical-writer (Molecule AI · technical-writer) triage-operator (Molecule AI · triage-operator)

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#2969