feat(vault): encrypt OAuth secrets with a safe rollout contract

[alexminza]

Closes #408.

OAuth tokens and client secrets currently remain readable in copied vaults and token caches. This adds optional per-value JWE encryption with MCPORTER_VAULT_PASSWORD, covers both persistence backends, migrates plaintext on write, and keeps existing plaintext defaults. The required policy, diagnostics, generated-CLI policy propagation, and child-process password filtering are included.

Maintainer decision: accept the bounded opt-in feature with a coordinated-upgrade requirement and an isolated downgrade procedure. Every CLI, daemon, and bundled generated CLI sharing the state must be upgraded and restarted before opt-in. Already-released binaries ignore encryption and can silently restore plaintext on re-authentication. A new top-level vault version would be worse because released readers repair unknown versions by replacing the vault. The prominent warning and rollback instructions in docs/config.md state this limitation explicitly.

Supporting versions now persist a versioned encryption marker in the vault and tokenCacheDir, retain it after the last credential is cleared, and refuse missing passwords or unknown marker versions without rewriting the store. Rollback uses separate HOME/XDG directories and separate explicit token caches; it does not decrypt the original store in place. The package test exercises this against the published 0.15.0 package, including the unsafe shared-store downgrade and re-sealing recovery.

Validation on an isolated Linux worker: pnpm check; 181 focused tests across persistence, refresh locking/recovery, vault commands/validation, diagnostics, and generated CLIs; pnpm test:package, including installed-candidate versus published 0.15.0 migration and downgrade checks. The four initial marker regressions were reproduced failing on the original contributor head and passing on the completed implementation. Independent Codex autoreview is clean for P0–P2 findings. The first broad test run identified two test-fixture assumptions, both corrected and retested; exact-head CI passed across Ubuntu/Node 24, Ubuntu/Node 26, macOS, and Windows. Post-merge CI and the no-publish source and native release rehearsal also passed at merge commit 90bb83f934cb68f5faf463461388135a20597293.

Thanks @alexminza for the implementation and @stenh0use for the original scanner-based validation and field-coverage feedback. No release or version bump.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: blocked before merge.

What this changes

This PR adds optional password-based encryption for persisted OAuth secrets, migration on write, diagnostics, and child-process password isolation.

Example: Run mcporter call demo.ping with an expired cached token and a generated 44-character MCPORTER_VAULT_PASSWORD.

• Before: The refreshed access token, refresh token, and ID token remain plaintext in credentials.json.
• After: The call returns pong and stores those secret values as authenticated JWE strings.

Review scores

Measure	Result	What it means
Overall readiness	🦐 gold shrimp (3/6)	Strong behavioral evidence and no actionable code finding; readiness is capped by the unresolved owner decision on the new feature and rollout contract.
Proof confidence	🦞 diamond lobster (5/6)	Sufficient (terminal): The attached built-CLI transcript exercises the vault and directory stores against a local OAuth and MCP server, showing encrypted writes, successful decrypted refresh and calls, unchanged files on password refusal, legacy migration, older-writer recovery, and absence of inherited vault variables in direct and daemon-managed child processes; the reporter also independently scanned real output.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Product

Kind: Feature · Worth it: Needs a maintainer decision · Fix scope: Complete
User problem: Backups, snapshots, and synced copies of OAuth storage contain readable bearer credentials and trigger secret scanners.
Reason: The user value is concrete and independently checked, but this adds encryption settings, a declared dependency, and an operational compatibility contract without recorded maintainer approval.

Merge readiness

⛔ Blocked before merge - 1 item remains

Keep this PR open: it provides useful encryption absent from main, with no concrete code defect found, but the new security configuration and coordinated-upgrade contract still need an area-owner decision.

Priority: P2
Reviewed head: 0c7e690a28402da345d4796e3e4ae786af207360
Owner decision: Required. See Decision needed.

Decision needed

• Question: Should MCPorter adopt this optional password-based encryption API and require every shared-state consumer to be upgraded and restarted before encryption is enabled?
• Recommendation: Approve the documented opt-in rollout: Accept per-value encryption, optional defaults, the required policy, and coordinated upgrades before setting the password; older writers can otherwise restore plaintext.
• Why: The implementation and proof establish the tradeoff, but neither the reporter's endorsement nor gogcli's design authorizes this repository's new configuration and compatibility contract.

Before merge

