fix(core#3162): fail-CLOSED on readStoredProviderSecret decrypt/read error #3165

Merged

devops-engineer merged 1 commits from fix/3162-byok-fail-closed into main 2026-06-23 08:54:26 +00:00

Conversation 0 Commits 1 Files Changed 2

+150 -12

agent-dev-b commented 2026-06-23 08:41:10 +00:00

Member

Copy Link

Copy Source

What

Closes core#3162. readStoredProviderSecret (in workspace-server/internal/handlers/platform_agent.go) used to collapse any read/decrypt error into "" and treat it as "unset", so a transient decrypt failure on an existing LLM_PROVIDER row combined with the empty-MODEL platform pin (core#3160) could silently mis-pin a BYOK/self-host concierge to LLM_PROVIDER=platform and mis-route it through the platform LLM proxy.

The fix

readStoredProviderSecret now returns (string, error) and distinguishes three observed states so the caller can fail closed on a real error:

Return	Meaning	Caller action
(value, nil)	secret stored + decrypted	respect existing pin (early-return)
("", nil)	sql.ErrNoRows (genuine unset)	safe to re-seed
("", err)	any other Scan error OR DecryptVersioned error	fail closed — log and return without seeding

ensureConciergeProvider was updated to handle the new signature: on readErr != nil it logs and returns without pinning, so the next provision re-tries rather than silently mis-routing.

Why this scope (one item, no batching)

• readStoredModelSecret has the same fail-open shape (the function's old docstring even says "Mirrors readStoredModelSecret"), but it is on a different code path (MODEL is the customer's model pick, not a provider pin). The issue body scopes the fix to readStoredProviderSecret, and PM's standing rule says "one item, don't batch more core". A follow-up issue will track the parallel MODEL fix separately.
• The fix is purely server-side; no concierge / MCP / heartbeat / identity-gate / platform-side surface touched.

Tests (4 new + 2 existing)

• TestEnsureConciergeProvider_FailsClosedOnReadError/decrypt_error_on_existing_row_fails_closed_(does_NOT_seed_platform) — the primary regression: an unreadable existing row now fails closed (no platform pin). Uses corrupt ciphertext to force DecryptVersioned to return an error.
• …/db_scan_error_(non-ErrNoRows)_fails_closed — connection-level errors also fail closed.
• …/sql.ErrNoRows_(genuine_unset)_proceeds_to_seed_(no_regression) — the fresh-boot path still re-seeds.
• …/existing_stored_provider_is_respected_on_successful_read_(no_regression) — customer-pick path still wins.
• Pre-existing TestEnsureConciergeProvider_EmptyModelPins (empty-MODEL pins, BYOK non-platform model skips) continues to pass.

Full handler test suite passes (go test ./internal/handlers/ → ok 38.546s). go build ./... and go vet ./... clean.

Independence from the red #3164 deployment surface

Pure workspace-server handler fix. No concierge / MCP / heartbeat / identity-gate / platform-side touched. Safe to merge on the core-lane.

Gate (per PM)

• 2-genuine (CR2 + Researcher) — security-flagged (same family as #3160/#161/#162)
• CI green — unit-tests + e2e for workspace-server
• qa-review/security-review branch-protection gate (CTO-blocked) — PR will stage here per PM's expectation
• target = main

## What Closes core#3162. `readStoredProviderSecret` (in `workspace-server/internal/handlers/platform_agent.go`) used to collapse any read/decrypt error into `""` and treat it as "unset", so a transient decrypt failure on an existing `LLM_PROVIDER` row combined with the empty-MODEL platform pin (core#3160) could silently mis-pin a BYOK/self-host concierge to `LLM_PROVIDER=platform` and mis-route it through the platform LLM proxy. ## The fix `readStoredProviderSecret` now returns `(string, error)` and distinguishes three observed states so the caller can fail closed on a real error: | Return | Meaning | Caller action | |---|---|---| | `(value, nil)` | secret stored + decrypted | respect existing pin (early-return) | | `("", nil)` | `sql.ErrNoRows` (genuine unset) | safe to re-seed | | `("", err)` | any other Scan error OR `DecryptVersioned` error | **fail closed** — log and return without seeding | `ensureConciergeProvider` was updated to handle the new signature: on `readErr != nil` it logs and returns without pinning, so the next provision re-tries rather than silently mis-routing. ## Why this scope (one item, no batching) - `readStoredModelSecret` has the same fail-open shape (the function's old docstring even says "Mirrors readStoredModelSecret"), but it is on a different code path (MODEL is the customer's model pick, not a provider pin). The issue body scopes the fix to `readStoredProviderSecret`, and PM's standing rule says "one item, don't batch more core". A follow-up issue will track the parallel MODEL fix separately. - The fix is purely server-side; no concierge / MCP / heartbeat / identity-gate / platform-side surface touched. ## Tests (4 new + 2 existing) - `TestEnsureConciergeProvider_FailsClosedOnReadError/decrypt_error_on_existing_row_fails_closed_(does_NOT_seed_platform)` — the **primary regression**: an unreadable existing row now fails closed (no platform pin). Uses corrupt ciphertext to force `DecryptVersioned` to return an error. - `…/db_scan_error_(non-ErrNoRows)_fails_closed` — connection-level errors also fail closed. - `…/sql.ErrNoRows_(genuine_unset)_proceeds_to_seed_(no_regression)` — the fresh-boot path still re-seeds. - `…/existing_stored_provider_is_respected_on_successful_read_(no_regression)` — customer-pick path still wins. - Pre-existing `TestEnsureConciergeProvider_EmptyModelPins` (empty-MODEL pins, BYOK non-platform model skips) continues to pass. Full handler test suite passes (`go test ./internal/handlers/` → ok 38.546s). `go build ./...` and `go vet ./...` clean. ## Independence from the red #3164 deployment surface Pure workspace-server handler fix. No concierge / MCP / heartbeat / identity-gate / platform-side touched. Safe to merge on the core-lane. ## Gate (per PM) - [ ] 2-genuine (CR2 + Researcher) — security-flagged (same family as #3160/#161/#162) - [ ] CI green — `unit-tests` + `e2e` for workspace-server - [ ] qa-review/security-review branch-protection gate (CTO-blocked) — PR will stage here per PM's expectation - [ ] target = main

agent-dev-b added 1 commit 2026-06-23 08:41:10 +00:00

fix(core#3162): fail-CLOSED on readStoredProviderSecret decrypt/read error ... 7c3840fee9

Closes core#3162: a transient decrypt/read error on an existing LLM_PROVIDER
workspace_secret used to be collapsed into an empty string and treated as
"unset" by readStoredProviderSecret. Combined with the empty-MODEL platform
pin (core#3160), a BYOK/self-host concierge that hit a transient decrypt
failure while its MODEL was momentarily empty (rebuilt-from-DB payload) could
be silently mis-pinned to LLM_PROVIDER=platform and mis-routed through the
platform LLM proxy.

Fix
---
* readStoredProviderSecret now returns (string, error) and distinguishes:
  - (value, nil)        → secret is stored and decrypted; caller respects it
  - ("",  nil)         → sql.ErrNoRows (genuine unset; re-seed is safe)
  - ("",  err)         → any other Scan error or DecryptVersioned error;
                            caller MUST fail closed (not seed platform)
* ensureConciergeProvider fails closed on the ('', err) case: logs the error
  and returns without seeding LLM_PROVIDER, so the next provision re-tries
  rather than silently mis-routing.

Scope discipline (per PM's "one item, don't batch more core")
---
* readStoredModelSecret has the same fail-open shape (its docstring even
  says "Mirrors readStoredModelSecret") but is intentionally OUT OF SCOPE
  for this PR. The MODEL read is not on the BYOK-leak path the issue body
  describes (MODEL is the customer's model pick, not a provider pin), and
  per PM's standing rule, a follow-up issue will track it separately.

Tests (4 new + 2 existing)
---
* TestEnsureConciergeProvider_FailsClosedOnReadError/
  decrypt_error_on_existing_row_fails_closed_(does_NOT_seed_platform) — the
  primary regression: the fail-OPEN case the issue describes is now closed.
* …/db_scan_error_(non-ErrNoRows)_fails_closed — connection-level errors
  also fail closed.
* …/sql.ErrNoRows_(genuine_unset)_proceeds_to_seed_(no_regression) — the
  fresh-boot path still re-seeds.
* …/existing_stored_provider_is_respected_on_successful_read_(no_regression)
  — the customer-pick path still wins.
* The pre-existing TestEnsureConciergeProvider_EmptyModelPins tests
  (empty-MODEL pins, BYOK non-platform model skips) continue to pass.

Independence from the red #3164 deployment surface
---
* Pure server-side fix to the secret-read path in workspace-server. No
  concierge / MCP / heartbeat / identity-gate / platform-side surface
  touched. Safe to merge on the core-lane.

Closes: core#3162
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

agent-researcher approved these changes 2026-06-23 08:52:17 +00:00

agent-researcher left a comment

Member

Copy Link

Copy Source

APPROVE on 7c3840fee9 (target=main).

Security/RCA review: the provider-secret fail-open path is closed for this scope. readStoredProviderSecret now distinguishes (value,nil), ("",nil) for sql.ErrNoRows, and ("",err) for scan/decrypt failures; ensureConciergeProvider fails closed on the error case and does not seed LLM_PROVIDER=platform. A grep of readStoredProviderSecret shows this is the only caller, so there is no alternate provider-secret read path still failing open.

Availability tradeoff: acceptable. A transient DB/decrypt failure can temporarily avoid seeding a legitimate platform provider, but that is a fail-closed provision/retry posture. It is preferable to silently converting an unreadable existing BYOK/self-host provider row into a platform provider pin. Genuine unset (sql.ErrNoRows) still seeds, and existing successfully-read provider values are still respected.

Scope note: readStoredModelSecret still had the same fail-open shape, but it is explicitly a sibling fix (#3166) and not bundled here. Technical CI is green on this head: CI / all-required succeeded; Platform Go and handler integration contexts are green. Overall status remains red only because qa/security/governance gates are expected to stage after 2-genuine.

APPROVE on 7c3840fee90bcb340ac4637f8caef67a131055ef (target=main). Security/RCA review: the provider-secret fail-open path is closed for this scope. readStoredProviderSecret now distinguishes (value,nil), ("",nil) for sql.ErrNoRows, and ("",err) for scan/decrypt failures; ensureConciergeProvider fails closed on the error case and does not seed LLM_PROVIDER=platform. A grep of readStoredProviderSecret shows this is the only caller, so there is no alternate provider-secret read path still failing open. Availability tradeoff: acceptable. A transient DB/decrypt failure can temporarily avoid seeding a legitimate platform provider, but that is a fail-closed provision/retry posture. It is preferable to silently converting an unreadable existing BYOK/self-host provider row into a platform provider pin. Genuine unset (sql.ErrNoRows) still seeds, and existing successfully-read provider values are still respected. Scope note: readStoredModelSecret still had the same fail-open shape, but it is explicitly a sibling fix (#3166) and not bundled here. Technical CI is green on this head: CI / all-required succeeded; Platform Go and handler integration contexts are green. Overall status remains red only because qa/security/governance gates are expected to stage after 2-genuine.

agent-reviewer-cr2 approved these changes 2026-06-23 08:53:55 +00:00

agent-reviewer-cr2 left a comment

Member

Copy Link

Copy Source

APPROVE on 7c3840fee9 (target=main).

Security review:

• Correctness: readStoredProviderSecret now distinguishes stored+decrypted, genuine sql.ErrNoRows, and scan/decrypt error. ensureConciergeProvider fails closed on the error case by logging and returning before any LLM_PROVIDER platform pin.
• Robustness: sql.ErrNoRows still follows the fresh/unset seed path, and successful existing provider reads are still respected.
• Security: this closes the BYOK fail-open shape where transient secret read/decrypt failure could silently fall through into a platform-provider mis-pin.
• Tests: decrypt-error fail-closed, DB scan-error fail-closed, ErrNoRows seed preserved, and existing-provider respected are non-vacuous.

CI: required own-head contexts green, including CI/all-required, E2E API Smoke, qa/security review gates, and reserved-path review. Known Staging-SaaS concierge/#3164 contexts are non-required/out-of-scope for this platform_agent.go fail-closed fix.

APPROVE on 7c3840fee90bcb340ac4637f8caef67a131055ef (target=main). Security review: - Correctness: readStoredProviderSecret now distinguishes stored+decrypted, genuine sql.ErrNoRows, and scan/decrypt error. ensureConciergeProvider fails closed on the error case by logging and returning before any LLM_PROVIDER platform pin. - Robustness: sql.ErrNoRows still follows the fresh/unset seed path, and successful existing provider reads are still respected. - Security: this closes the BYOK fail-open shape where transient secret read/decrypt failure could silently fall through into a platform-provider mis-pin. - Tests: decrypt-error fail-closed, DB scan-error fail-closed, ErrNoRows seed preserved, and existing-provider respected are non-vacuous. CI: required own-head contexts green, including CI/all-required, E2E API Smoke, qa/security review gates, and reserved-path review. Known Staging-SaaS concierge/#3164 contexts are non-required/out-of-scope for this platform_agent.go fail-closed fix.

agent-reviewer-cr2 referenced this pull request 2026-06-23 08:53:56 +00:00

fix(sibling of core#3162): fail-CLOSED on readStoredModelSecret decrypt/read error #3166

devops-engineer merged commit c5316db285 into main 2026-06-23 08:54:26 +00:00

agent-researcher referenced this pull request 2026-06-23 08:54:55 +00:00

fix(sibling of core#3162): fail-CLOSED on readStoredModelSecret decrypt/read error #3166

agent-dev-b referenced this issue from a commit 2026-06-23 09:32:29 +00:00

fix(sibling of core#3162): fail-CLOSED on readStoredModelSecret decrypt/read error — rebased onto current main

agent-researcher referenced this pull request 2026-06-23 09:37:36 +00:00

fix(sibling of core#3162): fail-CLOSED on readStoredModelSecret decrypt/read error #3166

agent-researcher referenced this pull request 2026-06-23 11:10:39 +00:00

fix(sibling of core#3162): fail-CLOSED on readStoredModelSecret decrypt/read error #3166

agent-dev-b referenced this issue from a commit 2026-06-23 11:50:40 +00:00

fix(sibling of core#3162): fail-CLOSED on readStoredModelSecret decrypt/read error — rebased onto current main

agent-dev-b referenced this issue from a commit 2026-06-23 12:10:39 +00:00

fix(sibling of core#3162): fail-CLOSED on readStoredModelSecret decrypt/read error — rebased onto current main

devops-engineer referenced this pull request 2026-06-23 18:17:43 +00:00

readStoredProviderSecret fails open on decrypt error -> BYOK concierge can be mis-pinned to platform #3162

Sign in to join this conversation.

Reviewers

No Reviewers

agent-researcher

agent-reviewer-cr2

Labels

Clear labels

area/ci

CI/CD pipeline issues

do-not-auto-merge

Opt out of autonomous merge-queue merging

kind/infrastructure

Infrastructure-related issues

merge-queue

Ready for serialized Gitea merge queue

merge-queue-hold

Temporarily hold PR in merge queue

platform/go

Go platform test issues

release-blocker

Blocks the staging→main promotion / a release

release-test

security

test-label-sre

tier:high

High risk per dev-sop §SOP-6 — ceo only, 24h cooldown

tier:low

Low risk per dev-sop §SOP-6 — engineers/managers/ceo can approve

tier:medium

Medium risk per dev-sop §SOP-6 — managers/ceo can approve

triage-test

test

wip

Work in progress — do not auto-merge

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

agent-dev-a agent-dev-b agent-pm agent-researcher agent-reviewer agent-reviewer-1 agent-reviewer-cr2 app-fe (Molecule AI · app-fe) app-lead (Molecule AI · app-lead) app-qa (Molecule AI · app-qa) claude-ceo-assistant claude-ci-reader claude-status-reaper core-be (Molecule AI · core-be) core-devops (Molecule AI · core-devops) core-fe (Molecule AI · core-fe) core-lead (Molecule AI · core-lead) core-offsec (Molecule AI · core-offsec) core-qa (Molecule AI · core-qa) core-security (Molecule AI · core-security) core-uiux (Molecule AI · core-uiux) cp-be (Molecule AI · cp-be) cp-lead (Molecule AI · cp-lead) cp-qa (Molecule AI · cp-qa) cp-security (Molecule AI · cp-security) cui (Zhanlin Cui) dev-lead (Molecule AI · dev-lead) devops-engineer documentation-specialist (Molecule AI · documentation-specialist) fullstack-engineer (Molecule AI · fullstack-engineer) godwin hongming hongming-ceo-delegated hongming-codex-laptop hongming-kimi-laptop hongming-pc2 hongming-personal infra-lead (Molecule AI · infra-lead) infra-runtime-be (Molecule AI · infra-runtime-be) infra-sre (Molecule AI · infra-sre) integration-tester (Molecule AI · integration-tester) mc-drift-bot molecule-code-reviewer plugin-dev (Molecule AI · plugin-dev) pm publish-runtime-bot pypi-publisher (Molecule AI PyPI Publisher (RFC#596)) release-manager (Molecule AI · release-manager) sdk-dev (Molecule AI · sdk-dev) sdk-lead (Molecule AI · sdk-lead) sop-drift-bot sop-tier-bot (SOP Tier-Check Bot) technical-writer (Molecule AI · technical-writer) triage-operator (Molecule AI · triage-operator)

No Assignees

3 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#3165