[IAmJSD]

Root cause

Two internal/api unit tests built real AWS clients:

• TestPerAccountPerms_GetRIUtilization_ScopedWithinAllowedProceedsToAWS sealed awsCfgOnce with an empty aws.Config and injected no client factories. getRIUtilization therefore built the real Cost Explorer client and sent an anonymous request to ce.us-east-1.amazonaws.com (3 attempts).
• TestGetExchangeQuote_InScopeDeploymentAccountPassesGate seeded h.awsCfg with failing credentials, but getExchangeQuote calls exchange.GetExchangeQuote, which ignores h.awsCfg and runs its own LoadDefaultConfig. The default credential chain then probed IMDS (12 requests, about 13 s).

Two more quote tests (TestGetExchangeQuote_ValidOfferingIDPassesFormat, TestGetExchangeQuote_EmptyRegionResolvesFromSDK) also went through the real exchange.GetExchangeQuote. They only stayed offline because no AWS region was set (or because of an upstream target_count check). With AWS_REGION and credentials in the environment, ValidOfferingIDPassesFormat sent GetReservedInstancesExchangeQuote to ec2.eu-west-1.amazonaws.com (shown below).

Fix

• Production seam (no behavior change): a new Handler.exchangeQuoteFn field and an awsExchangeQuoter() accessor. They mirror the existing executeExchangeFn / awsExchangeExecutor(). getExchangeQuote calls through the accessor, which returns exchange.GetExchangeQuote when the field is nil, so production is unchanged.

• Tests:

  • The RI utilization test now injects the existing reshapeRecsFactory / reshapeEC2Factory seams using the package's counting stubs. It asserts that the Cost Explorer fetch was reached and checks the rows returned.
  • The four quote tests inject a recording exchangeQuoteFn stub. Their assertions are stricter than before: the out-of-scope test must not reach the quote, and the in-scope test must reach it once with the resolved region. The old soft if err != nil { NotContains } checks are replaced with hard assertions on the call and the region.

• Guard (internal/api/main_nonet_test.go): a TestMain that extends the feat(ladder): prefetch commitment visibility for the tranche reconciler #852 recording-proxy guard. Before any test runs, it:

  • removes every AWS_* variable and points AWS_CONFIG_FILE / AWS_SHARED_CREDENTIALS_FILE at the null device. Local runs then resolve the same empty identity as CI and can never use a developer's real credentials.
  • routes proxy-aware clients into a local listener that records the request line and drops the connection. This covers the Go default transport, the AWS SDK including its IMDS client, and the Azure and GCP SDKs. IMDS is deliberately left enabled, so a fall-through to it gets caught instead of hidden.
  • replaces net.DefaultResolver with a Go resolver whose name server is an in-memory pipe. A direct dial to a hostname that isn't in the hosts file then fails locally, and the looked-up name is recorded.

  If anything was blocked, the run fails at the end and lists every blocked target. Each one is also printed as it happens, so with -v it appears next to the test that caused it. TestNoNetworkGuard_BlocksOutboundCalls covers the guard itself (an HTTPS call and a DNS lookup must both be recorded).

  • Not covered: dials to IP literals by a transport built without a proxy, and resolvers other than net.DefaultResolver.

Closes #855

Verification

All of this is unit, mock and fixture evidence. No real cloud calls were made: every run had AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null and no real credentials. This machine's ~/.aws/config default profile is invalid, so the variables were also needed locally. The guard now sets them itself.

Fail-before (origin/main test files plus only the new guard file):

$ AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test -short -count=1 ./internal/api/
no-network guard: blocked CONNECT ce.us-east-1.amazonaws.com:443 HTTP/1.1          (x3)
no-network guard: blocked PUT http://169.254.169.254/latest/api/token HTTP/1.1     (x3)
no-network guard: blocked GET http://169.254.169.254/latest/meta-data/iam/security-credentials/ HTTP/1.1  (x9)
FAIL: internal/api tests attempted 15 outbound network call(s); stub the client instead (#855):
FAIL	github.com/LeanerCloud/cloud-commitments-platform/internal/api	16.844s

The latent EC2 call, shown on origin/main tests with the guard's env isolation temporarily disabled, using fake test credentials and AWS_REGION=eu-west-1:

no-network guard: blocked CONNECT ec2.eu-west-1.amazonaws.com:443 HTTP/1.1   (x3)
--- PASS: TestGetExchangeQuote_ValidOfferingIDPassesFormat (3.16s)
FAIL: internal/api tests attempted 3 outbound network call(s)

Pass-after:

$ AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test -race -short -count=1 ./internal/api/
ok  	github.com/LeanerCloud/cloud-commitments-platform/internal/api	38.121s
$ AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test -race -tags=integration -count=1 -timeout=20m ./internal/api/
ok  	github.com/LeanerCloud/cloud-commitments-platform/internal/api	352.025s

The integration run used real testcontainers Postgres on local Docker. It shows that the guard's proxy env and resolver swap don't interfere with the Docker client, Ryuk or pgx, and nothing was blocked. The only guard output in either run is the self-test's own deliberate entries.

Mutation probe: reverting getExchangeQuote to call exchange.GetExchangeQuote directly fails the three in-scope quote tests and the guard (36 IMDS requests blocked).

Other checks:

• go build ./..., go vet ./internal/api/, go vet -tags integration ./internal/api/
• golangci-lint v2.10.1 run --build-tags integration ./internal/api/: 0 issues
• gocyclo -over 10 -ignore "_test\.go" on the changed non-test files: silent
• go test ./internal/mocks/ -count=1: ok
• scripts/gosec-hook.sh on the changed files: clean
• git-secrets (--register-aws plus scripts/setup-git-secrets.sh patterns) on the changed files, in a throwaway repo with an isolated HOME: clean. scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL
• gofmt, git diff --check: clean

Out of scope / notes

• No real-cloud coverage: the seam's production branch (exchange.GetExchangeQuote) is unchanged and isn't exercised against AWS here.
• Other packages don't have this guard. Only internal/purchase (feat(ladder): prefetch commitment visibility for the tranche reconciler #852) and now internal/api do. I didn't audit other packages for network access.
• ri_exchange_carveout_test.go still relies on t.Setenv("AWS_EC2_METADATA_DISABLED", "true") as defense in depth. I left it alone because it doesn't reach the network today; the guard would catch it if it did.

Maintainer: please mirror the issue's triage labels on this PR: priority/p2, severity/medium, urgency/this-quarter, impact/internal, effort/m, type/bug, triaged.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 7 billable files and costs up to $1.75.}

Or wait 58 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 8caed39
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 4c00d246-24d1-42ac-8c39-94096916641f

📥 Commits

Reviewing files that changed from the base of the PR and between 8caed39 and b495b3a.

📒 Files selected for processing: 7

• internal/api/handler.go
• internal/api/handler_per_account_perms_test.go
• internal/api/handler_ri_exchange.go
• internal/api/handler_ri_exchange_test.go
• internal/api/main_nonet_test.go
• internal/api/ri_exchange_detached.go
• internal/api/tenant_scope_sweep_test.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[cristim]

VERDICT: APPROVE b495b3a

Reviewed head b495b3a: 1 commit, 371 changed lines, under the 400 cap. The base 8caed39 is 41 commits behind. git merge-tree origin/main(ca1e8a82) b495b3a4 merges cleanly to tree 552534c0, and every run below used that merged tree.

Findings (none blocking):

• nit internal/api/main_nonet_test.go:61-110 - this nearly duplicates internal/purchase/main_nonet_test.go:17 (feat(ladder): prefetch commitment visibility for the tranche reconciler #852), and the two copies already diverge. purchase sets fake creds plus AWS_EC2_METADATA_DISABLED; api strips AWS_* and leaves IMDS enabled. A shared test helper package would keep them in sync. That fits a follow-up better than this PR.
• nit internal/api/main_nonet_test.go:46-51 - a blocked call fails the whole package run at exit, not the test that made it. The culprit only shows in order with -v. This is acceptable because the run still fails loudly.

Attacked:

• Mechanism: proxy env plus a net.DefaultResolver swapped to an in-memory pipe, installed in TestMain before m.Run. A test cannot switch it off with t.Setenv, because ProxyFromEnvironment caches its env on first use. The AWS SDK BuildableClient and the IMDS client both route through the proxy. Known gaps, stated in the doc comment: transports with no Proxy dialing IP literals, and custom resolvers.
• Loopback: integration-tagged TestConfigInfrastructureKeyHTTP (httptest.NewServer) and TestDBRateLimiter_ExceedsLimitDenies (testcontainers Postgres) pass under the guard with nothing blocked.
• Goroutines: the guard works through process-wide env and the default resolver, so it covers all goroutines.
• Merged tree go test -race -count=1 ./internal/api/: ok (36s), 0 blocked.
• Merged tree -race -count=2: ok (95s), so no flakiness.
• go vet and go vet -tags integration: clean.
• Mutation probes, each restored afterwards:
  1. getExchangeQuote calls exchange.GetExchangeQuote directly, with AWS_* set in the parent env. Three quote tests FAIL and the guard reports 36 IMDS calls, which also proves the AWS_* stripping works.
  2. Dropped reshapeRecsFactory from the per-account RI test. The test FAILs and the guard blocks CONNECT ce.us-east-1 x3.
  3. Removed the resolver swap. The self-test FAILs.
  4. Removed the proxy env. The self-test FAILs.
• Issue internal/api tests call real AWS Cost Explorer and probe IMDS #855: both network-reaching tests named in the issue are now stubbed, and the new assertions are stricter (call count, region).

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE b495b3a

Findings (none blocking):

• [non-blocking] internal/api/main_nonet_test.go:63-71 (isolateAWSEnv) - Removing the env isolation fails no test (probe P5 passes, both plain and with AWS_REGION plus fake keys).
  • The new stubs already keep every quote and utilization path offline, so the isolation is untested defence in depth.
  • A future test that relies on it would not notice a regression.
• [non-blocking] internal/api/main_nonet_test.go vs internal/purchase/main_nonet_test.go - The guard is a second, richer copy of the feat(ladder): prefetch commitment visibility for the tranche reconciler #852 guard, not a shared helper (internal/testutil already exists).
  • The two will drift: purchase sets AWS_EC2_METADATA_DISABLED=true and fake keys, while api unsets everything and leaves IMDS enabled.
  • Fine as a follow-up.
• [non-blocking] serve() handles Accept serially with a 5 s read deadline, so a silent client delays recording of later attempts by up to 5 s. This only slows a run that would fail anyway.
• Cosmetic: the Go resolver appends resolv.conf search domains, so the guard logs entries such as "DNS lookup .". This is harmless, and the self-test matches on a substring.

Against #855: this fixes the root cause.

• Both offending tests (RI utilization with no client factories, and the quote test where exchange.GetExchangeQuote ignored h.awsCfg) now go through injected seams.
• The new seam (handler.go:143-148, ri_exchange_detached.go:39-45) mirrors executeExchangeFn. Production behaviour is unchanged when the field is nil.
• The guard makes any regression fail the run.
• On the merged tree, exchange.GetExchangeQuote is referenced only from awsExchangeQuoter, so no production path bypasses the seam.

Attacked:

• Mutation probes on a scratch copy, each restored and confirmed byte-identical to the head blob with cmp:
  • P1, awsExchangeQuoter ignores the field: 3 quote tests FAIL, and the guard reports 36 blocked calls (IMDS). Caught.
  • P2, getExchangeQuote hard-codes Region "us-east-1": EmptyRegionResolvesFromSDK and InScopeDeploymentAccountPassesGate FAIL. Caught.
  • P3, the net.DefaultResolver swap removed: the guard self-test FAILS. Caught.
  • P4, the proxy stops recording: the guard self-test FAILS. Caught.
  • P5, isolateAWSEnv skipped: no failure. Gap (finding 1).
• Blast radius: the getExchangeQuote test callers (router_handlers_test.go:107 and handler_ri_exchange_test.go:94-129) hit early-400 paths before the quote. The new field is unexported and only tests set it.
• Merged tree:
  • origin/main moved internal/api heavily (25 files, including handler_ri_exchange.go and the audit trail), but git merge-tree is clean.
  • The quote function is untouched on main, and main's new tests do not trip the guard.
  • Testcontainers, Ryuk and pgx all run fine under the proxy env and the resolver swap.

Verified locally, on git archive exports with no cloud credentials, under the heavy lock:

• Head:
  • go build ./...: OK. go vet ./internal/api/, plain and with -tags integration: OK. gofmt -l on the 7 changed files: clean.
  • go test -race -short -count=1 ./internal/api/: ok. The same run with AWS_REGION=eu-west-1 and fake keys: ok.
• Fail-before, base plus only the guard file:
  • FAIL with 15 blocked calls (3x CONNECT to ce.us-east-1.amazonaws.com:443, 3x IMDS token PUT, 9x IMDS credentials GET), matching the PR's claim.
  • Base with head's test files does not compile, as expected.
• Merged tree:
  • Build and vet: OK.
  • go test -race -short: ok.
  • go test -race -tags=integration -timeout=25m ./internal/api/ (testcontainers Postgres): ok, with no blocked entries beyond the self-test.
• No migrations.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[IAmJSD]

Local verification at b495b3a4bb421842aef84ef0a7fd4f1d69650bbb merged onto current main (5758cd43; merge-tree cd115221; macOS):

• go build ./..., go vet ./... and go vet -tags integration ./... pass.
• go test -race -short -count=1 on the packages this PR changes passes: ./internal/api.
• The full go test -race -short ./... suite passes on the tree with every PR in this merge train applied (tree 99ee50e2).
• Nothing was applied to any cloud.

[IAmJSD]

Marked ready and merged by IAmJSD's agent, at IAmJSD's request, after 2 independent reviews and local verification (see the review and verification comments above, all at b495b3a4bb421842aef84ef0a7fd4f1d69650bbb).