[IAmJSD]

Root cause

TestMemberAccountPoliciesTagReservedInstancesOnly (#702, PR 773) read the two Terraform member-account policies (iac/federation/aws-cross-account/terraform/main.tf, iac/federation/aws-target/terraform/main.tf) through tfStatements. That function found statements by anchoring on Sid\s*= and taking the brace-balanced object around each match. Sid is optional, so a statement written without one was never examined. A Sid-less { Effect = "Allow", Action = ["ec2:CreateTags"], Resource = "*" } passed the test, and the role could then retag every EC2 resource. The CloudFormation files (YAML walk) and the CLI heredocs (JSON) were not affected.

Fix (test-only)

The module has no HCL dependency (hashicorp/hcl is in neither go.mod nor go.sum), so this PR adds none. Instead it uses a statement-boundary scan that never looks at Sid:

• internal/iacfiles/tfpolicy_scan_test.go (new): a small HCL tokenizer that knows where quoted strings (including ${...} / %{...} template sequences with nested quotes), # / // / /* */ comments, heredocs and brackets begin and end. Every object constructor or block body with an Action attribute (or actions, for aws_iam_policy_document statement blocks) counts as one statement. Keys match case-insensitively, and both = and : separators work. Its Action / Resource are read as one literal string or a list of them.
• The scan fails closed. Each of these is an error, not a silent skip:
  • a non-literal Action or Resource (a reference, a function call, an interpolation);
  • a repeated Action / Resource key;
  • a different number of parsed statements than Action keys counted by a regex over the raw text. This catches anything the tokenizer cannot see, such as a JSON policy in a heredoc.
• internal/iacfiles/templates_aws_tagging_test.go: the old Sid-anchored tfStatements and its regexes are removed. The fix(iac): grant ec2:CreateTags to member-account purchase roles so the idempotency tag is written #702 assertions move into createTagsViolations, so the mutation tests can reuse them. The main test's behavior on the real templates is unchanged.

Other tests in internal/iacfiles: I checked them for the same Sid-anchored scan and none use it. templates_aws_wif_test.go decodes JSON, the CLI heredocs and CFN templates go through encoding/json / yaml.v3, and templates_azure_wif_test.go only counts run blocks. tfStatements was the only text scan of IAM statements, so nothing else needed hardening.

Closes #782

Verification

All evidence comes from unit tests with fixtures and mutations. Nothing calls a cloud API or runs terraform. Tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid. The package doesn't need AWS config.

Real templates still pass, and every statement is seen. go test ./internal/iacfiles/ -count=1 passes. A throwaway debug run (not committed) showed the new scan finds 11 statements in the cross-account file (10 permission statements, the OrganizationsDiscovery statement and the trust aws_iam_policy_document statement) and 10 in the target file (9 permission statements and the trust statement). In each file exactly one ec2:CreateTags is scoped to arn:aws:ec2:*:*:reserved-instances/*.

Fail-before on main, against the real templates. I copied main's internal/iacfiles and the IaC files into a mktemp -d scratch module and inserted a Sid-less statement before CostExplorer in both main.tf copies:

{
  Effect   = "Allow"
  Action   = ["ec2:CreateTags"]
  Resource = "*"
}

• main's unchanged test: --- PASS: TestMemberAccountPoliciesTagReservedInstancesOnly (both terraform/main.tf subtests PASS, so the widening is undetected).
• this PR's test files on the same mutated copy: both terraform/main.tf subtests FAIL with ec2:CreateTags must be scoped to arn:aws:ec2:*:*:reserved-instances/*, got Resource "*" and ec2:CreateTags must appear exactly once, got 2.

Committed regression test, before and after. TestTFStatementsCatchWidenedCreateTags mutates both real templates in memory. To show fail-before, I ran this PR's test file against main's Sid-anchored tfStatements, copied verbatim behind a one-line parseTFStatements shim:

mutant (each template)	main's scan	this PR
Sid-less statement	FAIL (undetected)	PASS
Sid-less statement, scalar Action = "ec2:CreateTags"	FAIL (undetected)	PASS
Sid-less one-line { Effect = ..., Action = [...], Resource = "*" }	FAIL (undetected)	PASS
Sid-less, quoted keys with : and Resource = ["*"]	FAIL (undetected)	PASS
Sid-less, behind comments containing braces	FAIL (undetected)	PASS
Sid-less data "aws_iam_policy_document" statement {} block	FAIL (undetected)	PASS
real tagging statement loses its Sid and is widened in place	FAIL (main reports it only as "appears 0 times", not as the widened Resource)	PASS
control: statement with a Sid	PASS	PASS

TestParseTFStatementsRefusesWhatItCannotRead pins the fail-closed paths: Action = local.x, "${var.service}:CreateTags", Resource = var.x, a duplicate Action key, and a JSON policy in a heredoc (count mismatch). Each case checks the specific error message.

Mutation probes of the new scanner. I applied each mutation to a scratch copy and confirmed it turns at least one subtest red:

• drop the Action-key count cross-check: 2 red
• accept non-literal values: 6 red
• drop the : separator: 2 red
• stop skipping # comments: 2 red
• allow duplicate keys: 2 red
• drop the plural actions: 9 red
• comma not a separator: 2 red
• Resource always read as the RI ARN: 2 red
• accept bare words as strings: 4 red

CI-equivalent checks.

• go build ./...: ok
• go vet ./internal/iacfiles/: ok
• gofmt -l internal/iacfiles: clean
• golangci-lint v2.10.1 run --allow-parallel-runners ./internal/iacfiles/...: 0 issues
• gocyclo -over 10 -ignore "_test\.go" internal/iacfiles: silent (both changed files are _test.go; the highest new function is 12, under golangci's test threshold of 15)
• go test ./internal/mocks/ -count=1: ok
• git-secrets: ran scripts/setup-git-secrets.sh in a fresh git init repo with an isolated HOME, and git secrets --scan on both files came back clean. A 12-digit positive control was flagged. scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL.
• gosec (pre-commit scripts/gosec-hook.sh): not run, because gosec isn't installed on this machine.

Out of scope (not changed here)

• Wildcard and NotAction grants. Action = ["ec2:*"], "ec2:Create*", "*", or a NotAction statement on Resource = "*" would still grant tagging without matching the literal ec2:CreateTags. This gap exists for all seven policy formats (JSON, CFN and Terraform alike), not only the Sid-less case, so it deserves its own issue.
• The iac/federation/*/terraform/main.tf templates. This PR doesn't touch them. Open PR fix(iac): bound every Terraform provider constraint below the next major #1203 edits their provider constraints, and the mutants here are applied in memory.
• Sid-keyed lookups outside internal/iacfiles. terraform/environments/aws/db_pool_budget_test.go looks up the LambdaReservedConcurrency statement by Sid. That is a named lookup that fails closed if the Sid is renamed, but it would also miss a second, Sid-less grant of the same action. terraform/environments/aws/ci-cd-permissions/policy_guard_test.go already splits statements without Sid. Neither file is changed.

Labels

Could a maintainer please mirror the issue's triage labels on this PR: impact/internal, type/chore, triaged, priority/p3, severity/low, urgency/eventually, effort/xs.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 2 billable files and costs up to $0.50.}

Or wait 59 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: 4170465
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: fc5cb6b6-b56d-4d40-b3af-49ef9a5256c3

Commits

Reviewing files that changed from the base of the PR and between 4170465 and 406b184.

Files selected for processing: 2

• internal/iacfiles/templates_aws_tagging_test.go
• internal/iacfiles/tfpolicy_scan_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]

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

