[IAmJSD]

Summary

Issue #311 bundles four low-severity internal/api findings from the 2026-09-02 audit. I re-checked each one against current main (a14dc8f2). All four still reproduced. This PR fixes three and part of the fourth. The rest of the fourth is already done in an open PR.

Refs #311

Per-finding status

Finding	What	Status
A01-015 (a)	findDuplicatePendingExecution has no production caller; only its test keeps it	Deferred to open #1217 (fix/issue-760), which already deletes the function and TestFindDuplicatePendingExecution, and moves the missing cases onto matchDuplicateInList. Doing it here would edit the same lines in handler_purchases.go and handler_purchases_guards_test.go.
A01-015 (b)	_ = updated // kept for future use in revokeViaSession	Fixed. updated, err := is now _, err := and the dead line is gone. The doc comment also wrongly said CancelledBy is written by "a follow-up SavePurchaseExecution on the freshly-returned row". It now says what the code does: a narrow SetCancelledBy UPDATE keyed on the execution ID.
A01-022	Two bare t.Skip revoke+4-eyes placeholders (TestRevoke_4EyesMode_TODO, TestRevokePurchase_FourEyesApproval)	Fixed by deleting the placeholders (the issue's second option). Neither revoke path calls enforceFourEyesPolicy, and RequireDifferentApprover gates approvals only. revokeViaSession's doc comment now says this and calls it an open owner decision. Nothing about revoke behavior changes.
A01-020	The Azure exchange execute path calls GetCloudAccountByExternalID up to three times (scope check, client build, constraint AccountIDs), and each call handles "no account" differently	Fixed. authorizeAzureExchangeExecution looks the account up once and passes that row to requireAzureAccountScope, azureExchangeClientForAccount and azureExchangeAccountID.
A02-025	POST /api/register accepts any 5+ character contact_email containing @	Fixed. The new registrationContactEmail parses the value with mail.ParseAddress, applies validateEmailFormat to the bare address, and stores that bare address.

Root cause and fix

A01-020. authorizeAzureExchangeExecution was built from three helpers, and each one looked up the subscription's account itself. A scoped session made two lookups even with the test factory (scope check and AccountIDs), and production made three. Because they were separate reads, a concurrent re-registration between them could let the scope check approve one account while the constraint check ran against another. The fix:

• lookupAzureAccount runs once at the top of authorizeAzureExchangeExecution.
• requireAzureAccountScope / azureAccountInScope apply the same errNotFound rule as before to that row. Unregistered and out-of-scope accounts stay indistinguishable.
• azureExchangeClientForAccount builds credentials from that row. buildAzureExchangeClient is now a lookup followed by this, so listExchangeableAzureRIs and getAzureCompatibleOfferings behave as before.
• azureExchangeAccountID (a pure function replacing resolveAzureExchangeAccountID) maps a nil account to unattributedAccountConstraint as before.

Order and outcomes are unchanged. The scope check still runs before any credential resolution. A scoped caller still gets errNotFound for an unregistered or out-of-scope subscription. An unrestricted caller still gets the 404 "no Azure account registered" in production, and unattributedAccountConstraint with the injected factory. One small difference: a store error on the lookup now surfaces before a getAccountScope error. Both are 500s.

A02-025. validateRegistrationRequest checked strings.Contains(email, "@") && len >= 5. It now calls registrationContactEmail, which:

• refuses anything mail.ParseAddress refuses, such as a CRLF followed by a header, or a second address;
• refuses a bare address that fails validateEmailFormat. That covers no TLD, and a quoted local part like "a b"@example.com, which unquotes to a b@example.com and would not parse again;
• stores addr.Address. So Jane Doe <jane@example.com>, user@example.com and user@example.com (ops) are stored and sent to the admin notification as the bare address.

The error message is unchanged (contact_email must be a valid email address). Header injection was already unreachable, as the issue says. This narrows what can be stored, including the input to #111's federation-tfvars scenario.

No DB, migration or concurrency change: there is no new write, and the Azure change removes two reads.

Verification

All local on Linux, with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true. This is unit/mock evidence only: no Postgres, no cloud calls, no email.

New tests:

• TestExecuteAzureExchange_ResolvesAccountOnce (handler_ri_exchange_azure_test.go): a scoped, in-scope session. The store returns the caller's account on the first lookup and another account on any later one. The test asserts exactly one lookup and constraint AccountIDs == ["acct-mine"].
• TestValidateRegistrationRequest_ContactEmail (handler_registrations_validate_test.go): 4 accept cases with the stored bare address, and 8 reject cases (CRLF+Bcc twice, two addresses, no TLD, no local part, no @, quoted local part, display name only).
• TestSubmitRegistration_StoresBareContactEmail and TestSubmitRegistration_RejectsHeaderInjectionContactEmail (handler_registrations_submit_test.go): through the handler, the stored row and the admin notification carry the bare address, and a CRLF value is refused before any insert or email.

Fail-before: I put main's handler_ri_exchange.go and handler_registrations.go back with the new tests in place, then restored the fix.

TestExecuteAzureExchange_ResolvesAccountOnce: expected 1 lookup, actual 2; AccountIDs expected ["acct-mine"], actual ["acct-other"]
TestValidateRegistrationRequest_ContactEmail: 9 of 12 subtests FAIL (3 stored values not bare; 6 rejects returned nil, incl. "a@b\r\nBcc: x@y", "@example.com", "user@hostonly")
TestSubmitRegistration_StoresBareContactEmail: stored/notified "Jane Doe <jane@example.com>"
TestSubmitRegistration_RejectsHeaderInjectionContactEmail: want a 400 client error, got <nil>

Pass-after: all four pass.

Commands run on this branch:

go build ./... && go vet ./... && go vet -tags integration ./...      # OK
go test ./internal/api/ -count=1                                       # ok
go test ./internal/mocks/ -count=1                                     # ok
golangci-lint (v2.10.1) run --allow-parallel-runners ./internal/api/...  # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/api/handler_purchases.go internal/api/handler_ri_exchange.go internal/api/handler_registrations.go  # none

(authorizeAzureExchangeExecution 8, validateRegistrationRequest 8, registrationContactEmail 3.)

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ 936996ad = origin/main a14dc8f2 plus these heads, merged in this order:

Branch	Tested head
fix/issue-337	ff3420c
fix/issue-169	7d75eb5
fix/issue-342	66b221f
fix/issue-310	5e11c21
fix/issue-326	5f0ea59
fix/issue-243	365e959
fix/issue-343	6c94036
fix/issue-312	d2848a8
fix/issue-316	acd60dd
fix/issue-339	75ea9b9
fix/issue-339-commitment-type	73bdb19
fix/issue-353	5d25fcd
fix/issue-313	3156bc4
fix/issue-357	53d9e58
fix/issue-368	0ba727d
fix/issue-419	61648c9
fix/issue-111	b04c372
fix/issue-686	0ad90b7
fix/issue-674	793095d
fix/issue-1055	4ec96f1
fix/issue-559	dfa9525
fix/issue-558	034eed4
fix/issue-874	1720539
fix/issue-666	1e120da
fix/issue-668	fb26345
fix/issue-568	ce04173
fix/issue-311	e54f250
fix/issue-177	0cbb88a
fix/issue-151	c21377c
fix/issue-166	41fb77b
fix/issue-27	2d67622
fix/issue-323	69288ae
fix/issue-59	36463e8
fix/issue-43	35e4069
fix/issue-158	6f76f8a
fix/issue-104	bb00491
fix/issue-55	971ef1b
fix/issue-361	89ac220
fix/issue-120	b458719
fix/issue-36	42bac6e
fix/issue-525	57c97dd
fix/issue-712	e5bcda0
fix/issue-72	6cc63ed

fix/issue-72 and fix/issue-312 conflict on one line in internal/api/handler_dashboard.go (both change the calculateCommitmentMetrics call). The tree used #72's side with #312's validated provider variable; whichever PR merges second takes that same resolution.

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:

• Passed at 936996ad: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks at 936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. tests/e2e is a separate module, so it was vetted under its own e2e tag. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests, every changed package, one package at a time (-tags integration -count=1, 60-minute local timeout because the VM was shared):
  • At 936996ad, these pass: internal/server, internal/credentials, internal/scheduler, internal/database/postgres/migrations, internal/purchase and internal/api.
  • internal/config passes except TestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).
  • These were run on the first build of this tree (26795376, same branches at earlier heads): internal/analytics, internal/auth, internal/commitmentopts, internal/database and cmd/lambda/clear-rate-limit all pass. The later fixes don't touch those packages.
