Repository navigation
css: keep a url() in an at-rule prelude as written, and never print the resolved path of a url() - #45604
css: keep a url() in an at-rule prelude as written, and never print the resolved path of a url()#45604robobun wants to merge 5 commits into
Conversation
… a resolved path A url() in the token-list prelude of an unknown at-rule, or in an env() fallback of a media query, is not an import. The parser now keeps it as a raw token, so it gets no import record: the bundler does not resolve it, does not load its target, and it prints as written. The CSS printer no longer falls back to the resolved path of a url() whose target has no data: URL and no asset key. It prints the specifier as written, so the output does not carry the absolute path of the build directory.
The tests for a declaration url() with no asset URL now check only that the build directory is not printed. A plain-CSS composes has its own test. New tests: a url() in a CSS module token list is still an import, and a prelude url() builds with --no-bundle.
#45487 adds a ParserOpts flag at 0b10. Two flags with the same value compile, and the prelude flag would then be set for every CSS module.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 14 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Status: the PR is open and waits for CI. Reproduced with: mkdir -p sub && echo '.c0 { color: red }' > sub/g.css
echo '@media print { .c2 { z-index: 1 } @import url(sub/g.css); }' > entry.css
bun build ./entry.css --minify --outdir out && cat out/entry.css
# bun 1.4.3-canary (367d939d9) and main (5866b5f13d): @import url(/abs/build/dir/sub/g.css), "Bundled 2 modules"
# this branch: @import url(sub/g.css), "Bundled 1 module"
USE_SYSTEM_BUN=1 bun test test/bundler/esbuild/css.test.ts # 16 fail
bun bd test test/bundler/esbuild/css.test.ts # 72 pass, 1 skip, 1 todo
USE_SYSTEM_BUN=1 bun test test/bake/dev/css.test.ts # 1 fail
bun bd test test/bake/dev/css.test.ts # 16 passPR: #45604 |
& inside :global()
#45487
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline note, I also checked that the AT_RULE_PRELUDE_TOKENS flag cannot leak: parse_nested_block and try_parse reuse the same Parser, and parse_at_rule_prelude_tokens restores self.flags on both the Ok and Err path before try_parse rewinds, so a declaration url() later in the stylesheet still gets an import record. I also checked the printer fallthrough: original_path is only returned for records with a valid source_index, so external and unresolved url()s still print path.text as before. The user-visible semantics change (prelude url() no longer resolved, not in the metafile, not seen by plugin onResolve) and the two questions the description poses are design calls for a maintainer.
Extended reasoning...
The change touches the Rust CSS parser and printer in src/css/ (a new ParserOpts flag, a scoped wrapper, two TokenList::parse guards, a printer fallthrough) plus ~540 lines of bundler and dev-server tests; it touches no auth, crypto or injection surface. The flag scoping and the printer fallthrough were traced and hold up, but the change alters bundler behavior for url() in at-rule preludes and for url() to non-asset files, and the description itself asks a maintainer to rule on whether to follow esbuild's build error, so a human decision is still needed.
The linker clones an import condition and never frees the token list of an env() fallback. LeakSanitizer reports it for a spawned bun build, also for env(foo, 1px) on main. The bundled test for that input is skipped under ASAN. The --no-bundle test covers the same input there.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
The exact url() assertions in b075ef3 address my earlier comment. Beyond the inline ASAN-skip finding, I also checked the new original_path fallback in src/css/printer.rs for ?query#fragment loss — the suffix is a slice of original_path itself, so it is preserved (the ./sub/g.css?x=1#y case in the test covers it) — and the flag save/restore in parse_at_rule_prelude_tokens: ParserOpts is not part of ParserState, nested blocks reuse the same Parser, so the flag reaches the env() fallback token list and is restored even when the enclosing try_parse fails.
Extended reasoning...
The PR adds a parser flag so url() tokens in unknown at-rule preludes and media-query env() fallbacks stay raw tokens without import records, and changes the CSS printer to emit the written specifier instead of the resolver's absolute path for records with no URL form. It touches no security-sensitive surface. A human look remains worthwhile because the change alters user-visible bundler behavior (at-rule prelude url() targets are no longer loaded or copied, and declaration url() to stylesheets now prints as written where esbuild errors), and the description itself poses two open questions for a maintainer.
| // Before: `panic: index out of bounds: the len is 0 but the index is 0`. | ||
| // The url() record index pointed into the importing file's records, and | ||
| // the condition was printed with the records of the import chain. | ||
| // Skipped under ASAN: the linker clones an import condition and never frees | ||
| // the token list of an env() fallback (`Direct leak of 72 byte(s)` in | ||
| // `TokenList::deep_clone`), also for `env(foo, 1px)` on main. | ||
| (isASAN ? itBundled.skip : itBundled)("css/URLInEnvFallbackOfAtImportMediaQuery", { |
There was a problem hiding this comment.
🟡 nit (optional): maintainers get the only regression test for the @ import ... env(foo, url(...)) panic skipped on the ASAN lane, so that lane never exercises the fixed path. The skip at test/bundler/esbuild/css.test.ts:1436 routes around a leak instead of fixing it. The leak is pre-existing: MediaFeatureValue::deep_clone at src/css/media_query.rs:1379 clones the env fallback into a heap Vec inside an arena-allocated value that never runs Drop. Fix: free or arena-allocate the fallback TokenList in the linker's import-condition clone so the test runs under ASAN, then drop the isASAN ? itBundled.skip : itBundled gate.
Why this was flagged
The test css/URLInEnvFallbackOfAtImportMediaQuery at test/bundler/esbuild/css.test.ts:1436 is wrapped in (isASAN ? itBundled.skip : itBundled). On the ASAN CI lane it is skipped, so the input @ import "./b.css" (min-width: env(foo, url(sub/g.css))) that used to panic with index out of bounds: the len is 0 but the index is 0 is never built there. The comment at test/bundler/esbuild/css.test.ts:1433-1435 names the observed failure, Direct leak of 72 byte(s) in TokenList::deep_clone, which comes from MediaFeatureValue::deep_clone at src/css/media_query.rs:1379 cloning the env fallback; the token list is a heap Vec held by an arena value that never runs Drop. REVIEW.md says a test must not be edited to route around a runtime bug it exposed. On the base branch there is no test at all here; after this change the non-ASAN lanes cover the panic but the ASAN lane does not, and the leak stays.
Verification: test/bundler/esbuild/css.test.ts:1436 reads (isASAN ? itBundled.skip : itBundled), so under ASAN the only bundled fixture for @ import "./b.css" (min-width: env(foo, url(sub/g.css))) is never built. The leak is pre-existing: MediaFeatureValue::deep_clone at src/css/media_query.rs:1377-1380 clones the env fallback TokenList heap Vec into a slab that is never dropped.
There was a problem hiding this comment.
Agreed: the skip goes around the leak, and the ASAN lane does not build the bundled path for this input.
The leak is on main without this PR. On a debug build of 5866b5f, @import "./b.css" (min-width: env(foo, 1px)); reports Direct leak of 144 byte(s) in 2 object(s) from the same TokenList::deep_clone frames. The fix must change who owns the cloned import conditions in the linker. There are three clone sites (conditions_with_import_records, deep_clone_conditions, wrap_rules_with_conditions), and none of the clones is dropped, because the Vec headers are shared bitwise. That is a separate change, and it is in progress outside this PR. It removes this skip.
Until then, the --no-bundle test builds the same input under ASAN. It passes only when the url() gets no import record, but it does not run the linker. I leave this thread open.
This branch has not been deployed
2 participants
Problem
bun buildwrites the absolute build directory into CSS:@media print { @import url(sub/g.css); }prints@import url(/home/me/app/sub/g.css). So doesbackground: url(sub/g.css). Found by a fuzzer.parse_custom_at_rule_prelude(src/css/css_parser.rs:365) makes an import record for eachurl()in an unknown at-rule prelude, so the bundler loads the target or fails:Could not resolve: "sub/g.css".Printer::get_import_record_url(src/css/printer.rs:380) prints the resolvedpath.textwhen the target has nodata:URL or asset key.Fix
url()in an unknown at-rule prelude or in a media-queryenv()fallback stays a raw token with no import record. This also removespanic: index out of bounds: the len is 0 but the index is 0for@import "./b.css" (min-width: env(x, url(y.css)));.original_pathfor a resolved record with no URL form.test/bundler/esbuild/css.test.ts(17 new tests, 16 fail on main) andtest/bake/dev/css.test.ts(1). Alsotest/bundler/css/,test/js/bun/css/.composesgate, the linker pairing, hoist tests.Background
url()or@import. Resolution overwrites itspath.original_pathkeeps the written text.{or;. An unknown or misplaced at-rule (a nested@import) keeps it as tokens.parse_at_rule: 7 instructions per at-rule.Downsides
@foo url(x.png);is no longer inlined or copied.url(x.css)prints as written, not copied. esbuild fails this build..text+0 bytes. 15 stylesheets run the new code 18 times.Notes
Report. No GitHub issue. An automated fuzz ledger found it (ledger #503290, row 42055). Repro:
Before:
@media print{.c2{z-index:1}@import url(/abs/build/dir/sub/g.css);}.c0{color:#00f}andBundled 2 modules. After:@media print{.c2{z-index:1}@import url(sub/g.css);}.c0{color:#00f}andBundled 1 module. esbuild 0.21.5 prints the same bytes, with a warning that@importis only valid at the top level.Reach is small. A scan of 16,977 stylesheets in 556 npm packages found one file with a local
url()in an unknown at-rule prelude (a PostCSS source inmaplibre-glthat uses@svg-load). It found nourl()in anenv()in a prelude and no declarationurl()to a.css,.tomlor.shfile.Questions for a maintainer.
url()to a file with no URL form (.css,.toml,.sh). This PR keeps exit 0 and prints the specifier as written. The target is still resolved and parsed, and nothing is emitted for it. esbuild fails the build (Cannot use "x.css" as a URL). Bun already fails for.json,.jsand other loaders (Cannot import a ".json" file into a CSS file), andcss/InvalidImportURLInCSShas aTODO FIXfor the stylesheet case. The error is about 10 lines inscan_css_imports(src/bundler/LinkerContext.rs). It is a new build failure, so it is not in this PR. Say so if it must be. The tests for this case pin the written specifier. They becomebundleErrorstests if the answer is the error.url()in@container style(<declaration>)as written. Then the query no longer matches the rewritten custom property. This PR does not changestyle(). Say so if it must follow esbuild.Measured (base
5866b5f13d):How: the bun_css object is the crate compiled alone with the release rustc flags and no LTO (
llvm-objdump,llvm-nm -S). Its text grows 191 bytes (1,713,150 to 1,713,341). The two flag tests aretestb $0x4, 0x19(%r15)and a jump.parse_custom_at_rule_preludegrows 6 instructions andget_import_record_urlgrows 5. In the LTO release binary the.textsection is 81,071,045 bytes before and after. Four functions change size there, +189 bytes net (nm -Sonbun-profile). The corpus istest/js/bun/css/files/*.cssin onebun build, counted with gdb breakpoints on the debug build. The determinism run builds 15 inputs 60 times in each of two directories with--entry-naming '[name]-[hash].[ext]'. Before, each input gave one output per directory, and two files with aurl()to the same stylesheet gave 4. The pinned outputs are the fixtures ofcss/MissingImportURLInCSS,ExternalImportURLInCSS,TextImportURLInCSSText,Png,FileImportURLInCSS,URLQueryFragmentPreserved*,PackageURLsInCSS, and the 15 stylesheets with and without--minify. The release builds and the last three lines are from9cfe41342e. The head differs from it in comments, tests and the value of the flag bit.What changes for users.
url()in an unknown at-rule prelude (@foo url(x.png);, a nested@import url(), a nested@namespace url()) prints as written. Its target is not resolved, loaded, inlined or copied. A missing target no longer fails the build. A pluginonResolvedoes not see it. It is not a metafile import (esbuild lists it there as an external import).url()in anenv()fallback in@media,@container,@custom-mediaand the media list of@import.url()token is kept as written. The other prelude tokens still go through the value token list.@foo url(a.css); @import "./b.css";now inlinesb.css. Before, theurl()record took the place of the@importrecord anda.csswas inlined.url()in a declaration, a custom property,image-set(),@font-facesrcor an unknown at-rule block still resolves. When its target has no URL form, the specifier prints as written. This includes a file with thedataurlorbase64loader, which still gets no data URL (pluginonLoadwith dataurlloaderbundles empty content""broken and incompatible with esbuild #20917), andcomposes: a from "./x.css"in a file that is not a CSS module, which still bundles its target.url()to the same stylesheet print the same bytes in every build.bun build --no-bundleno longer rejects aurl()in an unknown at-rule prelude. A declarationurl()still fails there (bun build --no-bundle: keep css url() references instead of failing to parse #38528).@import url()to the top level (a style rule with nesting lowered,@media allunder--minify), the output now has a relative@import url(sub/g.css)that a browser loads. Before, it had the absolute path, or the build failed on a missing target. The string form@import "sub/g.css"already does this on main.Not in this PR. Each has its own cause.
url()with no URL form (question 1).@importthat becomes a live top-level@import(rules/style.rs,rules/media.rs).@importafter an unknown at-rule statement is inlined and also left in the output (prepareCssAstsForChunk.rsstrips only a leading run of imports).findImportedFilesInCSSOrder.rspairs an@importrule with its record by position, not byImportRule.import_record_idx.composes ... fromin a plain stylesheet creates an import and bundles the target.url()whose declaration the minifier drops, or whose@font-faceor@keyframesitem fails to parse, is still resolved and copied. Thedebug_asserthere covers only the two prelude scopes.@property --x { initial-value: url(x) }fails withUnexpected token: url(x)..darkprints as. dark).dataurlandbase64bundler loaders give an empty string (pluginonLoadwith dataurlloaderbundles empty content""broken and incompatible with esbuild #20917).env()fallback. LeakSanitizer reports it for@import "./b.css" (min-width: env(foo, 1px));on main. The input that panicked before now reaches the same leak.require.resolve()in JS prints the resolved absolute path (src/js_printer/lib.rs, bundler: print the require.resolve() specifier as written #42675).Related open PRs.
&inside :global() #45487 adds aParserOptsflag at0b10that is set for every CSS module. This PR uses0b100. Two flags with one value compile, and every token-listurl()in a CSS module would then lose its import.css/URLInCSSModuleTokenListIsStillAnImportfails in that case.get_import_record_url. The one that merges second wraps the new return.original_pathchange forrequire.resolve(). css: preserve url() query/fragment suffix when rewriting asset references #33685 added theoriginal_pathread that this PR reuses.Design. esbuild decides this in the CSS parser.
convertTokenspassesallowImports: falsefor an at-rule prelude, and the record is made withast.IsUnusedand never resolved. Declarations and unknown at-rule blocks passtrue. Bun has the same flag (IS_UNUSED), but a record that exists is not safe here: the linker pairs@importrules with records by position, and an import condition is printed with a different record list (the panic above). So the preludeurl()gets no record. The flag is set at the two places that parse a prelude as a token list (parse_custom_at_rule_prelude, and theenv()attempt inMediaFeatureValue::parse_unknown). It is read only by the twourlarms ofTokenList::parse_into_impl.parse_at_ruleandParser::add_import_recordare not changed. Adebug_assertin the helper checks that no import record appears while the flag is set.Suites run with the debug build:
test/bundler/esbuild/css.test.ts(72 pass, 1 skip, 1 todo),test/bake/dev/css.test.ts(16 pass), and 36 more files (test/bundler/css/,bundler_html,bundler_plugin,bundler_loader,bundler_files,metafile,standalone,test/js/bun/http/bun-serve-html.test.ts,test/js/bun/css/): 2992 pass. Threecss-fuzztests hit their own 10 s limit on the debug build, with and without this change. They pass on a release build of9cfe41342e.css/URLInEnvFallbackOfAtImportMediaQueryis skipped under ASAN because of the leak above. With a release build of9cfe41342ethe file is 73 pass, and on main this test fails with the panic. No release build of the head was run here. Under ASAN, the--no-bundletest covers the same input: it builds only when theurl()gets no import record.