add token based authantication#42
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds token-based API endpoints and client pages for Next/Nuxt/SvelteKit, globalizes model/morph registries on globalThis and preloads discovered model modules, refactors auth runtime and contracts for guard-scoped token/session behavior, updates CLI/type generation and adapters, extends tests and docs, and adjusts seeders/blog libs password hashing. ChangesCombined PR checkpoint
Estimated code review effort: Possibly related PRs:
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/blog-sveltekit/src/routes/api-token-posts/+page.svelte (1)
73-75: ⚡ Quick winConsider using standard
Authorizationheader casing.The HTTP convention is to use
Authorizationwith a capital 'A'. While headers are case-insensitive per the HTTP specification, using standard casing improves consistency with documentation and avoids potential issues with non-compliant middleware or proxies.📝 Proposed fix
const response = await fetch('/api/v1/posts', { - headers: token ? { authorization: `Bearer ${token}` } : undefined, + headers: token ? { Authorization: `Bearer ${token}` } : undefined, })🤖 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 `@apps/blog-sveltekit/src/routes/api-token-posts/`+page.svelte around lines 73 - 75, Update the fetch call that sets the bearer token so the header uses standard casing: change the header key from 'authorization' to 'Authorization' in the code that constructs the request (the const response = await fetch(...) block that conditionally sets headers using token). Ensure the conditional remains the same (token ? { Authorization: `Bearer ${token}` } : undefined) and that nothing else in the fetch call is altered.apps/blog-nuxt/app/pages/api-token-posts.vue (2)
73-75: ⚡ Quick winConsider using standard
Authorizationheader casing.The HTTP convention is to use
Authorizationwith a capital 'A'. While headers are case-insensitive per the HTTP specification, using standard casing improves consistency with documentation and avoids potential issues with non-compliant middleware or proxies.📝 Proposed fix
const response = await fetch('/api/v1/posts', { - headers: token ? { authorization: `Bearer ${token}` } : undefined, + headers: token ? { Authorization: `Bearer ${token}` } : undefined, })🤖 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 `@apps/blog-nuxt/app/pages/api-token-posts.vue` around lines 73 - 75, The fetch call building the request headers uses lowercase 'authorization'; change it to the standard 'Authorization' header name when token is present (the fetch invocation that assigns to response and uses the token variable for Bearer auth to '/api/v1/posts') so the headers object uses 'Authorization': `Bearer ${token}` instead of 'authorization' to ensure standard casing.
38-38: ⚡ Quick winUse
SubmitEventfor better type specificity.Form submit handlers should use
SubmitEventinstead of the genericEventtype for improved type safety and clarity.🔧 Proposed fix
-async function createToken(event: Event): Promise<void> { +async function createToken(event: SubmitEvent): Promise<void> { event.preventDefault() if (!(event.currentTarget instanceof HTMLFormElement)) { return-async function fetchPosts(event: Event): Promise<void> { +async function fetchPosts(event: SubmitEvent): Promise<void> { event.preventDefault() if (!(event.currentTarget instanceof HTMLFormElement)) { returnAlso applies to: 62-62
🤖 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 `@apps/blog-nuxt/app/pages/api-token-posts.vue` at line 38, The submit handlers use the generic Event type; update the function signatures to use SubmitEvent for stronger typing — e.g., change async function createToken(event: Event): Promise<void> to accept event: SubmitEvent, and apply the same change to the other handler referenced (the function at the second occurrence around line 62) so both form submit handlers use SubmitEvent for proper typings when accessing form data and preventDefault.apps/blog-next/app/api-token-posts/token-posts-client.tsx (1)
69-71: ⚡ Quick winConsider using standard
Authorizationheader casing.The HTTP convention is to use
Authorizationwith a capital 'A'. While headers are case-insensitive per the HTTP specification, using standard casing improves consistency with documentation and avoids potential issues with non-compliant middleware or proxies.📝 Proposed fix
const response = await fetch('/api/v1/posts', { - headers: token ? { authorization: `Bearer ${token}` } : undefined, + headers: token ? { Authorization: `Bearer ${token}` } : undefined, })🤖 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 `@apps/blog-next/app/api-token-posts/token-posts-client.tsx` around lines 69 - 71, The fetch call in token-posts-client.tsx sets the header key as "authorization" (see the headers block in the fetch call that defines response) — change the header key to standard "Authorization" casing when token is present (i.e., headers: token ? { Authorization: `Bearer ${token}` } : undefined) so the Authorization header uses the conventional capital 'A' while keeping the same token value and conditional behavior.
🤖 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 `@apps/blog-next/app/api/v1/tokens/route.ts`:
- Around line 17-28: The login flow currently skips verifyPassword when
User.where('email', email) returns no user, leaking timing; fix by adding a
module-level precomputed DUMMY_PASSWORD_HASH constant (argon2/bcrypt hash of a
fixed password) and always call verifyPassword(password, actualHashOrDummy) so
that verifyPassword is invoked whether or not currentUser exists — replace the
conditional that sets passwordMatches to instead set const hashToCheck = typeof
passwordHash === 'string' ? passwordHash : DUMMY_PASSWORD_HASH and then await
verifyPassword(password, hashToCheck); keep the generic error response
unchanged.
In `@packages/core/tests/runtime.test.ts`:
- Around line 1738-1746: The test currently deletes the test-only global
__holoCoreRuntimeModelPreload only on the success path, which can leak state if
assertions fail; update the block that reads state from globalThis (the symbol
__holoCoreRuntimeModelPreload) so the expect(...) assertion is executed inside a
try block and the delete state.__holoCoreRuntimeModelPreload is moved to a
finally block to guarantee cleanup regardless of test outcome.
---
Nitpick comments:
In `@apps/blog-next/app/api-token-posts/token-posts-client.tsx`:
- Around line 69-71: The fetch call in token-posts-client.tsx sets the header
key as "authorization" (see the headers block in the fetch call that defines
response) — change the header key to standard "Authorization" casing when token
is present (i.e., headers: token ? { Authorization: `Bearer ${token}` } :
undefined) so the Authorization header uses the conventional capital 'A' while
keeping the same token value and conditional behavior.
In `@apps/blog-nuxt/app/pages/api-token-posts.vue`:
- Around line 73-75: The fetch call building the request headers uses lowercase
'authorization'; change it to the standard 'Authorization' header name when
token is present (the fetch invocation that assigns to response and uses the
token variable for Bearer auth to '/api/v1/posts') so the headers object uses
'Authorization': `Bearer ${token}` instead of 'authorization' to ensure standard
casing.
- Line 38: The submit handlers use the generic Event type; update the function
signatures to use SubmitEvent for stronger typing — e.g., change async function
createToken(event: Event): Promise<void> to accept event: SubmitEvent, and apply
the same change to the other handler referenced (the function at the second
occurrence around line 62) so both form submit handlers use SubmitEvent for
proper typings when accessing form data and preventDefault.
In `@apps/blog-sveltekit/src/routes/api-token-posts/`+page.svelte:
- Around line 73-75: Update the fetch call that sets the bearer token so the
header uses standard casing: change the header key from 'authorization' to
'Authorization' in the code that constructs the request (the const response =
await fetch(...) block that conditionally sets headers using token). Ensure the
conditional remains the same (token ? { Authorization: `Bearer ${token}` } :
undefined) and that nothing else in the fetch call is altered.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8acb7c22-8320-4c1c-bc18-cd8f20fb3296
📒 Files selected for processing (25)
apps/blog-next/app/api-token-posts/page.tsxapps/blog-next/app/api-token-posts/token-posts-client.tsxapps/blog-next/app/api/v1/posts/route.tsapps/blog-next/app/api/v1/tokens/route.tsapps/blog-next/app/layout.tsxapps/blog-next/config/auth.tsapps/blog-next/tests/run.mjsapps/blog-nuxt/app/app.vueapps/blog-nuxt/app/pages/api-token-posts.vueapps/blog-nuxt/config/auth.tsapps/blog-nuxt/server/api/v1/posts.get.tsapps/blog-nuxt/server/api/v1/tokens.post.tsapps/blog-nuxt/tests/run.mjsapps/blog-sveltekit/config/auth.tsapps/blog-sveltekit/src/routes/+layout.svelteapps/blog-sveltekit/src/routes/api-token-posts/+page.svelteapps/blog-sveltekit/src/routes/api/v1/posts/+server.tsapps/blog-sveltekit/src/routes/api/v1/tokens/+server.tsapps/blog-sveltekit/tests/run.mjspackages/core/src/portable/holo.tspackages/core/tests/runtime.test.tspackages/db/src/model/ModelRegistry.tspackages/db/src/model/morphRegistry.tspackages/db/tests/model-core.test.tstests/example-app-token-auth-flow.mjs
Comment on lines
+17
to
+28
| const currentUser = await User.where('email', email).first() | ||
| const passwordHash = currentUser?.get('password') | ||
| const passwordMatches = typeof passwordHash === 'string' | ||
| ? await verifyPassword(password, passwordHash) | ||
| : false | ||
|
|
||
| if (!currentUser || !passwordMatches) { | ||
| return Response.json({ | ||
| ok: false, | ||
| message: 'Invalid credentials.', | ||
| }, { status: 401 }) | ||
| } |
There was a problem hiding this comment.
User enumeration via timing side-channel.
When the email doesn't match a user, verifyPassword is skipped and the response returns almost immediately, whereas a valid email with a wrong password incurs the full hash-verification cost (typically 100–500ms for argon2/bcrypt). Although the error message is intentionally generic, the timing delta lets an attacker enumerate valid accounts. The same pattern appears in the Nuxt and SvelteKit token endpoints.
Recommend always invoking verifyPassword against a stable dummy hash when the user is not found so the response time is independent of account existence.
🛡️ Proposed fix to equalize response timing
- const currentUser = await User.where('email', email).first()
- const passwordHash = currentUser?.get('password')
- const passwordMatches = typeof passwordHash === 'string'
- ? await verifyPassword(password, passwordHash)
- : false
+ const currentUser = await User.where('email', email).first()
+ const passwordHash = currentUser?.get('password')
+ // Use a precomputed dummy hash so response time does not leak account existence.
+ const hashToVerify = typeof passwordHash === 'string' ? passwordHash : DUMMY_PASSWORD_HASH
+ const verified = await verifyPassword(password, hashToVerify)
+ const passwordMatches = typeof passwordHash === 'string' && verifiedDUMMY_PASSWORD_HASH should be a precomputed argon2/bcrypt hash of an arbitrary string, generated once and stored as a module-level constant (not regenerated per request).
🤖 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 `@apps/blog-next/app/api/v1/tokens/route.ts` around lines 17 - 28, The login
flow currently skips verifyPassword when User.where('email', email) returns no
user, leaking timing; fix by adding a module-level precomputed
DUMMY_PASSWORD_HASH constant (argon2/bcrypt hash of a fixed password) and always
call verifyPassword(password, actualHashOrDummy) so that verifyPassword is
invoked whether or not currentUser exists — replace the conditional that sets
passwordMatches to instead set const hashToCheck = typeof passwordHash ===
'string' ? passwordHash : DUMMY_PASSWORD_HASH and then await
verifyPassword(password, hashToCheck); keep the generic error response
unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/blog-sveltekit/src/routes/api/v1/tokens/`+server.ts:
- Around line 34-39: The JSON response that returns the plaintext bearer token
(token.plainTextToken) and tokenId is currently cacheable; update the handler
that returns json({...}) in +server.ts so the response includes a Cache-Control:
no-store header to prevent browser/service-worker/intermediary caching. Modify
the return json(...) call to pass response headers (e.g., json(payload, {
headers: { 'Cache-Control': 'no-store' } })) ensuring the token.plainTextToken
and token.id are still returned but never stored by caches.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 996e7426-b416-4461-a87e-605846efcb36
📒 Files selected for processing (11)
apps/blog-next/app/api-token-posts/token-posts-client.tsxapps/blog-next/app/api/v1/tokens/route.tsapps/blog-nuxt/app/pages/api-token-posts.vueapps/blog-nuxt/server/api/v1/tokens.post.tsapps/blog-sveltekit/src/routes/api-token-posts/+page.svelteapps/blog-sveltekit/src/routes/api/v1/tokens/+server.tspackages/auth/src/runtime.tspackages/auth/tests/package.test.tspackages/core/tests/runtime.test.tspackages/db/tests/type-system.test.tstests/example-app-token-auth-flow.mjs
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/blog-nuxt/app/pages/api-token-posts.vue
- tests/example-app-token-auth-flow.mjs
- apps/blog-sveltekit/src/routes/api-token-posts/+page.svelte
- apps/blog-next/app/api-token-posts/token-posts-client.tsx
- apps/blog-nuxt/server/api/v1/tokens.post.ts
- apps/blog-next/app/api/v1/tokens/route.ts
- packages/core/tests/runtime.test.ts
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/auth/src/contracts.ts (1)
244-282:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftToken guards still expose session-only APIs.
AuthTokenGuardFacadeinheritsloginUsing*,impersonate*,impersonation(), andstopImpersonating()fromAuthGuardFacade. Those calls still route through session-only runtime paths, soauth.guard('api')type-checks methods that can only fail at runtime. Split the shared guard surface from the session-only surface before specializing the token facade.🤖 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/auth/src/contracts.ts` around lines 244 - 282, The token guard exposes session-only APIs because AuthTokenGuardFacade inherits session methods from AuthGuardFacade; extract a shared base interface (e.g. AuthBaseGuardFacade) containing purely token-agnostic methods (check, user, provider, id, currentAccessToken, login, register, refreshUser?, logout, etc.), move the session-only APIs (loginUsing, loginUsingId, impersonate, impersonateById, impersonation, stopImpersonating) into a new AuthSessionOnlyFacade, then have AuthSessionGuardFacade extend AuthBaseGuardFacade + AuthSessionOnlyFacade while changing AuthTokenGuardFacade to extend only AuthBaseGuardFacade; update references to AuthGuardFacade accordingly (rename or replace) so token guards no longer inherit the session methods.
🤖 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 `@apps/blog-next/app/api/v1/tokens/route.ts`:
- Around line 27-32: The response that returns the plaintext token (the
Response.json call in apps/blog-next/app/api/v1/tokens/route.ts which returns
token.plainTextToken, token.id, and token.abilities) must include a
Cache-Control: no-store header to prevent browsers or intermediaries from
caching the sensitive token; update the Response.json invocation to include a
headers object with "Cache-Control": "no-store" so the plaintext token is never
cached.
In `@apps/blog-next/config/auth.ts`:
- Around line 45-46: Change the personalAccessTokens.defaultAbilities to an
empty array and update token issuance code to pass explicit abilities when
creating tokens: set personalAccessTokens.defaultAbilities = [] in the auth
config and modify calls like auth.guard('api').login(submission.data) to include
an explicit abilities array (e.g., auth.guard('api').login({...submission.data,
abilities: [...]}) or supply abilities as the appropriate parameter), and
propagate the same config change and explicit-abilities update across the other
token routes/examples that issue personal access tokens so no token inherits the
old posts.read default.
In `@apps/blog-nuxt/server/api/v1/tokens.post.ts`:
- Around line 28-34: The response that returns token.plainTextToken must include
a Cache-Control: no-store header to prevent caching; before returning the object
in the POST handler (the block that returns ok:true, token:
token.plainTextToken, tokenId: token.id, abilities: token.abilities) set the
response header—e.g. call event.res.setHeader('Cache-Control','no-store') (or
the framework-specific response.setHeader) immediately prior to the return so
the plaintext bearer token is never cached by browsers or intermediaries.
In `@apps/blog-sveltekit/src/lib/server/blog.ts`:
- Around line 18-22: The code currently bootstraps a repo-known password by
calling hashPassword('secret-secret') and creating the account via
User.unguarded(() => User.create(...)) for editor@example.com; change this to
read the bootstrap password from configuration/environment (e.g.,
process.env.BOOTSTRAP_EDITOR_PASSWORD) and only create the user with that hashed
value if the env var is present, otherwise create the account with an
unusable/locked password (or a random hash and a flag requiring reset) so no
repo-known credential exists; update the same pattern in any companion blog
helpers and seeders that call hashPassword or create the editor user to use the
env-configured password or an unusable password path instead.
In `@packages/auth/src/contracts.ts`:
- Around line 176-178: The exported conditional type AuthGuardFacadeFor<TName
extends string> currently hard-codes the 'api' case to AuthTokenGuardFacade;
remove that special-case so the mapping is driven solely by the registry types
(i.e., rely on AuthGuardFacadeForDriver<RegisteredAuthGuardDriver<TName>> and
RegisteredAuthGuardDriver) so consumers who register 'api' as a different guard
get correct types; update the exported type definition for AuthGuardFacadeFor to
drop the 'api' branch and ensure examples/docs show augmenting
HoloAuth.TypeRegistry instead of relying on the built-in override.
In `@packages/auth/src/runtime.ts`:
- Around line 1600-1609: registerForGuard currently persists the user for token
guards via registerUserForGuard(guardName, input) then calls
createLoginTokenForGuard; if token creation fails the new account is left
behind. Modify registerForGuard so that when guard.driver === 'token' you
ensureTokenStore(), call registerUserForGuard, then attempt
createLoginTokenForGuard inside a try/catch; on any error delete/rollback the
just-created user using the same cleanup used by the email-verification path
(e.g., the existing unregister/delete user helper used elsewhere), rethrow the
error, and keep successful token return unchanged. Ensure you reference
registerForGuard, registerUserForGuard, createLoginTokenForGuard and
ensureTokenStore when making the change.
- Around line 1596-1598: The top-level facade currently assumes a session-backed
default but registerDefaultUser (and login) use
getRuntimeBindings().config.defaults.guard directly; add a runtime validation
that config.defaults.guard is session-backed and refuse/throw during startup
configuration if it points to a token-based driver, or alternatively make the
top-level AuthFacade proxy guard-aware and route register/login to the specific
guard driver. Concretely: in the runtime initialization (where
getRuntimeBindings or config is validated) check config.defaults.guard against
the guards map (e.g., config.guards[<name>].driver) and either throw a clear
error preventing token-based defaults or change
AuthFacade/registerDefaultUser/login to delegate to
auth.guard(default).register()/login() so behavior matches
auth.guard(default).register(); apply the same validation/delegation logic to
the other call sites mentioned (the login path and the other register wrappers).
---
Outside diff comments:
In `@packages/auth/src/contracts.ts`:
- Around line 244-282: The token guard exposes session-only APIs because
AuthTokenGuardFacade inherits session methods from AuthGuardFacade; extract a
shared base interface (e.g. AuthBaseGuardFacade) containing purely
token-agnostic methods (check, user, provider, id, currentAccessToken, login,
register, refreshUser?, logout, etc.), move the session-only APIs (loginUsing,
loginUsingId, impersonate, impersonateById, impersonation, stopImpersonating)
into a new AuthSessionOnlyFacade, then have AuthSessionGuardFacade extend
AuthBaseGuardFacade + AuthSessionOnlyFacade while changing AuthTokenGuardFacade
to extend only AuthBaseGuardFacade; update references to AuthGuardFacade
accordingly (rename or replace) so token guards no longer inherit the session
methods.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b736c43-a60c-40bd-ac1c-e485feaaf98b
📒 Files selected for processing (24)
apps/blog-next/app/api-token-posts/token-posts-client.tsxapps/blog-next/app/api/v1/tokens/route.tsapps/blog-next/config/auth.tsapps/blog-next/server/db/seeders/BlogSeeder.tsapps/blog-next/server/lib/blog.tsapps/blog-nuxt/app/pages/api-token-posts.vueapps/blog-nuxt/config/auth.tsapps/blog-nuxt/server/api/v1/tokens.post.tsapps/blog-nuxt/server/db/seeders/BlogSeeder.tsapps/blog-nuxt/server/lib/blog.tsapps/blog-sveltekit/config/auth.tsapps/blog-sveltekit/server/db/seeders/BlogSeeder.tsapps/blog-sveltekit/src/lib/server/blog.tsapps/blog-sveltekit/src/routes/api-token-posts/+page.svelteapps/blog-sveltekit/src/routes/api/v1/tokens/+server.tsapps/docs/docs/auth/guards-and-providers.mdapps/docs/docs/auth/personal-access-tokens.mdpackages/auth/src/contracts.tspackages/auth/src/index.tspackages/auth/src/runtime.tspackages/auth/tests/contracts.type.test.tspackages/auth/tests/docs-smoke.test.tspackages/auth/tests/package.test.tstests/example-app-token-auth-flow.mjs
✅ Files skipped from review due to trivial changes (1)
- apps/docs/docs/auth/guards-and-providers.md
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/blog-sveltekit/src/routes/api-token-posts/+page.svelte
- apps/blog-next/app/api-token-posts/token-posts-client.tsx
- tests/example-app-token-auth-flow.mjs
- apps/blog-nuxt/app/pages/api-token-posts.vue
Comment on lines
+18
to
+22
| const password = await hashPassword('secret-secret') | ||
| const user = await User.unguarded(() => User.create({ | ||
| name: 'Holo Editor', | ||
| email: 'editor@example.com', | ||
| password: 'secret', | ||
| password, |
There was a problem hiding this comment.
Avoid bootstrapping a repo-known password at runtime.
On an empty deployment this helper creates editor@example.com with a password published in the repo. With the new token auth flow, that becomes a reusable way to mint API tokens. Please source the bootstrap password from environment/config, or create the account with an unusable password until an operator sets it. The same fix should be applied to the companion blog helpers and seeders as well.
🤖 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 `@apps/blog-sveltekit/src/lib/server/blog.ts` around lines 18 - 22, The code
currently bootstraps a repo-known password by calling
hashPassword('secret-secret') and creating the account via User.unguarded(() =>
User.create(...)) for editor@example.com; change this to read the bootstrap
password from configuration/environment (e.g.,
process.env.BOOTSTRAP_EDITOR_PASSWORD) and only create the user with that hashed
value if the env var is present, otherwise create the account with an
unusable/locked password (or a random hash and a flag requiring reset) so no
repo-known credential exists; update the same pattern in any companion blog
helpers and seeders that call hashPassword or create the editor user to use the
env-configured password or an unusable password path instead.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/auth/src/runtime.ts (1)
1621-1628:⚠️ Potential issue | 🟠 Major | ⚡ Quick winToken-guard registration rollback misses the model-instance delete fallback.
On token registration failure,
registerForGuard()callsrollbackSerializedUserForGuard(), but that helper only triesadapter.delete. This misses thecreatedUser.delete()fallback already handled inrollbackRegisteredUser(), so failed token issuance can still leave a persisted user for adapters withoutdelete.💡 Suggested fix
async function rollbackSerializedUserForGuard(guardName: string, user: SerializedAuthUser): Promise<void> { const guard = getGuardConfig(guardName) const { adapter } = getProviderAdapter(guard.provider) + const created = await adapter.findById(user.id).catch(() => null) + if (created) { + await rollbackRegisteredUser(adapter, created, user) + return + } if (adapter.delete) { await adapter.delete(user.id) } }Also applies to: 1665-1670
🤖 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/auth/src/runtime.ts` around lines 1621 - 1628, The catch block after createLoginTokenForGuard(guardName, user, input) only calls rollbackSerializedUserForGuard(guardName, user) which relies solely on adapter.delete; update the rollback to also attempt the model-instance delete fallback used by rollbackRegisteredUser (or directly call rollbackRegisteredUser(guardName, user)) so adapters without delete won't leave a persisted user; apply the same change for the similar block around lines 1665-1670 to ensure both token-guard paths perform the adapter.delete then createdUser.delete fallback.
🤖 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 `@apps/blog-nuxt/server/api/v1/posts.get.ts`:
- Line 30: The ownership filter currently coerces userId to a number with
Number(userId) in the .where('user_id', ...) call which can produce NaN for
non-numeric IDs; update the .where call to use the native id value (e.g.,
currentUser.id or userId as-is) instead of Number(userId) so the query compares
the actual identifier type returned by your auth layer; locate the .where
invocation in posts.get.ts (the .where('user_id', Number(userId)) call) and
replace the numeric coercion with the original id value.
In `@apps/blog-sveltekit/src/routes/api/v1/posts/`+server.ts:
- Line 27: The ownership filter currently coerces userId with Number(userId) in
the query builder (the .where('user_id', Number(userId)) call), which can
produce NaN for non-numeric IDs; change the .where call to use the native id
value instead (e.g., pass userId or currentUser.id directly) so the query uses
the actual identifier type and avoid numeric coercion that can break filtering.
---
Duplicate comments:
In `@packages/auth/src/runtime.ts`:
- Around line 1621-1628: The catch block after
createLoginTokenForGuard(guardName, user, input) only calls
rollbackSerializedUserForGuard(guardName, user) which relies solely on
adapter.delete; update the rollback to also attempt the model-instance delete
fallback used by rollbackRegisteredUser (or directly call
rollbackRegisteredUser(guardName, user)) so adapters without delete won't leave
a persisted user; apply the same change for the similar block around lines
1665-1670 to ensure both token-guard paths perform the adapter.delete then
createdUser.delete fallback.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 428dc1fd-0112-4cff-8ce4-c094466f5260
📒 Files selected for processing (28)
apps/blog-next/app/api/v1/posts/route.tsapps/blog-next/app/api/v1/tokens/route.tsapps/blog-next/config/auth.tsapps/blog-nuxt/config/auth.tsapps/blog-nuxt/server/api/v1/posts.get.tsapps/blog-nuxt/server/api/v1/tokens.post.tsapps/blog-sveltekit/config/auth.tsapps/blog-sveltekit/src/routes/api/v1/posts/+server.tsapps/blog-sveltekit/src/routes/api/v1/tokens/+server.tsapps/docs/docs/auth/guards-and-providers.mdapps/docs/docs/auth/personal-access-tokens.mdpackages/adapter-nuxt/src/module.tspackages/adapter-nuxt/tests/module.test.tspackages/adapter-nuxt/tests/setup.test.tspackages/auth-clerk/src/contracts.tspackages/auth-clerk/src/index.tspackages/auth-workos/src/contracts.tspackages/auth-workos/src/index.tspackages/auth/src/contracts.tspackages/auth/src/index.tspackages/auth/src/runtime.tspackages/auth/src/runtime/context.tspackages/auth/tests/contracts.type.test.tspackages/auth/tests/docs-smoke.test.tspackages/auth/tests/package.test.tspackages/cli/src/project/registry.tspackages/cli/src/project/shared.tspackages/cli/tests/authorization-registry.test.ts
✅ Files skipped from review due to trivial changes (2)
- apps/docs/docs/auth/guards-and-providers.md
- packages/cli/src/project/shared.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/auth/tests/docs-smoke.test.ts
- apps/blog-next/app/api/v1/posts/route.ts
- packages/auth/src/index.ts
- apps/blog-nuxt/server/api/v1/tokens.post.ts
- packages/auth/tests/contracts.type.test.ts
- packages/auth/tests/package.test.ts
- apps/blog-next/app/api/v1/tokens/route.ts
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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
1 participant
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.
Summary by CodeRabbit
New Features
Refactor
Infrastructure
Tests
Documentation
Chores