• What the combined run caught, each fixed on its own branch before this PR opened:
  • a compile break between fix/issue-104's test and fix/issue-326's httpToLambdaRequest signature change;
  • fix/issue-316 rejecting legacy-keyed plans;
  • fix/issue-120 versus main's global-filter test.
• Secrets: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.
• Nothing was applied to any cloud.

Out of scope / notes

• findDuplicatePendingExecution: left to chore(api): remove dead submit-dedupe code and lock the retry-successor path #1217 (see table). Once chore(api): remove dead submit-dedupe code and lock the retry-successor path #1217 and this PR merge, every item in chore(api): dead duplicate check, skipped revoke placeholders, triple lookup, weak email #311 is done, which is why this PR says Refs rather than Closes.
• Revocation and 4-eyes (owner decision). Revoke is not dual-controlled. Both paths only record revocation_requested, and an operator then completes the cancellation with AWS Support. If revocation should need a second person, that needs a new issue covering both the token and session paths.
• getAzureCompatibleOfferings still makes two lookups (scope check, then buildAzureExchangeClient). The issue names only the execute path, and the new helpers would make the same change there easy. I left it so this PR stays scoped.
• I checked the overlap with in-flight work. Only chore(api): remove dead submit-dedupe code and lock the retry-successor path #1217 edits these functions. sec(purchase): pin the approval token in the token approve/cancel CAS; compare revoke token first #1242, fix(api/purchases): guard the email-failure write in finalizePurchaseStatus on pending #1237, fix(recommendations): key stored recs, rec ID and purchase identity on RDS az_config (#408) #1239, feat(plans): settle a failed-with-submission row so a managed plan can be deleted #1241, fix/issue-532 and fix/issue-54 touch other hunks of handler_purchases.go. fix(config): validate ri_exchange_mode on PUT /api/config #1243, feat(ri-exchange): pass offered instance types to reshape analysis #1246 and fix/issue-337 touch other parts of handler_ri_exchange.go. fix(registrations): leave reviewed_by NULL for admin-API-key reviews #1247 touches setReviewMetadata in handler_registrations.go. fix/issue-312 adds a function next to validateEmailFormat without changing it.

