Repository navigation
Conversation
…ure account once, validate registration email Three of the four internal/api findings in issue LeanerCloud#311: - revokeViaSession no longer binds TransitionExecutionStatus's returned row only to discard it, and its doc comment now says how CancelledBy is written and that neither revoke path applies the 4-eyes policy. - Delete the two bare t.Skip revoke+4-eyes placeholders; revocation is not dual-controlled, and whether it should be is an owner decision. - The Azure exchange execute path looks the subscription's CloudAccount up once and uses that row for the scope check, the client credentials and the constraint AccountIDs, instead of three lookups that could disagree. - POST /api/register validates contact_email with validateEmailFormat and stores the bare address, instead of accepting any 5+ character string containing "@" (which stored "a@b\r\nBcc: x@y"). The findDuplicatePendingExecution removal is left to the open LeanerCloud#1217, which already deletes it. Refs LeanerCloud#311 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD
added
impact/internal
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Issue #311 bundles four low-severity
internal/apifindings 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
findDuplicatePendingExecutionhas no production caller; only its test keeps itfix/issue-760), which already deletes the function andTestFindDuplicatePendingExecution, and moves the missing cases ontomatchDuplicateInList. Doing it here would edit the same lines inhandler_purchases.goandhandler_purchases_guards_test.go._ = updated // kept for future useinrevokeViaSessionupdated, 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 narrowSetCancelledByUPDATE keyed on the execution ID.t.Skiprevoke+4-eyes placeholders (TestRevoke_4EyesMode_TODO,TestRevokePurchase_FourEyesApproval)enforceFourEyesPolicy, andRequireDifferentApprovergates approvals only.revokeViaSession's doc comment now says this and calls it an open owner decision. Nothing about revoke behavior changes.GetCloudAccountByExternalIDup to three times (scope check, client build, constraint AccountIDs), and each call handles "no account" differentlyauthorizeAzureExchangeExecutionlooks the account up once and passes that row torequireAzureAccountScope,azureExchangeClientForAccountandazureExchangeAccountID.POST /api/registeraccepts any 5+ charactercontact_emailcontaining@registrationContactEmailparses the value withmail.ParseAddress, appliesvalidateEmailFormatto the bare address, and stores that bare address.Root cause and fix
A01-020.
authorizeAzureExchangeExecutionwas 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:lookupAzureAccountruns once at the top ofauthorizeAzureExchangeExecution.requireAzureAccountScope/azureAccountInScopeapply the same errNotFound rule as before to that row. Unregistered and out-of-scope accounts stay indistinguishable.azureExchangeClientForAccountbuilds credentials from that row.buildAzureExchangeClientis now a lookup followed by this, solistExchangeableAzureRIsandgetAzureCompatibleOfferingsbehave as before.azureExchangeAccountID(a pure function replacingresolveAzureExchangeAccountID) maps a nil account tounattributedAccountConstraintas 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
unattributedAccountConstraintwith the injected factory. One small difference: a store error on the lookup now surfaces before agetAccountScopeerror. Both are 500s.A02-025.
validateRegistrationRequestcheckedstrings.Contains(email, "@") && len >= 5. It now callsregistrationContactEmail, which:mail.ParseAddressrefuses, such as a CRLF followed by a header, or a second address;validateEmailFormat. That covers no TLD, and a quoted local part like"a b"@example.com, which unquotes toa b@example.comand would not parse again;addr.Address. SoJane Doe <jane@example.com>,user@example.comanduser@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 constraintAccountIDs == ["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_StoresBareContactEmailandTestSubmitRegistration_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.goandhandler_registrations.goback with the new tests in place, then restored the fix.Pass-after: all four pass.
Commands run on this branch:
(
authorizeAzureExchangeExecution8,validateRegistrationRequest8,registrationContactEmail3.)Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@936996ad= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-72andfix/issue-312conflict on one line ininternal/api/handler_dashboard.go(both change thecalculateCommitmentMetricscall). The tree used #72's side with #312's validatedprovidervariable; 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:936996ad:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without--build-tags integration.tests/e2eis a separate module, so it was vetted under its owne2etag.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout because the VM was shared):936996ad, these pass:internal/server,internal/credentials,internal/scheduler,internal/database/postgres/migrations,internal/purchaseandinternal/api.internal/configpasses exceptTestPostgresStoreDB_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).26795376, same branches at earlier heads):internal/analytics,internal/auth,internal/commitmentopts,internal/databaseandcmd/lambda/clear-rate-limitall pass. The later fixes don't touch those packages.httpToLambdaRequestsignature change;scripts/setup-git-secrets.shpatterns 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.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_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.getAzureCompatibleOfferingsstill makes two lookups (scope check, thenbuildAzureExchangeClient). 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.fix/issue-532andfix/issue-54touch other hunks ofhandler_purchases.go. fix(config): validate ri_exchange_mode on PUT /api/config #1243, feat(ri-exchange): pass offered instance types to reshape analysis #1246 andfix/issue-337touch other parts ofhandler_ri_exchange.go. fix(registrations): leave reviewed_by NULL for admin-API-key reviews #1247 touchessetReviewMetadatainhandler_registrations.go.fix/issue-312adds a function next tovalidateEmailFormatwithout 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