[IAmJSD]

Root cause

The original report has two parts. The first, the raw key-source string (e.g. CREDENTIAL_ENCRYPTION_KEY_SECRET_ARN) showing up in /health, is already fixed on main: checkCredentialStore never puts encryptionKeySource in the body. The second part was still live. When CREDENTIAL_ENCRYPTION_ALLOW_DEV_KEY=1 is set, checkCredentialStore (internal/api/health.go) returned {"status":"degraded","message":"dev_key_in_use"}. /health and /api/health are AuthPublic routes, so this told an unauthenticated caller that every stored cloud credential is encrypted with the publicly known all-zero key. The 2026-07-28 review comment on the issue asked for exactly this to be fixed. checkConfigStore in the same file already follows the rule for this route: a generic message in the body, detail in the log.

Fix

• checkCredentialStore still reports status: "degraded", so the overall status is unchanged and monitoring keeps alerting. The public message is now the generic "credential store degraded".
• The cause is logged server-side on each check: logging.Warnf("health: credential store is using the insecure all-zero dev key (CREDENTIAL_ENCRYPTION_ALLOW_DEV_KEY=1)"). Startup already logs WARN: credentials: ... using insecure all-zero dev key and credentials: loaded encryption key via <source>, so operators keep the full detail in logs.
• Updated the comments on Handler.encryptionKeySource and Application.encKeySource, which said the value was "surfaced via /health".

Consumers checked (none rely on the message)

