chore: trigger CodeRabbit review - templates & scripts (3/7)#9
chore: trigger CodeRabbit review - templates & scripts (3/7)#9Helal-maker wants to merge 1 commit intomainfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 29 minutes and 57 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (34)
WalkthroughThis PR applies consistent whitespace and formatting normalization across the entire codebase, including trailing space adjustments, import statement reformatting, and comment style updates, without altering functional logic, control flow, or exported APIs. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@templates/auth/src/routes/auth.ts`:
- Line 107: The OTP and backup-code generation in auth.ts (variables like otp at
the Math.random() line and the backup/code generation around lines 169-171 and
291) use non-cryptographic Math.random() and must be replaced with a
cryptographically secure RNG; import Node's crypto and use
crypto.randomInt(100000, 1000000) to produce a uniformly distributed 6-digit OTP
(ensure string zero-padding if needed) and use crypto.randomBytes (then
hex/base32/BASE62-encode or map bytes to the allowed alphabet) to generate
backup codes of the required length/entropy, replacing usages of Math.random()
throughout auth.ts (e.g., where otp and backup codes are created) so all auth
secrets come from crypto-secure randomness.
- Around line 135-136: The conditional guards in the verification endpoints
incorrectly allow bypass because they check "process.env.NODE_ENV ===
'development' || code.length === 6" (which is always true due to schema
validation) before issuing sessionId; remove the "|| code.length === 6" from
those conditionals and replace with actual verification logic: call the existing
OTP/verification routine or compare the provided code against the
stored/expected code (e.g., via a verifyCode/validateOtp function or DB lookup)
and only generate sessionId (crypto.randomUUID()) when that verification
succeeds; ensure the development-only bypass remains strictly tied to NODE_ENV
=== 'development' and update all occurrences that currently use the "code.length
=== 6" pattern.
- Around line 68-83: The current token check (token.startsWith("dev-token-")) in
the auth route unconditionally accepts dev magic links; restrict this behavior
so it only runs in development: guard the dev-branch with an environment/config
check (e.g., process.env.NODE_ENV === "development" or a feature flag) around
the token.startsWith check and its mock session creation (the code that calls
crypto.randomUUID() and returns the dev user object), and otherwise fall through
to the real verification path or reject the token; update any tests or comments
to reflect that dev-token handling is disabled outside development.
In `@templates/base/src/lib/realtime.ts`:
- Around line 263-265: Subscription lookup uses client.subscriptions.get(table)
but subscriptions are keyed by `${table}:${event}`, causing subscription to be
undefined and filters in matchesFilter to be skipped; update the lookup where
subscription is retrieved (the code that currently does
client.subscriptions.get(table)) to use the composite key `${table}:${event}`
(or otherwise derive the correct key from the message's event and table) so that
subscription?.filter is the actual stored filter before calling
this.matchesFilter(subscription?.filter, data), ensuring filters are enforced.
In `@templates/base/src/routes/index.ts`:
- Around line 16-23: The response currently exposes err.message and err.cause
for HTTP exceptions in non-development environments; change the logic so
sensitive details are only returned when env.NODE_ENV === "development". Update
the showDetailedError calculation (remove isHttpError from it) and restrict
stack and details to only be set when showDetailedError is true; for example
keep isHttpError only for setting status but ensure error: showDetailedError ?
err.message : "Internal Server Error" and details: showDetailedError &&
isHttpError ? ((err as { cause?: unknown }).cause ?? null) : null so no
exception messages/causes are leaked in production (referencing
showDetailedError, isHttpError, env.NODE_ENV, err.message, err.stack, and the
details expression).
In `@templates/base/src/routes/storage.ts`:
- Line 402: Update the three storage route patterns that currently use ":key" so
they accept nested paths by replacing each occurrence of "/:bucket/:key" with
Hono's regex form "/:bucket/:key{.+}" (and similarly "/:bucket/:key{.+}/public"
and "/:bucket/:key{.+}/sign"); locate the routes defined via
storageRouter.get(...) (the one starting at the shown diff and the two other
routes referenced) and change their route strings only, leaving parameter access
as c.req.param("key") unchanged.
In `@templates/base/src/routes/users.ts`:
- Line 91: The POST handler currently validates request input but doesn't
persist the new user; replace the TODO by calling the DB insert to save the
parsed user (e.g., invoke db.insert(users).values(parsed) and await the result)
or call a dedicated UsersService (e.g., UsersService.create(parsed)) to persist
and return the persisted record/ID; ensure you handle and propagate DB errors
(try/catch) and return appropriate HTTP responses (201 on success with created
user or ID, 500 on DB error) from the POST handler.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 41fcf258-772d-4374-966a-0edbb5be5163
📒 Files selected for processing (34)
scripts/test-summary.tstemplates/auth/src/auth/index.tstemplates/auth/src/auth/types.tstemplates/auth/src/db/auth-schema.tstemplates/auth/src/db/index.tstemplates/auth/src/db/schema.tstemplates/auth/src/middleware/auth.tstemplates/auth/src/routes/auth-example.tstemplates/auth/src/routes/auth.tstemplates/base/betterbase.config.tstemplates/base/drizzle.config.tstemplates/base/src/auth/index.tstemplates/base/src/auth/types.tstemplates/base/src/db/index.tstemplates/base/src/db/migrate.tstemplates/base/src/db/schema.tstemplates/base/src/index.tstemplates/base/src/lib/env.tstemplates/base/src/lib/realtime.tstemplates/base/src/middleware/auth.tstemplates/base/src/middleware/validation.tstemplates/base/src/routes/graphql.d.tstemplates/base/src/routes/health.tstemplates/base/src/routes/index.tstemplates/base/src/routes/storage.tstemplates/base/src/routes/users.tstemplates/base/test/crud.test.tstemplates/base/test/health.test.tstemplates/iac/betterbase.config.tstemplates/iac/betterbase/cron.tstemplates/iac/betterbase/mutations/todos.tstemplates/iac/betterbase/queries/todos.tstemplates/iac/betterbase/schema.tstemplates/iac/src/index.ts
| // TODO: Implement proper token verification using better-auth | ||
| // For now, simulate verification | ||
| if (token.startsWith("dev-token-")) { | ||
| // In dev mode, create a mock session | ||
| const sessionId = crypto.randomUUID(); | ||
| // Find or create user (in real implementation, this would be done by better-auth) | ||
| return c.json({ | ||
| token: sessionId, | ||
| user: { | ||
| id: "dev-user-id", | ||
| email: "dev@example.com", | ||
| name: "Dev User", | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Dev magic-link tokens are accepted in every environment.
Line 70 validates dev-token-* with no NODE_ENV gate, so a crafted token can create a session outside development.
Patch
- if (token.startsWith("dev-token-")) {
+ if (process.env.NODE_ENV === "development" && token.startsWith("dev-token-")) {
// In dev mode, create a mock session
const sessionId = crypto.randomUUID();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/auth/src/routes/auth.ts` around lines 68 - 83, The current token
check (token.startsWith("dev-token-")) in the auth route unconditionally accepts
dev magic links; restrict this behavior so it only runs in development: guard
the dev-branch with an environment/config check (e.g., process.env.NODE_ENV ===
"development" or a feature flag) around the token.startsWith check and its mock
session creation (the code that calls crypto.randomUUID() and returns the dev
user object), and otherwise fall through to the real verification path or reject
the token; update any tests or comments to reflect that dev-token handling is
disabled outside development.
| const isDev = process.env.NODE_ENV === "development"; | ||
| // Generate 6-digit OTP | ||
| const otp = Math.floor(100000 + Math.random() * 900000).toString(); |
There was a problem hiding this comment.
Auth secrets are generated with non-cryptographic randomness.
Line 107, Line 169-171, and Line 291 use Math.random() for OTP/backup codes. These values are predictable enough for brute-force-assisted attacks when used for authentication flows.
Patch
+import { randomInt, randomBytes } from "node:crypto";
...
- const otp = Math.floor(100000 + Math.random() * 900000).toString();
+ const otp = randomInt(100000, 1000000).toString();
- const backupCodes = Array.from({ length: 10 }, () =>
- Math.random().toString(36).substring(2, 10).toUpperCase(),
- );
+ const backupCodes = Array.from({ length: 10 }, () =>
+ randomBytes(6).toString("base64url").slice(0, 10).toUpperCase(),
+ );
- const code = Math.floor(100000 + Math.random() * 900000).toString();
+ const code = randomInt(100000, 1000000).toString();Also applies to: 169-171, 291-291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/auth/src/routes/auth.ts` at line 107, The OTP and backup-code
generation in auth.ts (variables like otp at the Math.random() line and the
backup/code generation around lines 169-171 and 291) use non-cryptographic
Math.random() and must be replaced with a cryptographically secure RNG; import
Node's crypto and use crypto.randomInt(100000, 1000000) to produce a uniformly
distributed 6-digit OTP (ensure string zero-padding if needed) and use
crypto.randomBytes (then hex/base32/BASE62-encode or map bytes to the allowed
alphabet) to generate backup codes of the required length/entropy, replacing
usages of Math.random() throughout auth.ts (e.g., where otp and backup codes are
created) so all auth secrets come from crypto-secure randomness.
| if (process.env.NODE_ENV === "development" || code.length === 6) { | ||
| const sessionId = crypto.randomUUID(); |
There was a problem hiding this comment.
Verification endpoints are effectively auth-bypassed in production.
Line 135 / 197 / 221 / 246 / 322 use ... || code.length === 6; schemas already enforce 6 chars, so these branches always pass and issue/accept sessions without real verification.
Patch pattern
- if (process.env.NODE_ENV === "development" || code.length === 6) {
+ if (process.env.NODE_ENV === "development") {
+ // dev-only shortcut
+ ...
+ } else {
+ // production: verify code against persisted challenge/OTP + expiry + attempt limits
+ // const ok = await verifyCode(...)
+ // if (!ok) return c.json({ error: "Invalid or expired code" }, 401);
+ ...
+ }
- ...
- }Also applies to: 197-198, 221-222, 246-247, 322-323
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/auth/src/routes/auth.ts` around lines 135 - 136, The conditional
guards in the verification endpoints incorrectly allow bypass because they check
"process.env.NODE_ENV === 'development' || code.length === 6" (which is always
true due to schema validation) before issuing sessionId; remove the "||
code.length === 6" from those conditionals and replace with actual verification
logic: call the existing OTP/verification routine or compare the provided code
against the stored/expected code (e.g., via a verifyCode/validateOtp function or
DB lookup) and only generate sessionId (crypto.randomUUID()) when that
verification succeeds; ensure the development-only bypass remains strictly tied
to NODE_ENV === 'development' and update all occurrences that currently use the
"code.length === 6" pattern.
| const subscription = client?.subscriptions.get(table); | ||
| if (!this.matchesFilter(subscription?.filter, data)) { | ||
| continue; |
There was a problem hiding this comment.
Filter enforcement is broken due to wrong subscription key lookup
At Line 263, client.subscriptions.get(table) is inconsistent with how subscriptions are stored (${table}:${event}). This causes subscription to be undefined, so Line 264 treats every message as matching and bypasses subscription filters.
Proposed fix
- const subscription = client?.subscriptions.get(table);
+ const subscription =
+ client?.subscriptions.get(`${table}:${event}`) ??
+ client?.subscriptions.get(`${table}:*`);
if (!this.matchesFilter(subscription?.filter, data)) {
continue;
} 📝 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.
| const subscription = client?.subscriptions.get(table); | |
| if (!this.matchesFilter(subscription?.filter, data)) { | |
| continue; | |
| const subscription = | |
| client?.subscriptions.get(`${table}:${event}`) ?? | |
| client?.subscriptions.get(`${table}:*`); | |
| if (!this.matchesFilter(subscription?.filter, data)) { | |
| continue; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/base/src/lib/realtime.ts` around lines 263 - 265, Subscription
lookup uses client.subscriptions.get(table) but subscriptions are keyed by
`${table}:${event}`, causing subscription to be undefined and filters in
matchesFilter to be skipped; update the lookup where subscription is retrieved
(the code that currently does client.subscriptions.get(table)) to use the
composite key `${table}:${event}` (or otherwise derive the correct key from the
message's event and table) so that subscription?.filter is the actual stored
filter before calling this.matchesFilter(subscription?.filter, data), ensuring
filters are enforced.
| const showDetailedError = env.NODE_ENV === "development" || isHttpError; | ||
| return c.json( | ||
| { | ||
| error: showDetailedError ? err.message : "Internal Server Error", | ||
| stack: env.NODE_ENV === "development" ? err.stack : undefined, | ||
| details: isHttpError ? ((err as { cause?: unknown }).cause ?? null) : null, | ||
| }, |
There was a problem hiding this comment.
Production error handler leaks internal exception details.
Line 16-23 returns HTTPException messages/causes in non-development environments. If upstream throws detailed HTTPException, internals are exposed to clients.
Patch
- const showDetailedError = env.NODE_ENV === "development" || isHttpError;
+ const isDev = env.NODE_ENV === "development";
...
- error: showDetailedError ? err.message : "Internal Server Error",
- stack: env.NODE_ENV === "development" ? err.stack : undefined,
- details: isHttpError ? ((err as { cause?: unknown }).cause ?? null) : null,
+ error: isDev ? err.message : isHttpError ? "Request failed" : "Internal Server Error",
+ stack: isDev ? err.stack : undefined,
+ details: isDev && isHttpError ? ((err as { cause?: unknown }).cause ?? null) : null,📝 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.
| const showDetailedError = env.NODE_ENV === "development" || isHttpError; | |
| return c.json( | |
| { | |
| error: showDetailedError ? err.message : "Internal Server Error", | |
| stack: env.NODE_ENV === "development" ? err.stack : undefined, | |
| details: isHttpError ? ((err as { cause?: unknown }).cause ?? null) : null, | |
| }, | |
| const isDev = env.NODE_ENV === "development"; | |
| return c.json( | |
| { | |
| error: isDev ? err.message : isHttpError ? "Request failed" : "Internal Server Error", | |
| stack: isDev ? err.stack : undefined, | |
| details: isDev && isHttpError ? ((err as { cause?: unknown }).cause ?? null) : null, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/base/src/routes/index.ts` around lines 16 - 23, The response
currently exposes err.message and err.cause for HTTP exceptions in
non-development environments; change the logic so sensitive details are only
returned when env.NODE_ENV === "development". Update the showDetailedError
calculation (remove isHttpError from it) and restrict stack and details to only
be set when showDetailedError is true; for example keep isHttpError only for
setting status but ensure error: showDetailedError ? err.message : "Internal
Server Error" and details: showDetailedError && isHttpError ? ((err as { cause?:
unknown }).cause ?? null) : null so no exception messages/causes are leaked in
production (referencing showDetailedError, isHttpError, env.NODE_ENV,
err.message, err.stack, and the details expression).
| }); | ||
| // GET /api/storage/:bucket/:key - Download a file | ||
| storageRouter.get("/:bucket/:key", async (c: StorageContext) => { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n templates/base/src/routes/storage.ts | sed -n '395,410p'Repository: Helal-maker/Betterbase
Length of output: 659
🏁 Script executed:
cat -n templates/base/src/routes/storage.ts | sed -n '445,460p'Repository: Helal-maker/Betterbase
Length of output: 676
🏁 Script executed:
cat -n templates/base/src/routes/storage.ts | sed -n '475,490p'Repository: Helal-maker/Betterbase
Length of output: 627
🏁 Script executed:
cat -n templates/base/src/routes/storage.ts | sed -n '1,50p'Repository: Helal-maker/Betterbase
Length of output: 1845
🏁 Script executed:
grep -n "storageRouter\.\(post\|put\)" templates/base/src/routes/storage.ts | head -5Repository: Helal-maker/Betterbase
Length of output: 216
🏁 Script executed:
cat -n templates/base/src/routes/storage.ts | sed -n '320,380p'Repository: Helal-maker/Betterbase
Length of output: 2092
🏁 Script executed:
rg "storageRouter.*post.*upload" templates/base/src/routes/storage.ts -A 15Repository: Helal-maker/Betterbase
Length of output: 548
🏁 Script executed:
rg "storageRouter.*\*" templates/base/src/routes/storage.tsRepository: Helal-maker/Betterbase
Length of output: 111
🏁 Script executed:
rg "hono.*wildcard|wildcard.*hono" -i --type tsRepository: Helal-maker/Betterbase
Length of output: 48
🌐 Web query:
Hono routing wildcard parameters capture slash characters
💡 Result:
No, standard Hono route parameters like /:id do not capture slash characters (/) because they match single path segments (non-greedy [^/]+ by default). Slashes terminate the parameter. To capture paths including slashes (e.g., for file paths or deep nesting), use regex constraints on parameters with patterns like {.+} or {.}, as shown in official docs: app.get('/posts/:path{.+}', (c) => { const fullPath = c.req.param('path'); // Captures everything after /posts/, including slashes, e.g. "folder/subfolder/file.txt" return c.text(fullPath); }); This converts to a regex with named capture groups allowing / matching. Plain wildcards like / match but do not populate c.req.param with the captured value as a named param. Examples from Hono docs: - /posts/:filename{.+\.png} captures filenames with slashes and .png extension. - Confirmed in GitHub issues #4158, #3190: Use /:path{.+} for wildcard params capturing slashes. Parameters are automatically URL-decoded, so %2F becomes / in c.req.param.
Citations:
- 1: Routing support for wildcard parameters honojs/hono#4158
- 2: https://www.mintlify.com/honojs/hono/api/routing
- 3: https://hono.dev/api/routing
- 4: https://mintlify.com/honojs/hono/routing/route-parameters
- 5: https://hono.dev/docs/api/routing
- 6: Support for for a wildcard param? honojs/hono#3190
- 7: Param parsing with a dot in the path doesn't capture all the parameters honojs/hono#3745
- 8: https://www.mintlify.com/honojs/hono/api/routers/pattern-router
- 9: https://hono.dev/docs/api/request
- 10:
hcclient does not escape:idparameter with forward slash in URL honojs/hono#4023
Fix route patterns to capture nested storage keys with slash delimiters.
Routes at 402, 451, and 480 use :key which matches only single path segments—slashes terminate the match. Files uploaded with nested paths (e.g., folder/file.txt) cannot be retrieved or signed. Use Hono's regex constraint syntax: /:bucket/:key{.+} instead of /:bucket/:key. Apply the same constraint to /:bucket/:key{.+}/public and /:bucket/:key{.+}/sign. Keep parameter extraction as c.req.param("key") (constraint syntax does not change the parameter name).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/base/src/routes/storage.ts` at line 402, Update the three storage
route patterns that currently use ":key" so they accept nested paths by
replacing each occurrence of "/:bucket/:key" with Hono's regex form
"/:bucket/:key{.+}" (and similarly "/:bucket/:key{.+}/public" and
"/:bucket/:key{.+}/sign"); locate the routes defined via storageRouter.get(...)
(the one starting at the shown diff and the two other routes referenced) and
change their route strings only, leaving parameter access as c.req.param("key")
unchanged.
| const body = await c.req.json(); | ||
| const parsed = parseBody(createUserSchema, body); | ||
| // TODO: persist parsed user via db.insert(users) or a dedicated UsersService. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
TODO: User persistence not implemented.
The POST handler validates input but doesn't persist to the database.
Do you want me to generate the implementation for persisting users via db.insert(users).values(parsed)?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/base/src/routes/users.ts` at line 91, The POST handler currently
validates request input but doesn't persist the new user; replace the TODO by
calling the DB insert to save the parsed user (e.g., invoke
db.insert(users).values(parsed) and await the result) or call a dedicated
UsersService (e.g., UsersService.create(parsed)) to persist and return the
persisted record/ID; ensure you handle and propagate DB errors (try/catch) and
return appropriate HTTP responses (201 on success with created user or ID, 500
on DB error) from the POST handler.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
1 participant
This PR adds a trailing space to template and script files to trigger CodeRabbit review. Part 3 of 7.
Summary by CodeRabbit