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: resolve 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).
| async verify(token: string): Promise<Result<VerifiedClaimsResult>> { | ||
| const { keys, urls } = this.keysAndUrls().Ok(); | ||
|
|
There was a problem hiding this comment.
Missing error check before calling .Ok() on keysAndUrls() result.
If keysAndUrls() returns an error (e.g., env resolution fails), calling .Ok() directly will throw or return undefined, leading to unexpected behavior.
Proposed fix
async verify(token: string): Promise<Result<VerifiedClaimsResult>> {
- const { keys, urls } = this.keysAndUrls().Ok();
+ const rKeysAndUrls = this.keysAndUrls();
+ if (rKeysAndUrls.isErr()) {
+ return Result.Err(rKeysAndUrls.Err());
+ }
+ const { keys, urls } = rKeysAndUrls.Ok();📝 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.
| async verify(token: string): Promise<Result<VerifiedClaimsResult>> { | |
| const { keys, urls } = this.keysAndUrls().Ok(); | |
| async verify(token: string): Promise<Result<VerifiedClaimsResult>> { | |
| const rKeysAndUrls = this.keysAndUrls(); | |
| if (rKeysAndUrls.isErr()) { | |
| return Result.Err(rKeysAndUrls.Err()); | |
| } | |
| const { keys, urls } = rKeysAndUrls.Ok(); |
🤖 Prompt for AI Agents
In @core/protocols/dashboard/token.ts around lines 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.
| async verify(token: string): Promise<Result<VerifiedClaimsResult>> { | ||
| const verify = new DeviceIdVerifyMsg(this.sthis.txt.base64, [(await this.opts.deviceIdCA.caCertificate()).Ok()], { | ||
| maxAge: 3600, | ||
| ...this.opts, | ||
| }); | ||
| const res = await verify.verifyWithCertificate(token, FPDeviceIDSessionSchema); |
There was a problem hiding this comment.
Missing error check on caCertificate() result.
The .Ok() is called without verifying the result isn't an error. If caCertificate() fails, this will cause unexpected behavior.
Proposed fix
async verify(token: string): Promise<Result<VerifiedClaimsResult>> {
- const verify = new DeviceIdVerifyMsg(this.sthis.txt.base64, [(await this.opts.deviceIdCA.caCertificate()).Ok()], {
+ const rCaCert = await this.opts.deviceIdCA.caCertificate();
+ if (rCaCert.isErr()) {
+ return Result.Err(rCaCert.Err());
+ }
+ const verify = new DeviceIdVerifyMsg(this.sthis.txt.base64, [rCaCert.Ok()], {
maxAge: 3600,
...this.opts,
});🤖 Prompt for AI Agents
In @core/protocols/dashboard/token.ts around lines 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.
| const creatingUser = (res.certificate.certificate.asCert() as unknown as { creatingUser: ClerkVerifiedAuth }).creatingUser; | ||
| // console.log("DeviceIdApiToken-verify", Object.keys(res.certificate.certificate.asCert())) | ||
| // console.log("DeviceIdApiToken-verify", creatingUser); | ||
| if (!creatingUser || creatingUser.type !== "clerk") { | ||
| return Result.Err(`DeviceIdApiToken-verify: unsupported creatingUser type: ${creatingUser}`); | ||
| } |
There was a problem hiding this comment.
Unsafe type assertion with as unknown as.
The double assertion as unknown as { creatingUser: ClerkVerifiedAuth } bypasses type safety. If the certificate structure doesn't match expectations, creatingUser could be undefined or malformed, causing silent failures or runtime errors downstream.
Consider validating the structure before accessing creatingUser, or use a Zod schema to parse and validate the certificate content.
🤖 Prompt for AI Agents
In @core/protocols/dashboard/token.ts around lines 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.
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.
| public_meta: `{ "role": "tester-${userId}" }`, | ||
| }, | ||
|
|
||
| role: "devide-id", |
There was a problem hiding this comment.
Fix typo: "devide-id" should be "device-id".
This typo in the role field could cause confusion when debugging or if any code matches against this value.
Proposed fix
- role: "devide-id",
+ role: "device-id",📝 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.
| role: "devide-id", | |
| role: "device-id", |
🤖 Prompt for AI Agents
In @core/device-id/create-test-user.ts at 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.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.