move dashboard token checker in protocol impl#1535
Conversation
WalkthroughCentralizes dashboard token verification into new core modules, adds a required Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Dashboard
participant TokenAPI
participant ClerkSTS
participant DeviceIdCA
participant DB
Client->>Dashboard: Request with token
Dashboard->>TokenAPI: verify(token, { clockTolerance, deviceIdCA })
alt Clerk token
TokenAPI->>ClerkSTS: fetch keys & verify JWT
ClerkSTS-->>TokenAPI: verified claims
TokenAPI-->>Dashboard: VerifiedClaimsResult(type: clerk, claims)
else Device-ID token
TokenAPI->>DeviceIdCA: DeviceIdVerifyMsg.verify(token, deviceIdCA)
DeviceIdCA-->>TokenAPI: verified claims
TokenAPI-->>Dashboard: VerifiedClaimsResult(type: device-id, claims)
end
Dashboard->>DB: proceed with verified identity
DB-->>Dashboard: result
Dashboard-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![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
🤖 Fix all issues with AI agents
In @core/protocols/dashboard/token.ts:
- Around line 117-122: The double assertion on
res.certificate.certificate.asCert() that creates creatingUser is unsafe and can
hide malformed certificate data; replace the `as unknown as { creatingUser:
ClerkVerifiedAuth }` with explicit validation: parse the cert payload (using a
Zod/schema or a type-guard function) to ensure it has a creatingUser property
and that creatingUser.type === "clerk" before using it, and return a Result.Err
with a clear message if validation fails; update the check around creatingUser
and the error branch labeled "DeviceIdApiToken-verify" to handle missing/invalid
shapes rather than assuming the cast succeeded.
- Around line 110-115: The verify method calls (await
this.opts.deviceIdCA.caCertificate()).Ok() without checking for an error result;
update verify to await this.opts.deviceIdCA.caCertificate(), check whether the
returned Result is Ok before calling .Ok(), handle the Err case (return a failed
Result or throw as per surrounding convention), and only pass the extracted
certificate into new DeviceIdVerifyMsg and subsequent
verify.verifyWithCertificate(FPDeviceIDSessionSchema) when Ok; reference the
verify function, this.opts.deviceIdCA.caCertificate(), DeviceIdVerifyMsg,
verify.verifyWithCertificate, and FPDeviceIDSessionSchema when making the
change.
- Around line 50-52: The call in verify currently assumes keysAndUrls()
succeeded by calling .Ok() directly; instead check the Result returned by
keysAndUrls() for an error before accessing .Ok(): call this.keysAndUrls(),
store the Result, if .Err() return or propagate a suitable error Result from
verify, otherwise extract .Ok() and continue; update the verify method to handle
the error path and avoid calling .Ok() on a failed Result.
🧹 Nitpick comments (5)
core/protocols/dashboard/token.ts (1)
15-48: Infinite loop pattern is valid but consider a more idiomatic approach.The
for (let idx = 0; true; idx++)pattern works correctly with the break condition, but awhile (true)or extracting to a generator might be clearer. The static analysis tool flagged this as a constant condition.This is a minor style consideration and the current implementation is functionally correct.
dashboard/backend/tests/auth-handler.test.ts (1)
88-89: Consider completing the device-id auth test.Given this PR introduces
DeviceIdApiTokensupport in the centralized token API, consider implementing this test to verify the device-id authentication path works correctly throughverifyAuth.Would you like me to help draft a test case for device-id authentication?
dashboard/backend/utils/auth.ts (1)
33-50: Minor: Typo in function namecorercedVerifiedAuthUser.The function name has a typo - should likely be
coercedVerifiedAuthUser. The logic correctly normalizes bothdevice-idandclerkauth types to a unifiedclerkverified auth structure.💅 Suggested fix
-export function corercedVerifiedAuthUser(ver: VerifiedClaimsResult): Result<VerifiedAuthResult["verifiedAuth"]> { +export function coercedVerifiedAuthUser(ver: VerifiedClaimsResult): Result<VerifiedAuthResult["verifiedAuth"]> {Note: This would require updating call sites (line 61 in this file).
dashboard/backend/types.ts (1)
38-52: Consider removing commented-out code.These commented blocks appear to be remnants from the type migration. If they're no longer needed, consider removing them to improve code clarity.
🧹 Suggested cleanup
-// return (obj as VerifiedAuthUser<T>).user !== undefined; -// } - -// export type ActiveUser<T extends DashAuthType = ClerkVerifyAuth> = WithVerifiedAuth<T> | VerifiedAuthUser<T>; - export type ReqWithVerifiedAuthUser<REQ extends { type: string; auth: DashAuthType }> = Omit<REQ, "auth"> & { readonly auth: VerifiedAuthUserResult; }; - -// export type ActiveUserWithUserId<T extends DashAuthType = ClerkVerifyAuth> = Omit<ActiveUser<T>, "user"> & { -// user: { -// userId: string; -// maxTenants: number; -// }; -// };dashboard/backend/create-handler.ts (1)
278-297: Minor: Redundant environment variable validation.The environment variables
DEVICE_ID_CA_PRIV_KEYandDEVICE_ID_CA_CERTare validated at line 278-283, thendeviceIdCAFromEnv(line 294) reads them again internally. While this works correctly, you could simplify by relying ondeviceIdCAFromEnv's built-in validation, or alternatively remove the device-id env vars from the initial check since they'll be validated during CA construction.♻️ Optional simplification
const rEnvVals = sthis.env.gets({ CLOUD_SESSION_TOKEN_PUBLIC: param.REQUIRED, CLERK_PUBLISHABLE_KEY: param.REQUIRED, - DEVICE_ID_CA_PRIV_KEY: param.REQUIRED, - DEVICE_ID_CA_CERT: param.REQUIRED, });This would let
deviceIdCAFromEnvhandle its own validation. However, keeping the explicit check provides clearer early-failure behavior, so this is purely optional.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
core/device-id/device-id-protocol.tscore/protocols/dashboard/index.tscore/protocols/dashboard/package.jsoncore/protocols/dashboard/token.tscore/tests/runtime/device-id.test.tscore/types/device-id/index.tscore/types/protocols/dashboard/index.tscore/types/protocols/dashboard/token.tsdashboard/backend/api.tsdashboard/backend/create-handler.tsdashboard/backend/internal/add-token-by-result-id.tsdashboard/backend/public/ensure-cloud-token.tsdashboard/backend/public/ensure-user.tsdashboard/backend/tests/auth-handler.test.tsdashboard/backend/types.tsdashboard/backend/utils/auth.tsdashboard/backend/utils/index.ts
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1515
File: dashboard/frontend/src/helpers.ts:2-2
Timestamp: 2026-01-09T21:46:15.277Z
Learning: In the fireproof monorepo, dashboard protocol types have been extracted into fireproof/core-types-protocols-dashboard while implementation code remains in fireproof/core-protocols-dashboard. Both packages are needed: the types package contains pure type definitions (LedgerUser, UserTenant, schemas), and the protocols package contains implementation classes (DashboardApiImpl, Api, clerkDashApi). This is an intentional architectural split, not an incomplete migration.
📚 Learning: 2026-01-09T21:46:15.277Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1515
File: dashboard/frontend/src/helpers.ts:2-2
Timestamp: 2026-01-09T21:46:15.277Z
Learning: In the fireproof monorepo, dashboard protocol types have been extracted into fireproof/core-types-protocols-dashboard while implementation code remains in fireproof/core-protocols-dashboard. Both packages are needed: the types package contains pure type definitions (LedgerUser, UserTenant, schemas), and the protocols package contains implementation classes (DashboardApiImpl, Api, clerkDashApi). This is an intentional architectural split, not an incomplete migration.
Applied to files:
core/protocols/dashboard/package.jsoncore/types/protocols/dashboard/index.tscore/types/protocols/dashboard/token.tsdashboard/backend/tests/auth-handler.test.tsdashboard/backend/api.tsdashboard/backend/utils/index.tsdashboard/backend/public/ensure-user.tscore/protocols/dashboard/token.tsdashboard/backend/internal/add-token-by-result-id.tsdashboard/backend/public/ensure-cloud-token.tscore/protocols/dashboard/index.tsdashboard/backend/create-handler.tsdashboard/backend/types.tsdashboard/backend/utils/auth.ts
📚 Learning: 2025-09-09T19:39:41.805Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: core/device-id/device-id-key.ts:18-29
Timestamp: 2025-09-09T19:39:41.805Z
Learning: In core/device-id/device-id-key.ts, mabels prefers using Zod's safeParse method for JWK validation instead of direct parse() calls, as it provides better error handling and aligns with the codebase's Result pattern.
Applied to files:
core/protocols/dashboard/package.jsoncore/types/device-id/index.tscore/protocols/dashboard/token.tscore/tests/runtime/device-id.test.tsdashboard/backend/create-handler.tscore/device-id/device-id-protocol.ts
📚 Learning: 2025-09-09T19:28:46.942Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: core/device-id/device-id-client.ts:41-48
Timestamp: 2025-09-09T19:28:46.942Z
Learning: In core/device-id/device-id-client.ts, the design intent is to consistently get the DeviceId key from deviceIdResult regardless of whether it was just created or already existed, ensuring the key always comes from the stored deviceId for consistency.
Applied to files:
core/types/device-id/index.tscore/tests/runtime/device-id.test.tsdashboard/backend/create-handler.tscore/device-id/device-id-protocol.ts
📚 Learning: 2025-09-09T19:32:49.609Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: core/device-id/device-id-CSR.ts:14-15
Timestamp: 2025-09-09T19:32:49.609Z
Learning: In core/device-id/device-id-CSR.ts, the Extensions type already has all properties defined as optional in the ExtensionsSchema Zod definition, so `extensions: Extensions = {}` is valid TypeScript and does not need to be changed to `Partial<Extensions>`.
Applied to files:
core/types/device-id/index.tscore/tests/runtime/device-id.test.tscore/device-id/device-id-protocol.ts
📚 Learning: 2025-09-09T19:56:31.545Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: core/device-id/device-id-protocol.ts:47-60
Timestamp: 2025-09-09T19:56:31.545Z
Learning: In the Result type from adviser/cement, Result.Err can accept Result objects directly without needing to unwrap them with .Err(). The codebase consistently uses Result.Err(resultObject) pattern, as seen in core/keybag/key-bag.ts lines 94 and 102.
Applied to files:
dashboard/backend/internal/add-token-by-result-id.ts
📚 Learning: 2025-09-10T09:53:26.979Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: use-fireproof/react/use-attach.ts:186-199
Timestamp: 2025-09-10T09:53:26.979Z
Learning: In use-fireproof/react/use-attach.ts, the current attach flow calls database.attach immediately via KeyedResolvOnce, which can run before a token is available and won't re-run after resetToken. The attach logic should be moved into the onTokenChange success path to ensure it only runs when a valid token exists and can re-attach after token reset.
Applied to files:
dashboard/backend/public/ensure-cloud-token.ts
📚 Learning: 2025-09-09T19:57:45.468Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: core/device-id/device-id-signed-msg.ts:17-18
Timestamp: 2025-09-09T19:57:45.468Z
Learning: In core/device-id/device-id-signed-msg.ts, mabels prefers using generic type constraints like `<T extends JWTPayload>` rather than direct type usage when maintaining type flexibility is beneficial, particularly for methods that need to preserve input type information while ensuring proper constraints.
Applied to files:
core/tests/runtime/device-id.test.tscore/device-id/device-id-protocol.ts
📚 Learning: 2025-09-09T19:26:15.184Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: core/device-id/device-id-client.ts:24-25
Timestamp: 2025-09-09T19:26:15.184Z
Learning: In core/device-id/device-id-client.ts, the global ResolveOnce singleton (onceDeviceId) is intentionally designed to be shared across DeviceIdClient instances because a device ID should represent a single, consistent identity per runtime environment.
Applied to files:
core/tests/runtime/device-id.test.tsdashboard/backend/create-handler.ts
🧬 Code graph analysis (2)
core/types/protocols/dashboard/token.ts (2)
core/types/protocols/dashboard/msg-types.ts (1)
DashAuthType(75-78)core/types/base/fp-clerk-claim.zod.ts (1)
ClerkClaim(47-47)
dashboard/backend/create-handler.ts (3)
core/protocols/dashboard/token.ts (2)
deviceIdCAFromEnv(134-154)tokenApi(156-167)core/base/ledger.ts (1)
sthis(113-115)dashboard/backend/api.ts (1)
createFPApiSQLCtx(8-24)
🪛 Biome (2.1.2)
core/protocols/dashboard/token.ts
[error] 19-19: Unexpected constant condition.
(lint/correctness/noConstantCondition)
🔇 Additional comments (17)
core/protocols/dashboard/index.ts (1)
3-3: LGTM!The re-export follows the established pattern and correctly exposes the token verification implementation from the protocols package. This aligns with the architectural split where implementation code lives in
core-protocols-dashboard. Based on learnings, this is the correct location for implementation exports.core/device-id/device-id-protocol.ts (1)
50-54: LGTM!The
deviceIdCAis correctly passed toDeviceIdVerifyMsgafter the error checks ensurerCais valid. This properly wires the CA certificate into the verification flow, enabling certificate-based message verification. The clock tolerance (60s) and max age (1 hour) are sensible defaults.dashboard/backend/internal/add-token-by-result-id.ts (1)
2-5: LGTM!The import reorganization correctly moves
TokenByResultIdParamto the centralized@fireproof/core-types-protocols-dashboardpackage alongsideResTokenByResultId. This aligns with the architectural split where types are extracted into the types package. Based on learnings, this is the intended pattern for the monorepo.core/types/device-id/index.ts (1)
94-98: Breaking change:deviceIdCAis now required inVerifyWithCertificateOptions.Adding a required field to an existing interface is a breaking change. All consumers have been properly updated:
core/device-id/device-id-protocol.ts,dashboard/backend/create-handler.ts, and all test cases incore/tests/runtime/device-id.test.tssupply the newdeviceIdCAfield alongside the existing required fields.core/protocols/dashboard/package.json (1)
44-48: jose version ^6.1.3 is current and secure.The dependency is at the latest released version on npm with no known security vulnerabilities affecting v6.x. Reported CVEs (CVE-2024-28176, CVE-2024-28180) only impact v2.x, v3.x, and v4.x; v5.x and later are unaffected.
core/tests/runtime/device-id.test.ts (1)
425-429: LGTM! Consistent propagation ofdeviceIdCAoption.The
deviceIdCAparameter is correctly passed to allDeviceIdVerifyMsgconstructor calls throughout the test file. This aligns with the updated public API that now requiresdeviceIdCAinVerifyWithCertificateOptions.core/types/protocols/dashboard/token.ts (1)
1-56: Well-structured type definitions for the token verification API.The type hierarchy is clean and follows the established types/protocols split pattern in this codebase. The type guards
isVerifiedAuthandisVerifiedAuthUserprovide proper runtime type narrowing. Based on learnings, this aligns with the architectural decision to keep pure type definitions infireproof/core-types-protocols-dashboard.core/types/protocols/dashboard/index.ts (1)
5-6: LGTM!Standard barrel export pattern, making token types available through the package's public API.
dashboard/backend/public/ensure-user.ts (1)
2-17: LGTM! Import reorganization aligns with centralized types architecture.The imports are correctly sourced from
@fireproof/core-types-protocols-dashboardfollowing the types/protocols split pattern. Based on learnings, this is the intended architecture where types are in the types package and implementations remain in the protocols package.dashboard/backend/api.ts (1)
1-6: LGTM!Import paths correctly updated to use centralized types from
@fireproof/core-types-protocols-dashboard. The function implementation remains unchanged.dashboard/backend/utils/index.ts (1)
6-7: LGTM!Import reorganization correctly sources
VerifiedAuthUserResultfrom the centralized types package while keeping local types from../types.js.dashboard/backend/public/ensure-cloud-token.ts (1)
2-12: LGTM! Import restructuring aligns with the types/implementation architectural split.The imports correctly source public types (
VerifiedAuthUserResult,ReqEnsureCloudToken, etc.) from@fireproof/core-types-protocols-dashboardwhile keeping local implementation types (FPApiSQLCtx,FPTokenContext,ReqWithVerifiedAuthUser) from../types.js. Based on learnings, this follows the intentional pattern where types reside incore-types-protocols-dashboardand implementations in separate packages.dashboard/backend/tests/auth-handler.test.ts (1)
11-11: LGTM! Import updated to use centralized type source.The
VerifiedClaimsResulttype is correctly sourced from the external types package, consistent with the PR's objective to centralize authentication types.dashboard/backend/utils/auth.ts (1)
2-11: LGTM! Import sources correctly reorganized.Public types (
VerifiedAuthResult,VerifiedClaimsResult,VerifiedResult,WithAuth) are sourced from the centralized types package, while implementation-specific types remain local.dashboard/backend/types.ts (1)
1-12: LGTM! Type imports correctly centralized.The authentication-related types (
VerifiedAuthUserResult,FPApiToken) are now sourced from@fireproof/core-types-protocols-dashboard, aligning with the PR's objective to centralize token verification types.dashboard/backend/create-handler.ts (2)
48-48: LGTM! Token API logic properly extracted to centralized module.The import of
deviceIdCAFromEnvandtokenApifrom@fireproof/core-protocols-dashboardaligns with the PR's objective to centralize token verification logic.
335-347: LGTM! Token API initialization correctly configured.The
tokenApiis properly awaited and configured with:
clockTolerance: 60- reasonable 1-minute tolerance for clock skew during token verificationdeviceIdCA- required for device-id token certificate verificationThe
deviceIdCAis correctly passed to bothtokenApi(for verification operations) andcreateFPApiSQLCtx(for other CA-dependent operations like CSR handling).
mabels
force-pushed
the
mabels/export-dash-token-check
branch
from
a672a6e to
2013871
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dashboard/backend/create-handler.ts (1)
333-345: Consider makingclockToleranceconfigurable.The hardcoded
clockTolerance: 60is a reasonable default for JWT clock skew tolerance, but consider making it configurable via environment variable for operational flexibility (e.g., in environments with known clock drift issues).♻️ Optional: Make clockTolerance configurable
+ const clockTolerance = coerceInt(env.JWT_CLOCK_TOLERANCE, 60); + const fpApiCtx = new AppContext().set( "fpApiCtx", createFPApiSQLCtx( sthis, db, await tokenApi(sthis, { - clockTolerance: 60, + clockTolerance, deviceIdCA: rDeviceIdCA.Ok(), }), rDeviceIdCA.Ok(), svcParams, ), );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/protocols/dashboard/get-cloud-pubkey-from-env.tscore/protocols/dashboard/index.tsdashboard/backend/create-handler.tsdashboard/backend/well-known-jwks.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- dashboard/backend/well-known-jwks.ts
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1515
File: dashboard/frontend/src/helpers.ts:2-2
Timestamp: 2026-01-09T21:46:15.277Z
Learning: In the fireproof monorepo, dashboard protocol types have been extracted into fireproof/core-types-protocols-dashboard while implementation code remains in fireproof/core-protocols-dashboard. Both packages are needed: the types package contains pure type definitions (LedgerUser, UserTenant, schemas), and the protocols package contains implementation classes (DashboardApiImpl, Api, clerkDashApi). This is an intentional architectural split, not an incomplete migration.
📚 Learning: 2026-01-09T21:46:15.277Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1515
File: dashboard/frontend/src/helpers.ts:2-2
Timestamp: 2026-01-09T21:46:15.277Z
Learning: In the fireproof monorepo, dashboard protocol types have been extracted into fireproof/core-types-protocols-dashboard while implementation code remains in fireproof/core-protocols-dashboard. Both packages are needed: the types package contains pure type definitions (LedgerUser, UserTenant, schemas), and the protocols package contains implementation classes (DashboardApiImpl, Api, clerkDashApi). This is an intentional architectural split, not an incomplete migration.
Applied to files:
core/protocols/dashboard/index.tscore/protocols/dashboard/get-cloud-pubkey-from-env.tsdashboard/backend/create-handler.ts
📚 Learning: 2025-09-09T19:39:41.805Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: core/device-id/device-id-key.ts:18-29
Timestamp: 2025-09-09T19:39:41.805Z
Learning: In core/device-id/device-id-key.ts, mabels prefers using Zod's safeParse method for JWK validation instead of direct parse() calls, as it provides better error handling and aligns with the codebase's Result pattern.
Applied to files:
core/protocols/dashboard/get-cloud-pubkey-from-env.tsdashboard/backend/create-handler.ts
📚 Learning: 2025-09-09T20:45:43.368Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: core/device-id/device-id-CA.ts:126-139
Timestamp: 2025-09-09T20:45:43.368Z
Learning: In core/device-id/device-id-key.ts, the exportPrivateJWK() method returns a JWKPrivate object (JWK format), not a KeyLike. When using this with jose's SignJWT.sign(), the JWK must be imported using importJWK() to convert it to a KeyLike that the signing method accepts.
Applied to files:
core/protocols/dashboard/get-cloud-pubkey-from-env.ts
📚 Learning: 2025-09-09T19:28:46.942Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: core/device-id/device-id-client.ts:41-48
Timestamp: 2025-09-09T19:28:46.942Z
Learning: In core/device-id/device-id-client.ts, the design intent is to consistently get the DeviceId key from deviceIdResult regardless of whether it was just created or already existed, ensuring the key always comes from the stored deviceId for consistency.
Applied to files:
dashboard/backend/create-handler.ts
📚 Learning: 2025-09-09T19:26:15.184Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: core/device-id/device-id-client.ts:24-25
Timestamp: 2025-09-09T19:26:15.184Z
Learning: In core/device-id/device-id-client.ts, the global ResolveOnce singleton (onceDeviceId) is intentionally designed to be shared across DeviceIdClient instances because a device ID should represent a single, consistent identity per runtime environment.
Applied to files:
dashboard/backend/create-handler.ts
🧬 Code graph analysis (1)
dashboard/backend/create-handler.ts (2)
core/protocols/dashboard/token.ts (2)
deviceIdCAFromEnv(134-154)tokenApi(156-167)dashboard/backend/api.ts (1)
createFPApiSQLCtx(8-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CI Core Publish
- GitHub Check: CI Core
🔇 Additional comments (4)
core/protocols/dashboard/get-cloud-pubkey-from-env.ts (1)
1-1: LGTM!Import consolidation is appropriate—
isArrayBuffer,isUint8Array, andResultare properly sourced from@adviser/cement. The function implementation follows good practices with comprehensive error handling and usessafeParsefor JWK validation. Based on learnings, this aligns with the codebase's Result pattern.dashboard/backend/create-handler.ts (2)
47-47: LGTM!Clean consolidation of token-related imports from
@fireproof/core-protocols-dashboard. This aligns with the PR objective of centralizing dashboard token verification into core protocol modules.
293-296: LGTM!Good refactoring to use centralized
deviceIdCAFromEnvinstead of inline construction. Error handling is properly preserved. The required environment variables are validated earlier (lines 280-281), so this will succeed if the handler initializes.core/protocols/dashboard/index.ts (1)
3-4: LGTM!New public exports for
token.jsandget-cloud-pubkey-from-env.jsfollow the existing export pattern and align with the architectural split where implementation code resides incore-protocols-dashboard. The modules exist and the exports are consistent with the package structure.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @core/device-id/create-test-user.ts:
- Line 48: In the object literal in create-test-user.ts where the role field is
set (role: "devide-id"), correct the typo by changing the string value to
"device-id" so any code matching or logging that role uses the correct
identifier.
🧹 Nitpick comments (4)
core/device-id/index.ts (1)
9-11: Consider separating test utilities from the main export.Exporting test helpers (
create-test-device-id.js,create-test-user.js) from the main package entry point exposes them to all consumers, which could lead to unintended usage in production code.A common pattern is to expose test utilities via a separate entry point (e.g.,
@fireproof/core-device-id/testing) to make the intent clearer. However, if the current approach is intentional for simplicity in your monorepo test setup, this is acceptable.core/device-id/create-test-user.ts (3)
25-26: Redundant key creation.The
devidkey is created and immediately re-exported/re-imported to createdevkey. SinceDeviceIdKey.create()already returns a usable key, this round-trip appears unnecessary.Proposed simplification
- const devid = await DeviceIdKey.create(); - const devkey = (await DeviceIdKey.createFromJWK(await devid.exportPrivateJWK())).Ok(); + const devkey = await DeviceIdKey.create();If there's a specific reason for this pattern (e.g., ensuring the key survives serialization/deserialization), please add a comment explaining the intent.
17-18: Global mutable state may cause race conditions in parallel tests.The
seqUserIdGlobaluses a shared mutable object withid++, which is not atomic. If tests run in parallel (e.g., with Vitest's parallel mode), multiple test workers could get duplicate or inconsistent user IDs.Consider passing the sequence explicitly or using an atomic counter pattern if parallel test execution is expected.
28-30: Missing error handling for CSR creation.The code calls
rCsrResult.Ok()without first checkingisErr(). IfcreateCSRfails, this will throw an unhelpful error.Proposed fix
const rCsrResult = await deviceIdCSR.createCSR({ commonName: "test-device-id" }); + if (rCsrResult.isErr()) { + throw new Error(`Failed to create CSR: ${rCsrResult.Err().message}`); + } const userId = `${session ?? sessionId(sthis)}-${seqUserId ?? seqUserIdGlobal(sthis).id++}`; const rProcessResult = await deviceCA.processCSR(rCsrResult.Ok(), {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/device-id/create-test-device-id.tscore/device-id/create-test-user.tscore/device-id/index.tsdashboard/backend/tests/auth-handler.test.tsdashboard/backend/tests/db-api.test.ts
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1515
File: dashboard/frontend/src/helpers.ts:2-2
Timestamp: 2026-01-09T21:46:15.277Z
Learning: In the fireproof monorepo, dashboard protocol types have been extracted into fireproof/core-types-protocols-dashboard while implementation code remains in fireproof/core-protocols-dashboard. Both packages are needed: the types package contains pure type definitions (LedgerUser, UserTenant, schemas), and the protocols package contains implementation classes (DashboardApiImpl, Api, clerkDashApi). This is an intentional architectural split, not an incomplete migration.
📚 Learning: 2025-09-09T19:28:46.942Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: core/device-id/device-id-client.ts:41-48
Timestamp: 2025-09-09T19:28:46.942Z
Learning: In core/device-id/device-id-client.ts, the design intent is to consistently get the DeviceId key from deviceIdResult regardless of whether it was just created or already existed, ensuring the key always comes from the stored deviceId for consistency.
Applied to files:
core/device-id/create-test-device-id.tscore/device-id/index.tscore/device-id/create-test-user.ts
📚 Learning: 2025-09-09T19:26:15.184Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: core/device-id/device-id-client.ts:24-25
Timestamp: 2025-09-09T19:26:15.184Z
Learning: In core/device-id/device-id-client.ts, the global ResolveOnce singleton (onceDeviceId) is intentionally designed to be shared across DeviceIdClient instances because a device ID should represent a single, consistent identity per runtime environment.
Applied to files:
core/device-id/create-test-device-id.tscore/device-id/index.ts
📚 Learning: 2025-09-09T19:32:49.609Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: core/device-id/device-id-CSR.ts:14-15
Timestamp: 2025-09-09T19:32:49.609Z
Learning: In core/device-id/device-id-CSR.ts, the Extensions type already has all properties defined as optional in the ExtensionsSchema Zod definition, so `extensions: Extensions = {}` is valid TypeScript and does not need to be changed to `Partial<Extensions>`.
Applied to files:
core/device-id/create-test-device-id.tscore/device-id/index.ts
📚 Learning: 2025-09-09T19:39:41.805Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: core/device-id/device-id-key.ts:18-29
Timestamp: 2025-09-09T19:39:41.805Z
Learning: In core/device-id/device-id-key.ts, mabels prefers using Zod's safeParse method for JWK validation instead of direct parse() calls, as it provides better error handling and aligns with the codebase's Result pattern.
Applied to files:
core/device-id/create-test-device-id.ts
📚 Learning: 2025-09-09T20:45:43.368Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: core/device-id/device-id-CA.ts:126-139
Timestamp: 2025-09-09T20:45:43.368Z
Learning: In core/device-id/device-id-key.ts, the exportPrivateJWK() method returns a JWKPrivate object (JWK format), not a KeyLike. When using this with jose's SignJWT.sign(), the JWK must be imported using importJWK() to convert it to a KeyLike that the signing method accepts.
Applied to files:
core/device-id/create-test-device-id.ts
📚 Learning: 2026-01-09T21:46:15.277Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1515
File: dashboard/frontend/src/helpers.ts:2-2
Timestamp: 2026-01-09T21:46:15.277Z
Learning: In the fireproof monorepo, dashboard protocol types have been extracted into fireproof/core-types-protocols-dashboard while implementation code remains in fireproof/core-protocols-dashboard. Both packages are needed: the types package contains pure type definitions (LedgerUser, UserTenant, schemas), and the protocols package contains implementation classes (DashboardApiImpl, Api, clerkDashApi). This is an intentional architectural split, not an incomplete migration.
Applied to files:
dashboard/backend/tests/auth-handler.test.ts
📚 Learning: 2025-09-09T19:56:31.545Z
Learnt from: mabels
Repo: fireproof-storage/fireproof PR: 1130
File: core/device-id/device-id-protocol.ts:47-60
Timestamp: 2025-09-09T19:56:31.545Z
Learning: In the Result type from adviser/cement, Result.Err can accept Result objects directly without needing to unwrap them with .Err(). The codebase consistently uses Result.Err(resultObject) pattern, as seen in core/keybag/key-bag.ts lines 94 and 102.
Applied to files:
dashboard/backend/tests/db-api.test.ts
🧬 Code graph analysis (1)
core/device-id/create-test-user.ts (7)
core/types/base/types.ts (1)
SuperThis(151-163)core/types/device-id/index.ts (1)
DeviceIdCAIf(106-136)core/device-id/device-id-key.ts (1)
DeviceIdKey(7-63)core/device-id/device-id-signed-msg.ts (1)
DeviceIdSignMsg(6-39)core/types/protocols/dashboard/msg-types.ts (1)
DashAuthType(75-78)core/device-id/device-id-CSR.ts (1)
DeviceIdCSR(6-59)core/types/base/fp-device-id-payload.zod.ts (1)
FPDeviceIDSession(123-123)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CI Core Publish
- GitHub Check: CI Core
🔇 Additional comments (5)
core/device-id/create-test-device-id.ts (1)
1-23: LGTM!The import source normalization from barrel imports to local module imports aligns with the PR's goal of centralizing device-id test utilities. The
createTestDeviceCAimplementation is clean and provides sensible defaults for test scenarios.dashboard/backend/tests/auth-handler.test.ts (1)
10-11: LGTM!Import sources correctly updated to use the externalized type and device-id packages. Based on learnings, this aligns with the intentional architectural split between
@fireproof/core-types-protocols-dashboard(pure type definitions) and@fireproof/core-device-id(implementation/utilities).dashboard/backend/tests/db-api.test.ts (2)
2-2: LGTM!Import changes properly consolidate test utilities from
@fireproof/core-device-idand update type annotations to match the newcreateTestUserreturn type.Also applies to: 20-20, 31-31
78-82: LGTM!All
createTestUsercall sites have been consistently updated to use the new parameter shape{ sthis, seqUserId, deviceCA }. The migration maintains test semantics while leveraging the centralized test utilities.Also applies to: 97-108, 955-956
core/device-id/create-test-user.ts (1)
52-52: Inconsistentaudfield type.The
aud(audience) field is set as an array["http://test-audience.localhost/"], while other test fixtures in the codebase (e.g.,auth-handler.test.tsline 16) use a string forazp. Verify that the expectedClerkClaimtype accepts an array foraud, as some JWT libraries expect a string.
mabels
force-pushed
the
mabels/export-dash-token-check
branch
from
81d3615 to
1c207b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@core/protocols/dashboard/token.ts`:
- Around line 19-20: The for-loop in token generation uses a constant condition
"for (let idx = 0; true; idx++)" which triggers Biome's noConstantCondition
lint; change it to the equivalent open condition form "for (let idx = 0;;
idx++)" inside core/protocols/dashboard/token.ts so the behavior (incrementing
idx and producing suffix via the suffix variable) remains the same but avoids
the lint error; update the loop header where suffix is computed (the block that
declares "const suffix = !idx ? \"\" : `_${idx}`") accordingly.
♻️ Duplicate comments (4)
core/device-id/create-test-user.ts (1)
48-48: Fix typo in role string (“devide-id”).✅ Proposed fix
- role: "devide-id", + role: "device-id",core/protocols/dashboard/token.ts (3)
50-52: HandlekeysAndUrls()errors before calling.Ok().
Line 51 assumes success and can throw on error.
110-115: CheckdeviceIdCA.caCertificate()result before.Ok().
Line 111 can throw if the CA fetch fails.
117-121: Replace unsafe double assertion on certificate payload.
Line 117 bypasses validation; malformed cert payloads could silently propagate. Consider a schema/type guard forcreatingUserbefore access.
🧹 Nitpick comments (1)
core/device-id/create-test-user.ts (1)
25-47: Handle Result errors explicitly before.Ok()access.
Current code assumes success fromcreateFromJWK,createCSR, andprocessCSR; explicit checks improve test failure clarity.♻️ Suggested error handling
- const devkey = (await DeviceIdKey.createFromJWK(await devid.exportPrivateJWK())).Ok(); + const rDevKey = await DeviceIdKey.createFromJWK(await devid.exportPrivateJWK()); + if (rDevKey.isErr()) { + throw new Error(`Failed to import device key: ${rDevKey.Err().message ?? rDevKey.Err()}`); + } + const devkey = rDevKey.Ok(); const deviceIdCSR = new DeviceIdCSR(sthis, devkey); const rCsrResult = await deviceIdCSR.createCSR({ commonName: "test-device-id" }); + if (rCsrResult.isErr()) { + throw new Error(`Failed to create CSR: ${rCsrResult.Err().message ?? rCsrResult.Err()}`); + } const userId = `${session ?? sessionId(sthis)}-${seqUserId ?? seqUserIdGlobal(sthis).id++}`; const rProcessResult = await deviceCA.processCSR(rCsrResult.Ok(), { azp: `test-app-${userId}-${sthis.nextId().str}`, exp: Math.floor(Date.now() / 1000) + 3600, iat: Math.floor(Date.now() / 1000), iss: "test-issuer", jti: sthis.nextId().str, nbf: Math.floor(Date.now() / 1000), params: { nick: `nick-${userId}`, email: `email-${userId}@example.com`, email_verified: true, first: `first-${userId}`, image_url: `http://example.com/image-${userId}.png`, last: `last-${userId}`, name: `name-${userId}`, public_meta: `{ "role": "tester-${userId}" }`, }, + }); + if (rProcessResult.isErr()) { + throw new Error(`Failed to process CSR: ${rProcessResult.Err().message ?? rProcessResult.Err()}`); });
…lock tolerance
- Add proper Result error propagation in ClerkApiToken.verify() and
DeviceIdApiToken.verify() instead of calling .Ok() on unchecked results
- Add CA_CLOCK_TOLERANCE env variable to configure token clock tolerance
(defaults to 60 seconds)
- Fix typo: corercedVerifiedAuthUser -> coercedVerifiedAuthUser
- Remove unused commented code from dashboard backend types
- Bump @adviser/cement from ^0.5.15 to ^0.5.17
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@cli/package.json`:
- Line 42: The dependency entry "@adviser/cement": "^0.5.17" is invalid; verify
the intended published version and update all occurrences: run `npm view
`@adviser/cement` versions` (or check internal registry) to find the correct
release (e.g., 0.5.7/0.5.5) or confirm it's a private package, then replace
"^0.5.17" with the valid version across package.json files (or add the private
registry/auth config if it's unpublished); search for the exact token
"@adviser/cement": "^0.5.17" to locate and update every file.
In `@core/protocols/dashboard/token.ts`:
- Around line 58-66: The parseSchema function currently logs the entire payload
on FPClerkClaimSchema parse failure which may expose PII; update parseSchema
(the FPClerkClaimSchema.safeParse error branch) to stop printing the raw payload
and instead log only the validation error (r.error) or a redacted/minimal
identifier, then return Result.Err(r.error) as before; ensure references are to
parseSchema, FPClerkClaimSchema, and Result.Ok/Result.Err so reviewers can find
the change.
In `@dashboard/frontend/package.json`:
- Line 28: The dependency entry for "@adviser/cement": "^0.5.17" is invalid
because that version is not on the public npm registry; locate the package.json
dependency line for "@adviser/cement" and either (a) correct the version to a
published release (e.g. "^0.4.32") or (b) add/confirm the private registry
resolution (npmrc or .npmrc registry/auth entries and/or package-lock overrides)
that hosts 0.5.17, then run npm install to verify; ensure the change updates
package.json and any lockfile (package-lock.json/yarn.lock) accordingly.
♻️ Duplicate comments (1)
core/protocols/dashboard/token.ts (1)
18-20: Biome noConstantCondition still applies here.
Consider switching tofor (let idx = 0;; idx++)or suppressing the lint in a project-wide, documented way.
🧹 Nitpick comments (1)
core/device-id/create-test-user.ts (1)
25-30: Missing error handling onResulttypes.Multiple calls to
.Ok()without checking.isErr()first:
- Line 26:
DeviceIdKey.createFromJWK()returnsResult<DeviceIdKey>- Line 28:
createCSR()returnsResult<string>- Line 30:
processCSR()returnsResult<...>If any operation fails, calling
.Ok()will throw or returnundefined, making test failures harder to diagnose.♻️ Suggested improvement with error handling
const devid = await DeviceIdKey.create(); - const devkey = (await DeviceIdKey.createFromJWK(await devid.exportPrivateJWK())).Ok(); + const rDevkey = await DeviceIdKey.createFromJWK(await devid.exportPrivateJWK()); + if (rDevkey.isErr()) { + throw new Error(`Failed to create devkey: ${rDevkey.Err().message}`); + } + const devkey = rDevkey.Ok(); const deviceIdCSR = new DeviceIdCSR(sthis, devkey); const rCsrResult = await deviceIdCSR.createCSR({ commonName: "test-device-id" }); + if (rCsrResult.isErr()) { + throw new Error(`Failed to create CSR: ${rCsrResult.Err().message}`); + }
| }, | ||
| "dependencies": { | ||
| "@adviser/cement": "^0.5.15", | ||
| "@adviser/cement": "^0.5.17", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@adviser/cement 0.5.17 JSR package
💡 Result:
I couldn't find a published @adviser/cement version 0.5.17. JSR shows the package (example listed at 0.5.0) and libraries.io shows published versions up to 0.4.32 — but no 0.5.17 entry. Sources: JSR package page and libraries.io. [1][2]
Sources:
[1] JSR — @adviser/cement package page.
[2] Libraries.io — @adviser/cement (npm) package page.
🌐 Web query:
@adviser/cement 0.5.17 changelog release notes
💡 Result:
I searched but couldn't find release notes or a changelog entry specifically for @adviser/cement version 0.5.17. The package pages I found list other versions (for example 0.5.0 and 0.5.5) but not 0.5.17. Sources checked: JSR package page and jsDelivr/Libraries.io entries. [1][2]
Would you like me to:
- look directly in the package's GitHub repo (if you want, send the repo link or I can search for it), or
- try npm registry/history snapshots for unpublished/private releases?
Sources:
- JSR package page for
@adviser/cement. [1] - jsDelivr / Libraries.io package listings for
@adviser/cement. [2]
🏁 Script executed:
# Find lock files to see resolved versions
fd -e lock -e lockfile | head -20Repository: fireproof-storage/fireproof
Length of output: 53
🏁 Script executed:
# Check current package.json structure and line 42 context
cat -n cli/package.json | sed -n '35,50p'Repository: fireproof-storage/fireproof
Length of output: 791
🏁 Script executed:
# Search for any references to `@adviser/cement` versions in the repo
rg "@adviser/cement" --type jsonRepository: fireproof-storage/fireproof
Length of output: 2886
@adviser/cement 0.5.17 does not exist in public registries and will block dependency resolution.
Web search confirms this version is not published on JSR or npm. The codebase specifies ^0.5.17 across 30+ package.json files, which will cause dependency resolution failures on install. Verify the intended version: check if this should be 0.5.7, 0.5.5, or another existing release, or confirm this is a private/unpublished version before merging.
🤖 Prompt for AI Agents
In `@cli/package.json` at line 42, The dependency entry "@adviser/cement":
"^0.5.17" is invalid; verify the intended published version and update all
occurrences: run `npm view `@adviser/cement` versions` (or check internal
registry) to find the correct release (e.g., 0.5.7/0.5.5) or confirm it's a
private package, then replace "^0.5.17" with the valid version across
package.json files (or add the private registry/auth config if it's
unpublished); search for the exact token "@adviser/cement": "^0.5.17" to locate
and update every file.
| parseSchema: (payload: unknown): Result<FPClerkClaim> => { | ||
| const r = FPClerkClaimSchema.safeParse(payload); | ||
| if (r.success) { | ||
| return Result.Ok(r.data); | ||
| } else { | ||
| // eslint-disable-next-line no-console | ||
| console.log("FPClerkClaimSchema parse error", payload, r.error); | ||
| return Result.Err(r.error); | ||
| } |
There was a problem hiding this comment.
Avoid logging raw Clerk claims in parse failures.
The payload likely includes PII (email/name). Logging it can violate privacy/compliance requirements. Prefer logging only the error (or a redacted subset).
♻️ Suggested change
- // eslint-disable-next-line no-console
- console.log("FPClerkClaimSchema parse error", payload, r.error);
+ // Avoid logging claims payload; it may contain PII.
+ // eslint-disable-next-line no-console
+ console.log("FPClerkClaimSchema parse error", r.error);🤖 Prompt for AI Agents
In `@core/protocols/dashboard/token.ts` around lines 58 - 66, The parseSchema
function currently logs the entire payload on FPClerkClaimSchema parse failure
which may expose PII; update parseSchema (the FPClerkClaimSchema.safeParse error
branch) to stop printing the raw payload and instead log only the validation
error (r.error) or a redacted/minimal identifier, then return
Result.Err(r.error) as before; ensure references are to parseSchema,
FPClerkClaimSchema, and Result.Ok/Result.Err so reviewers can find the change.
| }, | ||
| "dependencies": { | ||
| "@adviser/cement": "^0.5.15", | ||
| "@adviser/cement": "^0.5.17", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify `@adviser/cement` version 0.5.17 exists and check for security issues
echo "=== Checking if `@adviser/cement`@0.5.17 exists ==="
curl -s "https://registry.npmjs.org/@adviser/cement/0.5.17" | jq -r '.version // "Version not found"' 2>/dev/null || echo "Failed to fetch version info"
echo -e "\n=== Latest `@adviser/cement` version ==="
curl -s "https://registry.npmjs.org/@adviser/cement" | jq -r '.["dist-tags"].latest' 2>/dev/null || echo "Failed to fetch latest version"
echo -e "\n=== Full version information ==="
curl -s "https://registry.npmjs.org/@adviser/cement" | jq -r '.versions | keys | sort -V | tail -5' 2>/dev/null || echo "Failed to fetch versions list"Repository: fireproof-storage/fireproof
Length of output: 198
🌐 Web query:
@adviser/cement npm package version 0.5.17
💡 Result:
I can't find @adviser/cement@0.5.17 on the public npm registry. Libraries.io lists releases (latest shown 0.4.32), and a npm semver query returns no matching versions. [1][2]
If you need, tell me whether you want me to:
- search a private registry or GitHub for a tagged 0.5.17, or
- show how to install the latest public version.
Sources:
[1] Libraries.io — @adviser/cement package page (shows versions).
[2] npm semver search (no matching versions found).
@adviser/cement@0.5.17 does not exist on the public npm registry.
The version being pinned (0.5.17) cannot be found on npm. The latest public version is 0.4.32. Verify whether this package is sourced from a private registry or if the version number should be corrected. Without resolution, dependency installation will fail.
🤖 Prompt for AI Agents
In `@dashboard/frontend/package.json` at line 28, The dependency entry for
"@adviser/cement": "^0.5.17" is invalid because that version is not on the
public npm registry; locate the package.json dependency line for
"@adviser/cement" and either (a) correct the version to a published release
(e.g. "^0.4.32") or (b) add/confirm the private registry resolution (npmrc or
.npmrc registry/auth entries and/or package-lock overrides) that hosts 0.5.17,
then run npm install to verify; ensure the change updates package.json and any
lockfile (package-lock.json/yarn.lock) accordingly.
1 participant
Summary by CodeRabbit
New Features
Refactor
New Public APIs
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.