Repository navigation
Put the app's commands behind the capability ACL, trim plugin grants, keep the window on the app's pages - #123
Conversation
Tauri checks a command invoked from the app's own pages against the capability files only when it is a plugin command or the app ships an ACL manifest. Clavyn shipped none, so capabilities/default.json gated plugin commands alone, and every command in generate_handler! -- the local shell, session_write, the vault, SFTP -- answered any window on a local page without consulting it. Remote pages were already refused: the runtime puts every non-local origin through the ACL whatever the manifest says. permissions/app-commands.toml declares one allow-<command> permission per registered command, which gives the app a manifest, and the capability grants each one to the main window on local pages. Nothing else is granted them: another window, another webview or a remote page gets a permission error before the handler runs. The permissions are written out rather than generated by build.rs from a command list. tauri-build writes generated ones into the source tree, and an ignored leftover of them is picked up by the build of any commit without this change, giving that build a manifest with no grants, where every command is refused. The command list now lives in three places, and a command registered but not granted fails only when it is called. tests/app_command_acl.rs reads main.rs, the permissions file and the capability as they ship and fails when they disagree, and when the capability widens past the main window or adds a remote context. command_acl_tests.rs builds the shipped context on the mock runtime and invokes through on_message: the main window on a local page is served, another window and a remote page are refused, and the same call without a manifest is served from any local window.
core:default bundles the default set of every core plugin: menu, path, app, image, window, webview, resources and tray. The frontend uses one of them, listen and unlisten from the event plugin. The rest let a script in the page replace the app and window menus with items of its choosing and resolve the user's home and data directories, among others. os:default granted the os plugin's locale command. Nothing in the frontend imports @tauri-apps/plugin-os. The capability now grants event listen and unlisten, the debug-build devtools hotkey Tauri injects (inert in release builds, which do not register the command), and shell open, which tauri.conf.json already scopes to the two GitHub URLs the UI opens. A test pins that list, so widening it is a visible change to the test as well as to the capability.
The webview followed any top-level navigation: a script setting location, or a link dropped onto the window (the native drop handler is off, so the webview's own drop behaviour applies). A remote page loaded that way cannot invoke the app's commands, which are granted to local pages only, but it is drawn inside Clavyn's window, where it can pass for the vault unlock screen or a host password prompt. The app never navigates its window -- it is a single page, and its external links go through shell open -- so a plugin now cancels every navigation that leaves the app's origin: devUrl in a dev build, and the custom protocol (tauri.localhost on WebView2, tauri://localhost elsewhere) in a packaged one. A refused URL is logged by origin and not passed to the system browser, which would give page script an open outside the shell plugin's scope.
Scripts were governed by default-src alone, so any source later added to default-src for images, fonts or a new connection would also have become a place scripts load from. script-src now says 'self' itself. The policy test asserted the directive's absence on the grounds that Tauri would overwrite a declared one. It does not: it appends a hash of every bundled script file, and any inline-script hashes and nonces, to the declared directive, keeps 'self', and creates the directive only when it is missing. A packaged build sends the same script-src either way: 'self' followed by the bundle's hashes. The test now pins the declared value, without 'unsafe-eval' or 'wasm-unsafe-eval'; the bundle uses neither eval nor WebAssembly.
generate_context! embeds Info.plist on macOS under a fixed symbol, and the desktop binary's own context already defines it, so the test build failed to link on macOS with _EMBED_INFO_PLIST defined twice. test = true skips only that embed; the capabilities and ACL under test are the same.
computerbox124
left a comment
There was a problem hiding this comment.
Code review: 2 findings, inline.
- Medium: the navigation guard covers top-level navigations only.
window.open/target=_blankpopups bypass it, and on WebView2 can render a remote page that poses as an app screen (navigation.rs:22). - Low:
os:defaultis dropped, buttauri_plugin_os::init()and the crate are still there (capabilities/default.json,main.rs:128).
The ACL manifest, its grant test and the CSP change look sound. The comment-hygiene check passes on this branch.
Generated by Claude Code
|
|
||
| pub fn init<R: Runtime>() -> TauriPlugin<R> { | ||
| Builder::new("navigation-guard") | ||
| .on_navigation(|webview, url| { |
There was a problem hiding this comment.
Medium: on_navigation doesn't see new-window requests.
window.open(url) and target=_blank links produce new-window requests. They aren't top-level navigations of this webview, so this hook is never asked about them. On WebView2, unless something handles the new-window request, the default is a popup that renders the remote page. That popup can pose as the vault unlock or password prompt, which is the spoofing this module says it prevents. So the module doc's "every top-level navigation that would leave the app's origin is cancelled, whatever started it" overstates the guarantee.
The app itself calls window.open in SettingsView.vue (lines 66 and 74) and UpdateModal.vue, so this path is live.
Suggestion: deny new-window requests too (recent Tauri 2 has an on_new_window handler on the webview builder), and send the app's own external links through the shell plugin. Then check in a packaged Windows build that window.open('https://example.com') opens nothing. I haven't confirmed this on a real build.
Generated by Claude Code
There was a problem hiding this comment.
You're right that on_navigation never sees new-window requests. The popup doesn't open, though. I checked against the versions in Cargo.lock (tauri / tauri-runtime-wry 2.12.1, wry 0.57.0): no webview here registers on_new_window, and each backend refuses the popup when no handler is set.
- WebView2: wry always subscribes to
NewWindowRequested. With nonew_window_req_handler, it takes theelsebranch, callsargs.SetHandled(true)and never setsNewWindow.window.openreturnsnulland nothing renders (wry/src/webview2/mod.rs). - WKWebView:
webView:createWebViewWithConfiguration:forNavigationAction:windowFeatures:returnsNonewhen no handler is set (wry_web_view_ui_delegate.rs). - WebKitGTK: wry only calls
connect_createwhen a handler is set. The defaultcreatehandler returns no webview.
Tauri only passes a handler to wry when WebviewBuilder::on_new_window is called (tauri-runtime-wry lib.rs), and nothing in the app calls it. So the window.open calls in SettingsView.vue / UpdateModal.vue open nothing in the app. They're also only reached when the dynamic import of the bundled shell plugin fails. I haven't confirmed this on a packaged Windows build, only from source.
The doc did overstate what the module covers, though. f5f8972 adds a paragraph to the module doc: new-window requests never reach this hook, what refuses them on each backend, and that a webview registering on_new_window makes its handler responsible for refusing non-app URLs.
I haven't added an explicit on_new_window(|_, _| NewWindowResponse::Deny). The handler can only be set on the builder, so the main window would have to leave app.windows ("create": false) and be built in setup, which changes startup order. That would buy a log line for refused popups and independence from wry's no-handler default, which holds on all three backends. It wouldn't change what opens today. I'm leaving this thread open so you can decide. If you'd rather have the refusal in code, it's a small follow-up.
Generated by Claude Code
os:default left the capability because nothing in the frontend imports @tauri-apps/plugin-os, but the plugin itself stayed registered. Its init script runs in every page whatever the capability grants and defines window.__TAURI_OS_PLUGIN_INTERNALS__ with the platform, OS type, family, version, architecture, EOL and executable extension, so page script could still read them with no grant. Nothing in the app reads them. Dropping the plugin and the crate removes that, along with os_info, sys-locale and gethostname, which only it pulled in.
The navigation guard sees top-level navigations of the webview it is attached to. window.open and target=_blank links request a new window instead, which never reaches on_navigation, and the module doc did not say what handles them. They open nothing because no webview registers on_new_window. With no handler, wry 0.57 marks a WebView2 NewWindowRequested as handled without supplying a window, its WKWebView UI delegate returns none, and WebKitGTK falls back to its default create handler, which makes none. The doc now says so, and that a webview which registers a handler takes on refusing non-app URLs itself.
| "allow-connect-ssh", | ||
| "allow-create-local-terminal", | ||
| "allow-session-write", |
There was a problem hiding this comment.
1. Webview script can control local shells 📎 Requirement gap ⛨ Security
The capability grants the main app webview both create_local_terminal and session_write without any backend-attested authorization. JavaScript running on that page can therefore spawn a shell or send bytes to an existing local terminal by session ID, including terminals opened through the normal UI.
Agent Prompt
## Issue description
The main webview is granted both local-terminal creation and session input commands, while the backend authorizes requests only by command parameters and session ID. Webview JavaScript can create a shell or write to an existing local terminal without backend-attested authorization.
## Fix Focus Areas
- desktop/src-tauri/capabilities/default.json[51-53]
- desktop/src-tauri/permissions/app-commands.toml[177-183]
- desktop/src-tauri/src/commands.rs[669-766]
- desktop/src-tauri/src/commands.rs[866-894]
## Recommended Fix
Remove direct webview access to terminal creation and session input, or require a backend-issued authorization bound to the intended terminal and session before both commands execute. Ensure `session_write` cannot act solely on a caller-supplied session ID and that the normal UI obtains authorization through a backend-controlled flow.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
The packaged WebView2 branch of the navigation guard accepted both http://tauri.localhost and https://tauri.localhost. WebView2 intercepts only the scheme the window is configured with; the app leaves useHttpsScheme at its default, so https://tauri.localhost is not the app at all but an ordinary request to loopback port 443. Tauri still counts both schemes as local when it checks commands, so a page served there and let into the main window would get every granted command. The guard now reads useHttpsScheme from the window config with the webview's label and accepts that scheme alone. A webview with no window config counts as http, Tauri's default, so a mismatch refuses the app's own pages visibly instead of accepting a second origin.
Nothing uses tauri-plugin-process: the frontend does not import @tauri-apps/plugin-process, and restarts go through AppHandle::request_restart. With no grant in the capability its exit and restart commands were unreachable from the page, so it only added a registered plugin and a crate. Removing it means a later process: grant cannot hand page script exit or restart by accident.
Every page-level confirmation went through window.confirm, which the
dialog plugin's init script replaces with an async function. Its
Promise is truthy whatever the user answers, so `if (confirm(...))`
always went ahead: hosts, groups, identities, vault keys, workspaces,
known hosts and remote SFTP files were deleted the moment the button
was pressed. The question never even appeared. The replacement calls
plugin:dialog|confirm, which tauri-plugin-dialog 2.8 no longer
registers and no permission allows. The overwrite prompt for an SFTP
upload assigned that Promise to a bool, which reached the backend as
{} and failed every upload over an existing file. window.alert goes
through plugin:dialog|message, which was not granted, so every error
alert was dropped.
confirmAction() makes the call the plugin's own confirm() makes,
message with OK/Cancel buttons, awaits the answer, and treats a dialog
that cannot be shown as Cancel. Every confirm() site uses it. The
capability grants dialog:allow-message for it and for the alert
replacement; the file dialogs stay ungranted, since the backend shows
those itself.
The browser fixture answers the dialog through the browser's own
confirm/alert, so a harness scenario can accept or dismiss it.
IdentityManager's identity delete conflicted: main wraps the delete in a try/catch that stays quiet on a declined host change, and this branch asks through confirmAction instead of window.confirm. The resolution keeps main's early return and error handling and asks through confirmAction. The identity delete test from main now answers the dialog plugin's message command instead of stubbing window.confirm, which no longer decides anything. main adds no commands to generate_handler!, so the permissions and the capability grants stay in step.
The capability checks read capabilities/default.json alone, but Tauri enables every file under capabilities/ and any capability listed in tauri.conf.json's app.security.capabilities. A second capability that granted session_write to a remote URL or to another window passed every check. The test now fails unless default.json is the only capability, so adding one means extending the checks to it. The handler list was read up to the first `]`. generate_handler! takes outer attributes on its entries, so a #[cfg(...)] command cut the list short at the attribute, and a `//` comment in the list read as a command name. The reader now drops comments and attributes and stops at the bracket that closes the macro, with a test of its own.
|
@codex review the PR |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
computerbox124
left a comment
There was a problem hiding this comment.
@Sadykhzadeh thank you!
Refs #30. Refs #12.
What this does
#30 left a choice between (a) backend-attested terminal creation and (b) reducing webview→backend privilege generally. This takes (b). (a) does not close the primitive: keystrokes reach every session through
session_write, called from the webview where xterm lives, and that command resolves a session by id alone — local or SSH. A creation gate leaves script in the page able to type into the local terminal the user already has open (local terminals are the default tab), or into an SSH session, which is worse. All (a) adds is closing the moment when no local terminal exists, and it costs a UX change to the default tab and Cmd/Ctrl+N.1. App commands go through the capability ACL
Tauri checks an app command from the app's own pages against the capability files only when the app ships an ACL manifest. Clavyn had none, so
capabilities/default.jsongated plugin commands only, and every command ingenerate_handler!answered any window on a local page. (Remote pages were already refused: the runtime puts every non-local origin through the ACL.)permissions/app-commands.tomldeclares oneallow-<command>per registered command, and the capability grants each to themainwindow on local pages. Another window, another webview or a remote page is refused before the handler runs. The permissions are written out rather than generated: tauri-build writes generated ones into the source tree, and an ignored leftover gives the build of any older commit a manifest with no grants.tests/app_command_acl.rsreadsmain.rs, the permissions file and the capability as shipped and fails when they disagree, when the capability widens pastmainor addsremote, when a capability other thandefault.jsonappears (incapabilities/or inline intauri.conf.json), or when plugin grants change. A command added togenerate_handler!without a permission and a grant fails this test,#[cfg]-gated entries included.src/command_acl_tests.rsbuilds the shipped context on the mock runtime and invokes throughon_message: main window + local page is served; another window and a remote page get the ACL error; the same call without a manifest is served from any local window.2. Plugin grants trimmed to what the frontend calls
core:default(menus, path, app, window, image, …) becomes eventlisten/unlistenplus the debug-only devtools hotkey.shell:allow-openstays, already scoped to the two GitHub URLs (the plugin anchors the regex).dialog:allow-messageis granted for the page's native message dialogs (see 5); the file dialogs (open,save) stay ungranted, since the backend shows those itself.The OS and process plugins are no longer loaded; nothing in the app used either. The OS plugin's init script exposed platform, OS version and architecture to every page whatever the capability granted. The process plugin had no grant, so its
exit/restartwere unreachable, but a laterprocess:grant would have handed them to page script.3. The main window stays on the app's origin
A plugin
on_navigationhook cancels top-level navigations that leavedevUrl(dev) or the custom protocol (packaged:http://tauri.localhoston WebView2,tauri://localhostelsewhere). A remote page in the main window cannot invoke commands, but it would be drawn in Clavyn's frame and could pose as the vault unlock or password prompt. Refused URLs are logged by origin and not handed to the OS browser, which would bypass the shell scope.On WebView2 only the scheme the window is configured with is accepted, read from its
useHttpsScheme(unset here, sohttp). WebView2 intercepts that scheme alone;https://tauri.localhostis an ordinary request to loopback port 443, which Tauri still counts as local when it checks commands.window.openandtarget="_blank"ask for a new window rather than navigating, so they never reach this hook. They open nothing because no webview registerson_new_window: with no handler, wry 0.57 refuses the popup on all three backends (WebView2 marks the request handled without supplying a window, the WKWebView UI delegate returns none, WebKitGTK has nocreatehandler). The module doc records this, and that a webview which registers a handler has to refuse non-app URLs itself.4.
script-src 'self'declaredScripts no longer inherit whatever
default-srcbecomes. Tauri appends its bundle hashes to the declared directive and keeps'self', so the header a packaged build sends is unchanged (verified: identical before and after).5. Page confirmations wait for an answer
This belongs here because section 2 claims the grants are what the frontend calls, and the frontend calls this. Every delete in the UI asked through
window.confirm, and every error went throughwindow.alert. The dialog plugin's init script replaces both with IPC calls, and neither was granted, onmaineither. Theconfirmreplacement is also async: its Promise is truthy whatever the user answers, soif (confirm(...))always went ahead. Hosts, groups, identities, vault keys, workspaces, known hosts and remote SFTP files were deleted with no question shown. The SFTP overwrite prompt sent that Promise as a bool, so every upload over an existing file failed. Granting the command would not have been enough: the replacement callsplugin:dialog|confirm, which tauri-plugin-dialog 2.8 no longer registers.confirmAction()insrc/lib/confirmAction.tsmakes the call the plugin's ownconfirm()makes (plugin:dialog|messagewith OK/Cancel), awaits the answer, and treats a dialog that cannot be shown as Cancel. All nine call sites use it.window.alertworks oncedialog:allow-messageis granted. These questions guard against misclicks only; host key and host change confirmations stay in the backend's native dialogs, where page script cannot answer for the user. The browser fixture answers the dialog through the browser's ownconfirm/alert, so harness scenarios can accept or dismiss it.What this does not close
Script running in the bundled app's own page still reaches
session_write,create_local_terminaland every other command the UI uses. That is inherent to a terminal whose input handling lives in the webview. The barrier for it remains what #30 describes: a CSP with no'unsafe-eval'and no remote script sources, and nov-html/innerHTML/eval/new Functionor link addon indesktop/src. I think #30 can close with that recorded as accepted risk; further trust-boundary work belongs under #12.list_sessionsis registered and granted but the frontend never calls it; dropping it would stop handing page script the live session ids. Left for a separate change.security-auditfails onzip4.6.1 (RUSTSEC-2026-0367/0368/0369), pulled in bytauri-plugin-updater2.13.1.mainhas the same lockfile entry; the dependency group bump in #136 moves the updater to 2.14.1, which depends onzip9.Test plan
cargo check --workspace --lockedcargo test -p clavyn-core,cargo test -p clavyn-desktop(149 unit + 6 ACL + 2 CSP; new here: 6 file tests, 2 IPC tests, 5 navigation tests, updated CSP test)cd desktop && npx vue-tsc --noEmit,npm test(400 passed; new:confirmActiontests, cancelled known-host removal)node --test scripts/verify/check-comments.test.mjs,node scripts/verify/check-comments.mjscd desktop/e2e && UI_RECORD=0 npm test, after mergingmaindelete_host; OK sendsdelete_host("atlas")path,menu,app,os,windowplugin calls refused. Navigation to a remote URL blocked and logged. A build without the guard, navigated to a non-app origin, getslist_hosts not allowed on window "main" … allowed on: [windows: "main", URL: local].tauri dev(at e5ccd92): same checks againstdevUrl.https://tauri.localhostrefused on Windows, plus a screenshot of the dialog. Not run.tauri://localhostbranch of the navigation guard) — unit tests only<source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v3.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-light-v3.svg"><img alt="View with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v3.svg">https://backend.blacksmith.sh/track/enable-autofix?expires=1794093378&installation_model_id=438384&pr_number=123&ref=codesmith_pr_footer&repository=CodeAny-inc%2FClavyn&return_to=https%3A%2F%2Fgithub.com%2FCodeAny-inc%2FClavyn%2Fpull%2F123&signature=1a422c80b94638ccee2647d4ce9a4020473b5d263b5f3db98c75093c4cd6df70"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img alt="Autofix with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg">Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.