Skip to content

test(cli): fail the cmd package on any off-host request - #2229

Merged
cristim merged 1 commit into
mainfrom
test/offhost-request-guard
Oct 16, 2026
Merged

cristim merged 1 commit into
mainfrom
test/offhost-request-guard

Conversation

@cristim

@cristim cristim commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

Closes #2221

Summary

#2215, #2225 and #2227 removed the cmd tests' off-host requests. Nothing stopped a new test from adding one back: without credentials, an SDK call just fails slowly, and the test often still passes. This PR adds a package-level guard so that any off-host request fails the package.

  • cmd/offhost_guard_test.go (new) starts a local proxy that records and refuses every proxied request, CONNECT and plain-HTTP absolute-URI alike.
  • TestMain points HTTP_PROXY/HTTPS_PROXY (both upper- and lower-case) at that proxy. NO_PROXY covers loopback, so httptest servers and the SDK's loopback stubs keep working; Go's proxy logic always bypasses localhost and loopback IPs anyway.
  • After m.Run(), if anything was recorded, the package prints the hosts and exits 1, even when every test passed.
  • Child processes that inherit the parent environment (the IAM, GCP signal and extended-support children) are guarded too. A CUDLY_TEST_OFFHOST_GUARD marker keeps a child from installing a second guard. That second guard would overwrite a proxy the parent set on purpose (purchase_confirmation_test.go:150).
  • The recommendation-completeness child builds an explicit environment with its own fixture proxy, which already blocks egress, so the guard is skipped when CUDLY_COMPLETENESS_CHILD is set.
  • The change is test-only.

Blind spots, documented in the helper:

  • Clients built with Proxy: nil (configure_gcp_iam_test.go:154, configure_gcp_signal_test.go:220).
  • Tests that set their own proxy env.
  • Non-HTTP dials.

Evidence

All runs are local on macOS with GOTOOLCHAIN=go1.26.9.

  • go test -race ./cmd/ passes with 0 recorded requests.
  • Probe: a temporary test calling http.Get("https://example.com") and http.Get("http://example.org/x") makes the package exit 1 with FAIL: tests made off-host requests, listing CONNECT example.com:443 and GET example.org. The author and an independent reviewer each reproduced this. A failing test still exits non-zero.
  • Deletion probe: without the marker env var, children replace the proxy that purchase_confirmation_test.go sets.
  • go test ./cmd/ -shuffle=on -count=2 passes.
  • make build passes, go vet ./... is clean, golangci-lint v2.10.1 reports 0 issues, and scripts/test-git-secrets-allowlist.sh passes 44/44.
  • Review: two passes by an independent non-author reviewer (sonnet, high) found no actionable findings at d192c9a3. One cosmetic low remains: the failure line prints after go test's PASS line, but the exit code is correct.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Tests
    • Test runs now detect and report attempted off-host HTTP requests, and fail when such requests are recorded or the guard cannot start.
    • Requests to loopback hosts remain exempt.

TestMain starts a local listener, points the proxy env vars at it and
records and refuses every proxied request. After m.Run the package exits 1
naming the hosts if any were recorded. Loopback stays exempt via NO_PROXY.

Closes #2221

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cristim cristim added priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/internal Team-internal only effort/s Hours type/chore Maintenance / non-user-visible triaged Item has been triaged labels Oct 16, 2026
@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-cli/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 1c5a3b29-b37f-4603-af28-76fcff68a369
📥 Commits

Reviewing files that changed from the base of the PR and between cfdfdf2 and 4606fac.

📒 Files selected for processing: 2
  • cmd/main_test.go
  • cmd/offhost_guard_test.go

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

📝 Walkthrough

The cmd test package now starts a local proxy guard before running tests. The guard records and refuses requests. After tests finish, the package reports any recorded requests and exits with failure.

Changes: Off-host test guard

