Conversation
|
@RihanArfan is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR introduces a new basic authentication route rule for Nitro, leveraging h3's built-in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
commit: |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/build/virtual/routing.ts (1)
92-106:⚠️ Potential issue | 🟠 Major
authroute rule passwords are serialized in plaintext into the compiled bundle.
JSON.stringify(options)embeds the entire options object — includingpassword— into the generated JavaScript at build time. Every consumer of the build artifact (minified or not) can read the credential in plaintext. This applies specifically to theauthrule becausepasswordis a secret, unlike headers or redirect targets.Consider either:
- Filtering sensitive fields before serialization and resolving them at runtime via
useRuntimeConfig, or- Prominently documenting this limitation (the compiled bundle will contain the literal password string) and recommending that users pass
process.env.SECRET_PASSWORDin theirnitro.config.tsso the secret comes from the build environment rather than being committed to source.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/build/virtual/routing.ts` around lines 92 - 106, serializeRouteRule currently embeds full options (including auth.password) into the compiled bundle via JSON.stringify; update serializeRouteRule to strip or replace sensitive fields for the "auth" rule before serialization (e.g., if name === "auth", clone options and delete or replace the password field with a runtime reference token) and ensure consumers resolve the actual secret at runtime (via useRuntimeConfig or process.env) instead of emitting the literal password; keep existing behavior for other rule names and preserve RuntimeRouteRules and handler emission (__routeRules__, h._route, h._method) while serializing the sanitized options.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/1.docs/5.routing.md`:
- Around line 363-376: The example imports defineNitroConfig but calls
defineConfig — fix by making the symbols consistent: either import defineConfig
from "nitro" and leave the calls as defineConfig, or keep the import of
defineNitroConfig from "nitro/config" and rename the calls to defineNitroConfig;
update both basic-auth examples (the blocks that use
defineNitroConfig/defineConfig and the routeRules with auth) so the import and
the function call match (references: defineNitroConfig, defineConfig,
routeRules).
- Around line 359-401: Add a security note to the "Basic authentication" docs
warning that the `password` value is inlined verbatim into the compiled bundle
(because `serializeRouteRule` in src/build/virtual/routing.ts uses
JSON.stringify to embed route rules), and instruct users to supply secrets via
build-time environment variables (e.g., process.env.ADMIN_PASSWORD) while
explicitly warning that the resolved string will still appear in the generated
JS artifact; include guidance similar to other security-sensitive docs advising
against committing secrets to source and recommending runtime proxying or
external secret management if secrecy is required.
In `@src/types/route-rules.ts`:
- Line 34: Update the NitroBasicAuthOptions type to match h3's BasicAuthOptions
by adding optional realm?: string and validate?: (username: string, password:
string) => boolean | Promise<boolean>; then ensure wherever
NitroBasicAuthOptions is forwarded into requireBasicAuth (the call in
route-rules handling) accepts and passes these fields through unchanged so realm
customization and custom validation functions are preserved.
In `@test/tests.ts`:
- Around line 433-436: Add a new test that verifies child route rules can
override a parent auth rule: create overlapping fixtures analogous to the docs
(e.g., a parent rule matching "/rules/auth/**" that requires auth and a child
rule matching "/rules/auth/public/**" with auth: false), then use the existing
callHandler helper to request a child path like "/rules/auth/public/test" and
assert a 200 response (instead of the parent's 401). Place the test alongside
the existing "skips auth when set to false" test and reference the same
callHandler helper and route patterns to ensure the route-rule merger correctly
prefers the child auth: false override.
---
Outside diff comments:
In `@src/build/virtual/routing.ts`:
- Around line 92-106: serializeRouteRule currently embeds full options
(including auth.password) into the compiled bundle via JSON.stringify; update
serializeRouteRule to strip or replace sensitive fields for the "auth" rule
before serialization (e.g., if name === "auth", clone options and delete or
replace the password field with a runtime reference token) and ensure consumers
resolve the actual secret at runtime (via useRuntimeConfig or process.env)
instead of emitting the literal password; keep existing behavior for other rule
names and preserve RuntimeRouteRules and handler emission (__routeRules__,
h._route, h._method) while serializing the sanitized options.
| ### Basic authentication | ||
|
|
||
| The `auth` route rule protects matching routes with [HTTP Basic Authentication](https://developer.mozilla.org/en-US/docs/Web/HTTP/Guides/Authentication). When a request hits a protected route, the browser prompts the user for credentials before granting access. | ||
|
|
||
| ```ts [nitro.config.ts] | ||
| import { defineNitroConfig } from "nitro/config"; | ||
|
|
||
| export default defineConfig({ | ||
| routeRules: { | ||
| '/admin/**': { | ||
| auth: { | ||
| username: 'admin', | ||
| password: 'supersecret', | ||
| } | ||
| } | ||
| } | ||
| }); | ||
| ``` | ||
|
|
||
| The `auth` option accepts the following properties: | ||
|
|
||
| | Property | Type | Required | Description | | ||
| | ---------- | -------- | -------- | ------------------------------------------------------------------ | | ||
| | `password` | `string` | Yes | Password for basic auth validation. | | ||
| | `username` | `string` | No | Username for basic auth validation. If omitted, any username is accepted. | | ||
| | `realm` | `string` | No | Realm for the basic auth challenge. Defaults to `""`. | | ||
|
|
||
| You can also explicitly disable authentication for a sub-route by setting `auth: false`: | ||
|
|
||
| ```ts [nitro.config.ts] | ||
| import { defineNitroConfig } from "nitro/config"; | ||
|
|
||
| export default defineConfig({ | ||
| routeRules: { | ||
| '/admin/**': { | ||
| auth: { password: 'supersecret' } | ||
| }, | ||
| '/admin/public/**': { | ||
| auth: false, | ||
| } | ||
| } | ||
| }); | ||
| ``` |
There was a problem hiding this comment.
Document that the password is embedded verbatim in the compiled bundle.
serializeRouteRule in src/build/virtual/routing.ts calls JSON.stringify(options) to inline every route rule into the generated JavaScript, so password ends up in the build artifact in plaintext. Consider adding a note directing users to supply the value at build time via an environment variable (process.env.ADMIN_PASSWORD) and warning that the resolved string will still be present in the bundle, similar to the guidance in other security-sensitive integrations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/1.docs/5.routing.md` around lines 359 - 401, Add a security note to the
"Basic authentication" docs warning that the `password` value is inlined
verbatim into the compiled bundle (because `serializeRouteRule` in
src/build/virtual/routing.ts uses JSON.stringify to embed route rules), and
instruct users to supply secrets via build-time environment variables (e.g.,
process.env.ADMIN_PASSWORD) while explicitly warning that the resolved string
will still appear in the generated JS artifact; include guidance similar to
other security-sensitive docs advising against committing secrets to source and
recommending runtime proxying or external secret management if secrecy is
required.
| ```ts [nitro.config.ts] | ||
| import { defineNitroConfig } from "nitro/config"; | ||
|
|
||
| export default defineConfig({ | ||
| routeRules: { | ||
| '/admin/**': { | ||
| auth: { | ||
| username: 'admin', | ||
| password: 'supersecret', | ||
| } | ||
| } | ||
| } | ||
| }); | ||
| ``` |
There was a problem hiding this comment.
defineNitroConfig is imported but defineConfig is called — fix both new code examples.
Both new basic auth code blocks (and the pre-existing one at lines 339-357) import defineNitroConfig from "nitro/config" but call the undeclared defineConfig(). Either swap the import to defineConfig from "nitro" (matching the actual test fixture), or rename the calls to defineNitroConfig.
📝 Proposed fix (repeated in both new examples)
-import { defineNitroConfig } from "nitro/config";
+import { defineConfig } from "nitro";
-export default defineConfig({
+export default defineConfig({or consistently:
-import { defineNitroConfig } from "nitro/config";
+import { defineNitroConfig } from "nitro/config";
-export default defineConfig({
+export default defineNitroConfig({Also applies to: 388-401
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/1.docs/5.routing.md` around lines 363 - 376, The example imports
defineNitroConfig but calls defineConfig — fix by making the symbols consistent:
either import defineConfig from "nitro" and leave the calls as defineConfig, or
keep the import of defineNitroConfig from "nitro/config" and rename the calls to
defineNitroConfig; update both basic-auth examples (the blocks that use
defineNitroConfig/defineConfig and the routeRules with auth) so the import and
the function call match (references: defineNitroConfig, defineConfig,
routeRules).
| prerender?: boolean; | ||
| proxy?: string | ({ to: string } & ProxyOptions); | ||
| isr?: number /* expiration */ | boolean | VercelISRConfig; | ||
| auth?: NitroBasicAuthOptions | false; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the NitroBasicAuthOptions definition
rg -n "NitroBasicAuthOptions" --type ts -A 10 -B 2Repository: nitrojs/nitro
Length of output: 1371
🏁 Script executed:
# Check h3 version and look for BasicAuth types in the installed package
cat package.json | grep -A 5 -B 5 "h3"Repository: nitrojs/nitro
Length of output: 1034
🏁 Script executed:
# Search for requireBasicAuth in the codebase or node_modules
find . -path ./node_modules -prune -o -type f -name "*.ts" -o -name "*.d.ts" | xargs rg -l "requireBasicAuth" 2>/dev/null | head -20Repository: nitrojs/nitro
Length of output: 94
🏁 Script executed:
# Look for h3 package types and requireBasicAuth signature
find . -path ./node_modules/h3 -type f \( -name "*.d.ts" -o -name "*.ts" \) | head -20Repository: nitrojs/nitro
Length of output: 39
🏁 Script executed:
# Check how the auth field from NitroBasicAuthOptions is used with requireBasicAuth
rg -n "requireBasicAuth|basicAuth" --type ts -B 3 -A 3 src/Repository: nitrojs/nitro
Length of output: 789
🏁 Script executed:
# Look directly in node_modules h3 for the requireBasicAuth type definition
cat node_modules/h3/dist/index.d.ts 2>/dev/null | grep -A 10 "requireBasicAuth"Repository: nitrojs/nitro
Length of output: 39
🏁 Script executed:
# Try to find h3 types in alternative locations
ls -la node_modules/h3/dist/ 2>/dev/null | head -20Repository: nitrojs/nitro
Length of output: 39
🏁 Script executed:
# Check node_modules structure for h3
find node_modules -type d -name "h3" 2>/dev/nullRepository: nitrojs/nitro
Length of output: 39
🌐 Web query:
h3 requireBasicAuth 2.0.1-rc.14 TypeScript signature options shape
💡 Result:
In h3 2.0.1-rc.14, the function is documented as:
requireBasicAuth(event, opts)where opts is BasicAuthOptions. (h3.dev)
The published typings for this version point TypeScript at:
types: "./dist/_entries/generic.d.mts"(app.unpkg.com)
So, in TS, the “options shape” you’re looking for is exactly:
type RequireBasicAuthOptions = BasicAuthOptions;
type RequireBasicAuthSignature = typeof requireBasicAuth;and you can reveal it in-editor with:
import { requireBasicAuth } from "h3";
type Sig = typeof requireBasicAuth;
type Opts = Parameters<typeof requireBasicAuth>[1]; // BasicAuthOptions
``` ([app.unpkg.com](https://app.unpkg.com/h3%402.0.1-rc.14/files/package.json))
Citations:
- 1: https://www.h3.dev/utils/security
- 2: https://app.unpkg.com/h3%402.0.1-rc.14/files/package.json
- 3: https://app.unpkg.com/h3%402.0.1-rc.14/files/package.json
---
🌐 Web query:
`h3 BasicAuthOptions type definition 2.0.1-rc.14 github`
💡 Result:
For **h3@2.0.1-rc.14**, the package’s `package.json` declares its TypeScript types entry as:
- `types: "./dist/_entries/generic.d.mts"` and exports runtime entrypoints under `./dist/_entries/*` (including `generic.mjs`). [1]
So the **`BasicAuthOptions`** type definition you’re looking for should be in that bundled types file (or one of the files it references), i.e.:
- `dist/_entries/generic.d.mts` (in the **h3@2.0.1-rc.14** package contents). [1]
I wasn’t able to fetch that `.d.mts` file content via the browsing tool here (only the metadata page was accessible), so I can’t paste the exact `type`/`interface` text for `BasicAuthOptions`. If you open the types entry locally, you can jump straight to it with:
```bash
node -p "require('h3/package.json').types"
# then open that file and search for: BasicAuthOptionsSources:
[1] h3@2.0.1-rc.14 package.json on unpkg (shows types path and exports). (unpkg.com)
Citations:
🏁 Script executed:
# Try to access the h3 package type definitions through npm or git
curl -s https://unpkg.com/h3@2.0.1-rc.14/dist/_entries/generic.d.mts 2>/dev/null | grep -A 10 "BasicAuthOptions"Repository: nitrojs/nitro
Length of output: 5941
🌐 Web query:
h3 github BasicAuthOptions interface definition source code
💡 Result:
In h3 (v2) the BasicAuthOptions type is defined like this (from the published typings generated from the h3js/h3 repo):
type _BasicAuthOptions = {
/** Validate username for basic auth. */
username: string;
/** Simple password for basic auth. */
password: string;
/** Custom validation function for basic auth. */
validate: (username: string, password: string) => boolean | Promise<boolean>;
/**
* Realm for the basic auth challenge.
* Defaults to "auth".
*/
realm: string;
};
type BasicAuthOptions =
Partial<_BasicAuthOptions> &
({ validate: _BasicAuthOptions["validate"] } | { password: _BasicAuthOptions["password"] });Source (file): dist/h32.d.mts in the h3 package build output. [1] (Repo: h3js/h3.) [2]
References: [1] (app.unpkg.com) [2] (github.com)
Citations:
- 1: https://app.unpkg.com/h3%402.0.1-rc.8/files/dist/h32.d.mts
- 2: https://github.com/h3js/h3?utm_source=openai
🏁 Script executed:
# Try to extract h3 full types from unpkg with more specific content request
python3 << 'EOF'
import urllib.request
import re
url = "https://unpkg.com/h3@2.0.1-rc.14/dist/_entries/generic.d.mts"
try:
with urllib.request.urlopen(url, timeout=5) as response:
content = response.read().decode('utf-8')
# Search for BasicAuthOptions definition
match = re.search(r'(interface|type)\s+BasicAuthOptions\s*[{=][^}]*}', content, re.DOTALL)
if match:
print("Found BasicAuthOptions:")
print(match.group(0)[:500])
else:
# Try to find it in a different format
if 'BasicAuthOptions' in content:
idx = content.find('BasicAuthOptions')
print("BasicAuthOptions context:")
print(content[max(0, idx-100):min(len(content), idx+300)])
else:
print("BasicAuthOptions not found in file")
except Exception as e:
print(f"Error: {e}")
EOFRepository: nitrojs/nitro
Length of output: 191
Update NitroBasicAuthOptions to include realm and validate fields from h3's BasicAuthOptions.
h3's BasicAuthOptions in version 2.0.1-rc.14 includes realm?: string (defaults to "auth") and an optional validate?: (username: string, password: string) => boolean | Promise<boolean> for custom authentication logic. NitroBasicAuthOptions is currently missing both fields. Passing incomplete options to requireBasicAuth at line 89 of src/runtime/internal/route-rules.ts will silently drop support for realm customization and user-defined validation functions, causing incorrect behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/types/route-rules.ts` at line 34, Update the NitroBasicAuthOptions type
to match h3's BasicAuthOptions by adding optional realm?: string and validate?:
(username: string, password: string) => boolean | Promise<boolean>; then ensure
wherever NitroBasicAuthOptions is forwarded into requireBasicAuth (the call in
route-rules handling) accepts and passes these fields through unchanged so realm
customization and custom validation functions are preserved.
| it("skips auth when set to false", async () => { | ||
| const { status } = await callHandler({ url: "/rules/no-auth/test" }); | ||
| expect(status).toBe(200); | ||
| }); |
There was a problem hiding this comment.
auth: false test only covers a standalone route — the documented child-overrides-parent scenario is untested.
The docs explicitly show /admin/** with auth and /admin/public/** with auth: false as the use-case for this feature, but the test fixture tests an entirely separate /rules/no-auth/** route that never had a parent auth rule. If the route-rule merger doesn't correctly handle the case where both a parent (auth: {...}) and a child (auth: false) match simultaneously, the 401 from the parent handler would fire before the child's auth: false guard could take effect. Adding an overlapping fixture pair (e.g., /rules/auth/** + /rules/auth/public/** with auth: false) would validate this scenario.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tests.ts` around lines 433 - 436, Add a new test that verifies child
route rules can override a parent auth rule: create overlapping fixtures
analogous to the docs (e.g., a parent rule matching "/rules/auth/**" that
requires auth and a child rule matching "/rules/auth/public/**" with auth:
false), then use the existing callHandler helper to request a child path like
"/rules/auth/public/test" and assert a 200 response (instead of the parent's
401). Place the test alongside the existing "skips auth when set to false" test
and reference the same callHandler helper and route patterns to ensure the
route-rule merger correctly prefers the child auth: false override.
|
Will push fixes for those soon |
1 participant
🔗 Linked issue
Closes #3924
❓ Type of change
📚 Description
Adds support for configuring basic auth through route rules
📝 Checklist