feat(cli): add encrypted token store to integration templates - #3310
Conversation
Generated integration apps previously had two options for durable OAuth token storage: implement the full RefreshCapableTokenStore contract by hand or fall back to the development-only memory store. This adds a drop-in lib/encrypted-token-store.ts to the integration _base template that builds a RefreshCapableTokenStore over any durable key-value service (get/set/delete plus atomic compareAndSwap and withLock), with AES-256-GCM encryption at rest via the Web Crypto API: - fresh random 96-bit IV per encryption - the storage key bound as additional authenticated data, so ciphertext moved between slots fails authentication - key imported from TOKEN_ENCRYPTION_KEY (64 hex chars); creation fails closed with guidance when the key is missing or malformed, and reads refuse anything not in the versioned encrypted envelope - there is no plaintext path in either direction - one-shot OAuth state via atomic compare-and-swap plus a 10-minute freshness window enforced independently of backend TTL support lib/token-store-examples.ts ships a development-only in-memory backend (refused in production) and a Redis-shaped wiring sketch. The design is reworked from the codex/module-reconcile-20260723 branch, replacing its plaintext fallback with fail-closed behavior; the branch's revokeToken auth changes are deliberately not ported.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughChangesThe templates add encrypted OAuth token and state storage with AES-256-GCM, validation, rotation, revisioned updates, refresh locking, and one-time state consumption. They also add restricted in-memory storage, shared OAuth refresh handling, provider wiring, and generated-template tests. ChangesEncrypted OAuth storage
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant OAuthService
participant getRefreshableAccessToken
participant EncryptedKvBackend
participant OAuthProvider
OAuthService->>getRefreshableAccessToken: request access token
getRefreshableAccessToken->>EncryptedKvBackend: read token and acquire refresh lock
getRefreshableAccessToken->>OAuthProvider: refresh near-expiry token
OAuthProvider-->>getRefreshableAccessToken: return refreshed token
getRefreshableAccessToken->>EncryptedKvBackend: compareAndSwap encrypted token
getRefreshableAccessToken-->>OAuthService: return access token
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds an encrypted, KV-backed OAuth token store implementation to the CLI integration _base template so generated apps can get durable, encrypted token persistence while still satisfying the RefreshCapableTokenStore contract used by Veryfront OAuth.
Changes:
- Introduces
createEncryptedTokenStore()(AES-256-GCM, fail-closed key handling, slot binding via AAD) plus anEncryptedKvBackendcontract andgenerateEncryptionKey(). - Adds reference backend examples (dev-only in-memory backend + Redis-shaped sketch) and updates the template token-store docs to point to the new encrypted store.
- Regenerates the templates manifest, adds a focused test suite for the new template store, and removes two unused imports in LLM extensions.
Verification (not run in this review environment):
- PR description reports targeted
deno checkand multiple focuseddeno testsuites passing, plus manifest-generation checks.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| extensions/ext-llm-openai/src/openai-responses-request-builder.ts | Removes an unused stringifyJsonValue import. |
| extensions/ext-llm-anthropic/src/anthropic-request-builder.ts | Removes an unused stringifyJsonValue import. |
| cli/templates/manifest.json | Adds the new template library files to the generated manifest and updates the token-store blob. |
| cli/templates/integrations/_base/files/lib/token-store.ts | Adds a doc pointer to the new encrypted token-store implementation and examples. |
| cli/templates/integrations/_base/files/lib/encrypted-token-store.ts | New encrypted token-store implementation over a KV backend (AES-GCM, AAD slot binding, CAS + lock delegation). |
| cli/templates/integrations/_base/files/lib/token-store-examples.ts | New reference backends (dev-only memory KV + Redis-shaped wiring sketch). |
| cli/encrypted-token-store-template.test.ts | New test suite covering key handling, envelope refusal, tamper/slot-swap rejection, CAS, locks, and state semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The encrypted integration token store accepted arbitrary object shapes at the persistence boundary, so generated integrations could persist values through getters or keep non-string optional token fields. Normalize rows through own data descriptors and return detached snapshots before sealing or returning tokens. Constraint: Generated templates must remain self-contained and not depend on non-exported internal OAuth helpers. Rejected: Export normalizeStoredOAuthTokens for templates | would broaden the public OAuth surface to fix one generated file. Confidence: high Scope-risk: narrow Tested: npx --yes deno@2.7.7 test --no-check --allow-all cli/encrypted-token-store-template.test.ts Tested: npx --yes deno@2.7.7 lint cli/encrypted-token-store-template.test.ts cli/templates/integrations/_base/files/lib/encrypted-token-store.ts Tested: npx --yes deno@2.7.7 check --config=deno.json cli/encrypted-token-store-template.test.ts
The token-store template hardening changed the embedded integration template, so the committed manifest must carry the same generated source for scaffolded projects and manifest checks. Constraint: Template manifest checks compare the embedded source against cli/templates/integrations. Confidence: high Scope-risk: narrow Tested: npx --yes deno@2.7.7 task generate:manifests:check Tested: git diff --check
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cli/templates/integrations/_base/files/lib/encrypted-token-store.ts:491
consumeState()currently decrypts and validates the consumed row without guarding against decryption/envelope errors. If a stored state value is malformed/plaintext, written with a different key, or otherwise fails authentication,cipher.open()will throw and the OAuth callback handler will treat it as a genericcallback_errorrather than aninvalid_state. For state consumption, invalid/corrupted rows should be treated the same as unknown/expired state (returnnull) so the callback flow fails closed withinvalid_statesemantics.
const consumed = await backend.compareAndSwap(key, raw, null);
if (!consumed) return null;
const row = requireStateRow(await cipher.open(key, raw));
return isFreshState(row.createdAt, Date.now()) ? row : null;
kwakayama
left a comment
kwakayama
left a comment
There was a problem hiding this comment.
Critical Review — Score: 86/100
Verdict
This is a well-executed secret-handling PR: authenticated encryption done correctly (AES-256-GCM via crypto.subtle, fresh random 96-bit IV per seal, storage key bound as AAD), a genuinely fail-closed key policy with no plaintext read or write path, and tests that assert what actually hits storage rather than round-trip behavior. I found no blockers and no majors — only operational-hardening and hygiene items. Rubric band: 75–89, minor nits, safe to merge after small fixes (or as-is with follow-ups filed).
Findings
- [minor] Production guard on the example backend is fail-open.
isProductionRuntime()incli/templates/integrations/_base/files/lib/token-store-examples.tsonly trips whenNODE_ENV === "production". A production deployment that never setsNODE_ENV(common on Deno-based hosts) silently gets the process-local, non-durable dev backend — tokens vanish on restart and the cross-worker CAS/lock guarantees the store's design depends on don't hold. Consider inverting the check (allow only when the runtime is explicitly development/test) so the guard fails closed like the rest of the PR. - [minor] No key-rotation story. The envelope is versioned (
vf-aes-gcm.v1:inencrypted-token-store.ts) but carries no key identifier, and the store reads exactly one key fromTOKEN_ENCRYPTION_KEY. Rotating the key turns every stored row into a hard"failed authentication"error (confirmed by the "cannot read values written under a different key" test) with mass re-authentication as the only remedy. Acceptable for a template, but the doc comment should say this explicitly; a cheap improvement is accepting a comma-separated key list (encrypt with first, try-decrypt with the rest). - [minor] Read paths throw instead of degrading on undecryptable rows.
getTokens/getTokenSnapshot(viareadTokenEntry→cipher.open) surface a hard error for legacy-plaintext or wrong-key rows rather than returningnull. Fail-closed is the right default for a token store, but there is no self-heal or documented recovery beyond manually deleting rows — a single bad row can 500 an integrations/settings page indefinitely instead of showing "disconnected, reconnect". Worth documenting the operational recovery (clearTokens/ row deletion) next to the "re-authenticate affected users" error text. - [nit]
stateStorageKeydoes no charset validation. UnlikerequireKeyComponent(trimmed, bounded), thestateparameter is only length-checked (1–1024) before raw concatenation ontoSTATE_KEY_PREFIX. AAD binding prevents cross-slot decryption, but control characters or whitespace in a hostilestateflow into backend keys unsanitized (relevant for text-protocol backends). State normally comes from the framework's CSPRNG, hence nit. - [nit]
requireStateRowvalidatesuserId/serviceId/createdAtbut casts the rest.redirectUriandscopespass through viavalue as StoredOAuthStateunvalidated. Low risk since the row was authenticated-encrypted by this same store atsetStatetime, but the write-side validator is the same function, so those fields are never shape-checked at all. - [nit] Unrelated lint-fix commit bundled in. The
stringifyJsonValueimport removals inextensions/ext-llm-anthropic/src/anthropic-request-builder.tsandextensions/ext-llm-openai/src/openai-responses-request-builder.tsare declared and trivial, but they belong in their own PR so this one stays a pure template change.
What's good
- Textbook AEAD usage: fresh IV per encryption, storage slot bound as AAD (with a test proving ciphertext moved between user slots fails auth), key zeroed after
importKey, versioned envelope, and strict size caps on both plaintext and encoded input. - Genuinely fail-closed: missing/malformed
TOKEN_ENCRYPTION_KEYthrows at store creation with actionable guidance, and non-envelope rows are refused on read — the unsafe plaintext fallback from the original branch is fully gone, and tests assert the refusal in both directions. - One-shot OAuth state via atomic compare-and-swap plus an independent freshness window that holds even when the backend ignores TTL hints; revisioned CAS for refresh races; error messages never include token material.
🤖 Critical review by Claude Code
kwakayama
left a comment
kwakayama
left a comment
There was a problem hiding this comment.
Review: 89/100 — substantively merge-ready
| Axis | Score |
|---|---|
| Correctness | 36/40 |
| Test adequacy | 21/25 |
| Security / prod-safety | 19/20 |
| Maintainability | 13/15 |
| Total | 89/100 |
Merge base 82af3669ffe3, three-dot throughout.
I reviewed this expecting to find a default key, CBC-without-a-MAC, or a fail-open decrypt — the three things that most often go wrong in hand-rolled token storage, and which matter more than usual here because this code gets copied into customer projects that never get re-reviewed. None of them are present. Every security claim in the PR body traced to code and held. All findings below are P3, and none touches the cryptographic construction.
The construction, traced to code
cli/templates/integrations/_base/files/lib/encrypted-token-store.ts
| Property | Finding | Line |
|---|---|---|
| Primitive/mode | AES-256-GCM (AEAD) via crypto.subtle |
:334, :363 |
| Key length | 32 bytes, enforced by /^[0-9a-fA-F]{64}$/ |
:114 |
| IV | Fresh 12-byte crypto.getRandomValues per seal() — never reused, not a counter |
:333 |
| AAD | Storage key bound as additionalData on encrypt and decrypt |
:336, :365 |
| Key source | TOKEN_ENCRYPTION_KEY env var only |
:70, :127 |
| Default/fallback key | None. Missing or empty → throw at construction | :129-136 |
| KDF | None, correctly — the env var is a 256-bit random key, not a password | :112-123 |
| Key handle | importKey(..., extractable=false, ["encrypt","decrypt"]), keyBytes.fill(0) after |
:321-325 |
| Decrypt failure | Throws. No null return, no fall-through | :361-374 |
| Plaintext rows | Rejected — vf-aes-gcm.v1: prefix required before any parse |
:350-356 |
| Parse order | JSON.parse runs after GCM authentication |
:375 |
| Randomness | crypto.getRandomValues / crypto.randomUUID. Zero Math.random |
:107, :333, :427 |
| Disk writes | None — backend is customer-supplied | — |
| Secret logging | None. Zero console.*; error strings name fields, never values |
grep-verified |
Envelope: "vf-aes-gcm.v1:" || base64(IV[12] || ciphertext || tag[16]). Versioned, so a v2 is distinguishable.
keyBytes.fill(0) after an async importKey is safe rather than a race — WebCrypto takes a synchronous copy of the BufferSource in step 1 of importKey. Checked specifically, since zeroing a buffer an async call still needs would be a real bug.
Also verified:
compareAndSetTokens(:437) CASes on the exact stored ciphertext asexpected— a true CAS, not a read-then-write race.consumeState(:481) atomically CAS-deletes before returning the row, so a replayed OAuth callback cannot redeem the same state twice.- State freshness is re-checked in-process (
isFreshState,:296) rather than trusting backend TTL, with a bounded 60s skew allowance. requireTokenRowreads viaObject.getOwnPropertyDescriptorand rejects accessors (:214-217), so a hostile token object cannot run a getter during normalization.cli/templates/manifest.jsonis regenerated and contains both new files — scaffolded projects will actually receive them. Checked, because a template that never ships is a silent no-op.- Default behavior unchanged: the store is opt-in via
configureTokenStore(...), and the pre-existing default still hard-throws in production (token-store.ts:161-167). No silent downgrade path.
P3 — the "fresh IV" test cannot detect a fixed IV
cli/encrypted-token-store-template.test.ts:183-194
The test writes the same token twice and asserts the stored strings differ. But setTokens stamps revision: crypto.randomUUID() into the plaintext on every call (:427), so the ciphertexts differ regardless of the IV. Hardcode the IV to twelve zero bytes and this test still passes.
The IV genuinely is fresh — verified by reading seal() — but this test does not protect it. Fix: strip the prefix, base64-decode both rows, assert first.slice(0,12) !== second.slice(0,12).
P3 — rotating TOKEN_ENCRYPTION_KEY bricks every stored token, with no migration path
:70-71. The envelope carries a format version but no key id, and the store holds exactly one key. Rotating the env var makes every existing row fail authentication — correctly, but irrecoverably.
Concrete: a customer rotates the key after an employee offboards. Every connected user's integration breaks at once, and the only recovery is full re-authentication of the entire user base. The module header says only "Generate a key once per deployment" — no warning about this.
For a template, worth at least a documented warning, ideally a two-key envelope (vf-aes-gcm.v1:<keyId>:) with a decrypt-old / encrypt-new window.
P3 — tokens with surrounding whitespace are rejected outright
:230-236. requireOptionalTokenString throws when value.trim() !== value, and that applies to scope (:250).
Concrete: a provider returns scope: "read write " with a trailing space — sloppy but not rare in OAuth responses. setTokens throws, the callback fails, the user cannot connect at all. Loud, but a hard block caused by a cosmetic upstream quirk. Strict rejection is right for accessToken; for scope, normalize instead.
P3 — unrelated changes bundled in
The same two unused stringifyJsonValue import removals that rode along in #3306, #3312, #3315, and #3308, under a "unblock pre-push lint" commit. Harmless individually, but five PRs now independently carry this fix and it will produce merge noise. Worth fixing once at the source.
Test adequacy — genuinely strong
The adversarial cases are real, not round-trip theatre:
- Tamper detection (
:211) — flips a byte inside the ciphertext body, asserts"failed authentication". - Slot binding / AAD (
:234) — copies Alice's ciphertext into Mallory's key, asserts auth failure. This is the correct AAD test and would fail ifadditionalDatawere dropped. - Wrong key (
:250) — rotates the env var, rebuilds, asserts auth failure. - Missing key (
:78) and malformed key (:88) — fail closed at construction. - Plaintext refusal (
:196), no plaintext at rest (:110), accessor rejection (:126), TTL-ignoring backend (:332), production refusal of the memory backend (:384).
That set covers the adversarial surface. The IV test above is the single gap.
Production risk: low
Nothing here executes in Veryfront's own runtime — these are template files copied into scaffolded projects, and adoption is opt-in. Existing projects are unaffected and the default path still refuses to run unconfigured in production.
The load-bearing risk is real but points the other way: this code gets copied into projects that never get re-reviewed. That is exactly why I traced every claim rather than accepting the body, and the construction holds up.
Rollback clean. No runtime code, no migrations, no persisted state, no wire format in production use yet. Only coupling is cli/templates/manifest.json, which embeds file contents inline — same commit range, so a full revert handles it.
Follow-ups (non-blocking)
- Fix the IV test to decode and compare actual IV bytes.
- Document the key-rotation consequence; consider a key-id envelope.
- Trim rather than reject
scope. - Stop bundling the
stringifyJsonValuelint fix into unrelated PRs.
Note on the score: 89 sits one point under the 90 bar I have been applying to this batch, and every finding is P3 with none touching the crypto. I am flagging rather than merging so the owner can make that call, but my assessment is that this is ready.
Tighten the template boundary that persists OAuth credentials and state after review exposed fail-open runtime selection, under-validated callback state, and a test that did not isolate IV reuse. Constraint: Generated code must remain dependency-free and work in both Deno and Node runtimes. Rejected: Add multi-key decryption to the v1 envelope | a rotation protocol needs an explicit key identifier and migration design beyond this review fix. Confidence: high Scope-risk: narrow Directive: Keep the manifest synchronized with every template source change. Tested: Focused encrypted token-store tests, template tests, deno check, manifest check, and diff check.
Bring the template work onto the latest main history so its final diff excludes already-landed lint cleanup and can be validated against current generated artifacts. Constraint: Preserve the PR commit lineage without force-pushing. Confidence: high Scope-risk: moderate Tested: Pending post-merge focused and full verification.
|
Addressed the current encrypted token-store review items and pushed What changed:
Verification on the pushed head:
I have not posted merge confidence for this head yet. It still needs fresh GitHub CI and an exact-head independent review before it can be considered for the >90 percent merge bar. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
cli/templates/integrations/_base/files/lib/encrypted-token-store.ts:395
requireStateRow()forwardsmetadatavia a plain cast (metadata as Record<string, unknown>). Ifmetadatais not an object, or if it contains getter properties,EnvelopeCipher.seal()will invoke those getters duringJSON.stringify, which defeats the module’s "own data only" validation approach used elsewhere. Validate thatmetadatais a plain object (non-null, not an array) and snapshot only data properties before returning.
createdAt,
...(codeVerifier === undefined ? {} : { codeVerifier }),
...(metadata === undefined ? {} : { metadata: metadata as Record<string, unknown> }),
};
cli/templates/integrations/_base/files/lib/encrypted-token-store.ts:197
requireKeyComponent()validates trimming/length but still allows ASCII control characters (for example\n,\r,\0) inserviceId/userId. Those values become part of the backend key and AES-GCM additional authenticated data, and can create hard-to-debug storage keys and log/metric injection issues. Reject control characters the same waystateStorageKey()does.
function requireKeyComponent(value: string, label: string): string {
if (
typeof value !== "string" || value.length === 0 ||
value.length > MAX_KEY_COMPONENT_LENGTH || value.trim() !== value
) {
throw new TypeError(
`${label} must be a trimmed, non-empty string of at most ${MAX_KEY_COMPONENT_LENGTH} characters`,
);
}
return value;
|
All current review findings are addressed at exact head 621d660.
Verification:
|
The encrypted integration token-store template already avoids token-row accessors, but state metadata and storage key components still left two suppressed review gaps. This keeps validation data-only before sealing encrypted rows and rejects control characters in generated backend keys. Constraint: Generated integration code must fail closed before persistence and must not invoke user-provided getters during serialization. Rejected: Allow arbitrary metadata and rely on JSON.stringify | it invokes accessors and toJSON hooks outside the store's data-only validation model. Confidence: high Scope-risk: narrow Tested: npx --yes deno@2.7.7 test --no-check --allow-all cli/encrypted-token-store-template.test.ts Tested: npx --yes deno@2.7.7 fmt --check cli/encrypted-token-store-template.test.ts cli/templates/integrations/_base/files/lib/encrypted-token-store.ts Tested: npx --yes deno@2.7.7 check cli/encrypted-token-store-template.test.ts cli/templates/integrations/_base/files/lib/encrypted-token-store.ts cli/templates/integrations/_base/files/lib/token-store-examples.ts cli/templates/integrations/_base/files/lib/token-store.ts
|
Addressed the latest review findings on
Verification:
The local pre-push full suite reached |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cli/templates/integrations/_base/files/lib/encrypted-token-store.ts:447
isFreshState()extends the acceptable age window bySTATE_CLOCK_SKEW_MS(it usesSTATE_TTL_MS + STATE_CLOCK_SKEW_MS). Veryfront’s OAuth handlers treat clock skew as future tolerance only; they still cap maximum age at the TTL (seesrc/oauth/state-utils.ts:isFreshOAuthStateTimestamp). As written, this template store would consider an OAuth state redeemable for up to 11 minutes, which weakens CSRF state replay protection relative to core.
function isFreshState(createdAt: number, now: number): boolean {
return createdAt <= now + STATE_CLOCK_SKEW_MS &&
now - createdAt <= STATE_TTL_MS + STATE_CLOCK_SKEW_MS;
}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
cli/templates/integrations/_base/files/lib/token-store-examples.ts:48
runtimeMode()callsDeno.env.get()without a try/catch. In Deno, environment access can throw (yourcli/token-store-template.test.tsexplicitly models this as "PermissionDenied"), socreateMemoryKvBackend()can crash with an unexpected permission error instead of treating the mode as unset and throwing the intended "explicit development or test" error.
Wrap the Deno env read in a try/catch (matching token-store.ts) and return undefined on failure so the caller fails closed consistently.
function runtimeMode(): string | undefined {
if (typeof process !== "undefined" && process.env) return process.env.NODE_ENV;
return (globalThis as { Deno?: { env?: { get?: (name: string) => string | undefined } } }).Deno
?.env?.get?.("NODE_ENV");
}
cli/templates/integrations/_base/files/lib/encrypted-token-store.ts:160
readEnvironmentVariable()callsDeno.env.get()directly. If Deno env access is denied, this will throw a permission error and bypass the store’s intended fail-closed behavior and error messaging (e.g. the "TOKEN_ENCRYPTION_KEY is not set" guidance).
Handle denied env access the same way token-store.ts does for NODE_ENV: catch and treat it as unset. That keeps errors deterministic and prevents leaking environment-permission details through user-facing exceptions.
function readEnvironmentVariable(name: string): string | undefined {
if (typeof process !== "undefined" && process.env) return process.env[name];
return (globalThis as { Deno?: { env?: { get?: (name: string) => string | undefined } } })
.Deno?.env?.get?.(name);
}
Deno can throw when generated templates read environment variables without permission. The token store should treat that as an unset value so callers get the same fail-closed configuration errors as other missing-env cases. Constraint: Review comments called out denied Deno.env access in generated encrypted token store templates. Rejected: Propagate the permission error | it bypasses the generated store's deterministic configuration guidance. Confidence: high Scope-risk: narrow Tested: npx --yes deno@2.7.7 test --no-check --allow-all --unstable-worker-options --unstable-net cli/encrypted-token-store-template.test.ts Tested: npx --yes deno@2.7.7 check cli/encrypted-token-store-template.test.ts Tested: npx --yes deno@2.7.7 fmt --check cli/encrypted-token-store-template.test.ts cli/templates/integrations/_base/files/lib/encrypted-token-store.ts cli/templates/integrations/_base/files/lib/token-store-examples.ts Tested: npx --yes deno@2.7.7 lint cli/encrypted-token-store-template.test.ts cli/templates/integrations/_base/files/lib/encrypted-token-store.ts cli/templates/integrations/_base/files/lib/token-store-examples.ts Tested: git diff --check
|
Addressed the suppressed env-access review comments at exact head Changes:
Verification:
I dequeued the stale merge-queue entry before pushing because it was queued for the previous head. I am not re-queueing until hosted checks complete on this exact head. |
Rotation reports are used to decide when TOKEN_ENCRYPTION_KEY_PREVIOUS can be removed. Backends may ignore the OAuth state TTL hint, so expired one-shot state rows should not keep a rotation permanently incomplete after they are no longer consumable. Constraint: OAuth state freshness is already enforced by consumeState and setState; the scanner must match that boundary without deleting rows. Rejected: Delete expired rows during scan | a rotation report should not mutate backend state. Confidence: high Scope-risk: narrow Directive: Count malformed or undecryptable state rows as unreadable, but skip schema-valid expired state rows after authenticated decrypt. Tested: npx --yes deno@2.7.7 test --no-check --allow-all --unstable-worker-options --unstable-net cli/encrypted-token-store-template.test.ts Tested: npx --yes deno@2.7.7 test --no-check --allow-all --unstable-worker-options --unstable-net cli/encrypted-token-store-template.test.ts cli/token-store-template.test.ts cli/templates/index.test.ts Tested: npx --yes deno@2.7.7 run -A scripts/build/generate-templates-manifest.ts --check Tested: npx --yes deno@2.7.7 fmt --check cli/encrypted-token-store-template.test.ts cli/templates/integrations/_base/files/lib/encrypted-token-store.ts cli/templates/manifest.json Tested: npx --yes deno@2.7.7 lint cli/encrypted-token-store-template.test.ts cli/templates/integrations/_base/files/lib/encrypted-token-store.ts Tested: npx --yes deno@2.7.7 check cli/encrypted-token-store-template.test.ts cli/templates/integrations/_base/files/lib/encrypted-token-store.ts Tested: git diff --check
|
Addressed the remaining rotation-scan review finding at exact head What changed:
TDD evidence:
Verification:
Current PR state after push:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cli/templates/integrations/_base/files/lib/token-store.ts:10
- The header comment still says the built-in memory store is "development-only", but
createDefaultTokenStore()now allows it in bothdevelopmentandtest. Update the comment to match the actual behavior so generated apps do not get conflicting guidance.
* durable, extension-owned RefreshCapableTokenStore before the first OAuth
* request in production. The built-in memory store is development-only.
*
* To build that durable store on top of a plain key-value service with
* AES-256-GCM encryption at rest, see `encrypted-token-store.ts` (reference
|
Merge confidence: 91%. Reasoning:
Scheduling exact-head auto-merge now. |
The generated token-store header still described the process-local memory store as development-only after the runtime guard intentionally allowed both development and test. Update the template copy and add a template-contract assertion so future guard wording changes stay synchronized with generated guidance. Constraint: PR #3310 allows process-local OAuth storage only in explicit development or test modes. Rejected: Runtime behavior change | the implementation already enforces the intended development/test guard. Confidence: high Scope-risk: narrow Tested: npx --yes deno@2.7.7 test --no-check --allow-all --unstable-worker-options --unstable-net cli/encrypted-token-store-template.test.ts cli/token-store-template.test.ts cli/templates/index.test.ts Tested: npx --yes deno@2.7.7 run -A scripts/build/generate-templates-manifest.ts --check Tested: npx --yes deno@2.7.7 fmt --check cli/templates/integrations/_base/files/lib/token-store.ts cli/templates/index.test.ts cli/templates/manifest.json && npx --yes deno@2.7.7 lint cli/templates/integrations/_base/files/lib/token-store.ts cli/templates/index.test.ts && npx --yes deno@2.7.7 check cli/templates/integrations/_base/files/lib/token-store.ts cli/templates/index.test.ts && git diff --check
|
Addressed the latest review note on the generated token-store template at exact head Change made:
Verification:
Merge confidence is not above 90% yet because hosted CI is still running on the new head. I am not scheduling this PR until those exact-head checks complete green. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (4)
cli/templates/integrations/_base/files/lib/token-store.ts:205
- On refresh, the helper persists exactly the
OAuthTokensobject returned byrefresh(). Many providers omitrefresh_tokenon refresh (the template refresh helpers already guard this withdata.refresh_token ?? refreshToken), so a refresh callback that returns only{ accessToken, expiresAt }would clear a previously valid refresh token and break subsequent refreshes. Preserve the existing refresh token when the refreshed result omits it.
let refreshed: OAuthToken;
try {
refreshed = await refresh(token.refreshToken);
} catch {
// A provider failure must not unconditionally delete a row that may
// have been replaced by a concurrent reconnect outside the lock.
const latest = await store.getTokens(serviceId, userId);
return latest ? unexpiredAccessToken(latest) : null;
}
const replaced = await store.compareAndSetTokens(
serviceId,
userId,
current.revision,
refreshed,
cli/templates/integrations/_base/files/lib/token-store.ts:55
runtimeMode()returnsprocess.env.NODE_ENVwheneverprocess.envexists, even when that value is undefined or when accessingprocess.envthrows (for example in Deno without--allow-env). That prevents falling back toDeno.env.get("NODE_ENV"), and can incorrectly deny dev/test memory storage in Deno or template runtimes that inject an emptyprocess.envshim.
This issue also appears on line 191 of the same file.
function runtimeMode(): string | undefined {
if (typeof process !== "undefined" && process.env) return process.env.NODE_ENV;
try {
return (globalThis as { Deno?: { env?: { get?: (name: string) => string | undefined } } })
.Deno?.env?.get?.("NODE_ENV");
cli/templates/integrations/_base/files/lib/encrypted-token-store.ts:164
readEnvironmentVariable()returnsprocess.env[name]wheneverprocess.envexists, even when the value is undefined or whenprocess.envaccess throws (for example, in Deno without env permissions). That can prevent readingTOKEN_ENCRYPTION_KEYfromDeno.envand can cause the encrypted store to fail closed in environments that inject aprocess.envshim.
function readEnvironmentVariable(name: string): string | undefined {
if (typeof process !== "undefined" && process.env) return process.env[name];
try {
return (globalThis as { Deno?: { env?: { get?: (name: string) => string | undefined } } })
.Deno?.env?.get?.(name);
cli/templates/integrations/_base/files/lib/token-store-examples.ts:52
- Like
token-store.ts, thisruntimeMode()short-circuits toprocess.env.NODE_ENVwheneverprocess.envexists, which blocks falling back toDeno.env.get("NODE_ENV")and does not handleprocess.envthrowing when env access is denied. This can make the dev/test example backend fail closed in Deno even whenNODE_ENVis set.
function runtimeMode(): string | undefined {
if (typeof process !== "undefined" && process.env) return process.env.NODE_ENV;
try {
return (globalThis as { Deno?: { env?: { get?: (name: string) => string | undefined } } })
.Deno?.env?.get?.("NODE_ENV");
Some OAuth refresh endpoints return a fresh access token without echoing the durable refresh token. The generated lock-aware helper now carries the current refresh token forward before its revisioned compare-and-set write, so the next refresh attempt remains possible without weakening CAS protection. Constraint: Generated templates must remain durable across provider refresh responses that omit refresh_token. Rejected: Require every generated provider refresh helper to backfill refresh_token | duplicates a central store invariant across clients. Confidence: high Scope-risk: narrow Tested: npx --yes deno@2.7.7 test --no-check --allow-all --unstable-worker-options --unstable-net cli/encrypted-token-store-template.test.ts cli/token-store-template.test.ts cli/templates/index.test.ts Tested: npx --yes deno@2.7.7 run -A scripts/build/generate-templates-manifest.ts --check Tested: npx --yes deno@2.7.7 fmt --check cli/templates/integrations/_base/files/lib/token-store.ts cli/token-store-template.test.ts cli/templates/manifest.json Tested: npx --yes deno@2.7.7 lint cli/templates/integrations/_base/files/lib/token-store.ts cli/token-store-template.test.ts Tested: npx --yes deno@2.7.7 check cli/templates/integrations/_base/files/lib/token-store.ts cli/token-store-template.test.ts Tested: git diff --check
|
Addressed the latest suppressed Copilot review note at exact head Change made:
I also rechecked the other suppressed comments in that review: the current head already guards Verification:
Merge confidence is 88% until hosted checks finish on |
|
Fixed the Deno denied-environment fail-closed defect at exact head Deno 2 exposes the Node-compatible Verification before the guarded fast-forward push: 3 focused suites / 97 steps green, |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cli/templates/integrations/_base/files/lib/token-store-examples.ts:63
- The docstring says "Development-only" but
createMemoryKvBackend()explicitly allows bothNODE_ENV=developmentandNODE_ENV=test. Updating the wording avoids a mismatch between documentation and the runtime guard.
interface MemoryRow {
value: string;
expiresAt: number | null;
}
The generated encrypted-token-store example allows its in-memory backend in explicit development and test modes, but one public docstring still described it as development-only. Align the generated guidance with the runtime guard and pin the wording in the template inventory test. Constraint: The runtime behavior already allows both development and test; this change must not alter storage selection. Rejected: Change the guard to development-only | existing tests intentionally allow generated integration tests to use the backend under NODE_ENV=test. Confidence: high Scope-risk: narrow Tested: npx --yes deno@2.7.7 run -A scripts/build/generate-templates-manifest.ts Tested: npx --yes deno@2.7.7 test --no-check --allow-all --unstable-worker-options --unstable-net cli/encrypted-token-store-template.test.ts cli/token-store-template.test.ts cli/templates/index.test.ts Tested: npx --yes deno@2.7.7 run -A scripts/build/generate-templates-manifest.ts --check Tested: npx --yes deno@2.7.7 fmt --check cli/templates/integrations/_base/files/lib/token-store-examples.ts cli/templates/index.test.ts cli/templates/manifest.json Tested: npx --yes deno@2.7.7 check --allow-import cli/templates/index.test.ts cli/token-store-template.test.ts cli/encrypted-token-store-template.test.ts Tested: git diff --check
|
Addressed the remaining generated memory-backend guidance note at exact head Change made:
Verification:
Pushed with |
|
Merge readiness for aeafe66: Merge confidence: 94%. Reasoning:
I am scheduling this PR with |
|
Merge confidence: 92% for exact head Reasoning: the generated token-store template copy/docstring correction is narrow, all review feedback is addressed, and GitHub reports the hosted gate green on this exact head: format, lint, typecheck, unit, integration, coverage shards, RSC browser e2e, binary e2e, npm install smoke, sentry runtime packages, coverage gate, and CodeQL are successful, with release-only jobs skipped. There are no unresolved review threads. Local evidence already posted for this head includes template manifest generation/checks plus the encrypted token store, token store template, and template index suites. Residual risk is low and limited to generated template wording/manifest drift, which the manifest check covers. Scheduling this exact head for merge queue. |
|
Merge confidence: 92% for exact head aeafe66. Reasoning: the branch is clean, hosted CI is terminal green across format, lint, typecheck, unit, integration, binary e2e, RSC browser e2e, npm install smoke, sentry runtime packages, all coverage shards, coverage gate, CLA, and CodeQL. All review threads are resolved (14 total, 0 unresolved), including the encryption validation, memory-store fail-closed, refresh CAS/lock, Gmail refresh-capable adapter, Deno env denial, legacy envelope test, and CodeQL test-sanitization findings. I also ran exact-head local gates in the PR worktree: changed-file fmt/lint/check plus the three focused generated-template suites, 97 steps, 0 failures, and |
|
Merge confidence: 94% for exact head Reasoning: this head has no unresolved review threads, no failing or pending hosted checks, and GitHub reports the branch clean against current |
Summary
Adds an encrypted, key-value-backed OAuth token store to the integration
_basetemplate and hardens the shared generated token-store contract used by OAuth handlers and generated clients.Exact code head documented here:
aeafe6692e2e592a85a0a2dbab0e4a2dee38044f.This is a rework port from
codex/module-reconcile-20260723, not a cherry-pick. The useful storage idea was reimplemented on currentmainbecause the original branch allowed plaintext fallback, lacked storage-slot binding, and reduced the backend contract to non-atomic reads and writes. The concurrency model remains aligned with #3234.Encrypted storage contract
createEncryptedTokenStore(backend)implements the completeRefreshCapableTokenStorecontract over an application-suppliedEncryptedKvBackend.get,set, anddeleteoperations, atomiccompareAndSwap, TTL hints, and a bounded cross-workerwithLocklease.TOKEN_ENCRYPTION_KEYmust contain exactly 64 hexadecimal characters. Store creation fails when the key is missing or malformed; there is no plaintext write or read path.Key rotation and envelope compatibility
vf-aes-gcm.v2:<key-id>:envelope.TOKEN_ENCRYPTION_KEYis the current sealing key. OptionalTOKEN_ENCRYPTION_KEY_PREVIOUSkeeps rows from the retiring key readable during a rotation window.vf-aes-gcm.v1:envelopes have no key id and are decrypted by trying the configured current and previous keys.OAuth state and metadata
__proto__, without invoking accessors, inherited serializers, or legacy prototype setters on Node.null, preserving fail-closed callback behavior.Generated client concurrency
getRefreshableAccessToken()helper replaces unconditional read-refresh-write logic in the base, Google Docs, and Drive OAuth clients.compareAndSetTokens, preventing a stale refresh from overwriting a concurrent reconnect or revocation.getToken,setToken,revokeToken, andisConnected) remain available through the shared adapter.Default storage and extension composition
MemoryTokenStoreand the example memory KV backend are allowed only whenNODE_ENVis explicitlydevelopmentortest.Upgrade and compatibility
TOKEN_ENCRYPTION_KEYbefore store construction.TOKEN_ENCRYPTION_KEY_PREVIOUS; rows that are never written remain dependent on that key until reconnect, removal, or another explicit write.NODE_ENVselecting memory storage must configure the runtime mode for development or provide durable production storage.src/oauthAPI or runtime implementation changes in this PR.Deliberately excluded
userId:servicecolon-key migration.oauth-store-registryor automatic backend selection.Changed areas
cli/templates/integrations/_base/files/lib/encrypted-token-store.ts: encrypted backend adapter, envelope/keyring logic, validation, CAS, locking, and one-shot state.cli/templates/integrations/_base/files/lib/token-store-examples.ts: guarded development memory backend and dependency-free durable-backend boundary examples.cli/templates/integrations/_base/files/lib/token-store.ts: strict default selection, adapter aliases, and shared lock/CAS refresh helper.cli/encrypted-token-store-template.test.ts,cli/token-store-template.test.ts, andcli/templates/index.test.ts: encryption, rotation, validation, cross-runtime, default-mode, generated-client, and template contract coverage.cli/templates/manifest.json: regenerated template payloads.Verification
Verification completed on exact code head
aeafe6692e2e592a85a0a2dbab0e4a2dee38044f:deno fmt --check,deno lint, anddeno check: passed.--check: passed.git diff --check: passed.deno task verify:quick: passed, including manifests, workspace format and lint, dependency and module boundaries, extension contracts, documentation validation, and workspace typechecking.Summary by CodeRabbit
New Features
Bug Fixes