No frontend change is needed. The 400 message for a bad contact email is unchanged.

Maintainer: please mirror the issue's triage labels onto this PR: priority/p3, severity/low, urgency/eventually, impact/internal, effort/s, type/chore, triaged.

🤖 Generated with Claude Code

[coderabbitai]

Caution

Review failed

The pull request is closed.

Finishing Touches

🧪 Generate unit tests (beta)

• 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.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE d5584006925e865a87c08986d3bf24cdef1f0a04

Blocking:

• none

Non-blocking:

• Owner decision left open (A01-022). The PR deletes the two t.Skip placeholders and adds a doc comment on revokeViaSession saying revocation is not 4-eyes gated and that this is "an open owner decision". That matches the issue's second option. The question of whether a revocation should need a second person still has no tracking issue, so it may be lost once chore(api): dead duplicate check, skipped revoke placeholders, triple lookup, weak email #311 closes. A follow-up issue would be worth opening.
• internal/api/handler_ri_exchange.go authorizeAzureExchangeExecution: a lookup failure now reads "failed to resolve cloud account scope: ..." for unrestricted callers. Before, it read "failed to build Azure exchange client: azure: look up account ...". Both are 500s, so nothing user-visible changes, but log greps for the old text will miss it.
• internal/api/handler_registrations.go registrationContactEmail: there is still no length cap on contact_email, as before. Out of scope here.

