Skip to content

Commit

Permalink
Fix crash when using @source containing .. (#14831)
Browse files Browse the repository at this point in the history
This PR fixes an issue where a `@source` crashes when the path
eventually resolves to a path ending in `..`.

We have to make sure that we canonicalize the path to make sure that we
are working with the real directory.

---------

Co-authored-by: Jordan Pittman <[email protected]>
  • Loading branch information
RobinMalfait and thecrypticace authored Oct 30, 2024
1 parent eb54dcd commit 92007a5
Show file tree
Hide file tree
Showing 10 changed files with 134 additions and 14 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Fixed

- Detect classes in new files when using `@tailwindcss/postcss` ([#14829](https://github.com/tailwindlabs/tailwindcss/pull/14829))
- Fix crash when using `@source` containing `..` ([#14831](https://github.com/tailwindlabs/tailwindcss/pull/14831))
- _Upgrade (experimental)_: Install `@tailwindcss/postcss` next to `tailwindcss` ([#14830](https://github.com/tailwindlabs/tailwindcss/pull/14830))

## [4.0.0-alpha.31] - 2024-10-29
Expand Down
4 changes: 4 additions & 0 deletions crates/oxide/src/glob.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,10 @@ pub fn hoist_static_glob_parts(entries: &Vec<GlobEntry>) -> Vec<GlobEntry> {
// folders.
if pattern.is_empty() && base.is_file() {
result.push(GlobEntry {
// SAFETY: `parent()` will be available because we verify `base` is a file, thus a
// parent folder exists.
base: base.parent().unwrap().to_string_lossy().to_string(),
// SAFETY: `file_name()` will be available because we verify `base` is a file.
pattern: base.file_name().unwrap().to_string_lossy().to_string(),
});
}
Expand Down Expand Up @@ -100,6 +103,7 @@ pub fn optimize_patterns(entries: &Vec<GlobEntry>) -> Vec<GlobEntry> {
GlobEntry {
base,
pattern: match size {
// SAFETY: we can unwrap here because we know that the size is 1.
1 => patterns.next().unwrap(),
_ => {
let mut patterns = patterns.collect::<Vec<_>>();
Expand Down
20 changes: 15 additions & 5 deletions crates/oxide/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -322,17 +322,27 @@ impl Scanner {

fn join_paths(a: &str, b: &str) -> PathBuf {
let mut tmp = a.to_owned();
let b = b.trim_end_matches("**/*").trim_end_matches('/');

if b.starts_with('/') {
return PathBuf::from(b);
}

// On Windows a path like C:/foo.txt is absolute but C:foo.txt is not
// (the 2nd is relative to the CWD)
if b.chars().nth(1) == Some(':') && b.chars().nth(2) == Some('/') {
return PathBuf::from(b);
}

tmp += "/";
tmp += b.trim_end_matches("**/*").trim_end_matches('/');
tmp += b;

PathBuf::from(&tmp)
}

for path in auto_sources
.iter()
.map(|source| join_paths(&source.base, &source.pattern))
{
for path in auto_sources.iter().filter_map(|source| {
dunce::canonicalize(join_paths(&source.base, &source.pattern)).ok()
}) {
// Insert a glob for the base path, so we can see new files/folders in the directory itself.
self.globs.push(GlobEntry {
base: path.to_string_lossy().into(),
Expand Down
2 changes: 2 additions & 0 deletions crates/oxide/src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,8 @@ impl<'a> Extractor<'a> {
}

// The ':` must be preceded by a-Z0-9 because it represents a property name.
// SAFETY: the Self::validate_arbitrary_property function from above validates that the
// `:` exists.
let colon = utility.find(":").unwrap();

if !utility
Expand Down
6 changes: 3 additions & 3 deletions crates/oxide/src/scanner/allowed_paths.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,13 @@ static IGNORED_FILES: sync::LazyLock<Vec<&'static str>> = sync::LazyLock::new(||
static IGNORED_CONTENT_DIRS: sync::LazyLock<Vec<&'static str>> =
sync::LazyLock::new(|| vec![".git"]);

#[tracing::instrument(skip(root))]
#[tracing::instrument(skip_all)]
pub fn resolve_allowed_paths(root: &Path) -> impl Iterator<Item = DirEntry> {
// Read the directory recursively with no depth limit
read_dir(root, None)
}

#[tracing::instrument(skip(root))]
#[tracing::instrument(skip_all)]
pub fn resolve_paths(root: &Path) -> impl Iterator<Item = DirEntry> {
WalkBuilder::new(root)
.hidden(false)
Expand All @@ -40,7 +40,7 @@ pub fn resolve_paths(root: &Path) -> impl Iterator<Item = DirEntry> {
.filter_map(Result::ok)
}

#[tracing::instrument(skip(root))]
#[tracing::instrument(skip_all)]
pub fn read_dir(root: &Path, depth: Option<usize>) -> impl Iterator<Item = DirEntry> {
WalkBuilder::new(root)
.hidden(false)
Expand Down
87 changes: 86 additions & 1 deletion crates/oxide/tests/scanner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ mod scanner {
use tailwindcss_oxide::*;
use tempfile::tempdir;

fn create_files_in(dir: &path::PathBuf, paths: &[(&str, &str)]) {
fn create_files_in(dir: &path::Path, paths: &[(&str, &str)]) {
// Create the necessary files
for (path, contents) in paths {
// Ensure we use the right path separator for the current platform
Expand Down Expand Up @@ -334,6 +334,53 @@ mod scanner {
);
}

#[test]
fn it_should_be_possible_to_scan_in_the_parent_directory() {
let candidates = scan_with_globs(
&[("foo/bar/baz/foo.html", "content-['foo.html']")],
vec!["./foo/bar/baz/.."],
)
.1;

assert_eq!(candidates, vec!["content-['foo.html']"]);
}

#[test]
fn it_should_scan_files_without_extensions() {
// These look like folders, but they are files
let candidates =
scan_with_globs(&[("my-file", "content-['my-file']")], vec!["./my-file"]).1;

assert_eq!(candidates, vec!["content-['my-file']"]);
}

#[test]
fn it_should_scan_folders_with_extensions() {
// These look like files, but they are folders
let candidates = scan_with_globs(
&[
(
"my-folder.templates/foo.html",
"content-['my-folder.templates/foo.html']",
),
(
"my-folder.bin/foo.html",
"content-['my-folder.bin/foo.html']",
),
],
vec!["./my-folder.templates", "./my-folder.bin"],
)
.1;

assert_eq!(
candidates,
vec![
"content-['my-folder.bin/foo.html']",
"content-['my-folder.templates/foo.html']",
]
);
}

#[test]
fn it_should_scan_content_paths() {
let candidates = scan_with_globs(
Expand All @@ -349,6 +396,44 @@ mod scanner {
assert_eq!(candidates, vec!["content-['foo.styl']"]);
}

#[test]
fn it_should_scan_absolute_paths() {
// Create a temporary working directory
let dir = tempdir().unwrap().into_path();

// Initialize this directory as a git repository
let _ = Command::new("git").arg("init").current_dir(&dir).output();

// Create files
create_files_in(
&dir,
&[
("project-a/index.html", "content-['project-a/index.html']"),
("project-b/index.html", "content-['project-b/index.html']"),
],
);

// Get POSIX-style absolute path
let full_path = format!("{}", dir.display()).replace('\\', "/");

let sources = vec![GlobEntry {
base: full_path.clone(),
pattern: full_path.clone(),
}];

let mut scanner = Scanner::new(Some(sources));
let candidates = scanner.scan();

// We've done the initial scan and found the files
assert_eq!(
candidates,
vec![
"content-['project-a/index.html']".to_owned(),
"content-['project-b/index.html']".to_owned(),
]
);
}

#[test]
fn it_should_scan_content_paths_even_when_they_are_git_ignored() {
let candidates = scan_with_globs(
Expand Down
18 changes: 18 additions & 0 deletions integrations/cli/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,9 @@ test(
/* bar.html is git ignored, but explicitly listed here to scan */
@source '../../project-d/src/bar.html';
/* Project E's source ends with '..' */
@source '../../project-e/nested/..';
`,

// Project A is the current folder, but we explicitly configured
Expand Down Expand Up @@ -553,6 +556,13 @@ test(
class="content-['project-d/my-binary-file.bin']"
></div>
`,

// Project E's `@source "project-e/nested/.."` ends with `..`, which
// should look for files in `project-e` itself.
'project-e/index.html': html`<div class="content-['project-e/index.html']"></div>`,
'project-e/nested/index.html': html`<div
class="content-['project-e/nested/index.html']"
></div>`,
},
},
async ({ fs, exec, spawn, root }) => {
Expand Down Expand Up @@ -599,6 +609,14 @@ test(
--tw-content: 'project-d/src/index.html';
content: var(--tw-content);
}
.content-\\[\\'project-e\\/index\\.html\\'\\] {
--tw-content: 'project-e/index.html';
content: var(--tw-content);
}
.content-\\[\\'project-e\\/nested\\/index\\.html\\'\\] {
--tw-content: 'project-e/nested/index.html';
content: var(--tw-content);
}
@supports (-moz-orient: inline) {
@layer base {
*, ::before, ::after, ::backdrop {
Expand Down
2 changes: 1 addition & 1 deletion integrations/vite/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -679,7 +679,7 @@ for (let transformer of ['postcss', 'lightningcss']) {
},
async ({ root, fs, exec }) => {
await expect(() =>
exec('pnpm vite build', { cwd: path.join(root, 'project-a') }),
exec('pnpm vite build', { cwd: path.join(root, 'project-a') }, { ignoreStdErr: true }),
).rejects.toThrowError('The `source(../i-do-not-exist)` does not exist')

let files = await fs.glob('project-a/dist/**/*.css')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ function extractV3Base(
// ^^^^^^^^^ -> Base
let rawVariants = segment(rawCandidate, ':')

// Safety: At this point it is safe to use TypeScript's non-null assertion
// SAFETY: At this point it is safe to use TypeScript's non-null assertion
// operator because even if the `input` was an empty string, splitting an
// empty string by `:` will always result in an array with at least one
// element.
Expand Down
6 changes: 3 additions & 3 deletions packages/tailwindcss/src/compile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ export function compileCandidates(
}

astNodes.sort((a, z) => {
// Safety: At this point it is safe to use TypeScript's non-null assertion
// SAFETY: At this point it is safe to use TypeScript's non-null assertion
// operator because if the ast nodes didn't exist, we introduced a bug
// above, but there is no need to re-check just to be sure. If this relied
// on pure user input, then we would need to check for its existence.
Expand Down Expand Up @@ -194,7 +194,7 @@ export function applyVariant(
return
}

// Safety: At this point it is safe to use TypeScript's non-null assertion
// SAFETY: At this point it is safe to use TypeScript's non-null assertion
// operator because if the `candidate.root` didn't exist, `parseCandidate`
// would have returned `null` and we would have returned early resulting in
// not hitting this code path.
Expand Down Expand Up @@ -322,7 +322,7 @@ function getPropertySort(nodes: AstNode[]) {
let q: AstNode[] = nodes.slice()

while (q.length > 0) {
// Safety: At this point it is safe to use TypeScript's non-null assertion
// SAFETY: At this point it is safe to use TypeScript's non-null assertion
// operator because we guarded against `q.length > 0` above.
let node = q.shift()!
if (node.kind === 'declaration') {
Expand Down

0 comments on commit 92007a5

Please sign in to comment.