• Resolve maintainer decision - Resolve the maintainer decision shown above before merge.

Findings

None.

Agent review details

How this fits together

OAuth persistence connects CLI and runtime authentication to the shared vault and configured token-cache directories; it accepts provider credentials and returns usable credentials while encrypting their stored secret fields.

flowchart TD
  A[CLI and generated clients] --> B[OAuth runtime]
  B --> C[Persistence stores]
  D[Password and encryption policy] --> E[JWE codec]
  C --> E
  E --> F[Vault and token cache files]
  B --> G[OAuth and MCP servers]

Loading

Technical review

Best possible solution:

Approve the bounded opt-in encryption design and documented coordinated rollout, or choose storage isolation before landing.

Do we have a high-confidence way to reproduce the issue?

This is a feature request rather than a broken encryption contract; current main visibly persists plaintext JSON, and the reporter's scanner run confirms the practical concern.

Is this the best way to solve the issue?

A shared codec covering both credential stores is coherent and avoids whole-file corruption handling, but the permanent configuration and mixed-version contract require owner approval.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against e3fa5352d05f.

Provenance checked

• Shared JSON vault and corrupt-file recovery changes intended behavior with a stated reason (e1bdcda: Credential centralization established the shared vault; later hardening preserved atomic writes and repair of malformed JSON. Historical tree reads were partly unavailable, supplemented by REST commit metadata and current-main source.)
• Composite persistence fallback changes intended behavior with a stated reason (fix(oauth): preserve primary cache fallback #228: Keep a usable explicit token cache available when a lower-priority vault fails; this PR narrowly makes encryption refusals fatal to prevent bypassing the selected protection policy.)
• Refresh serialization and generation-aware recovery keeps the original intent (fix(oauth): serialize the refresh transaction across processes #306: Prevent concurrent redemption of rotating refresh tokens and preserve recovery generations; encryption remains inside the existing persistence and locking owners.)
• Daemon child environment and connection identity changes intended behavior with a stated reason (fix(daemon): share one user daemon across configurations #346: Resolve connection-scoped environments and identities for the user broker; the PR explains removing inherited vault secrets while preserving explicit child environment entries.)

Testing

Proof path: shipped entry point.

Security

None.

Evidence

What I checked:

• Applicable repository guidance: Read the complete root AGENTS.md and VISION.md; no nested AGENTS.md or maintainer-notes directory was present. Guidance requires discussion before new features, dependencies, or broad configuration changes; no maintainer approval was found in the full PR discussion, reviews, or linked issue. (AGENTS.md:8, 0c7e690a2840)
• Distinct capability absent from main: Main writes the vault as JSON without encryption; the pinned introduced diff adds authenticated per-value encryption and covers both the vault and DirectoryPersistence. Canonical search found the linked request Option to encrypt the OAuth vault at rest #408 and this PR, with no competing implementation. (src/oauth-vault.ts:116, 0c7e690a2840)
• Independent user validation and remaining owner choice: stenh0use built the branch and scanned actual output with Kingfisher, endorsed the approach, and identified exposed id_token and registration_access_token fields; the current field lists cover both. Their comment explicitly leaves compatibility approval to steipete: feat(vault): encrypt OAuth secrets with a safe rollout contract #425 (comment). The reporter's endorsement is not maintainer approval. (src/oauth-vault-encryption.ts:23, 0c7e690a2840)
• Shipped-entry-point proof: Read the complete attached transcript at https://github.com/user-attachments/files/33526566/2026-10-06-mcporter-vault-jwe-encryption-pr-proof.txt. Built CLI calls exercise encrypted persistence, refresh after provider rejection, missing-password refusal with unchanged hashes, mixed-version writes, directory migration, and direct and daemon-managed child environment isolation.
• Released storage compatibility: The transcript uses released v0.14.2; its vault and directory-persistence blobs are identical to current main's blobs, which also underlie v0.15.0. This independently connects the exercised persisted-state boundary to the current release without claiming a new v0.15.0 runtime run. Older writers restoring plaintext is demonstrated and documented. (docs/config.md:309, e3fa5352d05f)
• Feature history and provenance: History traces vault centralization to e1bdcda, modularization to refactor: dedupe and modularize runtime, OAuth, and CLI internals #249, ordered fallback to fix(oauth): preserve primary cache fallback #228, refresh locking to fix(oauth): serialize the refresh transaction across processes #306, and daemon ownership to fix(daemon): share one user daemon across configurations #346. Some historical tree reads and the earlier-head comparison encountered unavailable promisor objects; current introduced hunks and main blobs were readable. (src/oauth-vault.ts:95, e3fa5352d05f)

Likely related people:

• Peter Steinberger: Raw commit 95edea1 adds src/daemon/connection-identity.ts:33 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 95edea12d715; files: src/daemon/connection-identity.ts)
• Sebastian B Otaegui: Raw commit 304ff60 adds src/oauth-token-refresh.ts:306 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 304ff600e068; files: src/oauth-token-refresh.ts)

Labels

Label changes:

No label changes.

Label justifications:

• P2: Protecting copied OAuth credentials is valuable, but this optional feature does not establish an urgent exploit or broken existing workflow.
• merge-risk: 🚨 compatibility: After encryption is enabled, released readers cannot use sealed values and older writers can restore plaintext; accepting the coordinated-upgrade contract remains an owner decision.
• rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
• proof: sufficient: Contributor real behavior proof is sufficient.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

History

Review history (12 earlier review cycles; latest 8 shown)

• reviewed 2026-10-08T04:55:55.419Z sha c6ed6eb :: needs real behavior proof before merge. :: [P1] Strip the vault password from regular stdio child launches
• reviewed 2026-10-08T05:14:55.350Z sha 083887d :: needs real behavior proof before merge. :: [P1] Strip inherited vault secrets from broker-owned child launches
• reviewed 2026-10-08T05:28:52.492Z sha 69e4bd5 :: blocked before merge. :: none
• reviewed 2026-10-08T06:07:01.037Z sha 632426c :: blocked before merge. :: none
• reviewed 2026-10-08T06:18:45.452Z sha 632426c :: blocked before merge. :: none
• reviewed 2026-10-15T14:56:20.471Z sha 5ea891b :: blocked before merge. :: none
• reviewed 2026-10-15T15:19:54.974Z sha ddec532 :: blocked before merge. :: none
• reviewed 2026-10-15T16:30:39.990Z sha ddec532 :: blocked before merge. :: none

Reviewed October 16, 2026, 1:48 AM ET / 05:48 UTC (Revision 13).

[alexminza]

Thanks for the review. All findings are addressed in 99ff6ef ("clawsweeper review": src/oauth-persistence-stores.ts, tests/oauth-persistence.test.ts, docs/config.md, CHANGELOG.md). Details per item, then the real-behavior proof.

[P2] Propagate vault encryption refusals through cache fallback — fixed.

• DirectoryPersistence.reconcileServerUrl() now calls readVaultEncryptionSettings() first, and every public method goes through it, so a required policy refuses a plaintext tokenCacheDir exactly as it refuses the vault.
• CompositePersistence.readRecoveryValues() rethrows a rejected VaultEncryptionError from any store before the ordered fallback, so a readable plaintext directory can no longer hide the vault's missing-password error on the readTokens() → applyCachedAuthIfAvailable() path.
• Tests (tests/oauth-persistence.test.ts, vault encryption at rest): refuses a plaintext tokenCacheDir under required without a password, refuses every tokenCacheDir operation under a config required policy without a password, does not let a plaintext tokenCacheDir hide a sealed vault without a password.

[P2] Migrate existing secrets in every token-cache file — fixed.

• Every DirectoryPersistence save now also seals the other secret files that are still plaintext (tokens.json, client.json, code_verifier.txt, state.txt), under the same tokens.json lock that clearRejectedCredentials uses; already sealed files are left byte-identical. The first write with a password seals the whole directory, matching the vault, where the first write seals the whole document. A store that is only read is sealed on its next write, as documented.
• Test: seals every plaintext secret file in a tokenCacheDir on the first write with a password.

[P1 merge risk] Legacy files deleted without encryption — fixed with your recommended option.

• The legacy ~/.mcporter/<server>/ cache is still copied into the vault on first use, as on main; its secret files are removed after the copy only when MCPORTER_VAULT_PASSWORD is set. Without a password, behavior is unchanged from main.
• Interruption: the vault write completes before the removal; a cache that survives a crash is copied again on the next start, which is what main does on every start today. Concurrent starts can both copy; the worst case is re-saving the same values.
• Tests: moves a legacy per-server cache into the vault and removes it when a password is set, keeps the legacy per-server cache without a password, as before.

[P1] Prevent older writers from restoring plaintext — addressed as an explicit upgrade contract, not storage isolation.

• Storage isolation (a new file or location) is what the reproduction in the PR body shows wiping the vault under the released reader, so the PR keeps the file format and states the contract instead: upgrade and restart every install that shares the state directory before setting the password; a pre-encryption writer that re-authenticates writes that server's values in plaintext; the next write by a current version re-seals them; mcporter config doctor lists plaintext values until then; required is enforced only by versions that know it. See docs/config.md § Vault Encryption, "Mixed versions".
• Step 6 of the proof below runs this round trip with the released mcporter@0.14.2 as the older writer.

Also fixed after an independent re-check of the above

• DirectoryPersistence.clear() unlinked outside the sealing lock, so a concurrent first-sealing save could write a cleared file back; it now clears under that lock (test: clears a tokenCacheDir under the lock the sealing writes hold).
• clearLegacyOAuthArtifacts() now passes the config-level policy to its directory store (test: refuses to clear legacy artifacts under a config required policy without a password).

Validation: pnpm check clean; pnpm test 2127 passed, 27 skipped; pnpm test:package passes. Rebase onto main is next.

Real behavior proof

The built branch CLI and the released mcporter@0.14.2 from npm share one state directory and talk to a local OAuth authorization server plus Streamable HTTP MCP server (refresh-token grant, bearer-checked /mcp, a /revoke endpoint that invalidates access tokens so the next request gets 401). Secret values are classified by a small inspector and never printed; synthetic tokens only. Condensed transcript below; the full redacted transcript is attached.

Transcript (8 steps)

branch CLI: 0.14.2   released CLI: 0.14.2   server: http://127.0.0.1:52670

### 1. No password: legacy ~/.mcporter/<server>/ cache is used and kept, vault stays plaintext
$ mcporter(branch) call legacy-plain.ping
    pong  [exit 0]
    [server] POST /token -> 200 (refresh ok, issued access token #2)
    [server] POST /mcp -> 200 (bearer at-2 accepted, tools/call)
legacy dir: tokens.json: {"access_token":"<PLAINTEXT>","refresh_token":"<PLAINTEXT>",...}   (kept)
vault:      legacy-plain: {"tokens":{"refresh_token":"<PLAINTEXT>","access_token":"<PLAINTEXT>",...}}

### 2. Password set: vault set writes sealed values
$ MCPORTER_VAULT_PASSWORD=*** mcporter(branch) vault set demo --tokens-file <state>/seed.json
    Saved OAuth credentials for 'demo' to <state>/data/mcporter/credentials.json  [exit 0]
vault: demo: {"tokens":{"access_token":"<JWE alg=PBES2-HS512+A256KW enc=A256GCM p2c=8192 kid=demo|61dbd38746c5b742/tokens/access_token>", "refresh_token":"<JWE ... kid=demo|61dbd38746c5b742/tokens/refresh_token>","token_type":"Bearer","expires_at":1}}
       legacy-plain: sealed too (first write with the password seals the whole vault)

### 3. Password set: call refreshes the expired token (sealed read, sealed write) and uses it
$ MCPORTER_VAULT_PASSWORD=*** mcporter(branch) call demo.ping
    pong  [exit 0]
    [server] POST /token -> 200 (refresh ok, issued access token #3)
    [server] POST /mcp -> 200 (bearer at-3 accepted, tools/call)
vault: demo: new ciphertexts, same kids, "expires_in":3600, "issuer": plain
$ MCPORTER_VAULT_PASSWORD=*** mcporter(branch) config doctor
    OAuth vault: <state>/data/mcporter/credentials.json (2 entries, 4 sealed values, 0 plaintext values)
    Vault encryption: policy optional, MCPORTER_VAULT_PASSWORD set

### 4. Password unset with sealed values: refused, file untouched
$ mcporter(branch) call demo.ping
    VaultEncryptionError: The OAuth vault at <state>/data/mcporter/credentials.json holds encrypted values for 'legacy-plain'; set MCPORTER_VAULT_PASSWORD to use it.  { code: 'password_missing' }  [exit 1]
    vault sha256 before=da0787c27d526600 after=da0787c27d526600

### 5. required policy: refused without the password, works with it
$ MCPORTER_VAULT_ENCRYPTION=required mcporter(branch) call demo.ping
    VaultEncryptionError: MCPORTER_VAULT_ENCRYPTION=required but MCPORTER_VAULT_PASSWORD is not set; the OAuth vault cannot be used.  [exit 1]
$ curl -X POST <origin>/revoke   # provider revokes the access tokens
$ MCPORTER_VAULT_PASSWORD=*** MCPORTER_VAULT_ENCRYPTION=required mcporter(branch) call demo.ping
    pong  [exit 0]
    [server] POST /mcp -> 401 (bearer not recognized)
    [server] POST /token -> 200 (refresh ok, issued access token #4)
    [server] POST /mcp -> 200 (bearer at-4 accepted, tools/call)

### 6. Older writer: released 0.14.2 overwrites the demo entry with plaintext; doctor reports it; the next branch write re-seals it
$ mcporter(0.14.2) vault set demo --tokens-file <state>/seed-old.json
    Saved OAuth credentials for 'demo' to <state>/data/mcporter/credentials.json  [exit 0]
vault: demo: {"tokens":{"access_token":"<PLAINTEXT>","refresh_token":"<PLAINTEXT>",...}}   legacy-plain: still sealed
$ MCPORTER_VAULT_PASSWORD=*** mcporter(branch) config doctor
    OAuth vault: <state>/data/mcporter/credentials.json (2 entries, 2 sealed values, 2 plaintext values)
      - The OAuth vault holds 2 plaintext secret values; they are sealed on the next vault write.
$ MCPORTER_VAULT_PASSWORD=*** mcporter(branch) call demo.ping
    pong  [exit 0]
    [server] POST /token -> 200 (refresh ok, issued access token #5)
    [server] POST /mcp -> 200 (bearer at-5 accepted, tools/call)
vault: demo: sealed again (JWE, kid=demo|61dbd38746c5b742/tokens/...)

### 7. tokenCacheDir written in plaintext by 0.14.2 (vault set + call), plus plaintext client/verifier/state files as an older login leaves them; the first branch write seals every file
$ mcporter(0.14.2) vault set cached --tokens-file <state>/seed-cache.json   [exit 0]
$ mcporter(0.14.2) call cached.ping
    pong  [exit 0]
    [server] POST /token -> 200 (refresh ok, issued access token #6)
    [server] POST /mcp -> 200 (bearer at-6 accepted, tools/call)
tokenCacheDir before:
    client.json: {"client_id":"proof-client","client_secret":"<PLAINTEXT>","token_endpoint_auth_method":"none"}
    code_verifier.txt: <PLAINTEXT>
    state.txt: <PLAINTEXT>
    tokens.json: {"refresh_token":"<PLAINTEXT>","access_token":"<PLAINTEXT>","token_type":"Bearer","expires_in":3600,...}
$ curl -X POST <origin>/revoke
$ MCPORTER_VAULT_PASSWORD=*** mcporter(branch) call cached.ping
    pong  [exit 0]
    [server] POST /mcp -> 401 (bearer not recognized)
    [server] POST /token -> 200 (refresh ok, issued access token #7)
    [server] POST /mcp -> 200 (bearer at-7 accepted, tools/call)
tokenCacheDir after:
    client.json: {"client_id":"proof-client","client_secret":"<JWE ... kid=client.json/client_secret>","token_endpoint_auth_method":"none",...}
    code_verifier.txt: <JWE ... kid=code_verifier.txt>
    state.txt: <JWE ... kid=state.txt>
    tokens.json: {"refresh_token":"<JWE ... kid=tokens.json/refresh_token>","access_token":"<JWE ... kid=tokens.json/access_token>","token_type":"Bearer","expires_in":3600,...}
    discovery.json, authorization_server_url.txt, resource_url.txt, server_url.txt: plain (not secrets)
vault: cached: sealed

### 8. Password set: legacy ~/.mcporter/<server>/ cache is moved into the vault (sealed) and its secret files removed
$ MCPORTER_VAULT_PASSWORD=*** mcporter(branch) call legacy.ping
    pong  [exit 0]
    [server] POST /token -> 200 (refresh ok, issued access token #8)
    [server] POST /mcp -> 200 (bearer at-8 accepted, tools/call)
    legacy dir contents: server_url.txt          (tokens.json and client.json removed; the URL marker stays by design)
vault: legacy: sealed
$ MCPORTER_VAULT_PASSWORD=*** mcporter(branch) config doctor
    OAuth vault: <state>/data/mcporter/credentials.json (4 entries, 9 sealed values, 0 plaintext values)
    Vault encryption: policy optional, MCPORTER_VAULT_PASSWORD set

Step 1 shows two refreshes for one call: main copies a kept legacy cache into the vault on every start, so the stale legacy token overwrote the fresh one once; the branch behaves the same without a password and removes the cache with one, which ends that.

2026-10-06-mcporter-vault-jwe-encryption-pr-proof.txt

@clawsweeper re-review

[clawsweeper]

🦞👀
Exact review queued.

Re-review progress:

• State: Complete
• Detail: The durable review result and its route handoff completed.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/37673561996
• Updated: 2026-10-07T19:19:24.201Z

[alexminza]

Revision 2 and 3 items are addressed: directory writers in 8e616af ("clawsweeper review 2"), the generator in c45fb62 ("generate-cli").

• [P1] Reject passwordless directory writes before replacing sealed values — fixed (8e616af). Every DirectoryPersistence write and delete now goes through one locked path that first checks the directory's own sealed state: with sealed values present and no password set it refuses with password_missing and leaves every file untouched, exactly as the vault does. The guard also covers the server-URL-change invalidation in reconcileServerUrl(). Tests: refuses every write to a sealed tokenCacheDir without a password and leaves it untouched (direct and through CompositePersistence), refuses to invalidate a sealed tokenCacheDir on a server URL change without a password. Proof step 9 (transcript in the PR body) runs call and config logout against a sealed directory with the password unset: both refused, directory sha256 unchanged.
• [P2] Include metadata saves in the first-write encryption sweep — fixed (8e616af). saveDiscoveryState, saveAuthorizationServerUrl and saveResourceUrl use the same locked write path, so any first write with a password seals the whole directory. Test: seals a plaintext tokenCacheDir on a metadata save with a password.
• [P1] Preserve required encryption when generating from a config file — fixed (c45fb62). The generator now applies a config file's top-level oauthVaultEncryption to the embedded definition as loadServerDefinitions does; normalizeDefinition validates and keeps the field (snake-case alias too, so regeneration from the serialized definition round-trips); the shared-field list behind the artifact metadata includes it; an invalid value is rejected instead of weakened. Tests: carries a config-file oauthVaultEncryption policy into generated definitions, rejects an invalid oauthVaultEncryption instead of silently weakening policy, accepts the snake-case oauthVaultEncryption alias in inline generated definitions; the generated-CLI integration test asserts "oauthVaultEncryption": "required" in the artifact and its metadata. Documented: generating under required needs the password, since tool discovery goes through the vault.
• PR body aligned with the documented contract: design goal 3 now states the upgrade contract (upgrade and restart every consumer before setting the password; a pre-encryption writer that re-authenticates leaves plaintext until the next write by a current version; doctor reports it; required is enforced only by versions that know it) instead of "keep working". The maintainer decision on that contract is yours; the PR carries the evidence for either option.

Validation: pnpm check clean; pnpm test 2133 passed, 27 skipped; pnpm test:package passes.

@clawsweeper re-review

[clawsweeper]

🦞👀
Exact review queued.

Re-review progress:

• State: Complete
• Detail: A newer review tuple already exists; this stale result was superseded.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/37678752117
• Updated: 2026-10-07T20:00:38.871Z

[alexminza]

Revision 4's finding is addressed in c6ed6eb:

• [P2] Handle a JSON null vault before reading its entries — fixed. config doctor now applies the vault reader's shape check (version 1 or 2 and an object entries) before reading the document; null, arrays, strings or a non-object entries are reported as (unreadable) with an issue line, and the command continues. A SyntaxError still reports "not valid JSON". Test: reports a vault that is valid JSON but not a vault document as unreadable.

The remaining items are not code: the maintainer decision on the coordinated-upgrade contract (goal 3 of the PR body states it; the review recommends accepting it) and the rebase onto main.

@clawsweeper re-review

[clawsweeper]

🦞👀
Exact review queued.

Re-review progress:

• State: Complete
• Detail: The durable review result and its route handoff completed.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/37729943775
• Updated: 2026-10-08T04:57:22.973Z

[alexminza]

@steipete this is ready for your look. ClawSweeper has no contributor-facing finding left after eight rounds, the branch is merged with main and mergeable, and the description carries the design, the gogcli comparison and an eleven-step transcript against the released 0.14.2.

The one item still open is a decision rather than code: the coordinated-upgrade contract in design goal 3. Encryption is in place in the existing credentials.json, so a pre-encryption binary, daemon or bundled generated CLI that re-authenticates writes that server's values in plaintext until the next write by a current version re-seals them. The contract is therefore: set MCPORTER_VAULT_PASSWORD only after every consumer sharing the state directory is upgraded and restarted; required binds only versions that know it; config doctor reports any plaintext residue. The reviewer's alternative, a separate encrypted store, is what makes the released reader wipe the vault (reproduced in the description), and it would not stop older writers either.

If the contract is acceptable, a short confirmation here is all the review asks for. If you would rather have the residue window closed as well, I can add sealing on the next read with the password set as a follow-up. Anything else you want changed, say so and I will adjust.

[alexminza]

Synchronized with main at 627965f (the 0.14.3 release) in merge commit 5ea891b. The only conflict was CHANGELOG.md: 0.14.3 shipped on 2026-10-11 without this change, so the entry moved to the new ## [0.14.4] - Unreleased section; everything else from #426–#439 merged cleanly, and the auth-helper, stdio and daemon environment filtering from the earlier rounds is intact on the merged tree. On the merged branch: pnpm check clean, pnpm test 2248 passed / 27 skipped, pnpm test:package passes. The description's changelog link and test counts are updated.

The open item is unchanged: the coordinated-upgrade contract in design goal 3 needs a maintainer decision.

@clawsweeper re-review

[clawsweeper]

🦞👀
Exact review queued.

Re-review progress:

• State: Complete
• Detail: A newer review tuple already exists; this stale result was superseded.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/38758509211
• Updated: 2026-10-15T15:14:58.954Z

[alexminza]

Rank-up move applied in ddec532: the helper-only childEnvWithoutVaultSecrets unit test is removed; the transport and daemon tests plus proof steps 10 and 11 cover that boundary through real process launches. pnpm check clean, pnpm test 2247 passed / 27 skipped.

@clawsweeper re-review

[clawsweeper]

🦞👀
Exact review queued.

Re-review progress:

• State: Failed
• Detail: Durable publication exhausted its retry budget and was retained for operator dead-letter recovery.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/38769695970
• Updated: 2026-10-15T16:31:42.944Z

[alexminza]

On priority: the review rates this P2 as "a useful optional feature without evidence of an urgent exploit or broken existing workflow". The description now has a "Why this matters now" section with the evidence. In short: credentials.json holds long-lived refresh tokens and client secrets for third-party services; it sits in the home directory, so backups, synced homes, container images, volume snapshots and CI caches copy it, and the 0600 mode does not survive a copy; any deployment that replicates its state directory today either ships live tokens in the copy or drops OAuth persistence; and gogcli in this org already refuses to store the same secrets in plaintext at all. No attacker ingenuity is involved, only a backup or a sync existing.

@clawsweeper re-review

[clawsweeper]

🦞👀
Exact review queued.

Re-review progress:

• State: Complete
• Detail: A newer review tuple already exists; this stale result was superseded.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/38762614601
• Updated: 2026-10-15T15:42:13.623Z

[alexminza]

On the compatibility label: nothing changes at merge or on upgrade. Without MCPORTER_VAULT_PASSWORD the branch is byte-for-byte today's behavior, including for older binaries, daemons and generated CLIs sharing the state directory (proof steps 1, 6 and 7). The contract applies only once an operator sets the passphrase, which the docs tie to upgrading and restarting every consumer first, and a consumer that missed that fails loudly: 401, re-authentication, plaintext residue reported by config doctor. There is no silent failure path. That is a documented rollout precondition, the same one gogcli's keyring move had, rather than a merge-time risk. The description's TL;DR and "Why this matters now" now state this and the priority evidence.

@clawsweeper re-review

[clawsweeper]

🦞👀
Exact review queued.

Re-review progress:

• State: Review in progress
• Detail: The exact-review reservation is active; Codex is reviewing the item.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/38769011730
• Updated: 2026-10-15T16:27:19.807Z

[stenh0use]

Thanks @alexminza, this covers what I was after. Both departures make sense to me. Per-value sealing is the right call, since older readers treat a non-JSON vault as corrupt and wipe it, and a 401 with re-auth is a much better failure. JOSE over raw AES-GCM is fine too, since the cipher is the same and jose is now a declared dependency.

I built the branch and scanned real vault and tokenCacheDir output with Kingfisher 2.13.0 (--no-validate --no-dedup). Sealing works for the fields it covers: the JWT access token, ghr_ refresh token and gho_ token all stop matching. One gap is left, plus three smaller points.

Needed before merge

id_token isn't sealed, and betterleaks.jwt still flags it in credentials.json and tokencache/<server>/tokens.json. Scanner hits were the original trigger for this issue, so I think this one has to go in:

• add ['tokens','id_token'] to VAULT_SECRET_FIELDS (src/oauth-vault-encryption.ts:18-24)
• add 'id_token' to TOKEN_SECRET_FIELDS (src/oauth-persistence-stores.ts:61)
• add an id_token value to the fixture in tests/oauth-vault-encryption.test.ts:153
• add id_token to the sealed list in docs/config.md:299

Worth doing in the same PR

• clientInfo.registration_access_token is an RFC 7592 bearer secret and is stored as plaintext. Kingfisher doesn't flag it, but it should be sealed with the others: add it to VAULT_SECRET_FIELDS and to CLIENT_SECRET_FIELDS (src/oauth-persistence-stores.ts:62).
• Minor: with 8192 PBKDF2 iterations the vault is only as strong as the password's randomness. That's fine with the openssl rand -base64 32 the docs suggest, but weak for a memorable 16-character password. Could docs/config.md say plainly that it must be random? Rather than complexity rules, raising the minimum from 16 to 32 characters would nudge people toward a generated value, and nobody relies on 16 yet. The reader already accepts up to 100k iterations (src/oauth-vault-encryption.ts:15), so the count can be raised later without a format change.
• Minor: config doctor counts plaintext values in the vault but doesn't look at tokenCacheDir, so plaintext an older binary writes there goes unreported.

With id_token sealed, the approach looks good to me. Over to @steipete for the compatibility decision.

[alexminza]

@stenh0use thanks for the build-and-scan pass, that is exactly the check this needed.

• id_token: sealed in 8099035, in the vault and in tokenCacheDir (VAULT_SECRET_FIELDS, TOKEN_SECRET_FIELDS, the field-list fixture, docs/config.md, changelog). The proof provider now returns an id_token, so the transcript in the description shows it sealed in credentials.json and tokens.json; config doctor counts it with the others.
• registration_access_token: sealed in the same commit (VAULT_SECRET_FIELDS, CLIENT_SECRET_FIELDS). For the record, it only reaches the vault through vault set with a full registration response; the SDK's own registration flow strips unknown keys, and mcporter never reads it back. Still a bearer secret, so it is sealed rather than left as the one credential in clear.
• Password floor and random-passphrase wording: done as you suggested. The minimum is 32 characters (was 16), the error message and docs/config.md say the value must be generated rather than chosen, with openssl rand -base64 32 as the documented way; no complexity rules. Iterations stay at 8192 to match gogcli; the reader's 100 000 ceiling leaves room to raise them later.
• doctor and tokenCacheDir plaintext: done. config doctor now prints one line per configured tokenCacheDir with sealed and plaintext counts, without opening anything, and raises the same two issues as for the vault (sealed values without the password; plaintext values with it). Test: reports sealed and plaintext values in each configured tokenCacheDir.

Tests: names string fields of VaultEntry and counts them asserts both fields are in the list; seals the secret values and nothing else on write and seals a tokenCacheDir the same way carry both values. The branch is also synchronized with main at 0.15.0. pnpm check clean, pnpm test 2264 passed / 27 skipped, pnpm test:package passes on the merged tree.

@steipete with @stenh0use's sign-off above, the only item ClawSweeper still lists is your one-line OK on the compatibility contract (TL;DR of the description).

[alexminza]

@clawsweeper re-review

[clawsweeper]

🦞👀
Exact review queued.

Re-review progress:

• State: Complete
• Detail: The durable review result and its route handoff completed.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/38857770498
• Updated: 2026-10-16T05:49:29.867Z

[steipete]

Accepting the opt-in encryption design with the documented coordinated-upgrade requirement and isolated downgrade procedure. The versioned markers preserve the policy across empty stores and every directory deletion path; they deliberately do not claim enforcement by already-released binaries. The completed candidate has passed focused regression and installed-package downgrade proof, and independent P0–P2 review.