test(api): pin reshape security regressions — STS-error fail-closed + empty-region normalization (closes #151)

[cristim]

Summary

Closes #151. Adds two regression-guard tests covering security paths PR #79 + follow-up commits hardened — both fixes landed without coverage and a future refactor could silently break either gate.

This PR is tests-only. Production code is unchanged.

Pinned regressions

Test 1 — TestResolveAWSCloudAccountID_FailsClosedOnSTSError

Pins commit f44b06567's multi-tenant safety contract: when STS GetCallerIdentity fails (transient AWS error, denied IAM permission, expired token), resolveAWSCloudAccountID propagates the error rather than returning ("", nil). The empty-with-no-error path would have purchaseRecLookupFromStore skip the AccountIDs filter and surface another tenant's recs — exactly the leak the production fix prevents.

• Mock injection: a failing http.RoundTripper installed on the handler's pre-sealed aws.Config makes STS GetCallerIdentity fail without hitting the network.
• Why static creds + region are required: the AWS SDK's signer composes the request before invoking the transport; without static credentials and an explicit region the SDK errors with MissingRegion / NoCredentialProviders BEFORE the failing transport runs and the test would pass for the wrong reason.
• Asserts: error wraps "resolve source aws account for reshape scope" (the production prefix) AND ListCloudAccounts was never invoked — proving the resolver short-circuited before the DB read.

Test 2 — TestGetReshapeRecommendations_EmptyRegionUsesConfigRegion

Pins commit afc3aa1ff's region-normalization fix: when ?region= is empty (or omitted), the handler adopts cfg.Region (the value the AWS clients are actually talking to) before threading region through to the recs lookup. Without normalization the recs query lands as Region:"" — an unscoped read that surfaces alternatives from every region onto the reshape page.

• Mock injection: mirrors the integration-test helpers but lives in the unit-test file (no //go:build integration tag). MockConfigStore captures every ListStoredRecommendations filter via mock.Run so the assertion can pin filter.Region == cfg.Region.
• Silent-pass guard: require.NotEmpty(capturedFilters) fires if the analyzer ever stops invoking the closure for synthetic RIs, preventing a vacuously-passing test.

Would-fail-if-reverted verification (manual, before PR open)

This is the hard evidence that the tests pin the security behaviour:

• Commenting out the if region == "" { region = cfg.Region } block in internal/api/handler_ri_exchange.go made Test 2 FAIL with the captured filter showing Region:"" instead of "us-west-2".
• Changing the STS-error branch in internal/api/exchange_lookup.go from return "", fmt.Errorf("resolve source aws account for reshape scope: %w", err) back to return "", nil made Test 1 FAIL with "An error is expected but got nil".

The temporary reverts were never committed.

Out of scope

Issue #151 also lists two other test cases (no-AWS-context fall-through, ListCloudAccounts error propagation). The user request capped this PR's scope at the two security-critical paths above. The other two cases can land in a follow-up if desired.

Test plan

• go test ./internal/api/... -run 'TestResolveAWSCloudAccountID_FailsClosedOnSTSError|TestGetReshapeRecommendations_EmptyRegionUsesConfigRegion' -v -count=1 — both PASS.
• go test ./internal/api/... -count=1 — 1011 tests PASS (no regressions).
• go vet ./... — clean.
• gofmt -l . — empty output.
• git diff against base shows only the two test additions, no production-code changes (+221, -0 across two *_test.go files).
• Would-fail-if-reverted check executed locally for both tests; production code restored before commit.

Summary by CodeRabbit

• Tests
  • Added test coverage ensuring proper error handling when cloud authentication fails, preventing database access following credential errors.
  • Added test coverage verifying the system uses the configured default region when no region parameter is provided in requests.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 414953d6-703c-4e78-aa74-3b2e4d6374bc

📥 Commits

Reviewing files that changed from the base of the PR and between c84fd02 and 696cf5d.

📒 Files selected for processing (2)

• internal/api/exchange_lookup_test.go
• internal/api/handler_ri_exchange_test.go

---

📝 Walkthrough

Walkthrough

This PR adds two security regression test functions pinning the AWS STS error fail-closed behavior and region-normalization path, ensuring multi-tenant recommendation leaks are prevented and empty region parameters correctly default to the handler's configured region.

Changes

Reshape Security Regression Tests

Layer / File(s)	Summary
Test Imports & Mocking Stubs internal/api/exchange_lookup_test.go (lines 3–14), internal/api/handler_ri_exchange_test.go (lines 9–16)	Adds net/http and AWS SDK v2 imports; introduces failingRoundTripper HTTP transport stub, fakeReshapeEC2Stub, and fakeReshapeRecsStub test doubles.
Core STS Fail-Closed Test internal/api/exchange_lookup_test.go (lines 194–258)	TestResolveAWSCloudAccountID_FailsClosedOnSTSError injects a failing HTTP transport, sets CUDLY_SOURCE_CLOUD=aws, and asserts the resolver returns a non-nil error with production prefix, empty account ID, and never invokes ListCloudAccounts.
Region Normalization Test internal/api/handler_ri_exchange_test.go (lines 323–459)	TestGetReshapeRecommendations_EmptyRegionUsesConfigRegion pre-seeds handler AWS config with Region: "us-west-2", calls getReshapeRecommendations with empty region query param, and verifies ListStoredRecommendations is invoked with filters whose Region equals the configured region.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• PR #79 – The primary hardening PR introducing STS fail-closed logic and region normalization that these new tests now cover and pin.
• PR #146 – Related AWS identity and account-resolution refactoring that aligns with the account-ID resolution path being tested.

Suggested labels

triaged, type/security, priority/p1, severity/high, effort/s

Poem

🐰 A rabbit hops through STS gates,
Checking errors, closing fates—
Region blank? Config saves the day,
No cross-tenant leaks betray,
Security pinned, regressions slay! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the two security regression tests being added: STS-error fail-closed behavior and empty-region normalization.
Linked Issues check	✅ Passed	Both security-critical test paths from #151 are implemented: STS-error fail-closed test and empty-region normalization test, fulfilling the linked issue's highest-priority objectives.
Out of Scope Changes check	✅ Passed	All changes are strictly test-only additions scoped to the two security regression guards specified in #151; no production code modifications or unrelated functionality.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch test/issue-151-reshape-security-pins

---

_{Review rate limit: 1/5 review remaining, refill in 41 minutes and 18 seconds.}

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

[cristim]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.