feat!: remove deprecated auth token args from builtin RPC functions#965
Open
feat!: remove deprecated auth token args from builtin RPC functions#965
Conversation
Auth is now handled by Vite DevTools. Remove the `token: string` parameter from all 18 RPC function signatures, their server implementations, and all client call sites. Also remove `ensureDevAuthToken` from the server context and client composable. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
antfu
changed the title
Deploying nuxt-devtools with
|
| Latest commit: |
4018b47
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://aa8008c0.nuxt-devtools.pages.dev |
| Branch Preview URL: | https://antfu-remove-rpc-auth-token.nuxt-devtools.pages.dev |
Contributor
📝 WalkthroughWalkthroughThis pull request systematically removes token-based authentication from the Nuxt devtools RPC interface and implementation. The Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 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📝 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/devtools/src/server-rpc/terminals.ts (1)
84-88:⚠️ Potential issue | 🟠 MajorAwait termination before removing a live terminal.
Line 86 starts termination without awaiting it, so the entry can disappear from
terminalswhile the process is still running, and any rejection becomes unhandled.removeshould wait for termination to finish—or fail if the terminal cannot be terminated—before deleting the state.Suggested fix
case 'remove': - if (!terminal.isTerminated) - terminal.onActionTerminate?.() + if (!terminal.isTerminated) { + if (!terminal.onActionTerminate) + return false + await terminal.onActionTerminate() + } terminals.delete(id) refresh('getTerminals') return true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/devtools/src/server-rpc/terminals.ts` around lines 84 - 88, The remove case currently calls terminal.onActionTerminate without awaiting it, causing the terminals map to be deleted while the process may still run; update the 'remove' branch to: if terminal.isTerminated skip termination; otherwise call and await terminal.onActionTerminate(), catch and rethrow/log failures (do not delete on failure), and only call terminals.delete(id) and refresh('getTerminals') after the awaited termination succeeds (ensure you handle the case where onActionTerminate is undefined by treating it as already-terminated or by awaiting a resolved promise).packages/devtools/src/server-rpc/assets.ts (1)
78-145:⚠️ Potential issue | 🔴 CriticalFence every asset path to the public roots.
With the token parameter gone, these handlers now depend entirely on transport auth, so they still need server-side path confinement. Line 103 can resolve
folderoutsidepublicDir, and Lines 78-100 plus Lines 137-144 accept arbitrary filesystem paths for read/rename/delete. Please normalize each input and reject anything that is not insidepublicDiror one oflayerDirs; the current stringstartsWithcheck is bypassable by sibling prefixes like.../public2.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/devtools/src/server-rpc/assets.ts` around lines 78 - 145, Normalize and validate all incoming asset paths against the allowed public roots before any fs operation: for writeStaticAssets validate and resolve the provided folder into the computed baseDir and ensure each resolved finalPath is inside baseDir using path.resolve + path.relative (do not rely on startsWith), and also support checking against additional allowed layerDirs; for getImageMeta and getTextAssetContent, first resolve the given filepath and reject if it is not within publicDir or any layerDirs; likewise for deleteStaticAsset and renameStaticAsset validate both oldPath/newPath are resolved and contained in allowed roots before calling fsp.unlink/fsp.rename; on failure, throw a clear error and do not perform the fs operation.packages/devtools/src/server-rpc/npm.ts (1)
142-159:⚠️ Potential issue | 🔴 CriticalRemove the matched module, not its predecessor.
Line 157 uses
splice(index - 1, 1), so uninstalling the first module removes the last one, and every other match removes the entry before it. That can silently corruptnuxt.config.Suggested fix
const config = getDefaultExportOptions(mod) config.modules ||= [] - if (config.modules.includes(name)) { - Object.values(config.modules).forEach((value, index) => { - if (value === name) - config.modules.splice(index - 1, 1) - }) - } + for (let i = config.modules.length - 1; i >= 0; i--) { + if (config.modules[i] === name) + config.modules.splice(i, 1) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/devtools/src/server-rpc/npm.ts` around lines 142 - 159, In uninstallNuxtModule, the removal logic in the config.modules handling is wrong: it calls config.modules.splice(index - 1, 1) which deletes the previous element; change this to remove the matched element itself (splice(index, 1)) and make the loop safe against index shifting (iterate backwards or build a new array/filter instead). Update the block that currently uses Object.values(config.modules).forEach((value, index) => { ... }) to either loop from the end (for (let i = config.modules.length - 1; i >= 0; i--) ...) and splice(i, 1) when config.modules[i] === name, or replace config.modules with config.modules.filter(v => v !== name).
🧹 Nitpick comments (1)
packages/devtools/client/components/RestartDialogs.vue (1)
24-27: Make restart call explicitly fire-and-forget with error handling.Line 24 currently drops the returned promise. Consider handling rejection to avoid unhandled promise noise.
Suggested refactor
- rpc.restartNuxt() + void rpc.restartNuxt().catch((error) => { + console.error('[devtools] restartNuxt failed', error) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/devtools/client/components/RestartDialogs.vue` around lines 24 - 27, The call to rpc.restartNuxt() is currently dropped and can produce unhandled promise rejections; make it an explicit fire-and-forget by invoking rpc.restartNuxt() and attaching a rejection handler (for example using .catch or an awaited async IIFE) to log or swallow errors, while keeping the existing delayed client.value?.app.reload() call; update the code around rpc.restartNuxt() so the promise is consumed (e.g., void rpc.restartNuxt().catch(err => /* log */)) to prevent unhandled promise noise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/devtools/client/pages/modules/overview.vue`:
- Around line 39-41: The authorize() function is now a no-op but the UI still
prompts "Click to authorize now", so update the component to remove the
misleading click target: either delete the onClick binding that calls
authorize() or disable the button/anchor and change the label to reflect that
auth is handled by Vite DevTools (e.g., "Authorization handled by Vite
DevTools"), and ensure any click handlers or aria attributes referencing
authorize() are removed or updated; search for authorize() and the literal
"Click to authorize now" in the component to locate and update the UI elements
and event bindings accordingly.
---
Outside diff comments:
In `@packages/devtools/src/server-rpc/assets.ts`:
- Around line 78-145: Normalize and validate all incoming asset paths against
the allowed public roots before any fs operation: for writeStaticAssets validate
and resolve the provided folder into the computed baseDir and ensure each
resolved finalPath is inside baseDir using path.resolve + path.relative (do not
rely on startsWith), and also support checking against additional allowed
layerDirs; for getImageMeta and getTextAssetContent, first resolve the given
filepath and reject if it is not within publicDir or any layerDirs; likewise for
deleteStaticAsset and renameStaticAsset validate both oldPath/newPath are
resolved and contained in allowed roots before calling fsp.unlink/fsp.rename; on
failure, throw a clear error and do not perform the fs operation.
In `@packages/devtools/src/server-rpc/npm.ts`:
- Around line 142-159: In uninstallNuxtModule, the removal logic in the
config.modules handling is wrong: it calls config.modules.splice(index - 1, 1)
which deletes the previous element; change this to remove the matched element
itself (splice(index, 1)) and make the loop safe against index shifting (iterate
backwards or build a new array/filter instead). Update the block that currently
uses Object.values(config.modules).forEach((value, index) => { ... }) to either
loop from the end (for (let i = config.modules.length - 1; i >= 0; i--) ...) and
splice(i, 1) when config.modules[i] === name, or replace config.modules with
config.modules.filter(v => v !== name).
In `@packages/devtools/src/server-rpc/terminals.ts`:
- Around line 84-88: The remove case currently calls terminal.onActionTerminate
without awaiting it, causing the terminals map to be deleted while the process
may still run; update the 'remove' branch to: if terminal.isTerminated skip
termination; otherwise call and await terminal.onActionTerminate(), catch and
rethrow/log failures (do not delete on failure), and only call
terminals.delete(id) and refresh('getTerminals') after the awaited termination
succeeds (ensure you handle the case where onActionTerminate is undefined by
treating it as already-terminated or by awaiting a resolved promise).
---
Nitpick comments:
In `@packages/devtools/client/components/RestartDialogs.vue`:
- Around line 24-27: The call to rpc.restartNuxt() is currently dropped and can
produce unhandled promise rejections; make it an explicit fire-and-forget by
invoking rpc.restartNuxt() and attaching a rejection handler (for example using
.catch or an awaited async IIFE) to log or swallow errors, while keeping the
existing delayed client.value?.app.reload() call; update the code around
rpc.restartNuxt() so the promise is consumed (e.g., void
rpc.restartNuxt().catch(err => /* log */)) to prevent unhandled promise noise.
🪄 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: d9f69fc0-7d42-465d-b375-ce935d411124
📒 Files selected for processing (25)
packages/devtools-kit/src/_types/rpc.tspackages/devtools-kit/src/_types/server-ctx.tspackages/devtools/client/app.vuepackages/devtools/client/components/AssetDetails.vuepackages/devtools/client/components/AssetDropZone.vuepackages/devtools/client/components/BuildAnalyzeDetails.vuepackages/devtools/client/components/ModuleItemInstall.vuepackages/devtools/client/components/RestartDialogs.vuepackages/devtools/client/components/StorageDetails.vuepackages/devtools/client/components/TerminalPage.vuepackages/devtools/client/components/TerminalView.vuepackages/devtools/client/composables/dev-auth.tspackages/devtools/client/composables/npm.tspackages/devtools/client/pages/modules/analyze-build.vuepackages/devtools/client/pages/modules/overview.vuepackages/devtools/client/pages/modules/pages.vuepackages/devtools/client/pages/modules/server-discovery.vuepackages/devtools/src/server-rpc/analyze-build.tspackages/devtools/src/server-rpc/assets.tspackages/devtools/src/server-rpc/general.tspackages/devtools/src/server-rpc/index.tspackages/devtools/src/server-rpc/npm.tspackages/devtools/src/server-rpc/server-data.tspackages/devtools/src/server-rpc/storage.tspackages/devtools/src/server-rpc/terminals.ts
💤 Files with no reviewable changes (3)
- packages/devtools/client/composables/dev-auth.ts
- packages/devtools-kit/src/_types/server-ctx.ts
- packages/devtools/src/server-rpc/index.ts
Comment on lines
39
to
41
| function authorize() { | ||
| ensureDevAuthToken() | ||
| // Auth is now handled by Vite DevTools | ||
| } |
Contributor
There was a problem hiding this comment.
authorize() is now a dead click target while UI still prompts users to click.
Line 39–41 makes the action a no-op, but the page still says “Click to authorize now”, which is misleading and non-functional.
Suggested fix
-function authorize() {
- // Auth is now handled by Vite DevTools
-}
+// Auth is handled by Vite DevTools; no manual action is available.- <button title="Authorize" `@click`="authorize">
+ <div title="Authorize">
<NTip v-if="!isDevAuthed" n="orange5" icon="i-carbon-locked" justify-center>
- Access from an untrusted browser, some features are limited. Click to authorize now.
+ Access from an untrusted browser, some features are limited. Authorization is handled by Vite DevTools.
</NTip>
- </button>
+ </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/devtools/client/pages/modules/overview.vue` around lines 39 - 41,
The authorize() function is now a no-op but the UI still prompts "Click to
authorize now", so update the component to remove the misleading click target:
either delete the onClick binding that calls authorize() or disable the
button/anchor and change the label to reflect that auth is handled by Vite
DevTools (e.g., "Authorization handled by Vite DevTools"), and ensure any click
handlers or aria attributes referencing authorize() are removed or updated;
search for authorize() and the literal "Click to authorize now" in the component
to locate and update the UI elements and event bindings accordingly.
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
token: stringparameter from all 18 builtin RPC function signatures inServerFunctionsinterfaceensureDevAuthTokencalls from all server RPC implementations (7 files) and from the server contextensureDevAuthTokenexport from the clientdev-authcomposableAuth is now handled by Vite DevTools, so these token parameters were deprecated noops that only logged warnings.
Test plan
pnpm typecheckpasses🤖 Generated with Claude Code