Skip to content

[feat] Write-only vault secrets (values never readable back by users) - #6164

Merged
mmabrouk merged 31 commits into
release/v0.114.0from
write-only-secrets-api
Aug 23, 2026
Merged

[feat] Write-only vault secrets (values never readable back by users)#6164
mmabrouk merged 31 commits into
release/v0.114.0from
write-only-secrets-api

Conversation

@mmabrouk

@mmabrouk mmabrouk commented Aug 21, 2026

Copy link
Copy Markdown
Member

Context

Vault values must be usable by an authorized workload without being readable back through the user API. The earlier version removed the list cache, exposed key-specific status fields, allowed visibility to change on update, and could fall back to the administrator key for runtime proof. Those choices added latency and blurred the security boundary.

Changes

This PR defines the production write-only contract.

Before:

{"write_only": true, "has_key": true, "key_preview": "sk-****9Qa"}

After:

{"write_only": true, "value_status": {"configured": true, "preview": "sk-****9Qa"}}

The Vault list cache is restored with its existing TTL, namespace, invalidation, and shortened UUID packing. Redis stores the canonical trusted DTO. Every caller is projected after the cache read, so an ordinary caller receives redacted data while a verified runtime grant receives plaintext from the same entry.

write_only is selected at creation and cannot change on update. Omitted values keep the value from the row loaded under the DAO lock. Existing rows without the stored field remain readable. SSO and webhook creation explicitly set write_only=False, so their edit, login, test, and signature flows do not change.

The short-lived secret-resolve grant remains project-scoped and allowlisted. AGENTA_SERVICES_INTERNAL_KEY is the only proof accepted on the API-to-Services exchange. It has no AGENTA_AUTH_KEY fallback, never reaches a runner or sandbox, and missing or placeholder configuration now fails API startup. Standalone SDK runs keep the narrow provider-specific environment fallback.

Storage remains in the existing encrypted JSON payload. No database migration or feature flag is introduced.

The Python SDK now reads value_status.configured through one shared helper in all three redaction consumers. There is no production has_key fallback. Updates use one strict contract: omission keeps the stored credential, while an explicit blank provider credential is invalid. The frontend in #6174 omits untouched credentials, and the backend and frontend ship in the same release, so no transition compatibility path is needed.

Tests / notes

  • 64 focused write-only API tests passed after the final webhook cleanup.
  • The final combined API verification passed 229 tests after removing four unsupported write-only webhook compatibility cases.
  • Services credential exchange and secret mapping passed 4 tests.
  • SDK write-only resolution and provider-specific fallback passed 89 tests.
  • The final contract audit passed 31 focused SDK tests and 56 focused API tests; the complete local unit gates passed 2,241 SDK tests and 2,668 API tests.
  • Ruff formatting and checks passed.
  • Compose, Helm, and Railway templates were checked statically. Railway live QA is deferred while Railway is unavailable.
  • Full implementation report: docs/design/write-only-secrets/implementation-report.md.
  • Release QA: docs/design/write-only-secrets/qa.md.

What to QA

  • List the same project twice as an ordinary caller and as a granted runtime. Cache hit and miss must preserve redacted versus plaintext behavior.
  • Create, update, and delete a secret. The next list must reflect each mutation immediately.
  • Edit without entering a key and confirm the request omits it and the value is kept. Send an explicit blank directly and confirm the API rejects it.
  • Edit an SSO provider and send a signed webhook. Both existing readable-secret flows must remain unchanged.
  • Start without AGENTA_SERVICES_INTERNAL_KEY, then with replace-me. Both starts must fail and name the variable.
  • Run through Services with a valid key. The workload must resolve the secret, while the runner and sandbox receive no internal key.

Based on release/v0.114.0.

@vercel

vercel Bot commented Aug 21, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
agenta-documentation Ready Ready Preview Aug 23, 2026 2:09pm

Request Review

@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Summary by CodeRabbit

  • New Features
    • Added write-only secrets with redacted responses and value-status indicators.
    • Authorized platform runtime operations can securely resolve protected secret values.
    • Added actionable errors and environment-based alternatives for unavailable credentials.
    • Mobile web services are now enabled by default, with opt-out controls.
  • Bug Fixes
    • Prevented accidental exposure of provider, webhook, SSO, and custom-secret credentials.
    • Improved secret updates to preserve credentials safely and reject invalid changes.
  • Documentation
    • Updated deployment guidance for secure internal service authentication and write-only secrets.

Walkthrough

The change adds write-only secret storage and redacted API responses. It adds runtime-authenticated plaintext access through SECRET_RESOLVE_GRANT, updates SDK resolution behavior, and provisions AGENTA_SERVICES_INTERNAL_KEY to API and Services workloads.

Changes

Write-only secret lifecycle