Attacked:

• Head checked: d5584006. Against issue chore(api): dead duplicate check, skipped revoke placeholders, triple lookup, weak email #311: A01-015(b), A01-022, A01-020 and A02-025 are done. A01-015(a) is deferred to chore(api): remove dead submit-dedupe code and lock the retry-successor path #1217, so Refs #311 (not Closes) is right.
• Azure path:
  • The scope check still runs before any credential resolution.
  • Unregistered and out-of-scope accounts both still return errNotFound through azureAccountInScope.
  • buildAzureExchangeClient keeps the factory short-circuit before the lookup, so the list and offerings paths are unchanged.
  • The new test asserts exactly one lookup across the whole executeAzureExchange, and constraint AccountIDs taken from the first row.
• Registration: validateRegistrationRequest takes a pointer, and both the stored row and the admin notification read body.ContactEmail after it runs. CRLF values are refused before CreateAccountRegistration.
• Fail-before, with the merge-base handler_ri_exchange.go and handler_registrations.go restored: TestExecuteAzureExchange_ResolvesAccountOnce, both new TestSubmitRegistration_* tests and 9 of 12 TestValidateRegistrationRequest_ContactEmail subtests fail. All four pass on the head.
• Commands run: go build ./...; go vet ./internal/api/, with and without -tags integration; go test -race -short -count=1 ./internal/api/ (ok); golangci-lint v2.10.1 on internal/api (0 issues); gocyclo -over 10 on the three changed non-test files (none).

Body-stale:

• none