Layer / File(s) Summary
Guard lifecycle
cmd/main_test.go, cmd/offhost_guard_test.go
TestMain starts the guard and exits if startup fails. The guard configures proxy environment variables, records and refuses requests, and returns sorted records. After tests run, reportOffhost prints recorded requests and causes a failing exit status. The setup skips when either guard marker is set. Comments document the guard’s scope and blind spots.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Sequence Diagram(s)

sequenceDiagram
  participant TestMain
  participant ProxyAwareClient
  participant LocalHTTPListener
  TestMain->>LocalHTTPListener: start guard and configure proxy environment
  ProxyAwareClient->>LocalHTTPListener: send request through proxy
  LocalHTTPListener-->>ProxyAwareClient: return HTTP 403 and record method and host
  TestMain->>TestMain: report records and set exit status
Loading
Merge Risk: ⚪ Minimal · up to 4606f

No actionable issue remains from this review; the change is mergeable after normal checks.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: making the cmd package tests fail when they issue off-host requests.
Linked Issues check Passed Issue #2221 requires a package-level cmd guard. cmd/main_test.go starts the guard before m.Run() and changes the package result to failure when recorded requests exist. `cmd/offhost_guard_test.g…
Out of Scope Changes check Passed The reviewed changes are limited to cmd/main_test.go and the new cmd/offhost_guard_test.go. They implement the package-level off-host request guard, child-process handling, reporting, and loopback…
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files.
Full details: Linked Issues check

Explanation

Issue #2221 requires a package-level cmd guard. cmd/main_test.go starts the guard before m.Run() and changes the package result to failure when recorded requests exist. cmd/offhost_guard_test.go routes upper- and lower-case HTTP proxy variables to a local listener, records CONNECT and absolute-URI HTTP requests, refuses them, preserves loopback access with NO_PROXY, and reports sorted hosts. The helper documents the stated blind spots. The reported scratch-copy probe and test runs provide the requested regression evidence.

Full details: Out of Scope Changes check

Explanation

The reviewed changes are limited to cmd/main_test.go and the new cmd/offhost_guard_test.go. They implement the package-level off-host request guard, child-process handling, reporting, and loopback exceptions required by issue #2221. No unrelated production behavior is changed.

✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@cristim

cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member Author

Independent review verdict: CLEAN at 4606fac

Reviewer: fresh Claude agent (Sonnet, high effort), not the author. Coordinator: claude-cc-reviewer.

What was checked:

  • Mechanism: cmd/offhost_guard_test.go:25-70 points HTTP(S)_PROXY/http(s)_proxy at a local listener that records and refuses requests, with NO_PROXY exempting loopback. The doc comment (:19-23) and the body claim only proxy-aware HTTP clients, and they list the blind spots.
  • Probe, AWS SDK: an STS GetCallerIdentity against the real endpoint makes the package exit 1 with "tests made off-host requests ... CONNECT sts.us-east-1.amazonaws.com:443". SDK traffic is caught.
  • Probe, raw socket: a raw net.DialTimeout to a public IP is not caught. This is the documented non-HTTP blind spot, and it is non-blocking.
  • Probe, loopback: an httptest server via http.Get passes.
  • Ordering: a single TestMain (main_test.go:50). The guard starts after the stubs (:60) and is checked after m.Run() (:66-68). purchase_confirmation_test.go:151 overrides the proxy via t.Setenv and restores it correctly.
  • go vet ./cmd/... is clean. go test -race -count=1 ./cmd/... passes 1469, and -shuffle=on -count=3 passes 4407.

Non-blocking: GCP and Azure SDK clients were not probed; the body lists Proxy=nil GCP clients as unguarded. The failure line prints after go test's PASS line, as the body notes.

Gate:

@cristim
cristim merged commit e0a1db6 into main Oct 16, 2026
12 checks passed
@cristim
cristim deleted the test/offhost-request-guard branch October 16, 2026 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/chore Maintenance / non-user-visible urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(cli): fail the cmd package on any off-host request

1 participant