Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions agents/references/language-server-threat-model.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ when deciding which inputs are trusted. Browser integrations have a separate
- **Trusted local input:** the editor, its extensions and their bundled files, the local machine and
its file system, and the user's configuration and trust decisions.

Ruff treats workspaces as trusted unless the server starts with `--untrusted-workspace`.
ty treats workspaces as trusted unless `untrustedWorkspace` is true at initialization. Trust extends
to everything in the workspace, including source code, configuration, notebooks, and the targets of
symlinks that point outside it.
Expand Down
12 changes: 12 additions & 0 deletions crates/ruff/src/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -627,6 +627,10 @@ pub struct FormatCommand {

#[derive(Copy, Clone, Debug, clap::Parser)]
pub struct ServerCommand {
/// Treat all workspaces as untrusted, disabling the uv formatter backend.
#[arg(long)]
untrusted_workspace: bool,

/// Enable preview mode. Use `--no-preview` to disable.
///
/// This enables unstable server features and turns on the preview mode for the linter
Expand All @@ -638,6 +642,14 @@ pub struct ServerCommand {
}

impl ServerCommand {
pub(crate) fn resolve_workspace_trust(self) -> ruff_server::WorkspaceTrust {
if self.untrusted_workspace {
ruff_server::WorkspaceTrust::Untrusted
} else {
ruff_server::WorkspaceTrust::Trusted
}
}

pub(crate) fn resolve_preview(self) -> Option<bool> {
resolve_bool_arg(self.preview, self.no_preview)
}
Expand Down
8 changes: 6 additions & 2 deletions crates/ruff/src/commands/server.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
use crate::ExitStatus;
use anyhow::Result;
use ruff_server::WorkspaceTrust;

pub(crate) fn run_server(preview: Option<bool>) -> Result<ExitStatus> {
ruff_server::run(preview)?;
pub(crate) fn run_server(
preview: Option<bool>,
workspace_trust: WorkspaceTrust,
) -> Result<ExitStatus> {
ruff_server::run(preview, workspace_trust)?;
Ok(ExitStatus::Success)
}
2 changes: 1 addition & 1 deletion crates/ruff/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ fn analyze_graph(
}

fn server(args: ServerCommand) -> Result<ExitStatus> {
commands::server::run_server(args.resolve_preview())
commands::server::run_server(args.resolve_preview(), args.resolve_workspace_trust())
}

pub fn check(args: CheckCommand, global_options: GlobalConfigArgs) -> Result<ExitStatus> {
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_server/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ dunce = { workspace = true }
regex = { workspace = true }
smallvec = { workspace = true }
tempfile = { workspace = true }
test-case = { workspace = true }

[features]
test-uv = []
Expand Down
11 changes: 9 additions & 2 deletions crates/ruff_server/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,18 @@ const NOTEBOOK_SOURCE_ORGANIZE_IMPORTS_RUFF: CodeActionKind =
/// result type is needed.
pub(crate) type Result<T> = anyhow::Result<T>;

/// Whether the server may execute code from the workspace.
#[derive(Copy, Clone, Debug, Eq, PartialEq)]
pub enum WorkspaceTrust {
Trusted,
Untrusted,
}

fn version() -> &'static str {
ruff_linter::VERSION
}

pub fn run(preview: Option<bool>) -> Result<()> {
pub fn run(preview: Option<bool>, workspace_trust: WorkspaceTrust) -> Result<()> {
let four = NonZeroUsize::new(4).unwrap();

// by default, we set the number of worker threads to `num_cpus`, with a maximum of 4.
Expand All @@ -50,7 +57,7 @@ pub fn run(preview: Option<bool>) -> Result<()> {

let (connection, io_threads) = ConnectionInitializer::stdio();

let server_result = Server::new(worker_threads, connection, preview, false)
let server_result = Server::new(worker_threads, connection, preview, workspace_trust, false)
.context("Failed to start server")?
.run();

Expand Down
4 changes: 3 additions & 1 deletion crates/ruff_server/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,11 @@ use types::WorkspaceFoldersServerCapabilities;
pub use self::connection::ConnectionInitializer;
pub(crate) use self::connection::ConnectionSender;
use self::schedule::spawn_main_loop;
use crate::PositionEncoding;
pub(crate) use crate::server::main_loop::MainLoopSender;
pub(crate) use crate::server::main_loop::{Event, MainLoopReceiver};
use crate::session::{AllOptions, Client, Session};
use crate::workspace::Workspaces;
use crate::{PositionEncoding, WorkspaceTrust};
pub(crate) use api::Error;

mod api;
Expand All @@ -52,6 +52,7 @@ impl Server {
worker_threads: NonZeroUsize,
connection: ConnectionInitializer,
preview: Option<bool>,
workspace_trust: WorkspaceTrust,
is_test: bool,
) -> crate::Result<Self> {
let (id, init_params) = connection.initialize_start()?;
Expand Down Expand Up @@ -121,6 +122,7 @@ impl Server {
global,
&workspaces,
&client,
workspace_trust,
)?,
client_capabilities,
})
Expand Down
10 changes: 2 additions & 8 deletions crates/ruff_server/src/server/api/requests/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,7 @@ impl super::BackgroundDocumentRequestHandler for Format {
pub(super) fn format_full_document(snapshot: &DocumentSnapshot) -> Result<Fixes> {
let mut fixes = Fixes::default();
let query = snapshot.query();
let backend = snapshot
.client_settings()
.editor_settings()
.format_backend();
let backend = snapshot.format_backend();

match snapshot.query() {
DocumentQuery::Notebook { notebook, .. } => {
Expand Down Expand Up @@ -84,10 +81,7 @@ fn format_document(snapshot: &DocumentSnapshot) -> Result<super::FormatResponse>
.context("Failed to get text document for the format request")
.unwrap();
let query = snapshot.query();
let backend = snapshot
.client_settings()
.editor_settings()
.format_backend();
let backend = snapshot.format_backend();
format_text_document(
text_document,
query,
Expand Down
5 changes: 1 addition & 4 deletions crates/ruff_server/src/server/api/requests/format_range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,7 @@ fn format_document_range(
.context("Failed to get text document for the format range request")
.unwrap();
let query = snapshot.query();
let backend = snapshot
.client_settings()
.editor_settings()
.format_backend();
let backend = snapshot.format_backend();
format_text_document_range(text_document, range, query, snapshot.encoding(), backend)
}

Expand Down
71 changes: 70 additions & 1 deletion crates/ruff_server/src/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,11 @@ use lsp_types::{ClientCapabilities, FileEvent, NotebookDocumentCellChanges, Uri}
use settings::ClientSettings;

use crate::edit::{DocumentKey, DocumentVersion, NotebookDocument};
use crate::format::FormatBackend;
use crate::session::request_queue::RequestQueue;
use crate::session::settings::GlobalClientSettings;
use crate::workspace::Workspaces;
use crate::{PositionEncoding, TextDocument};
use crate::{PositionEncoding, TextDocument, WorkspaceTrust};

pub(crate) use self::capabilities::ResolvedClientCapabilities;
pub(crate) use self::index::DocumentQuery;
Expand All @@ -34,6 +35,9 @@ pub(crate) struct Session {
/// Global settings provided by the client.
global_settings: GlobalClientSettings,

/// Set at startup so client settings cannot enable external formatting in an untrusted workspace.
workspace_trust: WorkspaceTrust,

/// Tracks what LSP features the client supports and doesn't support.
resolved_client_capabilities: Arc<ResolvedClientCapabilities>,

Expand All @@ -51,6 +55,7 @@ pub(crate) struct DocumentSnapshot {
client_settings: Arc<settings::ClientSettings>,
document_ref: index::DocumentQuery,
position_encoding: PositionEncoding,
workspace_trust: WorkspaceTrust,
}

impl Session {
Expand All @@ -60,11 +65,13 @@ impl Session {
global: GlobalClientSettings,
workspaces: &Workspaces,
client: &Client,
workspace_trust: WorkspaceTrust,
) -> crate::Result<Self> {
Ok(Self {
position_encoding,
index: index::Index::new(workspaces, &global, client)?,
global_settings: global,
workspace_trust,
resolved_client_capabilities: Arc::new(ResolvedClientCapabilities::new(
client_capabilities,
)),
Expand Down Expand Up @@ -104,6 +111,7 @@ impl Session {
.unwrap_or_else(|| self.global_settings.to_settings_arc()),
document_ref: self.index.make_document_ref(key, &self.global_settings)?,
position_encoding: self.position_encoding,
workspace_trust: self.workspace_trust,
})
}

Expand Down Expand Up @@ -215,6 +223,21 @@ impl Session {
}

impl DocumentSnapshot {
pub(crate) fn format_backend(&self) -> FormatBackend {
let backend = self.client_settings.editor_settings().format_backend();
match self.workspace_trust {
WorkspaceTrust::Trusted => backend,
WorkspaceTrust::Untrusted => {
if backend == FormatBackend::Uv {
tracing::info!(
"Using the internal formatter because the workspace is untrusted; the uv backend is disabled"
);
}
FormatBackend::Internal
}
}
}

pub(crate) fn resolved_client_capabilities(&self) -> &ResolvedClientCapabilities {
&self.resolved_client_capabilities
}
Expand Down Expand Up @@ -242,3 +265,49 @@ impl DocumentSnapshot {
)
}
}

#[cfg(test)]
mod tests {
use std::sync::Arc;

use anyhow::{Context, Result};
use lsp_types::Uri;
use serde_json::json;
use test_case::test_case;

use super::index::Index;
use super::options::GlobalOptions;
use super::{Client, Session};
use crate::format::FormatBackend;
use crate::session::request_queue::RequestQueue;
use crate::{PositionEncoding, TextDocument, WorkspaceTrust};

#[test_case(WorkspaceTrust::Untrusted, FormatBackend::Internal)]
#[test_case(WorkspaceTrust::Trusted, FormatBackend::Uv)]
fn format_backend(workspace_trust: WorkspaceTrust, expected: FormatBackend) -> Result<()> {
let (main_loop_sender, _) = crossbeam::channel::unbounded();
let (client_sender, _) = crossbeam::channel::unbounded();
let client = Client::new(main_loop_sender, client_sender);
let options: GlobalOptions = serde_json::from_value(json!({
"format": {"backend": "uv"},
}))?;
let mut session = Session {
index: Index::default(),
position_encoding: PositionEncoding::default(),
global_settings: options.into_settings(client),
workspace_trust,
resolved_client_capabilities: Arc::default(),
request_queue: RequestQueue::new(),
shutdown_requested: false,
};
let uri: Uri = "untitled:test.py".parse()?;
session.open_text_document(uri.clone(), TextDocument::new(String::new(), 1));
let snapshot = session
.take_snapshot(uri)
.context("missing document snapshot")?;

assert_eq!(snapshot.format_backend(), expected);

Ok(())
}
}
10 changes: 8 additions & 2 deletions crates/ruff_server/tests/e2e/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ use lsp_types::{
DidChangeWorkspaceFoldersNotification, DidCloseTextDocumentNotification,
DidOpenTextDocumentNotification, ExitNotification, InitializedNotification, Notification,
};
use ruff_server::{ConnectionInitializer, LogLevel, Server, init_logging};
use ruff_server::{ConnectionInitializer, LogLevel, Server, WorkspaceTrust, init_logging};
use rustc_hash::FxHashMap;
use tempfile::TempDir;

Expand Down Expand Up @@ -207,7 +207,13 @@ impl TestServer {
// TODO: This should probably be configurable to test concurrency issues
let worker_threads = NonZeroUsize::new(1).unwrap();

match Server::new(worker_threads, server_connection, None, true) {
match Server::new(
worker_threads,
server_connection,
None,
WorkspaceTrust::Trusted,
true,
) {
Comment on lines +210 to +216

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a TestServer::untrusted factory method with an internal, shared new_impl? Or are there other reasons why it can't be a e2e test?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that the main issue is determining what to assert at the end of the test, not setting up the test server initially. There's no user-visible output that we're using one backend or the other (except the new logging call). So it makes it hard to assert something useful in the e2e tests. But you'd know better about our e2e test capabilities than me, I could certainly be missing something.

Codex started producing something like this when I asked for an e2e test, which is what I thought looked too janky to include:

fn without_uv(body: impl FnOnce() -> Result<()>) -> Result<()> {
    const CHILD: &str = "RUFF_TEST_ISOLATED_CHILD";
    let thread = std::thread::current();
    let name = thread.name().context("missing test name")?;
    if std::env::var(CHILD).as_deref() == Ok(name) {
        return body();
    }
    let empty_path = tempfile::tempdir()?;
    let output = Command::new(std::env::current_exe()?)
        .args(["--exact", name, "--nocapture"])
        .env(CHILD, name)
        .env("PATH", empty_path.path())
        .current_dir(empty_path.path())
        .output()?;
    ensure!(output.status.success(), "{}\n{}", String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr));
    println!("{}", String::from_utf8_lossy(&output.stdout));
    Ok(())
}

plus another ~50 lines of helpers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can of course assert that the code gets formatted somehow, which Codex also tried initially, but that didn't seem very useful, as both backends format code.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you override path and have uv format fail?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what Codex's snippet above was trying to do. I'll just push its patch so you can see what you think. Maybe it's not as bad as I thought.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, ty's server tests have more indirection here, through TestServerBuilder::with_env_var and the system stuff. Here, we end up calling straight into Command::new("uv"), which makes it much harder to inject environment variables at the right point, or without modifying other tests, which is what Codex is trying to avoid.

Anyway, I've included Codex's e2e test scheme. We could also do something more like ty, although it feels like that probably belongs in a separate PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I forgot that we're in Ruff's server where we don't have the ability to easily override commands. Hmm, I don't see a great solution other than plumbing through the argument somehow (e.g. by storing a CommandExecutor on Server). I'm not sure this is worth it for this change, or, at least, might be better as a follow up. Thanks for giving it a try

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem! I'll revert this last commit and merge then :)

Ok(server) => {
if let Err(err) = server.run() {
panic!("Server stopped with error: {err:?}");
Expand Down
4 changes: 4 additions & 0 deletions docs/editors/settings.md
Original file line number Diff line number Diff line change
Expand Up @@ -981,6 +981,10 @@ The backend to use for formatting files. Following options are available:
For `internal`, the formatter version will match the selected Ruff version while for `uv`, the
formatter version may differ.

Starting the server with `ruff server --untrusted-workspace` disables the uv backend. With this flag,
the server always uses the internal formatter, regardless of this setting. This prevents workspace
configuration from causing uv to execute untrusted code.

**Default value**: `"internal"`

**Type**: `"internal" | "uv"`
Expand Down
Loading