Skip to content

Comments

[ty] Fix playground crashes when accessing vendored files with leading slashes#20661

Merged
MichaReiser merged 8 commits intoastral-sh:mainfrom
danparizher:fix-astral-sh/ty/issues/1290
Oct 4, 2025
Merged

[ty] Fix playground crashes when accessing vendored files with leading slashes#20661
MichaReiser merged 8 commits intoastral-sh:mainfrom
danparizher:fix-astral-sh/ty/issues/1290

Conversation

@danparizher
Copy link
Contributor

@danparizher danparizher commented Oct 1, 2025

Summary

Fixes astral-sh/ty#1290

@github-actions
Copy link
Contributor

github-actions bot commented Oct 1, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@MeGaGiGaGon
Copy link
Contributor

MeGaGiGaGon commented Oct 1, 2025

Thanks for putting up with my silly. A couple things:

  1. I think you linked the wrong issue for closing, it should be Playground errors and crashes if you start a file name with vendored: ty#1290
  2. Looking at the docs for Utf8Component, it looks like the only variant that can hit the unsupported match after this is Prefix. I tested a bunch of paths like vendored:\\?\ and vendored:C:\, but could not trigger a crash with hitting it, so I guess it's fine? Though it may make since to not panic in there either just in-case it is reachable.

Edit: Looking at the docs more, for Prefix it says:

Does not occur on Unix.

So if the playground is being hosted on a Unix machine (which I image it is), that branch would be unreachable, but it could still crash on windows.

@MichaReiser
Copy link
Member

What's the code path where we call into VendoredPath::from that then crashes? I wonder if we should fix this at the API boundary instead.

@MichaReiser MichaReiser added the playground A playground-specific issue label Oct 1, 2025
@sharkdp sharkdp removed their request for review October 1, 2025 07:49
@danparizher
Copy link
Contributor Author

What's the code path where we call into VendoredPath::from that then crashes? I wonder if we should fix this at the API boundary instead.

The crash originated from the playground’s vendored URI flow. The path is:

  1. Frontend creates vendored://... URIs and extracts a path string (can start with “/”) for vendored files.
  2. That string crosses the WASM boundary via getVendoredFile(path) and is passed directly to VendoredPath::new(path).

/// Gets a file handle for a vendored file by its path.
/// This allows vendored files to participate in LSP features like hover, completions, etc.
#[wasm_bindgen(js_name = "getVendoredFile")]
pub fn get_vendored_file(&self, path: &str) -> Result<FileHandle, Error> {
let vendored_path = VendoredPath::new(path);
// Try to get the vendored file as a File
let file = vendored_path_to_file(&self.db, vendored_path)
.map_err(|err| Error::new(&format!("Vendored file not found: {path}: {err}")))?;
Ok(FileHandle {
file,
path: vendored_path.to_path_buf().into(),
})
}
}

  1. Inside vendored FS, we normalize via NormalizedVendoredPath::from(&VendoredPath). Previously, a leading “/” became a RootDir component and hit the “unsupported” arm, which panicked. We now ignore RootDir and Windows Prefix components so paths are treated as relative to the archive root instead of panicking.

impl<'a> From<&'a VendoredPath> for NormalizedVendoredPath<'a> {
/// Normalize the path.
///
/// The normalizations are:
/// - Remove `.` and `..` components
/// - Strip trailing slashes
/// - Normalize `\\` separators to `/`
/// - Validate that the path does not have any unsupported components
///
/// ## Panics:
/// If a path with an unsupported component for vendored paths is passed.
/// Unsupported components are path prefixes (but root directories are now supported).
fn from(path: &'a VendoredPath) -> Self {
/// Remove `.` and `..` components, and validate that unsupported components are not present.
///
/// This inner routine also strips trailing slashes,
/// and normalizes paths to use Unix `/` separators.
/// However, it always allocates, so avoid calling it if possible.
/// In most cases, the path should already be normalized.
fn normalize_unnormalized_path(path: &VendoredPath) -> String {
let mut normalized_parts = Vec::new();
for component in path.components() {
match component {
camino::Utf8Component::Normal(part) => normalized_parts.push(part),
camino::Utf8Component::CurDir => continue,
camino::Utf8Component::ParentDir => {
// `VendoredPath("")`, `VendoredPath("..")` and `VendoredPath("../..")`
// all resolve to the same path relative to the zip archive
// (see https://github.com/astral-sh/ruff/pull/11991#issuecomment-2185278014)
normalized_parts.pop();
}
camino::Utf8Component::RootDir => {
// Ignore root directory components - vendored paths should be relative
// to the zip archive root anyway. This handles cases like "/module1.py"
// which can occur when paths are parsed from URIs.
continue;
}
unsupported => {
panic!("Unsupported component in a vendored path: {unsupported}")
}
}
}
normalized_parts.join("/")
}
let path_str = path.as_str();
if std::path::MAIN_SEPARATOR == '\\' && path_str.contains('\\') {
// Normalize paths so that they always use Unix path separators
NormalizedVendoredPath(Cow::Owned(normalize_unnormalized_path(path)))
} else if !path
.components()
.all(|component| matches!(component, camino::Utf8Component::Normal(_)))
{
// Remove non-`Normal` components
NormalizedVendoredPath(Cow::Owned(normalize_unnormalized_path(path)))
} else {
// Strip trailing slashes from the path
NormalizedVendoredPath(Cow::Borrowed(path_str.trim_end_matches('/')))
}
}

@MichaReiser
Copy link
Member

MichaReiser commented Oct 2, 2025

Frontend creates vendored://... URIs and extracts a path string (can start with “/”) for vendored files.

Yes, I think we should prevent the UI from creating vendored file paths similar to #20666

@danparizher
Copy link
Contributor Author

Would we like to remove the logic from crates/ruff_db/src/vendored.rs or keep it as a fallback?

Comment on lines 206 to 211
private getVendoredPath(uri: Uri): string {
// Monaco parses "vendored://stdlib/typing.pyi" as authority="stdlib", path="/typing.pyi"
// We need to reconstruct the full path
return uri.authority ? `${uri.authority}${uri.path}` : uri.path;
const fullPath = uri.authority ? `${uri.authority}${uri.path}` : uri.path;
return fullPath.startsWith("/") ? fullPath.slice(1) : fullPath;
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the right place. we need to prevent users from creating a file named vendored:// , similar to what we've done in #20666

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Thanks. I don't think it's strictly necessary here as the implementation is very close to how we handle leading / but I very appreciate if playground changes have a short video as test plan. It helps me to understand what you've tested :)

@MichaReiser MichaReiser merged commit 2d44ad2 into astral-sh:main Oct 4, 2025
35 checks passed
@danparizher danparizher deleted the fix-astral-sh/ty/issues/1290 branch October 4, 2025 13:56
@amyreese amyreese added the ty Multi-file analysis & type inference label Oct 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

playground A playground-specific issue ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Playground errors and crashes if you start a file name with vendored:

4 participants