Layer / File(s) Summary
Secret contracts, persistence, and redaction
api/oss/src/core/secrets/*, api/oss/src/dbs/postgres/secrets/*, api/oss/tests/pytest/unit/secrets/*
Secret DTOs separate create and update validation. Services preserve omitted credentials during compatible updates. PostgreSQL stores write-only metadata. Public responses redact credentials and expose value status.
Vault and outward response boundaries
api/oss/src/apis/fastapi/vault/router.py, api/oss/src/core/webhooks/service.py, api/ee/src/core/organizations/service.py, api/oss/tests/pytest/unit/vault/*, api/oss/tests/pytest/unit/webhooks/*, api/ee/tests/pytest/unit/*
Vault routes apply caller-specific redaction. Webhook and organization-provider responses use outward redaction while internal readers retain plaintext access.
Secret-resolution grants
api/oss/src/middlewares/auth.py, api/oss/src/apis/fastapi/access/router.py, api/oss/src/core/workflows/service.py, api/oss/tests/pytest/unit/access/*, api/oss/tests/pytest/unit/middlewares/*
Secret tokens validate and carry secret-resolve. Access and workflow paths issue the grant only for approved runtime requests.
SDK credential resolution
sdks/python/agenta/sdk/agents/connections/*, sdks/python/agenta/sdk/agents/platform/*, sdks/python/agenta/sdk/middlewares/*, sdks/python/oss/tests/pytest/unit/agents/*, sdks/python/oss/tests/pytest/unit/test_auth_middleware_credentials.py
The SDK classifies credential extras, detects redacted write-only records, supports environment credential fallback, raises WriteOnlySecretError, and filters unusable vault entries.

Runtime credential deployment

Layer / File(s) Summary
Runtime key validation and provisioning
api/oss/src/utils/*, api/entrypoints/routers.py, hosting/docker-compose/*, hosting/kubernetes/*, hosting/railway/*
The dedicated runtime key is normalized and validated during startup. Compose, Helm, and Railway configurations provide it to API and Services while excluding it from worker and auxiliary workloads.
Deployment validation
hosting/kubernetes/helm/tests/test_runner_secret_absence.py, api/oss/tests/pytest/unit/utils/test_env_helpers.py, services/oss/tests/pytest/unit/agent/test_credential_exchange.py
Tests verify runtime-key validation, credential forwarding, required deployment placement, and exclusion from runner and worker environments.

Design documentation and fixture scanning

Layer / File(s) Summary
Write-only secret design
docs/design/write-only-secrets/README.md
The design document defines storage, redaction, update, grant, deployment, consumer, and cache behavior.
Synthetic fixture allowlists
.gitleaks.toml, .gitleaksignore
Gitleaks allows documented synthetic fixture values and historical fake test fixtures.

Estimated code review effort: 5 (Critical) | ~120 minutes

Merge Risk: 🟠 High · up to 92fc4

The change introduces write-only secret resolution and restores caching, but the current implementation caches decrypted credentials before redaction, creating a direct secret-exposure risk; unresolved tenant-isolation, credential-routing, API-contract, and deployment-startup issues also remain. The PR is not merge-ready until the high-impact security and availability risks are fixed or explicitly accepted.

Sequence Diagram(s)

sequenceDiagram
  participant AccessRouter
  participant sign_secret_token
  participant verify_secret_token
  participant VaultRouter
  AccessRouter->>sign_secret_token: issue run_service token with secret-resolve
  sign_secret_token->>verify_secret_token: validate grants claim
  verify_secret_token->>VaultRouter: expose verified token_grants
  VaultRouter->>VaultRouter: return plaintext only with secret-resolve
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 15.64% which is insufficient. The required threshold is 60.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 326 functions across 39 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the main change: adding write-only vault secrets whose values users cannot read back.
Description check ✅ Passed The description directly explains the write-only secret contract, runtime access, redaction behavior, configuration, testing, and deployment changes.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch write-only-secrets-api

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

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

@mmabrouk

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor
⚠️ 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.

@mmabrouk

Copy link
Copy Markdown
Member Author

Review requested by @mmabrouk. Generated by OpenAI Codex CLI (gpt-5.6-sol, xhigh reasoning) with full cross-PR context; posted by the orchestrating agent. Findings are being addressed on this branch.

Codex review (gpt-5.6-sol, xhigh)

Request changes. The primary vault routes redact common value fields, and the grant remains signed, additive, and limited to 15 minutes, but several user-facing paths still expose write-only values. The tightening transition also has database and Redis races that can make or serve a secret as readable again, so this is not safe to enable.

Findings

  1. [P1] Custom-provider credential extras still escape redaction — The backend classifies only four extras as credentials at api/oss/src/core/secrets/redaction.py:22, while the SDK treats many additional extras as runtime credentials, including ANTHROPIC_AUTH_TOKEN, AWS_BEARER_TOKEN_BEDROCK, uppercase AWS_SECRET_ACCESS_KEY, and AZURE_OPENAI_API_KEY at sdks/python/agenta/sdk/agents/platform/connections.py:121. Those values remain verbatim in create/list/get/update responses for a write-only custom provider. The same incomplete set drives update carry-over at api/oss/src/core/secrets/services.py:101, so a partial extras update can also erase these unrecoverable credentials. Even the stripped AWS trio is not included when computing has_key at api/oss/src/core/secrets/redaction.py:65; an AWS-only secret therefore reports has_key=false, and a surviving config env such as AWS_REGION makes env nonempty and bypasses WriteOnlySecretError at sdks/python/agenta/sdk/agents/platform/connections.py:609. Redaction, presence detection, carry-over, and SDK credential resolution need one exhaustive credential classifier.

  2. [P1] Router-only redaction leaves webhook and EE SSO endpoints returning plaintext — The env gate applies to every flag-less VaultService.create_secret call at api/oss/src/core/secrets/services.py:153, but webhook creation stores such a secret and immediately returns secret_value at api/oss/src/core/webhooks/service.py:122 and api/oss/src/core/webhooks/service.py:150; fetches resolve it again at api/oss/src/core/webhooks/service.py:327, and the user-facing router returns or decrypts it at api/oss/src/apis/fastapi/webhooks/router.py:223. Likewise, EE organization-provider responses dump the SSO vault DTO, including client_secret, at api/ee/src/core/organizations/service.py:883 and serialize it as settings at api/ee/src/core/organizations/service.py:899. Once the default gate is enabled, these records are marked write-only but these endpoints still expose their values; a manually tightened webhook secret leaks the same way today. Outward consumers need write-only-aware response shaping even though their internal runtime reads must remain plaintext.

  3. [P1] A list/update race can repopulate Redis with plaintext after tightening — On a cache miss, list reads the database at api/oss/src/apis/fastapi/vault/router.py:187 and later writes its snapshot at api/oss/src/apis/fastapi/vault/router.py:193. If that read observes a readable secret, then a concurrent update commits write_only=true and invalidates Redis at api/oss/src/apis/fastapi/vault/router.py:259 and api/oss/src/apis/fastapi/vault/router.py:274, the older list request can subsequently cache its unredacted DTO because its snapshot still says write_only=false. Every following user list can then receive plaintext until the five-minute TTL expires. Tightening needs versioned/cache-generation semantics or another ordering mechanism that prevents a pre-transition snapshot from being written after invalidation.

  4. [P1] The one-way transition is not enforced atomicallyVaultService checks the stored flag in one DAO read at api/oss/src/core/secrets/services.py:274, then performs the update through a separate DAO transaction at api/oss/src/core/secrets/services.py:296. Two concurrent updates can both observe false; the first writes true, then the second’s stale explicit false reaches the mapper, which trusts it and removes the flag at api/oss/src/dbs/postgres/secrets/mappings.py:69. That restores plaintext reads despite the one-way contract. The false-to-true/true-to-false check and mutation must occur under one row lock or conditional update.

  5. [P2] Keep-on-omit is not defined safely across kind changesUpdateSecretDTO accepts a complete replacement SecretDTO without binding its kind to the stored record at api/oss/src/core/secrets/dtos.py:298. Carry-over then matches only structural field names at api/oss/src/core/secrets/services.py:62, while the mapper overwrites both kind and data at api/oss/src/dbs/postgres/secrets/mappings.py:73. An OpenAI-to-Anthropic update with an omitted key silently reuses the OpenAI credential for Anthropic, while a provider-key-to-custom-secret update with omitted content finds no matching container and irreversibly replaces the stored credential with None. Require an explicit new value when either secret kind or provider family changes, or reject such transitions.

  6. [P2] The standalone remediation omits the required mode change — The new error tells users only to provide an environment variable at sdks/python/agenta/sdk/agents/connections/errors.py:75, but an agenta connection never consults that environment variable: only self_managed short-circuits to runtime-provided credentials at sdks/python/agenta/sdk/agents/platform/connections.py:578. Leaving the model configuration unchanged means the user follows the instruction, fetches the same redacted record, and receives the same error again. Either fall back to the environment for this case or explicitly instruct the user to select self_managed authentication as well as setting the variable.

  7. [P2] The preview threshold reveals half of a 12-character secretmask_secret_value starts returning three prefix and three suffix characters at length 12 at api/oss/src/core/secrets/redaction.py:36. Because write-only values include arbitrary custom secrets and SSO client secrets, not only long provider keys, this can disclose six of twelve characters and sharply reduce the remaining search space. Keep the agreed preview, but require a substantially longer minimum or enforce a maximum disclosed fraction.

  8. [P2] The shared list cache is not tenant-unique — The vault list uses an empty logical key scoped only by project at api/oss/src/apis/fastapi/vault/router.py:172, while the cache helper truncates project IDs to their last 12 characters at api/oss/src/utils/caching.py:50. Two projects with the same UUID suffix therefore share the list_secrets entry. Write-only values are normally redacted, but metadata crosses tenants and legacy or explicitly readable secrets still place plaintext in this cache. A security-sensitive cache key must retain the complete tenant identifier.

Test gaps

  • Exercise the real /access/permissions/check route through authentication: allowed and denied run_service, cached allow, non-run actions, expiry, and confirmation that only the successful run exchange returns the grant.
  • Drive create, list, get, and update through actual signed granted and ungranted tokens. Current route tests inject request.state.token_grants directly.
  • Cover every secret kind and every SDK-consumed credential extra, including AWS-only credentials, uppercase env keys, config-plus-redacted-key custom providers, and partial extras updates.
  • Add webhook and EE organization-provider response tests with the default gate enabled and with an already-tightened secret.
  • Add coordinated concurrency tests for readable-to-write-only cache population and competing write_only=true/write_only=false updates.
  • Use real Redis serialization and keys in at least one test, including two full project UUIDs sharing the same 12-character suffix.
  • Test kind/provider-family changes with omitted, empty, and replacement values.
  • Assert malformed create/update validation responses and captured logs never contain the submitted secret; also snapshot the OpenAPI schemas/examples.
  • Pin preview behavior at 11, 12, and longer boundary lengths.

Nits

  • CreateSecretDTO still says the platform default “is write-only” at api/oss/src/core/secrets/dtos.py:250, although the new default is env-gated and currently false.
  • The SDK module says logs never contain secret names at sdks/python/agenta/sdk/agents/platform/secrets.py:13, but the new error logs the name at sdks/python/agenta/sdk/agents/platform/secrets.py:69; the legacy middleware similarly logs joined names at sdks/python/agenta/sdk/middlewares/running/vault.py:368.

Good: the non-mutating redaction helper, explicit-create precedence, and separation between additive grants and confining scopes are all sound foundations.

@mmabrouk

mmabrouk commented Aug 21, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

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.

@mmabrouk

Copy link
Copy Markdown
Member Author

Review round addressed in 04e8cb6975 (commit 3). All 8 Codex findings are closed; the PR body's "Review response" section maps each finding to its fix:

  1. One canonical credential classifier (SDK module, imported by the API; parity test) drives redaction, has_key, and carry-over; AWS-only secrets report has_key=true; the SDK raises WriteOnlySecretError before the env-emptiness check.
  2. Webhook and EE SSO outward responses redact write-only values; internal signing stays plaintext.
  3. Per-project cache generations close the stale-snapshot race.
  4. One-way flag enforced under SELECT ... FOR UPDATE; the mapper never clears a stored flag.
  5. Keep-on-omit is identity-local; kind/family changes require an explicit new value (400 otherwise).
  6. Remediation text now says: switch the connection to self_managed and set the env variable.
  7. Preview policy: under 20 chars fully masked; from 20, at most 3+3 and never more than 25%.
  8. Full-project-id cache keys for the vault list and generation entries.

Tests: api unit 2,940 passed / 0 failed; real-token route tests added. Known gaps stated in the PR body (no live-Postgres concurrency test in the unit environment; legacy VaultMiddleware still logs display names, never values).

(This comment restores a summary that was accidentally overwritten by a review-trigger edit.)

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
api/oss/src/utils/caching.py (1)

43-59: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

invalidate_cache cannot reach full_project_id keys.

_pack, pack, get_cache, and set_cache accept full_project_id, but invalidate_cache does not. Its pattern branch always truncates the project id, so cache:p:<last12>:u:...:* never matches a key written as cache:p:<full-uuid>:....

The vault router still calls invalidate_cache(project_id=...) on every secret write. That call is now a no-op for the list_secrets and list_secrets_generation namespaces, and only the generation bump keeps readers correct. Add the parameter so the invalidation API matches the read and write APIs, and so a future caller does not assume invalidation works.

🛠️ Proposed parameter addition
async def invalidate_cache(
    namespace: Optional[str] = None,
    key: Optional[Union[str, dict]] = None,
    project_id: Optional[str] = None,
    user_id: Optional[str] = None,
    full_project_id: bool = False,
) -> Optional[bool]:
    ...
    cache_name = _pack(
        namespace=namespace,
        key=key,
        project_id=project_id,
        user_id=user_id,
        pattern=True,
        full_project_id=full_project_id,
    )
api/oss/src/core/webhooks/service.py (1)

149-149: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

A generated webhook signing secret becomes unrecoverable once the write-only gate is on.

Line 149 generates the signing secret when the caller supplies none. That secret is created without an explicit write_only, so it inherits AGENTA_VAULT_WRITE_ONLY_DEFAULT. Line 183 then redacts the create echo. When the gate is enabled, the create response returns secret=None and no later response returns the value, so the subscriber can never obtain the key needed to verify HMAC signatures.

A webhook signing secret is a shared verification key, not a write-only credential. Set write_only=False on this create, or return the generated plaintext on the create response only.

🛠️ Proposed fix
         secret_dto = await self.vault_service.create_secret(
             project_id=project_id,
             #
             create_secret_dto=CreateSecretDTO(
+                write_only=False,
                 header={
                     "name": f"webhook-{subscription.name or 'subscription'}",
                     "description": "Webhook signing secret",
                 },

Also applies to: 179-184

🧹 Nitpick comments (5)
api/oss/tests/pytest/unit/secrets/test_write_only.py (1)

36-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Three copies of the in-memory secrets DAO have already drifted. Each file defines its own _FakeSecretsDAO, and the copies differ in stored shape and surface. create stores data as a plain dict from model_dump(exclude_none=True), while update stores the DTO object taken from the request and aliases it. The three suites therefore assert against three different approximations of persistence. Extract one shared double into a test-utils module and normalize data the same way on create and update.

  • api/oss/tests/pytest/unit/secrets/test_write_only.py#L36-L82: move this class into a shared helper module and import it here; store data with the same normalization on both create and update, and deep-copy the incoming DTO data to avoid aliasing the request object.
  • api/oss/tests/pytest/unit/vault/test_write_only_routes.py#L31-L84: delete the local copy and import the shared double; keep the string-keyed record map as a thin adapter if the route tests need it.
  • api/oss/tests/pytest/unit/webhooks/test_write_only_outward.py#L27-L62: delete the local copy and import the shared double, which also restores the get_by_slug and delete methods missing here.
sdks/python/oss/tests/pytest/unit/agents/connections/test_credentials_parity.py (1)

34-41: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Add API-side parity coverage for SecretKind. A new SecretKind value can bypass write-only redaction because redact_secret_response relies on PRIMARY_CREDENTIAL_FIELDS. Assert that {kind.value for kind in SecretKind} equals set(PRIMARY_CREDENTIAL_FIELDS).

api/oss/src/core/secrets/redaction.py (2)

59-87: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Guard against unmapped secret kinds. All current SecretKind values are mapped. If a new kind is added without a mapping, redact_secret_response leaves its credential value unchanged while setting has_key=False; raise for unmapped kinds or add an exhaustive coverage check.


17-20: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Import credential metadata from its owner module. CREDENTIAL_EXTRAS_KEYS and PRIMARY_CREDENTIAL_FIELDS are defined in agenta.sdk.agents.connections.credentials, but the API currently imports them through redaction.py, making that module an implicit re-export surface. Import the constants directly in both redaction.py and services.py, and confirm agenta is declared as an API runtime dependency because this import executes during API startup.

api/oss/src/dbs/postgres/secrets/mappings.py (1)

75-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use the same exclude_none policy on create and update.

map_secrets_dto_to_dbe serializes with exclude_none=True, but Line 81 serializes the update payload without it. An update therefore persists explicit nulls for omitted optional fields, such as models, harnesses, and model_keys, while a create omits those keys. The stored shape then depends on which path wrote the row.

♻️ Proposed alignment
             if key == "data" and hasattr(secrets_dbe, key):
                 secrets_dbe.data = _data_payload(
-                    update_secret_dto.secret.data.model_dump(),
+                    update_secret_dto.secret.data.model_dump(exclude_none=True),
                     write_only=write_only,
                 )

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: d2ed5c5b-207b-4b48-875a-bd10c1dac5c7

📥 Commits

Reviewing files that changed from the base of the PR and between 00db686 and 04e8cb6.

📒 Files selected for processing (29)
  • api/ee/src/core/organizations/service.py
  • api/ee/tests/pytest/unit/test_write_only_provider_settings.py
  • api/oss/src/apis/fastapi/access/router.py
  • api/oss/src/apis/fastapi/vault/router.py
  • api/oss/src/core/secrets/dtos.py
  • api/oss/src/core/secrets/redaction.py
  • api/oss/src/core/secrets/services.py
  • api/oss/src/core/webhooks/service.py
  • api/oss/src/core/workflows/service.py
  • api/oss/src/dbs/postgres/secrets/dao.py
  • api/oss/src/dbs/postgres/secrets/mappings.py
  • api/oss/src/middlewares/auth.py
  • api/oss/src/utils/caching.py
  • api/oss/src/utils/env.py
  • api/oss/tests/pytest/unit/access/test_grant_exchange.py
  • api/oss/tests/pytest/unit/middlewares/test_auth_grants.py
  • api/oss/tests/pytest/unit/secrets/test_write_only.py
  • api/oss/tests/pytest/unit/utils/test_cache_key_tenancy.py
  • api/oss/tests/pytest/unit/vault/test_write_only_routes.py
  • api/oss/tests/pytest/unit/webhooks/test_write_only_outward.py
  • docs/design/write-only-secrets/README.md
  • sdks/python/agenta/sdk/agents/connections/__init__.py
  • sdks/python/agenta/sdk/agents/connections/credentials.py
  • sdks/python/agenta/sdk/agents/connections/errors.py
  • sdks/python/agenta/sdk/agents/platform/connections.py
  • sdks/python/agenta/sdk/agents/platform/secrets.py
  • sdks/python/agenta/sdk/middlewares/running/vault.py
  • sdks/python/oss/tests/pytest/unit/agents/connections/test_credentials_parity.py
  • sdks/python/oss/tests/pytest/unit/agents/platform/test_write_only_secrets.py

Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.

Comment thread api/oss/src/core/secrets/dtos.py Outdated
Comment thread api/oss/tests/pytest/unit/middlewares/test_auth_grants.py Outdated
Comment thread api/oss/tests/pytest/unit/utils/test_cache_key_tenancy.py Outdated
Comment thread api/oss/tests/pytest/unit/vault/test_write_only_routes.py Outdated
Comment thread docs/design/write-only-secrets/README.md Outdated
mmabrouk added a commit that referenced this pull request Aug 21, 2026
…call site

CodeRabbit review round on #6164:

- Webhook secret rotation and EE SSO provider updates built the parent SecretDTO for
  UpdateSecretDTO.secret, which pydantic rejects; both now build UpdateSecretPayloadDTO.
  A source walk pins every call site, and the webhook rotation path gets real coverage.
- The forged-token test asserts UnauthorizedException/401/invalid_token instead of any
  exception, and is renamed for what it forges (a foreign-signed token, not an unsigned one).
- The cache-key tenancy test no longer reads as if the truncated-id collision were an
  invariant: it records the tracked exception (issue #6166) that flipping the default needs
  invalidate_cache to carry the flag and a deploy plan for the evaluation lock keys.
- The vault route fake now models production invalidation faithfully: it reaches neither
  full-id namespace, so the stale-snapshot test passes on the generation, not on the fake.
- The design note names #6065 (frontend refactor, merged) and #6135 (stacking base)
  separately, and records the cache-tenancy gap.
mmabrouk added a commit that referenced this pull request Aug 21, 2026
The row is Agenta's from the moment it exists: `managed_by="starter-credits-bridge"` on
the create refuses user deletes and re-credentialing, and `write_only=True` keeps the proxy
virtual key out of every user-facing read. Setting both on CREATE (rather than tightening
afterwards) leaves no window in which the seeded connection is readable or removable.

Every write this component makes to its own row now passes `allow_managed=True`, the
in-process owner's key past the managed guard: the grant-record upsert and the finalize
that writes the minted key. There is no delete path in the bridge, so nothing else needs
the flag.

Also documents that the mint-policy cache key is deliberately global, unlike the per-org
feature-flag cache above it: the policy is one program-wide payload, identical for every
organization.

Stacked on managed-secrets (#6165) and write-only-secrets-api (#6164), which provide the
two flags.

@mmabrouk mmabrouk left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewer guide for this PR. The change is wide but has one spine: values are stripped at the response boundary, never below it, and exactly one kind of caller gets plaintext. The inline notes follow that spine in order - where redaction happens, what the value fields had to become to allow a value-less shape, when an omitted value means keep-the-stored-one, who is allowed to read plaintext, and the three places that could leak around the boundary (the list cache, the one-way flag under concurrency, and the webhook echo). Note the base: this is stacked on #6135 and must be re-targeted to main once that merges. The default is env-gated off, so merging changes nothing for anyone until the flag flips.

Comment thread api/oss/src/core/secrets/redaction.py Outdated
Comment thread sdks/python/agenta/sdk/agents/connections/credentials.py Outdated
Comment thread api/oss/src/core/secrets/dtos.py Outdated
Comment thread api/oss/src/core/secrets/services.py Outdated
Comment thread api/oss/src/core/secrets/services.py Outdated
Comment thread api/oss/src/apis/fastapi/access/router.py Outdated
Comment thread api/oss/src/apis/fastapi/vault/router.py Outdated
Comment thread api/oss/src/dbs/postgres/secrets/dao.py
Comment thread api/oss/src/core/webhooks/service.py Outdated
Comment thread sdks/python/agenta/sdk/agents/connections/errors.py
mmabrouk added a commit that referenced this pull request Aug 21, 2026
The row is Agenta's from the moment it exists: `managed_by="starter-credits-bridge"` on
the create refuses user deletes and re-credentialing, and `write_only=True` keeps the proxy
virtual key out of every user-facing read. Setting both on CREATE (rather than tightening
afterwards) leaves no window in which the seeded connection is readable or removable.

Every write this component makes to its own row now passes `allow_managed=True`, the
in-process owner's key past the managed guard: the grant-record upsert and the finalize
that writes the minted key. There is no delete path in the bridge, so nothing else needs
the flag.

Also documents that the mint-policy cache key is deliberately global, unlike the per-org
feature-flag cache above it: the policy is one program-wide payload, identical for every
organization.

Stacked on managed-secrets (#6165) and write-only-secrets-api (#6164), which provide the
two flags.
mmabrouk added a commit that referenced this pull request Aug 21, 2026
The row is Agenta's from the moment it exists: `managed_by="starter-credits-bridge"` on
the create refuses user deletes and re-credentialing, and `write_only=True` keeps the proxy
virtual key out of every user-facing read. Setting both on CREATE (rather than tightening
afterwards) leaves no window in which the seeded connection is readable or removable.

Every write this component makes to its own row now passes `allow_managed=True`, the
in-process owner's key past the managed guard: the grant-record upsert and the finalize
that writes the minted key. There is no delete path in the bridge, so nothing else needs
the flag.

Also documents that the mint-policy cache key is deliberately global, unlike the per-org
feature-flag cache above it: the policy is one program-wide payload, identical for every
organization.

Stacked on managed-secrets (#6165) and write-only-secrets-api (#6164), which provide the
two flags.
mmabrouk added a commit that referenced this pull request Aug 21, 2026
…call site

CodeRabbit review round on #6164:

- Webhook secret rotation and EE SSO provider updates built the parent SecretDTO for
  UpdateSecretDTO.secret, which pydantic rejects; both now build UpdateSecretPayloadDTO.
  A source walk pins every call site, and the webhook rotation path gets real coverage.
- The forged-token test asserts UnauthorizedException/401/invalid_token instead of any
  exception, and is renamed for what it forges (a foreign-signed token, not an unsigned one).
- The cache-key tenancy test no longer reads as if the truncated-id collision were an
  invariant: it records the tracked exception (issue #6166) that flipping the default needs
  invalidate_cache to carry the flag and a deploy plan for the evaluation lock keys.
- The vault route fake now models production invalidation faithfully: it reaches neither
  full-id namespace, so the stale-snapshot test passes on the generation, not on the fake.
- The design note names #6065 (frontend refactor, merged) and #6135 (stacking base)
  separately, and records the cache-tenancy gap.
@mmabrouk
mmabrouk force-pushed the write-only-secrets-api branch from 8bbd078 to 3ef279c Compare August 21, 2026 17:09
mmabrouk added a commit that referenced this pull request Aug 21, 2026
The row is Agenta's from the moment it exists: `managed_by="starter-credits-bridge"` on
the create refuses user deletes and re-credentialing, and `write_only=True` keeps the proxy
virtual key out of every user-facing read. Setting both on CREATE (rather than tightening
afterwards) leaves no window in which the seeded connection is readable or removable.

Every write this component makes to its own row now passes `allow_managed=True`, the
in-process owner's key past the managed guard: the grant-record upsert and the finalize
that writes the minted key. There is no delete path in the bridge, so nothing else needs
the flag.

Also documents that the mint-policy cache key is deliberately global, unlike the per-org
feature-flag cache above it: the policy is one program-wide payload, identical for every
organization.

Stacked on managed-secrets (#6165) and write-only-secrets-api (#6164), which provide the
two flags.
@mmabrouk
mmabrouk marked this pull request as ready for review August 21, 2026 19:54
@dosubot dosubot Bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Aug 21, 2026
The list is small and only the settings page reads it, and the runtime path already went straight to the database. Caching it bought little while costing a whole class of question — what a shared Redis entry holds, and whether a stale reader can repopulate it with plaintext after a tighten — which the generation counter and the full-project-id cache keys existed only to answer. The route now reads the database on every request and redacts at the response boundary, so what a caller sees is what the row says. The per-project sweep each secret write already fired is untouched: it predates write-only secrets and serves the other namespaces.
…ite-only secret

The error told the user to provide the provider key in the environment, but nothing read it: an agenta-mode connection resolved from the vault alone, so a standalone run against a write-only secret failed even with OPENAI_API_KEY exported. Resolution now reads the variable the harness itself would use for that connection — the provider family's key, or Bedrock's and Azure's own channels, so one service's credential is never sent to another — and raises only when that variable is empty too. The error text says what the resolver already tried.
…ve grant

The exchange attached the grant whenever a caller asked with action=run_service, and any member who may run a service can ask: they could call it with their own session or ApiKey, take the returned credential to the vault routes, and read every write-only value in plaintext. That is the whole guarantee, self-serve. The exchange now only carries forward a grant the caller already holds on a verified Secret token, and never creates one. Creation stays where a run actually starts, in-process at the invoke and inspect hops, so the credential still travels with the run: the workflow service and the runner both re-exchange the granted token they were handed, and refresh keeps working with no new credential to distribute and no deployment change.
Keep-on-omit filled an omitted credential from a snapshot the service read before the DAO took its row lock, so a rotation that committed in between was silently undone: the update wrote the older value back over the newer one, and nothing reported it. The DAO now resolves the carry-over inside the locked transaction, against the row as it actually stands — the shape GitDAO.commit_revision already uses for the same reason. The identity check travels with it, because deciding same-identity against a stale row is how a credential crosses identities. The fake DAOs call the resolver at the same point, so keep-on-omit stays exercised rather than skipped, and a new test rotates the stored row inside that window and pins that the value carried over is the rotated one.
… use

The environment fallback for a write-only connection read one variable per candidate, so a Bedrock connection could only be credentialed by AWS_BEARER_TOKEN_BEDROCK and a Vertex one not at all — even though the vault path accepts an AWS key pair or service-account material for exactly those connections. The fallback now offers the same channels the stored credential could have used, and a channel counts only when every variable in it is set: half an AWS key pair authenticates nothing, and passing it on would fail at the provider with a misleading error instead of here with an actionable one.
The SSO branch checked that client_secret was PRESENT, not that it held a value, so a create carrying an explicit null stored an SSO record with no credential — while the webhook and custom-secret branches beside it already checked the value. It now checks the value on the create path only: omission still means "keep the stored one" on update, which is what redacted responses and the edit form depend on.
The fixtures were rewritten to obviously-fake, digit-free strings so the scanner has nothing to find going forward. Four occurrences remain inside commits whose amend could not be replayed, and the scan reads history, not the tree. Each is a unit-test constant handed to a fake DAO; no real credential was ever involved. Fingerprints are anchored to their commit, so they need regenerating if either lane's history is rewritten again.
Redaction was applied inside the one helper every caller used, so the connection test and the edit path read a write-only provider's client secret as empty. A redacted read there does not hide a value from anyone — it tests the provider without its secret, then writes is_valid false and is_active false, taking a working provider out of the login screen. There are now two resolvers, the split the webhooks service already uses for signing secrets: the plain one stays plaintext for the internal callers, and an outward one shapes responses and drops the secret once the record is write-only. Both are documented by what the caller does with the value, not by where it is called from.
Keep-on-omit compared kind and provider family but not the stored format, so a text-to-json update that omitted its content carried the stored string into the json shape. The validators could not catch it: they run when the payload is built, before the carry-over fills the value in, so what they saw was a value-less shape and what reached the row was a json secret holding a string. A format change is now an identity change and requires an explicit value, and the merged payload is re-validated under the lock before it is persisted, so nothing is stored that a create of the same shape would have refused. Also moves the per-kind primary credential field out of the SDK classifier and onto the API side: it covers kinds the SDK never resolves (SSO providers, webhook signing secrets) and no SDK code reads it. The extras vocabulary stays shared, which is where drift would actually hurt.
…e, not to whoever asks

Closing the self-serve grant broke every agent run, and the reason is the shape of the product's path: the playground and the gate post straight to the workflow service with the user's own ApiKey, so the service exchanges THAT credential and the API's in-process mint never runs. Carry-forward alone therefore had nothing to carry, and runs got the redacted shape. The exchange now mints the grant when the caller proves it is the platform runtime and otherwise carries forward what the caller already holds. The proof is a secret only the backend has, sent on the internal hop and compared in constant time, because that route is publicly reachable and the user's token cannot say who is asking. It resolves from AGENTA_SERVICES_INTERNAL_KEY, falling back to AGENTA_AUTH_KEY, which the services container already receives through the same env file as the API — so deployments keep working unchanged, and a dedicated value narrows what one leaked secret can do. The runner keeps carry-forward and is given nothing new; the key never reaches it or a sandbox, and is never logged.
…untime

The runtime key falls back to AGENTA_AUTH_KEY, whose unconfigured value is the string 'replace-me' committed in every example env file — so a deployment that changed neither would have handed a run credential to anyone who sent it. Both sides now treat the placeholder as no key at all: such a deployment issues no grant, which costs it only the ability to run against write-only secrets (off by default) and never gives that ability to a stranger. The variable is documented in the example env files, and the design note now says who may hold the grant and why the exchange cannot decide it from the requested action.
…er's key

The bug that broke every run was invisible to unit tests because they exercised the hop that already had a granted token, while the product uses the hop that never did. This drives the real agent app with the exchange stubbed and asserts the request carries both the caller's own credential and the platform's runtime key — and that a service configured without one sends no header at all rather than an empty value.
…aming the provider key

The placeholder this refuses is the shipped default in every example env file, so a deployment that never set AGENTA_AUTH_KEY silently loses runs against write-only connections — and what it sees is the SDK telling it to provide OPENAI_API_KEY, which is true for a standalone run and useless here. The service now says it once, at the point of use, and names the variable to set. Live QA hit exactly this and read it as a regression, which is the cost of a failure that points somewhere else.
The fixtures were rewritten to digit-free names, but these two spellings survive in commits whose amend could not be replayed, and both scans read history rather than the tree. A fingerprint names the commit its finding was seen in, so it goes stale every time a lane below is rebased — which happened twice while landing this stack. Exempting the values is stable, and it follows what this file already says: exempt the VALUE, never the path, so a real key added to a fixture would still be seen.
A deployment that turned write-only on, or enabled a component that seeds write-only rows, cannot read those secrets at all without a platform runtime key — and the failure it gets says to provide a provider key, which is right for a standalone run and useless here. The API now says it once at boot, naming the variable and the consequence, next to the other startup validations. The placeholder counts as unset, since it is what the example env files ship.
The warning read env.starter_credits_bridge directly, but that config is an EE addition that this branch does not carry — so on a build without it the API would have raised AttributeError during startup validation, before serving anything. It now asks for the attribute rather than assuming it, and the case that needs the bridge is tested where the bridge exists. My own CI caught it because the test referenced the same missing attribute; the production path had the same bug.
@mmabrouk
mmabrouk force-pushed the write-only-secrets-api branch from 5f3601b to 52d3ca5 Compare August 23, 2026 14:08
@mmabrouk
mmabrouk merged commit 1a4e9f3 into release/v0.114.0 Aug 23, 2026
47 of 48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend documentation Improvements or additions to documentation enhancement New feature or request lgtm This PR has been approved by a maintainer python Pull requests that update Python code size:XXL This PR changes 1000+ lines, ignoring generated files. tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant