[cristim]

Why

An account registration carries an encrypted credential in account_registrations.reg_credential_payload. Approving a registration copies that credential to the account's credential store, and rejecting one never uses it. In both cases the encrypted payload stayed in the registrations table: neither status-transition statement writes the column, and nothing else clears it. The credential is no longer needed there once the outcome is decided.

What

• New store method: ClearRegistrationCredentialPayload(ctx, id) sets reg_credential_payload to NULL for one registration. It is a no-op for an unknown id, like DeleteAccountRegistration.
• Approval: storeRegistrationCredentials now reports success, and the handler clears the payload only after SaveCredential succeeds.
  • When the save fails, or there is nothing to save, the payload is kept, because it is the only copy. That failure now logs at Errorf, not Warnf.
  • A failure of the clear itself is logged and does not fail the approval, because the account already exists and its credential is saved.
• Rejection: the transition to rejected sets the payload to NULL in the same UPDATE, so the change is atomic and needs no extra call.

Tests

• Integration: the payload is NULL after an approval that saved the credential, and after a rejection. Clearing a missing id is a no-op.
• Unit (api): the clear runs once after a successful save, and never runs when the save fails or nothing was saved.

Removing the approval clear, or the rejection NULL, makes the matching test fail.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 12 minutes.

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: fix/registration-credential-payload-clear
• File: .coderabbit.yaml
• Commit: 306c8a7
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 7ce3c4a4-edbe-4921-b56c-4e90806d614e

Commits

Reviewing files that changed from the base of the PR and between 824a923 and 306c8a7.

Files selected for processing: 7

• internal/api/handler_registrations.go
• internal/api/handler_registrations_credentials.go
• internal/api/handler_registrations_credentials_test.go
• internal/config/interfaces.go
• internal/config/store_postgres_registrations.go
• internal/config/store_postgres_registrations_integration_test.go
• internal/mocks/stores.go

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

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[cristim]

@coderabbitai review

[coderabbitai]

Action not completed

Review rate limited.

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]

APPROVED 097acb0 by claude-ui-fix-2 (code review; runtime checks pending)

Second cold review, reading only (memory guard: no local runs).

Checked, holds

• Approval clears only after a successful save. approveRegistration (handler_registrations.go:310-315) calls ClearRegistrationCredentialPayload only when storeRegistrationCredentials returns true. That function returns false for no payload, a nil credStore, or a failed SaveCredential (now Errorf). So the registration's copy is never dropped while it's the only one. The clear runs after the ApproveAccountRegistration transaction commits, and a clear failure is logged without failing an approval that already created the account.

• Rejection is atomic. TransitionRegistrationStatus sets reg_credential_payload = CASE WHEN $10 THEN NULL ELSE reg_credential_payload END, with $10 = reg.Status == "rejected", in the same conditional UPDATE (WHERE id = $1 AND status = $8). A lost race (ErrRegistrationConflict) clears nothing. Inside ApproveAccountRegistration the status is approved, so $10 is false there and the payload survives until the post-save clear.

• Nothing can bring the payload back or need it later:

  • every transition starts from "pending" (handler_registrations.go:404, store_postgres_registrations.go:282), so a rejected registration can't be re-opened and approved without its credential;
  • UpdateAccountRegistration writes only the status and review columns, never the payload, so the stale in-memory struct can't restore it;
  • the only other readers are creation (:106) and the approval decrypt (handler_registrations_credentials.go:73-79);
  • the field is json:"-", so it never reaches API responses.

• Tests cover each branch:

  • integration: approval alone keeps the payload, the explicit clear NULLs it, rejection NULLs it, and a missing id is a no-op;
  • unit: one clear after a successful save, none after a failed save or an empty payload.

  CI's Integration Tests (api) and (config) pass at this SHA.

CI

Security Scanning and CI Success fail at this SHA because of GO-2026-6865 (klauspost/compress), which isn't this PR's doing. #1805 fixes it, after which this branch needs main merged in and green CI.

Notes, not blocking

• Rows decided before this PR keep their encrypted payloads, since nothing backfills them. A one-off UPDATE account_registrations SET reg_credential_payload = NULL WHERE status IN ('approved','rejected') AND cloud_account_id IS NOT NULL … (scoped to approved rows whose credential is saved) would finish the cleanup. It'd be a migration or ops step, so it'd be held while the deploy is red. Possibly a follow-up issue.
• An approval on a deployment without a credStore keeps the payload by design (it's the only copy), as the PR body says.

Runtime checks are pending: no local fail-before or probe runs under the memory guard.

[cristim]

APPROVED 097acb0 by claude-qa-ui-4

Runtime checks (after the memory guard, GOMAXPROCS=4 -p 2): go test ./internal/api/ -run Registration and go test -tags integration ./internal/config/ -run Registration pass. Probes: removing the approval clear fails TestApproveRegistration_ClearsPayloadAfterSave; removing the rejection NULL fails TestRegistrationCredentialPayload_ClearedOnRejectionPostgres (real Postgres).

• internal/config/store_postgres_registrations.go:222, :243: the rejection UPDATE sets reg_credential_payload to NULL when reg.Status == "rejected" ($10), in the same statement as the status change, guarded by status = $8. Placeholders $1-$10 line up with the argument list.
• store_postgres_registrations.go:257-263: ClearRegistrationCredentialPayload is a single-row UPDATE by id; a missing id affects no rows.
• internal/api/handler_registrations.go:310-316 with handler_registrations_credentials.go:94-104: the clear runs only when SaveCredential returned nil; a nil payload, a nil credStore or a save error keep the payload. A failed clear is logged, and the approval stands.
• Transitions: both callers of TransitionRegistrationStatus move from "pending" only (handler_registrations.go:404, store_postgres_registrations.go:282), so no path reopens a decided registration and needs the cleared payload. The only readers of the payload are the approval decrypt (handler_registrations_credentials.go:73-79) and the store scan; the field is json:"-" (config/types.go:1525).
• Tests: TestApproveRegistration_ClearsPayloadAfterSave plus the integration cases for approve, reject and a missing id.

CI: "Security Scanning" fails on GO-2026-6865 (klauspost/compress), which #1805 fixes; every other check passes. Re-run CI after #1805 merges.

Non-blocking: when SaveCredential fails on approval, the payload stays on the registration but nothing retries the save later, so the account is left without a stored credential (logged at Errorf). That predates this PR.

[cristim]

Runtime checks for my approval at 097acb01dae0a273a016ffe67aab9b50dff2c66c, run after the memory guard lifted (scratch worktree, heavy-review-2, GOMAXPROCS=4 -p 2). The baseline runs (the api unit tests and -tags integration for ./internal/config) pass. Each mutation was applied on its own and reverted before the next, and each one is killed:

• P1: the approval clears even when the save fails. KeepsPayloadWhenSaveFails and KeepsPayloadWhenNothingSaved fail.
• P2: the rejection keeps the payload ($10 forced to false). ClearedOnRejectionPostgres fails.
• P3: the clear statement becomes a no-op. ClearedAfterApprovalPostgres fails.

The approval stands, now with runtime checks done.

[cristim]

APPROVED 306c8a7 by claude-qa-ui-4

Merge-only delta from my approval at 097acb0.

• 306c8a7 is a merge with parents 097acb0 and main 824a923 (fix(deps): bump klauspost/compress to v1.20.2 for GO-2026-6865 #1805); git show --remerge-diff is empty, so there are no conflict resolutions.
• git range-diff shows the PR commit unchanged (097acb0 = 097acb0).
• CI at this head: 40 pass, 1 skipped, including Security Scanning.

[cristim]

APPROVED 306c8a7 by claude-ui-fix-2

Merge-only delta from 097acb01 (approved, with runtime probes P1, P2 and P3 all killed) to 306c8a74085623e67bd7fba55abf8e4d167d0340.

• Parents: 097acb01 and main 824a9239 (fix(deps): bump klauspost/compress to v1.20.2 for GO-2026-6865 #1805, the compress fix).
• PR side: git diff 824a9239 306c8a74085623e67bd7fba55abf8e4d167d0340 is byte-identical to the approved merge-base..097acb01 diff, index lines ignored.
• Main side: git diff 097acb01 306c8a74085623e67bd7fba55abf8e4d167d0340 is only main's own changes, hunk offsets ignored.
• Remerge: git show --remerge-diff is empty, so there are no conflict resolutions and no hand edits.
• CI at this SHA: 40 pass, 1 skipped, 0 failing. Security Scanning is green now that fix(deps): bump klauspost/compress to v1.20.2 for GO-2026-6865 #1805 is in.