Skip to content

bundler: resolve a percent-encoded CSS url() or @import by its decoded path #46511

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 1 commit into
base: main
Choose a base branch
from
+700 −50
Open

bundler: resolve a percent-encoded CSS url() or @import by its decoded path #46511

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
136 changes: 91 additions & 45 deletions src/bundler/bundle_v2.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2766,7 +2766,7 @@ pub mod bv2_impl {
}

let mut had_busted_dir_cache = false;
let resolve_result: _resolver::Result = loop {
let mut resolve_result: _resolver::Result = loop {
// SAFETY: see `transpiler` note above.
match unsafe { &mut *transpiler }.resolver.resolve(
source_dir,
Expand All @@ -2777,31 +2777,55 @@ pub mod bv2_impl {
Err(err) => {
// Only perform directory busting when hot-reloading is enabled
if err == _resolver::Error::ModuleNotFound {
let importer_loader = self.graph.input_files.items_loader()
[import_record.importer_source_index as usize];
let decoded = _resolver::percent_decoded_css_url(
self.arena(),
importer_loader,
&import_record.specifier,
import_record.kind,
);
if let Some(decoded) = decoded
// SAFETY: see `transpiler` note above.
&& let Ok(result) = unsafe { &mut *transpiler }.resolver.resolve(
source_dir,
decoded,
import_record.kind,
)
{
break result;
}
let spellings =
core::iter::once(&*import_record.specifier).chain(decoded);
if let Some(dev) = &self.dev_server {
if !had_busted_dir_cache {
// Only re-query if we previously had something cached.
// SAFETY: see `transpiler` note above.
if unsafe { &mut *transpiler }
.resolver
.bust_dir_cache_from_specifier(
&import_record.source_file,
&import_record.specifier,
)
{
let mut busted = false;
for spelling in spellings.clone() {
// SAFETY: see `transpiler` note above.
busted |= unsafe { &mut *transpiler }
.resolver
.bust_dir_cache_from_specifier(
&import_record.source_file,
spelling,
);
}
if busted {
had_busted_dir_cache = true;
continue;
}
}

// Tell Bake's Dev Server to wait for the file to be imported.
dev.track_resolution_failure(
&import_record.source_file,
&import_record.specifier,
target.bake_graph(),
self.graph.input_files.items_loader()
[import_record.importer_source_index as usize],
)
.expect("oom");
for spelling in spellings {
Comment on lines +2811 to +2820

@claude claude Bot Oct 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 (optional) Dev-server users with a percent-encoded CSS url() whose file appears later keep an OS directory watch and a dead dependency entry for the rest of the session, where the base released both once the file appeared. Both spellings are tracked at src/bundler/bundle_v2.rs:2811-2820 (and bundle_v2.rs:7040-7048), but hot_reload_event.rs:140-152 frees only the dep that resolves and frees the watcher only when no dep remains; the raw ./my%20photo.png dep never resolves, so the watcher on that directory is never freed and the raw spelling is re-resolved on every event in that directory. Fix: when one spelling of a record resolves, free the sibling deps registered for the same (source file, record), e.g. track the decoded spelling only, or key deps by record so a hit removes both.

Why this was flagged

Trigger: Bake dev server, a stylesheet with url(./my%20photo.png) while my photo.png does not exist yet, then the user creates the file. Both spellings are passed to dev.track_resolution_failure at src/bundler/bundle_v2.rs:2811-2820 and src/bundler/bundle_v2.rs:7040-7048; directory_watch_store.rs:257-287 appends one Dep per spelling to the same directory watcher. On the create event, src/runtime/bake/dev_server/hot_reload_event.rs:129-147 resolves each Dep; only the decoded one resolves and is freed (line 147), the raw one is pushed back onto the chain (lines 149-151), so the watcher at line 155-161 is never freed. On the base branch there was only the raw dep, which stayed only while the import was genuinely unresolved; here the import now resolves but the directory watch and the stale dep persist for the session and every later change in that directory re-runs resolver.resolve on the raw spelling. Remedy: free sibling deps of the same record when any spelling resolves.

Verification: Trigger: Bake dev server, url(./my%20photo.png) missing at first build, my photo.png created later. On the directory event, only a dep whose resolve succeeds is freed (src/runtime/bake/dev_server/hot_reload_event.rs:147), every other dep is re-linked (lines 150-151). ./my%20photo.png never resolves once my photo.png exists, so that dep and the directory watch persist.

dev.track_resolution_failure(
&import_record.source_file,
spelling,
target.bake_graph(),
importer_loader,
)
.expect("oom");
}

// Turn this into an invalid AST, so that incremental mode skips it when printing.
// SAFETY: truncating to len 0 never exposes uninitialized elements.
Expand Down Expand Up @@ -2916,7 +2940,6 @@ pub mod bv2_impl {
}
}
};
let mut resolve_result = resolve_result;

let out_source_index: Option<Index>;

Expand Down Expand Up @@ -6962,30 +6985,33 @@ pub mod bv2_impl {
}

let mut had_busted_dir_cache = false;
let resolve_result: _resolver::Result = 'inner: loop {
let mut resolve_result: _resolver::Result = 'inner: loop {
match transpiler.resolver.resolve_with_framework(
source_dir,
import_record.path.text,
import_record.kind,
) {
Ok(r) => break r,
Err(err) => {
// borrowck — `log_for_resolution_failures` returns
// `&mut Log` tied to `&mut self`, but it's always a raw-ptr
// deref (DevServer vtable or `transpiler.log`). Detach via
// `*mut` so later `self.*` reads don't conflict.
// SAFETY: log lives in DevServer/transpiler, disjoint from `self.graph`.
let log: &mut bun_ast::Log = unsafe {
&mut *std::ptr::from_mut::<bun_ast::Log>(
self.log_for_resolution_failures(
source.path.text,
ctx.target.bake_graph(),
),
)
};

// Only perform directory busting when hot-reloading is enabled
if err == _resolver::Error::ModuleNotFound {
let decoded = _resolver::percent_decoded_css_url(
self.arena(),
loader,
import_record.path.text,
import_record.kind,
);
if let Some(decoded) = decoded
&& let Ok(result) = transpiler.resolver.resolve(
source_dir,
decoded,
import_record.kind,
)
{
break 'inner result;
Comment on lines +7004 to +7011

@claude claude Bot Oct 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔴 Users serving a bundled HTML page with Bun.serve whose CSS has url(./my%20photo.png) to a copied asset now get a 404 for it where the base failed the build. The decoded hit adopted at src/bundler/bundle_v2.rs:7011 (and :2796) names the output ./my photo-<hash>.png, so HTMLBundle.rs:584 registers the route /my photo-<hash>.png. The browser requests /my%20photo-<hash>.png and the uws router matches segments literally, so nothing answers. Fix: a decoded hit must produce an output name a browser can fetch back, e.g. percent-encode the copied asset's name in the emitted CSS and route, or refuse decoded names the server cannot route.

Why this was flagged

A stylesheet in a Bun.serve HTML bundle (production, Bun.serve({ routes: { '/': index.html } })) contains url(./my%20photo.png) and the file on disk is my photo.png of 128 KiB or more (COPY_THRESHOLD, src/bundler/bundled_ast.rs:374) or uses the file loader. The raw resolve fails with ModuleNotFound; percent_decoded_css_url returns the decoded name and the second resolve at src/bundler/bundle_v2.rs:7005-7011 succeeds, so the record's path becomes /dir/my photo.png and the copied output file is ./my photo-<hash>.png. src/runtime/server/HTMLBundle.rs:571-588 strips the . and calls append_static_route with /my photo-<hash>.png. The browser percent-encodes the space in the request path, /my%20photo-<hash>.png; packages/bun-uws/src/HttpContext.h:593 routes getUrlForRouting() unchanged and HttpRouter.h:255 compares segments byte-for-byte without decoding, so the static route never matches and the server falls to its 404/fallback. On the base the build itself failed with Could not resolve: "./my%20photo.png", so the user saw the problem at build time.

Verification: src/bundler/bundle_v2.rs:7005-7011 break 'inner result on the decoded second resolver.resolve(source_dir, decoded, ...), so the record now resolves to my photo.png where base raised Could not resolve. A browser resolving url("./my photo-<hash>.png") requests /my%20photo-<hash>.png, which does not byte-equal /my photo-<hash>.png, so no static route fires and the asset 404s.

}
Comment on lines +6998 to +7012

@claude claude Bot Oct 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟣 pre-existing, not blocking: pre-existing (partial fix): a Bun.build({ files }) user whose in-memory CSS references url(./my%20photo.png) beside an in-memory my photo.png still gets Could not resolve, while the same layout on disk now builds. Both resolve sites consult file_map.resolve only for the raw spelling (src/bundler/bundle_v2.rs:6926, src/bundler/bundle_v2.rs:2696) and the decoded retry at src/bundler/bundle_v2.rs:7005 goes to the disk resolver only. Fix: try the decoded spelling against the FileMap before (or alongside) the disk resolver at both sites, ideally inside one shared retry helper so the two loops stay in lockstep. [also at: src/bundler/bundle_v2.rs:7003 - pre-existing, partial fix: users of Bun.build({ files }) whose in-memory CSS says url(./my%20photo.png) still get Could not resolve after this change.]
A small fix can ride a push you are already making; otherwise a short reply is enough.

Why this was flagged

A JS API build passes files: { "/app/entry.css": ".a{background:url(./my%20photo.png)}", "/app/my photo.png": <bytes> } with /app/entry.css as entrypoint. In resolve_import_records the FileMap lookup at src/bundler/bundle_v2.rs:6926 is keyed by the raw text ./my%20photo.png and misses; resolve_with_framework at src/bundler/bundle_v2.rs:6989 misses on disk; the new retry at src/bundler/bundle_v2.rs:6998-7012 decodes to ./my photo.png but only calls transpiler.resolver.resolve, which reads the real filesystem, so the virtual file is never consulted and the record falls through to the Could not resolve: "./my%20photo.png" error at src/bundler/bundle_v2.rs:7175. run_resolver has the same shape: FileMap at src/bundler/bundle_v2.rs:2696 for the raw text only, decoded retry at src/bundler/bundle_v2.rs:2782-2797 against the disk resolver only. The base branch fails this input the same way, so this is the sibling input source the fix leaves behind rather than a regression; the PR's variant matrix (api/cli/plugin-declines) does not include files.

Verification: pre-existing (partial fix: the base branch fails this by the same route). A Bun.build({ files: {...} }) build whose in-memory CSS references url(./my%20photo.png). FileMap::resolve (src/bundler/bundle_v2.rs:968-1060) never percent-decodes. The decoded retry at lines 6998-7012 calls only the disk resolver, so the build logs Could not resolve. None of the new tests exercise it.

let spellings =
core::iter::once(import_record.path.text).chain(decoded);
if self.bun_watcher.is_some() {
if !had_busted_dir_cache {
bun_core::scoped_log!(
Expand All @@ -6995,27 +7021,48 @@ pub mod bv2_impl {
bstr::BStr::new(&import_record.path.text)
);
// Only re-query if we previously had something cached.
if transpiler.resolver.bust_dir_cache_from_specifier(
source.path.text,
import_record.path.text,
) {
let mut busted = false;
for spelling in spellings.clone() {
busted |=
transpiler.resolver.bust_dir_cache_from_specifier(
source.path.text,
spelling,
);
}
if busted {
had_busted_dir_cache = true;
continue 'inner;
}
}
if let Some(dev) = self.dev_server {
// Tell DevServer about the resolution failure.
dev.track_resolution_failure(
source.path.text,
import_record.path.text,
ctx.target.bake_graph(), // use the source file target not the altered one
loader,
)
.expect("oom");
for spelling in spellings {
dev.track_resolution_failure(
source.path.text,
spelling,
ctx.target.bake_graph(), // use the source file target not the altered one
loader,
)
.expect("oom");
}
}
}
}

// borrowck — `log_for_resolution_failures` returns
// `&mut Log` tied to `&mut self`, but it's always a raw-ptr
// deref (DevServer vtable or `transpiler.log`). Detach via
// `*mut` so later `self.*` reads don't conflict.
// SAFETY: log lives in DevServer/transpiler, disjoint from `self.graph`.
let log: &mut bun_ast::Log = unsafe {
&mut *std::ptr::from_mut::<bun_ast::Log>(
self.log_for_resolution_failures(
source.path.text,
ctx.target.bake_graph(),
),
)
};

// Disable failing packages from being printed.
// This may cause broken code to write.
// However, doing this means we tell them all the resolve errors
Expand Down Expand Up @@ -7155,7 +7202,6 @@ pub mod bv2_impl {
}
}
};
let mut resolve_result = resolve_result;
// if there were errors, lets go ahead and collect them all
if last_error.is_some() {
continue;
Expand Down
1 change: 1 addition & 0 deletions src/resolver/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ pub use tsconfig_json::TSConfigJSON;
pub use ::bun_install_types::resolver_hooks as install_types;
pub use resolver::{
AnyResolveWatcher, BrowserMapPathKind, Bufs, Dirname, Resolver, module_type_from_ext,
percent_decoded_css_url,
};
pub use result::{
DebugLogs, DirEntryResolveQueueItem, ExternalKind, FlushMode, LoadResult, MatchResult,
Expand Down
2 changes: 1 addition & 1 deletion src/resolver/package_json.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1564,7 +1564,7 @@ impl<'a> Package<'a> {

use bun_core::strings::{replace, replacement_size};

const INVALID_PERCENT_CHARS: [&[u8]; 4] = [b"%2f", b"%2F", b"%5c", b"%5C"];
pub(crate) const INVALID_PERCENT_CHARS: [&[u8]; 4] = [b"%2f", b"%2F", b"%5c", b"%5C"];

struct ModuleBufs {
resolved_path_buf_percent: PathBuffer,
Expand Down
55 changes: 53 additions & 2 deletions src/resolver/resolver.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4577,7 +4577,11 @@ impl<'a> Resolver<'a> {
// ...
self.dir_cache_mut().mark_not_found(queue_top.result);
rfs!().entries.mark_not_found(cached_dir_entry_result);
if err != crate::Error::Sys(bun_errno::SystemErrno::ENOENT) {
if !matches!(
err,
crate::Error::Sys(bun_errno::SystemErrno::ENOENT)
| crate::Error::Sys(bun_errno::SystemErrno::ENAMETOOLONG)
) {
Comment on lines +4580 to +4584

@claude claude Bot Oct 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 (optional) Users of bun build and the dev server lose the Cannot read directory "...": ENAMETOOLONG diagnostic for every import kind, not only the CSS percent-decoded retry. Both directory-read sites (src/resolver/resolver.rs:4580-4584 and src/resolver/resolver.rs:5911-5913) now treat ENAMETOOLONG like ENOENT, so a JS import whose directory exceeds NAME_MAX or whose absolute path exceeds PATH_MAX reports only Could not resolve. Fix: keep the errno visible for the non-CSS case, for example by emitting it as a non-fatal note instead of dropping it, or by silencing it only when the probe comes from a CSS url()/@ import record that has a % escape; the decoded retry must still succeed without a logged error.

Why this was flagged

A JS file does import "./<300 a>.js" or lives in a tree whose absolute path is longer than PATH_MAX (such a file can exist on Linux). read_dir_info reaches the ENAMETOOLONG branch at src/resolver/resolver.rs:4580 or load_as_file reaches src/resolver/resolver.rs:5911, and with this diff both skip add_error_fmt for ENAMETOOLONG. The base branch prints Cannot read directory "<dir>": ENAMETOOLONG followed by Could not resolve; after merging only Could not resolve is printed, so an operator cannot tell a missing file from a path-length limit. The silencing is needed only so the raw probe of an escaped CSS name (29 CJK letters become a 261-byte segment) does not fail the build before the decoded retry at src/bundler/bundle_v2.rs:2790 and src/bundler/bundle_v2.rs:7003 succeeds, but the change applies to every kind and every caller of the resolver. The repository review guidance says to map only the specific expected errno to the benign path; nothing in the diff bounds this to CSS records.

Verification: The trigger is any importer whose specifier or resolved directory path makes openat/readdir return ENAMETOOLONG. src/resolver/resolver.rs:4580-4584 now reads if !matches!(err, Sys(ENOENT) | Sys(ENAMETOOLONG)), and src/resolver/resolver.rs:5911-5913 adds | Sys(ENAMETOOLONG) => {} to the silent arm. Neither site consults the import kind. The build still fails with Could not resolve; only the extra logged error is dropped.

if enable_logging {
let pretty = queue_top_unsafe_path;
let _ = self.log_mut().add_error_fmt(
Expand Down Expand Up @@ -5905,7 +5909,8 @@ impl<'a> Resolver<'a> {
if let Fs::file_system::real_fs::EntriesOption::Err(err) = dir_entry.get() {
match err.original_err {
crate::Error::Sys(bun_errno::SystemErrno::ENOENT)
| crate::Error::Sys(bun_errno::SystemErrno::ENOTDIR) => {}
| crate::Error::Sys(bun_errno::SystemErrno::ENOTDIR)
| crate::Error::Sys(bun_errno::SystemErrno::ENAMETOOLONG) => {}
_ => {
let _ = self.log_mut().add_error_fmt(
None,
Expand Down Expand Up @@ -6916,6 +6921,52 @@ pub fn module_type_from_ext(ext: &[u8]) -> Option<options::ModuleType> {
MODULE_TYPE_FROM_EXT.get(ext).copied()
}

/// The percent-decoded path to try for a stylesheet `url()`/`@import` that is not a file as written.
#[cold]
pub fn percent_decoded_css_url<'a>(
arena: &'a bun_alloc::Arena,
importer: bun_ast::Loader,
url: &[u8],
kind: ast::ImportKind,
) -> Option<&'a [u8]> {
// HTML `src`/`href` records have these kinds too, and `composes .. from` names a module.
if !importer.is_css()
|| !matches!(
kind,
ast::ImportKind::Url | ast::ImportKind::At | ast::ImportKind::AtConditional
)
{
return None;
}
let path = &url[..strings::index_of_any(url, b"?#").unwrap_or(url.len())];
if !strings::contains_char(path, b'%')
|| crate::package_json::INVALID_PERCENT_CHARS
.iter()
.any(|escape| strings::contains(path, escape))
{
return None;
}
// A Result can borrow its specifier, so the decoded bytes must outlive it.
let decoded = arena.alloc_slice_fill_copy(path.len(), 0u8);
let len = bun_url::PercentEncoding::decode_into(decoded, path).ok()? as usize;
let decoded: &[u8] = &decoded[..len];
let first_segment =
&decoded[..strings::index_of_char_usize(decoded, b'/').unwrap_or(decoded.len())];
// `resolve` and a browser would cut at a decoded `#` or `?`, and decode a `%` once more.
if strings::contains_any(decoded, b"#?%")
// The output is the raw file name, which cannot carry these to a browser.
|| strings::contains_char(decoded, b'"')
|| decoded.iter().any(u8::is_ascii_control)
|| !strings::is_valid_utf8(decoded)
|| strings::ends_with_char(decoded, b' ')
// A `:` here reads as a scheme or a drive letter.
|| strings::contains_char(first_segment, b':')
{
return None;
}
Comment on lines +6956 to +6966

@claude claude Bot Oct 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔴 Users running bun build --minify on url(./my%20photo.png) now get a passing build whose rule the browser drops, where the base failed with Could not resolve. The guard at src/resolver/resolver.rs:6958 refuses " and control bytes because the copied asset's name is substituted verbatim into the printed url(), yet accepts space, ', ( and ), which the unquoted minified form cannot carry either. Fix: make the emitted url() valid for every byte the decoder accepts in both quoted and unquoted output, e.g. escape at the asset substitution in src/bundler/Chunk.rs:781, or until then refuse those bytes here as " is. The PR calls this accepted; it turns a loud build error into silently broken production CSS. [also at: src/resolver/resolver.rs:6967 - Users bundling a CSS url() to a copied asset with --minify now get a build that passes but a rule the browser drops, where the base failed loudly.]

Why this was flagged

A stylesheet contains url(./my%20photo.png) (or any name whose decoded form has a space, ', ( or )) and the build runs with --minify/minifyWhitespace. percent_decoded_css_url at src/resolver/resolver.rs:6956-6966 accepts the decoded ./my photo.png. With minify, src/css/values/url.rs:110-140 prints the asset placeholder unquoted, and src/bundler/Chunk.rs:769-781 substitutes dest_path verbatim, so the output is url(./my photo-<hash>.png). Per CSS Syntax an unquoted url token with whitespace followed by a non-) is a bad-url token, so the declaration is dropped. On the base branch the same input fails at build time with Could not resolve: "./my%20photo.png", so the author sees it. The guard's own comment at src/resolver/resolver.rs:6957 says bytes the raw output name cannot carry are refused, but that criterion is applied only to ", control bytes and a trailing space, not to the bytes serialize_unquoted_url (src/css/css_parser.rs:5842-5844) must escape.

Verification: Acknowledged in the PR description's "Downsides" and a pinned test. percent_decoded_css_url (src/resolver/resolver.rs:6956-6966) accepts ./my photo.png; src/css/values/url.rs:110-124 prints the placeholder unquoted and src/bundler/Chunk.rs:769-781 substitutes dest_path verbatim, yielding url(./my photo-<hash>.png), a bad-url token the browser drops, where the base printed Could not resolve.

Some(decoded)
}

/// TypeScript matches `./foo.js` with `./foo.ts` (microsoft/TypeScript#4595). This
/// is esbuild's `rewrittenFileExtensions` table plus Bun's `.js` → `.mts`
/// (oven-sh/bun#12580). `inside_node_modules` gates only `.mjs` and `.cjs`:
Expand Down
Loading
Loading