• Frontend (frontend/src): does not call /health or /api/health.
• Deploy and smoke scripts (scripts/wait-for-healthy.sh, scripts/test-deployment.sh, scripts/test-wait-for-healthy.sh) and workflows (deploy-gcp.yml, deploy-aws-*.yml, rollback.yml): check HTTP status and/or the top-level status. They use the server-side /health and /ready (internal/server/health.go), which have no credential_store check.
• e2e (tests/e2e/e2e_test.go): checks for config_store, auth_store and migrations on the server /health. Not affected.
• IaC (terraform/modules/monitoring/*, terraform/modules/deployment-checks, the compute modules' health probes): match on path or status code only. Nothing matches dev_key_in_use.
• Docs: cmd/rekey/README.md checks for credential_store: { status: "healthy" }, which is unchanged. openapi.yaml HealthResponse has no enum for messages. grep -rn dev_key_in_use now finds only the updated test, which asserts its absence.

No detail needs to move to an authenticated endpoint because nothing reads it. The server log covers the operator use case.

Verification (unit / mock evidence)

Commands were run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid.

Fail-before: the new and updated tests were run against unmodified health.go:

go test ./internal/api/ -run 'CredentialStore|GetHealth'
--- FAIL: TestHandler_checkCredentialStore_DevKeyInUse   expected "credential store degraded", actual "dev_key_in_use"
--- FAIL: TestHandler_GetHealth_DevKeyNotDisclosed       body {"...credential_store":{"status":"degraded","message":"dev_key_in_use"}...} should not contain "dev_key"

Pass-after:

go build ./...
go vet ./internal/api/ ./internal/server/
go test ./internal/api/ ./internal/server/          # ok / ok
golangci-lint run --allow-parallel-runners ./internal/api/... ./internal/server/...   # v2.10.1: 0 issues
gocyclo -over 10 internal/api/health.go internal/api/handler.go internal/server/app.go  # silent
bash scripts/gosec-hook.sh internal/api/health.go internal/api/handler.go internal/server/app.go  # clean
gofmt -l internal/ ; git diff --check   # clean

TestHandler_GetHealth_DevKeyNotDisclosed marshals the full public response for a dev-key handler. It asserts that overall and check status are both still degraded, and that the body contains none of dev_key, dev key, CREDENTIAL_ENCRYPTION_ALLOW_DEV_KEY or CREDENTIAL_ENCRYPTION (case-insensitive). Nothing was run against a real deployment.

Out of scope / notes

• A degraded credential_store status on its own still hints that something is wrong with the key, because the dev key is currently the only thing that makes this check degraded. Hiding that as well would remove the signal that monitoring and the rekey runbook rely on. The issue's remediation explicitly allows "healthy"/"degraded" without detail. If maintainers want the state hidden completely, that needs an admin-authenticated health endpoint, and it would be a separate change.
• The issue still carries the legacy bug label next to type/security (noted in the review comment).

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

Closes #15

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 4 billable files and costs up to $1.00.}

Or wait 41 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

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 1b50a929-8199-471a-8eb2-bd370120bb45

📥 Commits

Reviewing files that changed from the base of the PR and between 2ef2f5d and f8f9eb5.

📒 Files selected for processing: 4

• internal/api/handler.go
• internal/api/health.go
• internal/api/health_test.go
• internal/server/app.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 of 5a7a31b: draft: blocks ready

No BLOCKING findings.

NON-BLOCKING

• internal/api/health.go:104: logging.Warnf fires on every /health call. A load balancer probing every few seconds will log the same warning continuously in a dev-key environment. That is arguably the desired alert signal; if it gets noisy, log once per process (sync.Once). Follow-up at most.
• Residual anonymous signal: the body still carries credential_store: {status: "degraded"} and top-level status: "degraded" in the dev-key case. This is intentional (monitoring keeps alerting) and does not name the key or env var. It reveals only that something is degraded, the same class of signal as config_store unhealthy.

What I attacked

• Anonymous vs authenticated: /health and /api/health are AuthPublic (router.go:361-362). GetHealth (health.go:23-50) returns only the three check statuses and generic messages: config_store uses a generic message, auth_service is generic, credential_store is now generic. encryptionKeySource is read only at health.go:103 and is not marshalled anywhere (grep of internal/ and cmd/). There is no authenticated variant that exposes more; nothing needs one.
• Consumers: grepped terraform, scripts, .github, cloudformation, arm, iac, frontend/src and docs for dev_key, credential_store and degraded. Nothing matches the removed dev_key_in_use message; scripts/wait-for-healthy.sh and the compute-module probes use HTTP status or the top-level status only. cmd/rekey/README.md checks credential_store.status == "healthy", unchanged.
• Fail-before/pass-after on this SHA: restored origin/main's health.go in the worktree and ran the PR's tests. TestHandler_checkCredentialStore_DevKeyInUse and TestHandler_GetHealth_DevKeyNotDisclosed both FAIL; with the PR's health.go both pass. Reverted. go test ./internal/api/ ./internal/server/ passes at the head (3663 tests).
• The leak test lowercases the marshalled body and checks "dev_key", "dev key", the env var name and "CREDENTIAL_ENCRYPTION", so it covers the whole response, not just the one field.
• Scope: comment-only edits in handler.go and server/app.go. Reasonable.

Gate status (origin/main Review gate)

• Independent review: this comment, against the SHA above.
• CI on the SHA: all checks pass (Snyk skipped, pre-commit and CI - Build & Test success). CodeRabbit skipped (draft); no threads.
• Labels: match issue #15's triage set (effort/s, impact/all-users, priority/p3, severity/low, triaged, type/security, urgency/eventually).
• Local verification: unit tests via the real handler with mock stores (the dev-key path is pure in-process logic, so no real account is needed).

Blocks ready

• Public body no longer names the dev key; degraded status preserved for monitoring.
• Fail-before/pass-after confirmed independently; no consumer depends on the removed string.

[IAmJSD]

Addressed the non-blocking note at 2659825, one commit on top of the reviewed 5a7a31b. No rebase.

logging.Warnf fired on every /health call. The warning now goes through a package-level sync.Once (devKeyWarnOnce in internal/api/health.go), so it is logged once per process. The response is unchanged: credential_store and the top-level status still report degraded on every call, so monitoring keeps alerting.

New test TestHandler_checkCredentialStore_DevKeyWarnsOnce resets the Once, captures the default log, calls checkCredentialStore three times and expects the warning exactly once.

• Fail-before: with the Do wrapper removed, the warning is logged three times and the test fails. It passes at 2659825.

Checks on macOS, with AWS config and credentials set to /dev/null:

• go build ./... passed and go vet ./internal/api/ was clean.
• go test ./internal/api/ ./internal/server/ -count=1: ok.
• golangci-lint v2.10.1 reports 0 issues, gocyclo -over 10 is silent and git secrets --scan is clean.

These are unit tests with mock stores. Marking ready.

🤖 Generated with Claude Code

[cristim]

Independent delta review of 2659825 (previous: 5a7a31b): no actionable findings

Delta: OLD is an ancestor of NEW, one new commit (2659825), no rebase. It touches internal/api/health.go (+9 -1) and internal/api/health_test.go (+16). Merge-tree of NEW against origin/main is conflict-free.

Previous finding (health.go:104 warning on every /health call): RESOLVED. The Warnf at health.go:110 now runs inside a package-level sync.Once (health.go:100). The response is untouched: Status "degraded", Message "credential store degraded" (health.go:113-116), so monitoring still alerts.

NON-BLOCKING:

• health.go:97-101: the new devKeyWarnOnce comment and var sit between the existing checkCredentialStore doc comment (health.go:83-96) and the func, so godoc attaches the var's comment, not the func's, and the func loses its doc. Move the var above the doc comment.
• The Once makes the warning a per-process one-off. A log pipeline that rotates or starts after boot will not see it; acceptable since the degraded status stays on every response.

Re-checks at NEW:

• Anonymous /health: the credential_store body never names the env var or the dev key (health.go:109-117). The only signal is the generic "degraded" status, unchanged from OLD. The existing test at health_test.go:92 asserts none of "dev_key", "dev key", the env var name, "CREDENTIAL_ENCRYPTION" appear in the body.
• Authenticated behaviour: /health has no authenticated variant, so the generic body applies to all callers. This matches the intent of #15.
• Consumers: grep of non-test code and IaC finds no reader of the removed message text. The only references are cmd/rekey/README.md, which checks status "healthy".

What I attacked:

• Deletion probe: removed the Once wrapper in a scratch worktree, TestHandler_checkCredentialStore_DevKeyWarnsOnce failed (expected 1, actual 3). Reverted.
• Test hygiene: the test resets the Once before and after, so later tests are not affected. It is not parallel-safe, but no t.Parallel is used here.
• go test ./internal/api/ -run 'Health|CredentialStore|DevKey': 41 passed.

Gate (origin/main Review gate):

• Independent review names the SHA: this comment.
• CI on NEW (run 38889318670 and pre-commit run 38889318663): most checks pass; Lint Code, Security Scanning, Unit Tests, Build Docker Image, Integration Tests and E2E Tests were still pending when I looked. No action_required runs seen. Merge must wait for these.
• CodeRabbit: rate limited, no threads, never pinged.
• Labels mirror the closing issue #15 labels present on the PR (triaged, priority/p3, severity/low, impact/all-users, urgency/eventually, effort/s, type/security).
• Local verification is unit-level with stub stores only; no real-deployment check was run.

[IAmJSD]

I merged origin/main into this branch at f8f9eb5 so CI picks up #960, the scheduler test fix behind the red Unit and Integration runs. I merged instead of rebasing, so there's no force-push. The merge resolved automatically with no conflicts, and the PR's own commits are unchanged since 2659825. Locally at f8f9eb5, go build ./... passes and so does go test ./internal/api/ ./internal/scheduler/. The fork CI run for the new head needs approval.

🤖 Generated with Claude Code

[cristim]

Independent delta review of f8f9eb5 (previous: 2659825): no actionable findings

What changed: OLD_SHA is an ancestor of NEW_SHA. The only new commit on the PR branch is a merge of origin/main (f8f9eb5, "picks up #960"); the rest of OLD..NEW is main content. git range-diff of the PR's own commits against their merge-base with main shows both commits unchanged (5a7a31b =, 2659825 =). The PR's own diff vs origin/main is identical in content at both SHAs (4 files, +68/-7), and the merge-tree against current origin/main is conflict-free.

Previous findings: no blocking findings. The prior NON-BLOCKING nit is NOT RESOLVED and unchanged: internal/api/health.go:94-101, the devKeyWarnOnce var and its comment sit between checkCredentialStore's doc comment and the func, so the doc comment is detached from the func in godoc. Optional cosmetic follow-up: move the var above the doc comment.

Anonymous /health re-confirmed at NEW_SHA: the dev-key branch (health.go:109-116) returns the fixed message "credential store degraded"; the env var name and "dev key" appear only in a server-side logging.Warnf, once per process. encryptionKeySource has no other consumer (grep of non-test Go/TS: handler.go:137,141,192 and health.go:109 only), and no dev_key_in_use string remains outside tests. TestHandler_GetHealth_DevKeyNotDisclosed checks the marshalled body for "dev_key", "dev key", the env var name and "CREDENTIAL_ENCRYPTION".

Tests at NEW_SHA: go test ./internal/api -run 'Health|CredentialStore' 41 passed; go test ./internal/server 532 passed.

CI on NEW_SHA: run 38897002646 (CI - Build & Test) success; run 38897002624 (pre-commit) success; all checks pass (Snyk skipping; CodeRabbit reports rate limited, not pinged). No action_required runs. Labels mirror #15 (triaged, priority/p3, severity/low, impact/all-users, urgency/eventually, effort/s, type/security).

Gate: independent review names this SHA; CI green on it; local verification above (unit tests with a body-leak assertion; the real GetHealth path, no real-account verification applicable). The verdict is recorded here.

[cristim]

Gate record for f8f9eb5 (origin/main CLAUDE.md "Review gate"):

• Independent review of 5a7a31b: no blocking findings. Anonymous /health no longer names the dev encryption key, and no consumer uses the removed dev_key_in_use string.
• Independent delta review of 2659825: no actionable findings. The per-call warning nit is resolved with a package-level sync.Once, and the response still reports degraded. Non-blocking: the Once var sits between the doc comment and the func.
• Independent delta review of f8f9eb5: no actionable findings. The only new commit is a merge of origin/main (picks up test(scheduler): kick the stale refresh with an unrestricted reader #960). Range-diff shows the PR's commits are unchanged.
• CI on f8f9eb5: CI Success and Run pre-commit hooks pass (runs 38897002646, 38897002624). internal/server: 532 tests pass.
• Local verification (macOS, stub stores):
  • The health, credential-store and dev-key tests pass (41).
  • Removing the Once fails the new test (expected 1, got 3).
  • With origin/main's health.go, both original new tests fail.
• CR skipped: quota-limited; covered by the independent reviews plus local verification.
• Labels mirrored from sec: health endpoint exposes encryptionKeySource field — information disclosure #15.

Merging at f8f9eb5 with --match-head-commit.