Findings (none blocking):

• [non-blocking] internal/iacfiles/tfpolicy_scan_test.go:79 with :47 - The fail-closed cross-check compares only totals: Action keys found by the regex vs statements parsed. The two can offset each other.
  • A tokenizer-visible statement whose key regex \s* misses (Action /* c */ = [...]), plus a heredoc JSON policy granting ec2:CreateTags on "*", gives equal counts (12 = 12). The widened grant then passes.
  • I reproduced this with a gap probe. Nobody writes it by accident, so it is a test blind spot, not a live defect.
  • Matching key positions instead of totals would close it.
• [non-blocking, pre-existing] internal/iacfiles/templates_aws_tagging_test.go:128 - Action matching is case-sensitive, but IAM action names are case-insensitive.
  • A Sid-less Action = ["ec2:createtags"] on Resource = "*" parses and goes undetected (gap probe).
  • This applies to all seven policy formats. It is the same class as the wildcard/NotAction gap the PR already lists as out of scope, and worth adding to that follow-up.
• [non-blocking] tfpolicy_scan_test.go:100 - No test exercises the multi-resource join.
  • Mutation P6 (keep only list[0]) survives every test.
  • Head does catch Resource = [RI-ARN, "*"] ("got Resource ...reserved-instances/,"), so the behaviour is right but unpinned.
• [design note] The issue's cheaper option would also catch every mutant here: keep the Sid scan and assert that the count of Action = keys equals the count of Sid = anchors.
  • Its cost is that every jsonencode statement must carry a Sid.
  • The PR chose a 323-line test-only tokenizer that actually reads Sid-less statements. That is defensible and adds no dependency.

Against #782:

• The old tfStatements anchored on Sid\s*=, as the issue describes. The new scan does not key off Sid at all, so this fixes the cause.
• It is complete for the two Terraform files test(iacfiles): Terraform IAM scoping test skips statements without a Sid #782 names: cross-account main.tf, including the concat(...)/ternary list, and the trust aws_iam_policy_document.

