feat(stack): protect-ffi 0.26.0 + auth 0.39 OidcFederationStrategy (stacked on #496)#497
Open
coderdan wants to merge 6 commits into
Open
feat(stack): protect-ffi 0.26.0 + auth 0.39 OidcFederationStrategy (stacked on #496)#497coderdan wants to merge 6 commits into
coderdan wants to merge 6 commits into
Conversation
📝 WalkthroughWalkthroughUpgrades ChangesOIDC Strategy & LockContextInput Migration
Sequence Diagram(s)sequenceDiagram
participant App
participant EncryptionClient
participant OidcFederationStrategy
participant EncryptOperation
participant resolveLockContext
participant protectFFI as protect-ffi
rect rgba(70, 130, 180, 0.5)
note over App,EncryptionClient: Initialization
App->>OidcFederationStrategy: OidcFederationStrategy.create(workspaceCrn, jwt)
OidcFederationStrategy-->>App: strategy
App->>EncryptionClient: Encryption({ workspaceCrn, clientKey, strategy })
EncryptionClient->>protectFFI: newClient({ strategy, clientOpts: { workspaceCrn, clientKey } })
protectFFI-->>EncryptionClient: client
end
rect rgba(60, 179, 113, 0.5)
note over App,protectFFI: Identity-bound encrypt
App->>EncryptionClient: encrypt(plaintext).withLockContext({ identityClaim: ["sub"] })
EncryptionClient-->>App: EncryptOperationWithLockContext
App->>EncryptOperation: execute()
EncryptOperation->>resolveLockContext: resolveLockContext({ identityClaim })
resolveLockContext-->>EncryptOperation: Context
EncryptOperation->>protectFFI: ffiEncrypt(payload, { lockContext: context })
note right of protectFFI: strategy.getToken() called per request — no serviceToken
protectFFI-->>EncryptOperation: ciphertext
EncryptOperation-->>App: Result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
🦋 Changeset detectedLatest commit: a23fe7e The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
coderdan
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR upgrades Stack’s Protect/Auth dependencies and updates the identity-bound encryption flow to match protect-ffi ≥0.25’s removal of per-operation serviceToken, moving authentication to a client-level config.strategy (notably via OidcFederationStrategy) while keeping lock context as a pure { identityClaim } value.
Changes:
- Bump
@cipherstash/protect-ffito0.26.0and@cipherstash/authto0.39.0, updating workspace config to useworkspaceCrnconsistently (including/wasm-inline). - Replace the lock-context “token ceremony” with a synchronous lock-context resolution (
.withLockContext({ identityClaim })orLockContext), and stop forwardingserviceTokenin all operations. - Re-export auth strategies from
@cipherstash/stack(and selected ones from/wasm-inline), add wiring tests to ensureidentityClaimis forwarded andserviceTokenis never sent.
Reviewed changes
Copilot reviewed 34 out of 35 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates identity-aware encryption docs to the strategy-based approach. |
| pnpm-workspace.yaml | Bumps @cipherstash/auth* catalog versions to 0.39.0. |
| pnpm-lock.yaml | Regenerated lockfile reflecting new auth/protect versions and optional deps. |
| packages/stack/src/wasm-inline.ts | Switches WASM config to workspaceCrn, supports OIDC strategy, updates protect-ffi wasm newClient call shape, and re-exports strategies. |
| packages/stack/src/types.ts | Re-exports protect-ffi AuthStrategy type and adds ClientConfig.strategy. |
| packages/stack/src/types-public.ts | Exposes AuthStrategy in public type surface. |
| packages/stack/src/index.ts | Re-exports auth strategies from @cipherstash/auth in an ESM-compatible way. |
| packages/stack/src/identity/index.ts | Adds LockContextInput + resolveLockContext, deprecates old token ceremony methods, makes CTS token optional in response type. |
| packages/stack/src/encryption/operations/encrypt.ts | Uses synchronous lock-context resolution; removes serviceToken forwarding. |
| packages/stack/src/encryption/operations/encrypt-query.ts | Uses synchronous lock-context resolution; removes serviceToken forwarding. |
| packages/stack/src/encryption/operations/encrypt-model.ts | Uses synchronous lock-context resolution and passes Context through model helpers. |
| packages/stack/src/encryption/operations/decrypt.ts | Uses synchronous lock-context resolution; removes serviceToken forwarding. |
| packages/stack/src/encryption/operations/decrypt-model.ts | Uses synchronous lock-context resolution and passes Context through model helpers. |
| packages/stack/src/encryption/operations/bulk-encrypt.ts | Uses synchronous lock-context resolution; removes serviceToken forwarding. |
| packages/stack/src/encryption/operations/bulk-encrypt-models.ts | Uses synchronous lock-context resolution and passes Context through model helpers. |
| packages/stack/src/encryption/operations/bulk-decrypt.ts | Uses synchronous lock-context resolution; removes serviceToken forwarding. |
| packages/stack/src/encryption/operations/bulk-decrypt-models.ts | Uses synchronous lock-context resolution and passes Context through model helpers. |
| packages/stack/src/encryption/operations/batch-encrypt-query.ts | Uses synchronous lock-context resolution; removes serviceToken forwarding. |
| packages/stack/src/encryption/index.ts | Plumbs optional config.strategy into protect-ffi newClient and documents identity-bound usage. |
| packages/stack/src/encryption/helpers/model-helpers.ts | Removes CTS token plumbing from model bulk encrypt/decrypt helpers. |
| packages/stack/package.json | Bumps protect-ffi and adds optionalDependencies for auth platform binaries. |
| packages/stack/tests/lock-context.test.ts | Rewrites live identity-bound tests to use OidcFederationStrategy + { identityClaim }. |
| packages/stack/tests/lock-context-wiring.test.ts | Adds offline mocks ensuring identityClaim is forwarded and serviceToken is never sent. |
| packages/stack/tests/init-strategy.test.ts | Adds tests verifying config.strategy is forwarded to protect-ffi newClient. |
| packages/stack/tests/fixtures/index.ts | Simplifies lock-context fixtures to a plain { identityClaim } input. |
| packages/stack/tests/encrypt-query.test.ts | Updates lock-context tests to use plain { identityClaim } input and removes CTS-token-failure cases. |
| packages/stack/tests/encrypt-query-searchable-json.test.ts | Updates lock-context tests to use plain { identityClaim } input and removes CTS-token-failure cases. |
| examples/supabase-worker/supabase/functions/cipherstash-roundtrip/index.ts | Updates example config to use CS_WORKSPACE_CRN (no CS_REGION). |
| examples/supabase-worker/README.md | Updates env setup instructions to include CS_WORKSPACE_CRN. |
| examples/supabase-worker/.env.example | Updates example env file to use CS_WORKSPACE_CRN and remove CS_REGION. |
| e2e/wasm/roundtrip.test.ts | Updates WASM e2e to require/use CS_WORKSPACE_CRN and drop explicit region. |
| e2e/wasm/deno.json | Pins protect/auth wasm-inline imports to 0.26.0 / 0.39.0. |
| AGENTS.md | Updates guidance to the strategy-based identity-aware encryption flow. |
| .github/workflows/tests.yml | Exposes CS_WORKSPACE_CRN to wasm e2e job and asserts it’s present. |
| .changeset/stack-protect-ffi-0-26-oidc-strategy.md | Adds release notes for the dependency bumps and new identity-bound strategy flow. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
packages/stack/tests/lock-context.test.ts:145
- This test tries to assert that decrypting without the lock context fails, but
decryptModel()returns a Result (it doesn’t throw). As written, the try/catch never runs and the test will pass without asserting anything. Assert on the returned Result’sfailureinstead.
try {
await protectClient.decryptModel(encryptedModel.data)
} catch (error) {
const e = error as Error
expect(e.message.startsWith('Failed to retrieve key')).toEqual(true)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * "ap-southeast-2.aws", workspaceId, () => getClerkSessionToken(req), | ||
| * { store: cookieStore({ request: req, responseHeaders }) }, | ||
| * ) | ||
| * const client = await Encryption({ schemas, config: { strategy, clientId, clientKey } }) |
auxesis
requested review from
auxesis and
freshtonic
and removed request for
a team
protect-ffi 0.25.0 is a breaking release for both entries: WASM (@cipherstash/stack/wasm-inline): - newClient(strategy, opts) -> newClient(opts) with strategy nested. - Config takes a workspaceCrn instead of region; the AccessKeyStrategy region is derived from the CRN (crn:<region>:<workspace-id>). CS_REGION is no longer consulted; set CS_WORKSPACE_CRN. Node: - serviceToken removed from the encrypt/decrypt/query option types (and the CtsToken export). The per-operation CTS token is no longer forwarded; lock contexts still travel as lockContext.identityClaim. Public LockContext/identify() API is unchanged. Adds offline lock-context wiring tests (mock protect-ffi) asserting every operation forwards identityClaim and never sends serviceToken, plus extractRegionFromCrn unit tests. Updates the Deno e2e test, Supabase example, and wasm-e2e CI job to CS_WORKSPACE_CRN.
protect-ffi 0.25 lets newClient take an AuthStrategy (any
{ getToken(): Promise<{ token }> } object). Expose it on the Node
Encryption client via config.strategy: when supplied, getToken() is
invoked on every ZeroKMS request, taking precedence over the
credentials-derived default (clientKey is still used for encryption).
Omitting it preserves existing credentials/env behaviour.
Kept on init (rather than a separate initWithStrategy) so a future
keyProvider option can land in the same config. AuthStrategy is
re-exported from @cipherstash/stack for consumers to type their own.
…lace lock-context ceremony
Supersedes the 0.25.0 bump with protect-ffi 0.26.0 (API-identical; internal
fixes only) and @cipherstash/auth 0.39.0, and uses the new
OidcFederationStrategy to replace the lock-context token ceremony with a
strategy-based approach for identity-bound encryption.
- bump @cipherstash/protect-ffi 0.25.0 -> 0.26.0; @cipherstash/auth catalog
(and platform entries) 0.38.0 -> 0.39.0; e2e/wasm/deno.json pins; lockfile
- .withLockContext() now accepts a plain { identityClaim } (or a LockContext)
and resolves the claim synchronously — no CTS token, no identify() call
- deprecate LockContext.identify() / getLockContext(); the client strategy
(OidcFederationStrategy) now handles user token acquisition
- re-export OidcFederationStrategy/AccessKeyStrategy/AutoStrategy/
DeviceSessionStrategy from @cipherstash/stack, and the strategies from
@cipherstash/stack/wasm-inline
- broaden the wasm-inline config strategy type to accept OidcFederationStrategy
- declare @cipherstash/auth platform optionalDependencies (auth ships them as
optional peer deps, not auto-installed) so the re-exported Node strategies
resolve their native binding for consumers
- update wiring/init/live tests, JSDoc, AGENTS.md, README, changeset
…Dependencies The optionalDependencies block added to packages/stack/package.json was not reflected in pnpm-lock.yaml, breaking `pnpm install --frozen-lockfile` in CI.
@cipherstash/auth 0.39 changed AccessKeyStrategy.create(region, accessKey) to AccessKeyStrategy.create(workspaceCrn, accessKey) — it derives the region from the CRN itself. The wasm-inline resolveStrategy still passed a derived region, so the Deno WASM e2e failed with 'Invalid CRN: <region>'. Pass the CRN directly and drop the now-obsolete extractRegionFromCrn helper + tests. (OidcFederationStrategy.create still takes region + workspaceId.)
…ext tests
- index.ts: @cipherstash/auth's Node entry is CJS with `module.exports =
{ ...native }`; the spread defeats cjs-module-lexer so a static
`export { AccessKeyStrategy } from` throws 'Named export not found' under
real Node ESM (the E2E cli failure). Default-import the module (which is
module.exports at runtime, all names present) and re-export each binding
explicitly, with instance types for the strategy classes.
- encrypt-query / encrypt-query-searchable-json tests + fixtures: the ops no
longer call getLockContext(); .withLockContext() takes a plain
{ identityClaim }. createMockLockContext() now returns that shape; dropped
the getLockContext spy assertions and the obsolete failure / null-context
cases (resolveLockContext is synchronous and cannot fail).
coderdan
force-pushed
the
feat/stack-protect-ffi-025
branch
from
1efdd03 to
a23fe7e
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/stack/src/encryption/operations/batch-encrypt-query.ts`:
- Around line 207-209: The resolveLockContext call is positioned outside the
withResult wrapper, which allows resolution errors to escape instead of being
caught and returned as a failure Result. Move the
resolveLockContext(this.lockContext) call from before the withResult statement
into the callback function that is passed to withResult, so that any errors
during lock context resolution are properly wrapped and returned as { failure }
according to the Result contract required by the encryption operations
guidelines.
In `@packages/stack/src/encryption/operations/encrypt-query.ts`:
- Around line 151-153: Move the resolveLockContext(this.lockContext) call inside
the withResult callback to ensure backward-compatible LockContext resolution
errors are properly caught and converted to the Result contract shape { failure
}. The context resolution should happen as the first step within the async
callback passed to withResult, matching the pattern used in other migrated
encryption operations in this codebase. This ensures all potential failures are
wrapped in the Result contract rather than rejecting before withResult can
handle them.
In `@packages/stack/src/identity/index.ts`:
- Around line 66-67: The OIDC example using OidcFederationStrategy.create() at
lines 66–67 uses the outdated signature with separate region and workspaceId
parameters. Update the example to use the current signature where workspaceCrn
is passed as the first parameter instead of region and workspaceId, while
keeping the callback function () => getJwt() as the second parameter to match
the `@cipherstash/auth` v0.39.0+ API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4cfb06e-dd68-43d7-ae11-abd864deb732
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
.changeset/stack-protect-ffi-0-26-oidc-strategy.md.github/workflows/tests.ymlAGENTS.mdREADME.mde2e/wasm/deno.jsone2e/wasm/roundtrip.test.tsexamples/supabase-worker/.env.exampleexamples/supabase-worker/README.mdexamples/supabase-worker/supabase/functions/cipherstash-roundtrip/index.tspackages/stack/__tests__/encrypt-query-searchable-json.test.tspackages/stack/__tests__/encrypt-query.test.tspackages/stack/__tests__/fixtures/index.tspackages/stack/__tests__/init-strategy.test.tspackages/stack/__tests__/lock-context-wiring.test.tspackages/stack/__tests__/lock-context.test.tspackages/stack/package.jsonpackages/stack/src/encryption/helpers/model-helpers.tspackages/stack/src/encryption/index.tspackages/stack/src/encryption/operations/batch-encrypt-query.tspackages/stack/src/encryption/operations/bulk-decrypt-models.tspackages/stack/src/encryption/operations/bulk-decrypt.tspackages/stack/src/encryption/operations/bulk-encrypt-models.tspackages/stack/src/encryption/operations/bulk-encrypt.tspackages/stack/src/encryption/operations/decrypt-model.tspackages/stack/src/encryption/operations/decrypt.tspackages/stack/src/encryption/operations/encrypt-model.tspackages/stack/src/encryption/operations/encrypt-query.tspackages/stack/src/encryption/operations/encrypt.tspackages/stack/src/identity/index.tspackages/stack/src/index.tspackages/stack/src/types-public.tspackages/stack/src/types.tspackages/stack/src/wasm-inline.tspnpm-workspace.yaml
Comment on lines
+207
to
209
| const context = resolveLockContext(this.lockContext) | ||
|
|
||
| const result = await withResult( |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep lock-context resolution inside the Result wrapper.
This locked batch path resolves before withResult, so a resolution error can escape instead of returning { failure }. Move it into the callback before building the payloads. As per coding guidelines, **/src/**/*.{ts,tsx} must preserve the Result contract shape { data } or { failure } returned by encryption operations.
Proposed fix
- const context = resolveLockContext(this.lockContext)
-
const result = await withResult(
async () => {
if (!this.client) throw noClientError()
+ const context = resolveLockContext(this.lockContext)
const { metadata } = this.getAuditData()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const context = resolveLockContext(this.lockContext) | |
| const result = await withResult( | |
| const result = await withResult( | |
| async () => { | |
| if (!this.client) throw noClientError() | |
| const context = resolveLockContext(this.lockContext) | |
| const { metadata } = this.getAuditData() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/stack/src/encryption/operations/batch-encrypt-query.ts` around lines
207 - 209, The resolveLockContext call is positioned outside the withResult
wrapper, which allows resolution errors to escape instead of being caught and
returned as a failure Result. Move the resolveLockContext(this.lockContext) call
from before the withResult statement into the callback function that is passed
to withResult, so that any errors during lock context resolution are properly
wrapped and returned as { failure } according to the Result contract required by
the encryption operations guidelines.
Source: Coding guidelines
Comment on lines
+151
to
153
| const context = resolveLockContext(this.lockContext) | ||
|
|
||
| const result = await withResult( |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep lock-context resolution inside the Result wrapper.
If resolving a backward-compatible LockContext fails, this currently rejects before withResult can convert it to { failure }. Move the resolution into the callback, matching the other migrated operations. As per coding guidelines, **/src/**/*.{ts,tsx} must preserve the Result contract shape { data } or { failure } returned by encryption operations.
Proposed fix
- const context = resolveLockContext(this.lockContext)
-
const result = await withResult(
async () => {
if (!this.client) throw noClientError()
+ const context = resolveLockContext(this.lockContext)
const { metadata } = this.getAuditData()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const context = resolveLockContext(this.lockContext) | |
| const result = await withResult( | |
| const result = await withResult( | |
| async () => { | |
| if (!this.client) throw noClientError() | |
| const context = resolveLockContext(this.lockContext) | |
| const { metadata } = this.getAuditData() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/stack/src/encryption/operations/encrypt-query.ts` around lines 151 -
153, Move the resolveLockContext(this.lockContext) call inside the withResult
callback to ensure backward-compatible LockContext resolution errors are
properly caught and converted to the Result contract shape { failure }. The
context resolution should happen as the first step within the async callback
passed to withResult, matching the pattern used in other migrated encryption
operations in this codebase. This ensures all potential failures are wrapped in
the Result contract rather than rejecting before withResult can handle them.
Source: Coding guidelines
Comment on lines
+66
to
+67
| * strategy: OidcFederationStrategy.create(region, workspaceId, () => getJwt()), | ||
| * }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🌐 Web query:
For @cipherstash/authv0.39.0, what is the exactOidcFederationStrategy.create(...)signature, and does it useworkspaceCrninstead ofregion/workspaceId?
💡 Result:
For @cipherstash/auth v0.39.0, the OidcFederationStrategy.create signature is: OidcFederationStrategy.create(workspaceCrn, getJwt) In this version, the strategy uses workspaceCrn instead of separate region and workspaceId parameters [1]. The getJwt parameter is an asynchronous function that returns the OIDC identity token (JWT) [1]. This change reflects a broader transition in CipherStash's SDKs to use the Workspace CRN (CipherStash Resource Name) as the primary identifier for workspace configuration, replacing the older combination of region and workspace ID [1].
Citations:
🏁 Script executed:
cat -n packages/stack/src/identity/index.ts | sed -n '60,75p'Repository: cipherstash/stack
Length of output: 808
Update the OIDC example to use the current OidcFederationStrategy.create() signature.
The example at lines 66–67 uses the outdated OidcFederationStrategy.create(region, workspaceId, () => getJwt()) signature. In @cipherstash/auth v0.39.0+, this method accepts workspaceCrn as the first parameter instead of separate region and workspaceId arguments.
✏️ Suggested doc fix
- strategy: OidcFederationStrategy.create(region, workspaceId, () => getJwt()),
+ strategy: OidcFederationStrategy.create(workspaceCrn, () => getJwt()),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| * strategy: OidcFederationStrategy.create(region, workspaceId, () => getJwt()), | |
| * }, | |
| * strategy: OidcFederationStrategy.create(workspaceCrn, () => getJwt()), | |
| * }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/stack/src/identity/index.ts` around lines 66 - 67, The OIDC example
using OidcFederationStrategy.create() at lines 66–67 uses the outdated signature
with separate region and workspaceId parameters. Update the example to use the
current signature where workspaceCrn is passed as the first parameter instead of
region and workspaceId, while keeping the callback function () => getJwt() as
the second parameter to match the `@cipherstash/auth` v0.39.0+ API.
auxesis
reviewed
Jun 24, 2026
auxesis
left a comment
auxesis
left a comment
Contributor
There was a problem hiding this comment.
Test-coverage review — PR #497 (protect-ffi 0.26 / auth 0.39, strategy-based lock context)
Verdict: Strong test coverage for the core change. The new lock-context-wiring.test.ts exhaustively asserts that every operation forwards identityClaim and never re-introduces serviceToken, and init-strategy.test.ts covers config.strategy forwarding well. Two gaps are clearly worth closing: the PR's headline auth-strategy re-exports have no test guarding the deliberate ESM workaround, and the new central resolveLockContext branch has no direct unit test that distinguishes a constructed claim from the default.
No crypto/security concerns to flag — the change removes the per-operation CTS token entirely and moves auth onto the client strategy, which is a design decision reviewed elsewhere.
Additional coverage gaps not posted inline
packages/stack/src/identity/index.ts:187—getLockContext()'s contract changed: the guard that threw when no CTS token was set has been removed, andctsTokenmay now beundefined. No test asserts the new non-throwing behaviour. It's a deprecated method (low priority), but a cheap regression test would lock it down:const r = await new LockContext().getLockContext(); expect(r.failure).toBeUndefined(); expect(r.data.ctsToken).toBeUndefined()(withCS_WORKSPACE_CRNset).packages/stack/src/wasm-inline.ts:374— the WASMEncryptionaccessKey+workspaceCrnpath now buildsAccessKeyStrategy.create(cfg.workspaceCrn, …)(region derived from the CRN, replacing the oldregionfield). This is only covered by the gated Deno e2e (e2e/wasm/roundtrip.test.ts), which skips without realCS_*secrets. There's no offline unit test — mirroringwasm-inline-normalize.test.ts— asserting the CRN reachesAccessKeyStrategy.create(or thatstrategy+accessKeytogether still throw).packages/stack/__tests__/lock-context-wiring.test.ts— the plain{ identityClaim }input (as opposed to aLockContextinstance) is only asserted forencrypt/decrypt; the model/bulk/query operations are exercised only with aLockContext. Low risk since they all funnel throughresolveLockContext, but the alternate-input axis is lopsided across the 11 operations.
| // instance type — so this works under real Node ESM, not just the bundler. | ||
| import auth from '@cipherstash/auth' | ||
|
|
||
| export const AccessKeyStrategy = auth.AccessKeyStrategy |
Contributor
There was a problem hiding this comment.
Gap: The four newly re-exported auth strategies (AccessKeyStrategy, AutoStrategy, DeviceSessionStrategy, OidcFederationStrategy) — the PR's headline "no separate @cipherstash/auth install" feature — have no test asserting they resolve to defined values. The explicit default-import + per-name re-export is a deliberate ESM workaround (the comment above notes a naive export { X } from '@cipherstash/auth' resolves to undefined/throws under real Node ESM). If it ever regresses, the names silently become undefined and OidcFederationStrategy.create(...) blows up only at call time, with nothing catching it at build/test.
import auth from '@cipherstash/auth'
import { describe, expect, it } from 'vitest'
import * as stack from '@/index'
describe('@cipherstash/stack auth strategy re-exports', () => {
it.each([
'AccessKeyStrategy',
'AutoStrategy',
'DeviceSessionStrategy',
'OidcFederationStrategy',
] as const)('re-exports %s as the real auth binding', (name) => {
// biome-ignore lint/suspicious/noExplicitAny: dynamic key lookup
const exported = (stack as any)[name]
expect(exported, `${name} re-export is undefined`).toBeDefined()
expect(typeof exported).toBe('function')
// biome-ignore lint/suspicious/noExplicitAny: dynamic key lookup
expect(exported).toBe((auth as any)[name])
})
})Expected: passes against the current explicit re-export; fails on toBeDefined() if it's swapped back to a plain export { … } from '@cipherstash/auth' that goes undefined under Node ESM. (init-strategy.test.ts already imports @/index, so the native @cipherstash/auth binding loads fine in this harness.)
| * Resolve a {@link LockContextInput} to the {@link Context} (identity claim) | ||
| * that protect-ffi expects. Synchronous — no token round-trip. | ||
| */ | ||
| export function resolveLockContext(input: LockContextInput): Context { |
Contributor
There was a problem hiding this comment.
Gap: resolveLockContext is the new synchronous branch that replaced the old await getLockContext() flow in every operation, but it has no direct unit test. Both branches are only exercised indirectly via new LockContext(), whose context is the default { identityClaim: ['sub'] } — so a regression where the LockContext branch returns a hardcoded ['sub'] (instead of the claim actually constructed) would pass every existing test, including lock-context-wiring.test.ts.
import { beforeAll, describe, expect, it } from 'vitest'
import { LockContext, resolveLockContext } from '@/identity'
describe('resolveLockContext', () => {
beforeAll(() => {
// LockContext's constructor resolves the workspace id from the env.
process.env.CS_WORKSPACE_CRN = 'crn:ap-southeast-2.aws:test-workspace'
})
it('returns a plain Context input unchanged', () => {
const ctx = { identityClaim: ['email'] }
expect(resolveLockContext(ctx)).toBe(ctx)
})
it('extracts the constructed claim from a LockContext (not the default)', () => {
const lc = new LockContext({ context: { identityClaim: ['email'] } })
expect(resolveLockContext(lc)).toEqual({ identityClaim: ['email'] })
})
})Expected: both pass today; the second fails if identityContext/resolveLockContext ever returns the default ['sub'] rather than the claim the LockContext was constructed with.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on top of #496 (
feat/stack-wasm-inline) — review/merge that first; this PR's base is the #496 branch, notmain.Supersedes the earlier 0.25.0 work: bumps to
@cipherstash/protect-ffi@0.26.0and@cipherstash/auth@0.39.0, and uses the newOidcFederationStrategyto replace the lock-context token ceremony with a simpler, strategy-based approach for identity-bound encryption.1. Version bumps
@cipherstash/protect-ffi0.25.0→0.26.0. The public TypeScript API is identical to0.25.0(verified by diffing the publishedlib/index.d.cts);0.26.0is internal fixes only (per-isolate NeonChannelcleanup,try_catcharound the JSgetToken).@cipherstash/authcatalog (and the six platform entries)0.38.0→0.39.0, which addsOidcFederationStrategy.e2e/wasm/deno.jsonpins +pnpm-lock.yamlregenerated.2. Strategy-based, identity-bound encryption (replaces the ceremony)
protect-ffi 0.25 removed the per-operation
serviceToken, which left the oldLockContextceremony half-broken:identify()fetched a CTS token the operations no longer sent, so the request authenticated as the service whileidentityClaimasked ZeroKMS to bind to a user it couldn't verify.OidcFederationStrategyandidentityClaimcompose: the strategy federates the end user's OIDC JWT into a CTS token at the client level (so requests authenticate as the user), andidentityClaimstill selects which claim ZeroKMS bakes into the data-key tag..withLockContext()now accepts a plain{ identityClaim }(or aLockContext) and resolves the claim synchronously — no CTS token, noidentify()call.LockContext.identify()/getLockContext()are deprecated (kept for back-compat); the client strategy handles token acquisition.config.strategyand existing credential/env behaviour is unchanged; existing.withLockContext(lockContext)call sites still compile.3. Strategy re-exports
OidcFederationStrategy,AccessKeyStrategy,AutoStrategy,DeviceSessionStrategyre-exported from@cipherstash/stack;OidcFederationStrategy/AccessKeyStrategyfrom@cipherstash/stack/wasm-inline(+ the wasmconfig.strategytype broadened to accept either). Integrators no longer need a separate@cipherstash/authinstall.@cipherstash/stacknow declares the@cipherstash/auth-<platform>packages asoptionalDependencies—@cipherstash/authships them as optional peer deps (not auto-installed), so this is required for the re-exported Node strategies to resolve their native binding for consumers.Also updated
lock-context-wiring.test.ts— asserts both aLockContextand a plain{ identityClaim }forwardidentityClaim, andserviceTokenis still never sent.init-strategy.test.ts— adds anOidcFederationStrategy-shaped forwarding case.lock-context.test.ts— live (USER_JWT-gated) round-trip rewritten to useOidcFederationStrategy+.withLockContext({ identityClaim }), confirming per-user binding.Encryption(),LockContext,ClientConfig.strategy),AGENTS.md,README.md, changeset.Summary by CodeRabbit
Release Notes
New Features
OidcFederationStrategyfor identity-bound encryption.Documentation
CS_WORKSPACE_CRN(replacesCS_REGION).Deprecations
LockContext.identify()andgetLockContext()deprecated (kept for backward compatibility).Chores