_{Reviewer: rB19. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE d5584006925e865a87c08986d3bf24cdef1f0a04

Blocking:

• none

Non-blocking:

• The PR is CONFLICTING/DIRTY against current main. The conflict is in internal/api/handler_registrations.go at the end of validateRegistrationRequest: main's fix(api): validate registration credentials and GCP WIF endpoint URLs #1414 appended a parseRegistrationCredential call where this PR adds registrationContactEmail. The fix is to keep both blocks (contact email first, then the credential). Merging main makes a new head, so this review will not cover it and the gate starts again.
• getAzureCompatibleOfferings still does two lookups (scope check, then buildAzureExchangeClient). The body already says so. It's fine to leave, because the issue names only the execute path.
• internal/api/handler_ri_exchange.go: azureExchangeClientForAccount repeats the azureExchangeFactory short-circuit that buildAzureExchangeClient already does. It's harmless, but if the guard ever moves out of one of them, the two will drift apart.

Attacked:

• Azure execute path (scope and money). Checked that the order is unchanged. requirePermission runs first, then the one lookup, then the scope check before any credential resolution. An unregistered subscription and an out-of-scope one still both get errNotFound for scoped callers. An unrestricted caller still gets a 404 when no account is registered. The constraint AccountIDs now comes from the same row as the scope check, so a re-registration between reads can't split them. The only behavior change is that a store error now surfaces before a getAccountScope error, and both are 500s.
• Registration email. mail.ParseAddress plus validateEmailFormat on addr.Address rejects CRLF, two addresses, no TLD and a quoted local part. The bare address is what gets stored, sent to the notifier, and later copied to the account's contact email. Revoke: only the dead binding and the doc comment change, and the two t.Skip placeholders are deleted, as the issue allows.
• Commands. go build ./..., go vet (with and without -tags integration) on ./internal/api/, go test -race -short -count=1 ./internal/api/ (ok), golangci-lint v2.10.1 on ./internal/api/ (0 issues).
• Fail-before. I restored the merge-base (503eb2ef) versions of handler_ri_exchange.go and handler_registrations.go. All four new tests then fail: ResolvesAccountOnce, 9 of 12 ContactEmail subtests, StoresBareContactEmail and RejectsHeaderInjectionContactEmail. They pass again once the fix is back.

Body-stale:

• The body calls chore(api): remove dead submit-dedupe code and lock the retry-successor path #1217 "open" and defers findDuplicatePendingExecution to it. chore(api): remove dead submit-dedupe code and lock the retry-successor path #1217 has since merged, and main no longer has the function. After this PR merges, every item in chore(api): dead duplicate check, skipped revoke placeholders, triple lookup, weak email #311 is done, so the body can say Closes #311 instead of Refs.

_{Reviewer: rB23. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Merged origin/main (now b071f8db). There was one conflict, at the end of validateRegistrationRequest; I kept both this branch's normalized contact email and main's registration credential validation (#1414), since they touch different fields. Build, vet (both tag sets), api race unit tests, the registration integration tests and golangci-lint pass; secrets scan clean. Needs a fresh review at this head.

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE b071f8db7130104431dee1edcf8dbf99d11ed00a

Blocking:

• none

Non-blocking:

• internal/api/handler_registrations.go registrationContactEmail: the public POST /api/register now rejects addresses that were accepted before, such as no-TLD hosts (admin@localhost) and quoted local parts, and it stores the bare address. That's the intended tightening, and it matches validateEmailFormat elsewhere in the package. Self-hosted operators who used a no-TLD contact address will now get a 400.

• The getAzureCompatibleOfferings and listExchangeableAzureRIs paths still look the account up twice (scope check, then buildAzureExchangeClient). They're read-only, and the body says so, so I'm fine leaving them.

• From an earlier review, confirmed here: the revoke 4-eyes question (A01-022) is now only a doc comment calling it "an open owner decision". No issue tracks it, so it will be lost once chore(api): dead duplicate check, skipped revoke placeholders, triple lookup, weak email #311 closes. Open a follow-up issue.

Attacked:

• Azure execute path: walked authorizeAzureExchangeExecution before and after. The scope check still runs before any credential resolution. A scoped caller still gets errNotFound alike for an unregistered or out-of-scope subscription. An unrestricted caller with no account still gets the 404 in production, and the injected factory still wins in azureExchangeClientForAccount. The only ordering change is that a store error now surfaces before a getAccountScope error, and both are 500s. A single lookup feeding the scope, the client and the constraint AccountIDs removes the TOCTOU between them.
• Revoke: the change only drops the unused updated binding and fixes the doc comment. The CAS transition and SetCancelledBy are unchanged. The two deleted tests were bare t.Skip.
• Fail-before: restored main's handler_ri_exchange.go and handler_registrations.go. TestExecuteAzureExchange_ResolvesAccountOnce, TestValidateRegistrationRequest_ContactEmail (9 subtests), TestSubmitRegistration_StoresBareContactEmail and TestSubmitRegistration_RejectsHeaderInjectionContactEmail all fail. They pass after restoring.
• Ran go build ./... and go vet ./internal/api/ (plain and -tags integration), go test -race -short -count=1 ./internal/api/ (ok), go test -race -tags integration -count=1 ./internal/api/ (ok, 139s), golangci-lint (with and without --build-tags integration, 0 issues), and gocyclo -over 10 on the three changed files (silent).

Body-stale:

• The body says Refs #311 because A01-015(a) was "deferred to open chore(api): remove dead submit-dedupe code and lock the retry-successor path #1217". chore(api): remove dead submit-dedupe code and lock the retry-successor path #1217 has since merged, and findDuplicatePendingExecution is gone from main. With this PR every chore(api): dead duplicate check, skipped revoke placeholders, triple lookup, weak email #311 item is done, so Closes #311 would now be accurate. Otherwise chore(api): dead duplicate check, skipped revoke placeholders, triple lookup, weak email #311 needs a manual close after merge.

_{Reviewer: rB26. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE b071f8db7130104431dee1edcf8dbf99d11ed00a

Blocking:

• none

Non-blocking:

• internal/api/handler_ri_exchange.go:1005-1009 (getAzureCompatibleOfferings): still two separate GetCloudAccountByExternalID reads (scope check, then buildAzureExchangeClient), so the quote path keeps the same scope-check/credential TOCTOU that this PR closes on execute. The PR body says so. lookupAzureAccount + requireAzureAccountScope + azureExchangeClientForAccount make it a 3-line follow-up. Worth a ticket, because the quote path resolves credentials too.
• internal/api/handler_registrations.go:481: the new check is stricter for some inputs the old one accepted (no-TLD hosts, quoted local parts, comment/display-name forms are now reduced to the bare address). This only affects new registrations, the 400 message is unchanged, and stored rows are not revalidated. I'm noting it in case support sees a registrant who used to get through.
• (From an earlier review, which I agree with.) Whether revocation should be 4-eyes gated is now recorded only as a doc comment calling it an open owner decision. Nothing tracks it once chore(api): dead duplicate check, skipped revoke placeholders, triple lookup, weak email #311 closes, so it needs a follow-up issue or an owner call.
• With chore(api): dead duplicate check, skipped revoke placeholders, triple lookup, weak email #311 split between this PR and chore(api): remove dead submit-dedupe code and lock the retry-successor path #1217, Refs #311 is correct. Close chore(api): dead duplicate check, skipped revoke placeholders, triple lookup, weak email #311 by hand once both merge.

Attacked:

• Azure execute ordering. Every outcome of authorizeAzureExchangeExecution is unchanged except store errors. The scope check still precedes credential resolution. Unregistered and out-of-scope accounts are both errNotFound for scoped callers. An unrestricted caller still gets the 404 in production, and unattributedAccountConstraint with the injected factory, so an AccountIDs-constrained grant still fails closed. The one row now feeds scope, credentials and AccountIDs, which removes the re-registration TOCTOU. The extra lookup for unrestricted sessions replaces the one buildAzureExchangeClient did, so production goes from 3 reads to 1.
• Revoke. Only a dead binding and a comment changed, and no behavior did. I confirmed that neither revoke path calls enforceFourEyesPolicy, so the comment is accurate.
• Email. Checked that mail.ParseAddress + validateEmailFormat rejects CRLF, multi-address and no-TLD values, and that the bare address is what reaches both CreateAccountRegistration and the notifier.
• Fail-before. In my scratch checkout I put main's handler_ri_exchange.go and handler_registrations.go back and ran the 4 new tests. All 4 failed (ResolvesAccountOnce, ValidateRegistrationRequest_ContactEmail, StoresBareContactEmail, RejectsHeaderInjectionContactEmail). I then restored the PR's versions.
• Commands. go build ./...; go vet with and without -tags integration on internal/api; go test -race -short -count=1 ./internal/api/ ok; go test -race -tags integration -count=1 ./internal/api/ ok (227s); golangci-lint v2.10.1 with and without the integration tag, 0 issues; gocyclo -over 10 on the 3 changed files, silent.

Body-stale: none found

_{Reviewer: rB27. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at b071f8db7130104431dee1edcf8dbf99d11ed00a merged onto main (9cdeb787), together with #1285 in that order (combined tree eceb3548, 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 2 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: 2 approving independent review(s) on b071f8db (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly b071f8db7130104431dee1edcf8dbf99d11ed00a.

[cristim]

Post-merge independent review (cold, fresh context). This PR merged as f316add before the review finished; nothing blocking, the nits are follow-up material.

VERDICT: APPROVE b071f8d

Note: PR #1285 was already MERGED (squash f316add, 2026-10-26T02:09Z) when this review ran. This is a post-merge review; the squash diff matches git diff origin/main...b071f8db except for hunk offsets in handler_registrations.go.

Findings:

• nit internal/api/handler_ri_exchange.go:1125-1130 - no test pins the Azure nil-account -> unattributedAccountConstraint mapping in azureExchangeAccountID. Mutating it to return "acct-mutant" leaves the whole internal/api suite green. This predates the PR: the old resolveAzureExchangeAccountID had the same gap. Only the injected-factory path reaches it today, because production returns a 404 on a nil client first. Failure scenario: a later change that lets a nil account past the client check would silently stop an AccountIDs-constrained permission from failing closed. Follow-up: add an Azure analogue of handler_ri_exchange_test.go:2033.
• nit internal/api/handler_ri_exchange.go:349-354 - getAzureCompatibleOfferings still does two lookups (requireAzureSubscriptionScope, then buildAzureExchangeClient). The PR body says this is out of scope. The new helpers make it a 3-line follow-up.
• nit internal/api/handler_ri_exchange.go:1080 - a store error on the lookup now returns before a getAccountScope error, and its message changed from "azure: look up account for subscription ..." to "failed to resolve cloud account scope". Both are 500s with an internal message, so the client sees no difference.

No blocking findings.

Attacked:

• Deadness of the removed code: updated had no other use after _ = updated (the build passes with _, err :=). The two t.Skip placeholders referenced sec(api): owner-vs-key HasPermissionAPI on remaining purchase authz helpers #1005, which is CLOSED with an unrelated title, so they tracked nothing. No route, handler, OpenAPI entry, email link or frontend call was removed (grepped Go, frontend/src and docs). The revoke endpoints and their behavior are unchanged, so old revoke emails still work.
• The claim that revoke skips 4-eyes: checked. The new doc comment describes current behavior and changes no code.
• Azure single lookup: checked every caller of requireAzureSubscriptionScope, buildAzureExchangeClient and the removed resolveAzureExchangeAccountID (the removed function has no remaining callers). The order is still scope check, then client build, then source ownership, then constraints. In production a nil account still gives errNotFound for a scoped caller and a 404 for an unrestricted one. With the factory it gives unattributedAccountConstraint. Factory precedence is kept in azureExchangeClientForAccount.
• Registration email: validateRegistrationRequest has one production caller (submitRegistration, handler_registrations.go:68). It normalizes req.ContactEmail before the row is built (:83) and before the notification (:120). Approval re-runs validateEmailFormat (handler_accounts.go:374), so the submit-time check now matches approval and a registration that could never be approved is refused up front. Quoted local parts, CRLF+Bcc, a second address and a missing TLD are all rejected. Display-name and whitespace forms are reduced to the bare address. The error string is unchanged, so the frontend needs no change.
• Tests: go test -race -count=1 ./internal/api/ passes at b071f8d on macOS. No store, SQL or migration changes, so no integration run was needed.
• Mutation probes (each reverted, file diff-checked):
  M1 authorize uses requireAzureSubscriptionScope (re-lookup) instead of requireAzureAccountScope -> KILLED (TestExecuteAzureExchange_ResolvesAccountOnce)
  M2 registrationContactEmail returns raw instead of addr.Address -> KILLED (TestSubmitRegistration_StoresBareContactEmail, TestValidateRegistrationRequest_ContactEmail)
  M3 skip validateEmailFormat on the parsed address -> KILLED (TestValidateRegistrationRequest_ContactEmail)
  M4 azureAccountInScope lets a nil account through -> KILLED (3 ScopeCheck tests)
  M5 azureExchangeAccountID nil -> "acct-mutant" -> SURVIVED (pre-existing gap, nit above)