Attacked:

• Tokenizer edge cases:
  • A (local.k) = ... key truncates the object early and drops the Resource that follows. Resource then reads as "", so the statement is flagged (fails closed).
  • !=/==, a ternary : and [for ... : ...] all stay inside values, and a heredoc is one string token.
• Mutation probes on a scratch copy, each restored and confirmed identical to the head with cmp:
  • P1, the count check dropped: 3 FAIL.
  • P2, ${} interpolation accepted: 3 FAIL.
  • P3, the plural actions dropped: 11 FAIL.
  • P4, # comments no longer skipped: 3 FAIL.
  • P5, duplicate keys allowed: 3 FAIL.
  • P6, first resource only: 0 FAIL (the gap above).
  • P7, the : separator dropped: 3 FAIL.
  • P8, the heredoc case dropped: 3 FAIL.
• Fail-before, on base plus the PR's tagging test plus main's old Sid-anchored scan behind a shim:
  • All 14 Sid-less mutant subtests FAIL, in both files, including policy_document and widened-in-place.
  • Both Sid controls pass, and the main test passes.
• Blast radius: tfStatements, parseTFStatements, createTagsViolations and withStatementBeforeCostExplorer are test-only, used only within internal/iacfiles. The removed regex vars had no other users.
• Drift: git merge-tree --write-tree origin/main 406b1844 is clean. Since the base, main changed only go.mod; internal/iacfiles and both main.tf files are unchanged.

Verified locally, with no cloud credentials and GOPROXY=off, under the heavy lock:

• Head:
  • go build ./... and go vet ./internal/iacfiles/: OK.
  • go test -count=1 ./internal/iacfiles/: ok, with the 36 targeted subtests passing.
  • gofmt -l on both changed files: clean.
• Merged tree (GOTOOLCHAIN=go1.26.10): build, vet and tests ok.

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

[cristim]

VERDICT: APPROVE 406b184

Independent adversarial review of #1214 (closes #782) at 406b184, base main. Test-only change in internal/iacfiles. The fix does what #782 asks: the Sid-anchored text scan is gone, and every object constructor or block body with an Action/actions key now counts as a statement whether or not it has a Sid. Non-literal values and statements the tokenizer cannot see fail closed.

Findings (none blocking):

• nit internal/iacfiles/tfpolicy_scan_test.go:106 - the resources > 1 half of the duplicate-key guard has no test. Only a duplicate Action key is exercised. Probe: making the second Resource key silently ignored left every test green. Scenario: Resource = "<RI ARN>" followed by Resource = "*" in one statement would read as scoped if the guard regressed. One more case in TestParseTFStatementsRefusesWhatItCannotRead would pin it.

Attacked:

• Ran go test ./internal/iacfiles/ -count=1 at the head: all pass (36 subtests in the touched tests). go vet is clean and gofmt -l is empty.
• Checked the claimed fix against test(iacfiles): Terraform IAM scoping test skips statements without a Sid #782 and against main's removed tfStatements. main only looked at brace-balanced objects around Sid\s*=, so a Sid-less statement was never seen. The new parse never reads Sid.
• Ad hoc fixtures through parseTFStatements + createTagsViolations (scratch only). All of these were caught: a statement defined in locals {}, a dynamic "statement" content {} block with literal actions, a statement with a nested Condition object, NotResource with no Resource (got Resource ""), and a multi-resource list [RI ARN, "*"] (joined, never equals the ARN). concat([...]) was refused as non-literal. A stray # Action = comment was refused by the count cross-check (fails closed, no false pass).
• Blast radius: the removed identifiers (tfActionList, tfResource, quoted) have no other users. tfStatements keeps its signature and its two callers. Both files are _test.go, so no production code changes. The real templates still parse with no error.
• Mutation probes in a scratch copy, each reverted:
  1. Drop the Action-key count cross-check: 2 heredoc subtests FAIL.
  2. Accept non-literal tokens instead of erroring: 6 subtests FAIL.
  3. Force Resource to the RI ARN: 2 FAIL (widened in place).
  4. Keep only the first statement found: 16 FAIL.
  5. Match keys case-sensitively, so actions is lost: 24 FAIL.
  6. Ignore a second Resource key: 0 FAIL (the nit above).

[IAmJSD]

Local verification at 406b18441cd1f1bcf07528742e3da983cbf8b912 merged onto main (2514c01a), together with #1203, #1207, #1211, #1214, #1216 in that order (combined tree 995207c1, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 1 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 1 approving independent review(s) on 406b1844, green CI, and the combined-tree verification above. Squash-merged at exactly 406b18441cd1f1bcf07528742e3da983cbf8b912.