Security fixes for allowed tauri features and packages update#292
Security fixes for allowed tauri features and packages update#292
Conversation
🦋 Changeset detectedLatest commit: a6922be The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull Request Overview
Updates Tauri permissions for improved security, removes obsolete dependencies in favor of local paths, and fixes input focus propagation in the UI.
- Refine
tauri.conf.jsonallowlist fromallto granular permissions - Switch unused remote crates to local path dependencies in
Cargo.toml - Add
onMouseDownstop-propagation handlers to textareas to prevent input blur
Reviewed Changes
Copilot reviewed 49 out of 68 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src-tauri/tauri.conf.json | Replace broad all allowlist with explicit feature flags |
| src-tauri/Cargo.toml | Remove remote plugin deps, point to local paths, trim crates |
| packages/.../ClipEdit.tsx | Add onMouseDown to stop click propagation |
| packages/.../BoardEdit.tsx | Add onMouseDown to stop click propagation |
| packages/.../textarea/index.tsx | Expose and forward onMouseDown/onClick props to textarea |
Files not reviewed (2)
- src-tauri/libs/tauri-plugin-single-instance/examples/emit-event/pnpm-lock.yaml: Language not supported
- src-tauri/libs/tauri-plugin-single-instance/examples/vanilla/pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (2)
src-tauri/Cargo.toml:106
- [nitpick] The umbrella
"clipboard"feature overlaps with the granularclipboard-read-textandclipboard-write-textflags; remove the redundant umbrella feature to align with the tightened allowlist.
"clipboard",
src-tauri/libs/tauri-plugin-single-instance/src/platform_impl/windows.rs:39
- [nitpick] The new mutex and IPC logic for single-instance behavior is critical and complex; consider adding unit or integration tests covering both first-instance and subsequent-instance flows to ensure reliable behavior across edge cases.
unsafe { CreateMutexW(std::ptr::null(), true.into(), mutex_name.as_ptr()) };
src-tauri/tauri.conf.json
Outdated
| "https://**", | ||
| "http://**" |
There was a problem hiding this comment.
Using a wildcard "http://**" allows calls to any HTTP endpoint, which can be a security risk; tighten this scope to trusted origins to minimize attack surface.
| "https://**", | |
| "http://**" | |
| "https://**" |
src-tauri/libs/tauri-plugin-single-instance/src/platform_impl/macos.rs
Outdated
Show resolved
Hide resolved
…oved-unused-packages
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…macos.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 49 out of 68 changed files in this pull request and generated 6 comments.
Files not reviewed (2)
- src-tauri/libs/tauri-plugin-single-instance/examples/emit-event/pnpm-lock.yaml: Language not supported
- src-tauri/libs/tauri-plugin-single-instance/examples/vanilla/pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "http://localhost:8788/api/error-report", | ||
| "https://contact.pastebar.app/api/error-report" | ||
| "https://contact.pastebar.app/api/error-report", | ||
| "http://**" |
There was a problem hiding this comment.
The HTTP scope includes "http://**" which allows the application to make requests to any HTTP endpoint. This is a significant security risk as it:
- Bypasses the principle of least privilege
- Could allow malicious actors to exploit the application to make arbitrary HTTP requests
- Defeats the purpose of having a scope whitelist
Consider removing this wildcard and explicitly listing only the HTTP endpoints that are actually needed by the application. If localhost access is required for development, consider using environment-specific configurations or a more restrictive pattern like "http://localhost:" or "http://127.0.0.1:".
| Platforms: | ||
| - [x] Windows | ||
| - [x] Linux | ||
| - [ ] macOS No newline at end of file |
There was a problem hiding this comment.
The README states that macOS support is not implemented (marked with [ ]), but the code includes a complete macOS implementation in platform_impl/macos.rs. This documentation is misleading and should be updated to reflect that macOS is actually supported. Please update line 8 to - [x] macOS.
| - [ ] macOS | |
| - [x] macOS |
| std::env::temp_dir() | ||
| }; | ||
|
|
||
| path.push(".pastebar"); |
There was a problem hiding this comment.
The socket path hardcodes ".pastebar" as the directory name (line 80), which appears to be application-specific. This makes the plugin less reusable for other applications. Consider deriving this from the application identifier or making it configurable. For example, you could use the sanitized bundle identifier instead of a hardcoded "pastebar" string.
| --- | ||
| 'pastebar-app-ui': patch | ||
| --- | ||
|
|
||
| Security fixes for allowed tauri features |
There was a problem hiding this comment.
The PR description states only "Edit Clip input focus fix" and "Removed unused packages", but the PR actually includes substantial additional changes:
- Complete implementation of a custom tauri-plugin-single-instance (Windows, macOS, Linux)
- Addition of macos-accessibility-client library
- Migration from external Git dependencies to local path dependencies
These are significant architectural changes that should be documented in the PR description. The title mentions "security fixes for allowed tauri features" which covers the tauri.conf.json changes, but the plugin implementations represent a major addition to the codebase that reviewers should be aware of upfront.
| #![cfg(target_os = "macos")] | ||
|
|
||
| use crate::SingleInstanceCallback; | ||
| use std::{ | ||
| io::{BufWriter, Error, ErrorKind, Read, Write}, | ||
| os::unix::net::{UnixListener, UnixStream}, | ||
| path::PathBuf, | ||
| }; | ||
| use tauri::{ | ||
| plugin::{self, TauriPlugin}, | ||
| AppHandle, Manager, RunEvent, Runtime, | ||
| }; | ||
|
|
||
| pub fn init<R: Runtime>(cb: Box<SingleInstanceCallback<R>>) -> TauriPlugin<R> { | ||
| plugin::Builder::new("single-instance") | ||
| .setup(|app| { | ||
| let socket = socket_path(&app.config()); | ||
|
|
||
| println!("Single-instance: Socket path: {:?}", socket); | ||
|
|
||
| // First, check if there's a stale socket file and clean it up | ||
| if is_socket_stale(&socket) { | ||
| println!("Single-instance: Cleaning up stale socket from previous crash"); | ||
| socket_cleanup(&socket); | ||
| } | ||
|
|
||
| // Notify the singleton which may or may not exist. | ||
| match notify_singleton(&socket) { | ||
| Ok(_) => { | ||
| println!("Single-instance: Found existing instance, exiting"); | ||
| std::process::exit(0); | ||
| } | ||
| Err(e) => { | ||
| match e.kind() { | ||
| ErrorKind::NotFound | ErrorKind::ConnectionRefused => { | ||
| println!("Single-instance: No existing instance found, creating new one"); | ||
| // This process claims itself as singleton as likely none exists | ||
| socket_cleanup(&socket); | ||
| listen_for_other_instances(&socket, app.clone(), cb); | ||
| } | ||
| _ => { | ||
| println!( | ||
| "Single-instance: Failed to notify existing instance, launching normally: {}", | ||
| e | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Ok(()) | ||
| }) | ||
| .on_event(|app, event| { | ||
| if let RunEvent::Exit = event { | ||
| destroy(app); | ||
| } | ||
| }) | ||
| .build() | ||
| } | ||
|
|
||
| pub fn destroy<R: Runtime, M: Manager<R>>(manager: &M) { | ||
| let socket = socket_path(&manager.config()); | ||
| println!("Single-instance: Cleaning up socket on exit: {:?}", socket); | ||
| socket_cleanup(&socket); | ||
| } | ||
|
|
||
| fn socket_path(config: &tauri::Config) -> PathBuf { | ||
| let identifier = config | ||
| .tauri | ||
| .bundle | ||
| .identifier | ||
| .replace(['.', '-'].as_ref(), "_"); | ||
|
|
||
| // Use home directory with .pastebar subdirectory for better organization | ||
| let mut path = if let Some(home) = std::env::var_os("HOME") { | ||
| PathBuf::from(home) | ||
| } else { | ||
| std::env::temp_dir() | ||
| }; | ||
|
|
||
| path.push(".pastebar"); | ||
|
|
||
| // Create the directory if it doesn't exist | ||
| if let Err(e) = std::fs::create_dir_all(&path) { | ||
| println!("Single-instance: Failed to create app directory: {}", e); | ||
| // Fallback to temp directory | ||
| return PathBuf::from(format!("/tmp/{}_si.sock", identifier)); | ||
| } | ||
|
|
||
| path.push(format!("{}_si.sock", identifier)); | ||
| path | ||
| } | ||
|
|
||
| fn socket_cleanup(socket: &PathBuf) { | ||
| if let Err(e) = std::fs::remove_file(socket) { | ||
| // Only log if it's not a "file not found" error | ||
| if e.kind() != ErrorKind::NotFound { | ||
| println!("Single-instance: Warning - failed to remove socket file: {}", e); | ||
| } | ||
| } else { | ||
| println!("Single-instance: Successfully removed socket file"); | ||
| } | ||
| } | ||
|
|
||
| fn is_socket_stale(socket: &PathBuf) -> bool { | ||
| // If socket file doesn't exist, it's not stale | ||
| if !socket.exists() { | ||
| return false; | ||
| } | ||
|
|
||
| // Try to connect to see if anyone is listening | ||
| match UnixStream::connect(socket) { | ||
| Ok(_) => { | ||
| // Someone is listening, socket is active | ||
| false | ||
| } | ||
| Err(e) => { | ||
| // Connection failed, socket is likely stale | ||
| match e.kind() { | ||
| ErrorKind::ConnectionRefused | ErrorKind::NotFound => { | ||
| println!("Single-instance: Detected stale socket file, will clean up"); | ||
| true | ||
| } | ||
| _ => { | ||
| println!("Single-instance: Unexpected error testing socket: {}", e); | ||
| false | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn notify_singleton(socket: &PathBuf) -> Result<(), Error> { | ||
| println!("Single-instance: Trying to notify existing instance"); | ||
| let stream = UnixStream::connect(socket)?; | ||
| let mut bf = BufWriter::new(&stream); | ||
| let cwd = std::env::current_dir() | ||
| .unwrap_or_default() | ||
| .to_str() | ||
| .unwrap_or_default() | ||
| .to_string(); | ||
| bf.write_all(cwd.as_bytes())?; | ||
| bf.write_all(b"\0\0")?; | ||
| let args_joined = std::env::args().collect::<Vec<String>>().join("\0"); | ||
| bf.write_all(args_joined.as_bytes())?; | ||
| bf.flush()?; | ||
| drop(bf); | ||
| println!("Single-instance: Successfully notified existing instance"); | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn listen_for_other_instances<A: Runtime>( | ||
| socket: &PathBuf, | ||
| app: AppHandle<A>, | ||
| mut cb: Box<SingleInstanceCallback<A>>, | ||
| ) { | ||
| // Ensure socket is clean before binding | ||
| socket_cleanup(socket); | ||
|
|
||
| match UnixListener::bind(socket) { | ||
| Ok(listener) => { | ||
| println!("Single-instance: Successfully bound to socket, listening for other instances"); | ||
| tauri::async_runtime::spawn(async move { | ||
| for stream in listener.incoming() { | ||
| match stream { | ||
| Ok(mut stream) => { | ||
| println!("Single-instance: Received connection from another instance"); | ||
| let mut s = String::new(); | ||
| match stream.read_to_string(&mut s) { | ||
| Ok(_) => { | ||
| let (cwd, args) = s.split_once("\0\0").unwrap_or_default(); | ||
| let args: Vec<String> = args.split('\0').map(String::from).collect(); | ||
| println!( | ||
| "Single-instance: Calling callback for new instance with args: {:?}", | ||
| args | ||
| ); | ||
| cb(&app, args, cwd.to_string()); | ||
| } | ||
| Err(e) => { | ||
| println!("Single-instance: Failed to read from stream: {}", e); | ||
| } | ||
| } | ||
| } | ||
| Err(err) => { | ||
| println!("Single-instance: Failed to accept connection: {}", err); | ||
| continue; | ||
| } | ||
| } | ||
| } | ||
| }); | ||
| } | ||
| Err(err) => { | ||
| println!( | ||
| "Single-instance: Failed to bind socket ({}), trying to clean up and retry once", | ||
| err | ||
| ); | ||
|
|
||
| // Try cleaning up and retrying once more | ||
| socket_cleanup(socket); | ||
| match UnixListener::bind(socket) { | ||
| Ok(listener) => { | ||
| println!("Single-instance: Successfully bound to socket on retry"); | ||
| // Same listening logic as above | ||
| tauri::async_runtime::spawn(async move { | ||
| for stream in listener.incoming() { | ||
| match stream { | ||
| Ok(mut stream) => { | ||
| println!("Single-instance: Received connection from another instance"); | ||
| let mut s = String::new(); | ||
| match stream.read_to_string(&mut s) { | ||
| Ok(_) => { | ||
| let (cwd, args) = s.split_once("\0\0").unwrap_or_default(); | ||
| let args: Vec<String> = args.split('\0').map(String::from).collect(); | ||
| println!( | ||
| "Single-instance: Calling callback for new instance with args: {:?}", | ||
| args | ||
| ); | ||
| cb(&app, args, cwd.to_string()); | ||
| } | ||
| Err(e) => { | ||
| println!("Single-instance: Failed to read from stream: {}", e); | ||
| } | ||
| } | ||
| } | ||
| Err(err) => { | ||
| println!("Single-instance: Failed to accept connection: {}", err); | ||
| continue; | ||
| } | ||
| } | ||
| } | ||
| }); | ||
| } | ||
| Err(retry_err) => { | ||
| println!( | ||
| "Single-instance: Failed to bind socket even after cleanup, launching normally: {}", | ||
| retry_err | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The macOS implementation contains numerous println! statements for debugging (lines 19, 23, 30, 36, 42, 62, 84, 97, 100, 120, 124, 133, 147, 161, 166, 172, 179, 184, 192, 201, 207, 213, 220, 225, 233). While useful during development, these should be replaced with proper logging using a logging framework (like the log crate or tracing which is already used in the main application based on Cargo.toml). This would allow users to control the verbosity level and avoid polluting stdout in production.
| match ConnectionBuilder::session() | ||
| .unwrap() | ||
| .name(dbus_name.as_str()) | ||
| .unwrap() | ||
| .serve_at(dbus_path.as_str(), single_instance_dbus) | ||
| .unwrap() | ||
| .build() |
There was a problem hiding this comment.
The Linux implementation uses multiple .unwrap() calls (lines 50, 52, 54) which will cause a panic if any of these operations fail. These should be handled more gracefully with proper error handling. Consider:
- Using
.expect()with descriptive messages for operations that shouldn't fail - Propagating errors up to the caller using
?operator - Handling errors explicitly with match or if-let patterns
This is especially important for DBus operations which can fail for various reasons (permission issues, DBus daemon not running, etc.).
Edit Clip input focus fix
Removed unused packages