Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
104 changes: 91 additions & 13 deletions src/bundler/Chunk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -566,6 +566,69 @@ impl IntermediateOutput {
dst
}

/// Extra bytes needed to render `path` inside a double-quoted JS string
/// literal. The printer emits every unique-key placeholder inside `"..."`,
/// so only the characters that terminate or corrupt such a literal need
/// escaping here: `"`, `\`, LF, CR, and U+2028/U+2029.
Comment on lines +569 to +572

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

fn js_string_extra_escape_bytes(path: &[u8]) -> usize {
let mut extra: usize = 0;
let mut i: usize = 0;
while i < path.len() {
match path[i] {
b'"' | b'\\' | b'\n' | b'\r' => extra += 1,
0xE2 if i + 2 < path.len() && path[i + 1] == 0x80 && (path[i + 2] & !1) == 0xA8 => {
extra += "\\u2028".len() - 3;
i += 2;
}
_ => {}
}
i += 1;
}
extra
}

/// Copy `path` into `dest`, escaping the bytes counted by
/// `js_string_extra_escape_bytes`. Returns bytes written.
Comment on lines +590 to +591

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

fn memcpy_js_string_escaped(dest: &mut [u8], path: &[u8]) -> usize {
let mut dst: usize = 0;
let mut i: usize = 0;
while i < path.len() {
let b = path[i];
match b {
b'"' | b'\\' => {
dest[dst] = b'\\';
dest[dst + 1] = b;
dst += 2;
}
b'\n' => {
dest[dst] = b'\\';
dest[dst + 1] = b'n';
dst += 2;
}
b'\r' => {
dest[dst] = b'\\';
dest[dst + 1] = b'r';
dst += 2;
}
0xE2 if i + 2 < path.len() && path[i + 1] == 0x80 && (path[i + 2] & !1) == 0xA8 => {
dest[dst..][..6].copy_from_slice(if path[i + 2] == 0xA8 {
b"\\u2028"
} else {
b"\\u2029"
});
dst += 6;
i += 2;
}
_ => {
dest[dst] = b;
dst += 1;
}
}
i += 1;
}
dst
}

pub(crate) fn get_size(&self) -> usize {
match self {
IntermediateOutput::Pieces(pieces) => {
Expand Down Expand Up @@ -696,6 +759,10 @@ impl IntermediateOutput {
graph.input_files.items_unique_key_for_additional_file();
let mut relative_platform_buf = bun_paths::path_buffer_pool::get();
let mut file_path_buf = bun_paths::path_buffer_pool::get();
// In JS chunks every placeholder lands inside a printer-emitted `"..."`
// literal; the substituted path must be JS-string-escaped so filename
// bytes cannot terminate the literal.
Comment on lines +762 to +764

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

let escape_for_js = chunk.content.is_javascript();
match self {
IntermediateOutput::Pieces(pieces) => {
let entry_point_chunks_for_scb = linker_graph.files.items_entry_point_chunk_index();
Expand Down Expand Up @@ -808,6 +875,15 @@ impl IntermediateOutput {
QueryKind::None => unreachable!(),
};

// Same `\` → `/` normalization as the write pass so the
// escape-byte count below matches what will be emitted.
Comment on lines +878 to +879

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

let file_path: &[u8] = {
let n = file_path.len();
let dst = &mut file_path_buf[..n];
dst.copy_from_slice(file_path);
bun_paths::resolve_path::platform_to_posix_in_place::<u8>(dst);
dst
};
let cheap_normalizer = cheap_prefix_normalizer(
import_prefix,
if use_outdir_relative_path {
Expand All @@ -822,6 +898,10 @@ impl IntermediateOutput {
},
);
count += cheap_normalizer[0].len() + cheap_normalizer[1].len();
if escape_for_js {
count += Self::js_string_extra_escape_bytes(cheap_normalizer[0])
+ Self::js_string_extra_escape_bytes(cheap_normalizer[1]);
}
Comment thread
robobun marked this conversation as resolved.
}
QueryKind::None => {}
}
Expand Down Expand Up @@ -1006,22 +1086,20 @@ impl IntermediateOutput {
},
);

if !cheap_normalizer[0].is_empty() {
remain[..cheap_normalizer[0].len()]
.copy_from_slice(cheap_normalizer[0]);
remain = &mut remain[cheap_normalizer[0].len()..];
if ENABLE_SOURCE_MAP_SHIFTS {
shift.after.advance(cheap_normalizer[0]);
for part in cheap_normalizer {
if part.is_empty() {
continue;
}
}

if !cheap_normalizer[1].is_empty() {
remain[..cheap_normalizer[1].len()]
.copy_from_slice(cheap_normalizer[1]);
remain = &mut remain[cheap_normalizer[1].len()..];
let written = if escape_for_js {
Self::memcpy_js_string_escaped(remain, part)
} else {
remain[..part.len()].copy_from_slice(part);
part.len()
};
if ENABLE_SOURCE_MAP_SHIFTS {
shift.after.advance(cheap_normalizer[1]);
shift.after.advance(&remain[..written]);
}
remain = &mut remain[written..];
}

if ENABLE_SOURCE_MAP_SHIFTS {
Expand Down
168 changes: 167 additions & 1 deletion test/bundler/bundler_loader.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { fileURLToPath, Loader } from "bun";
import { describe, expect } from "bun:test";
import { describe, expect, test } from "bun:test";
import { bunEnv, bunExe, isWindows, tempDir } from "harness";
import fs, { readdirSync } from "node:fs";
import { join } from "path";
import { itBundled } from "./expectBundled";
Expand Down Expand Up @@ -521,6 +522,171 @@ describe("bundler", async () => {
}
});

// Windows cannot represent ", \, \n in filenames.
describe.skipIf(isWindows)("file loader escapes asset path in JS output", () => {
const assetContent = "asset-bytes";
const cases: Array<[label: string, name: string]> = [
["double quote injection", 'x";process.exit(42);"y.txt'],
["newline", "nl\nname.txt"],
["carriage return", "cr\rname.txt"],
["line separator U+2028", "ls\u2028name.txt"],
];

for (const [label, name] of cases) {
test.concurrent(`bundle: ${label}`, async () => {
using dir = tempDir("file-loader-escape", {
"entry.ts":
`import p from ${JSON.stringify("./" + name)} with { type: "file" };\n` +
`import path from "node:path";\n` +
`import fs from "node:fs";\n` +
`const abs = path.resolve(import.meta.dir, p);\n` +
`console.log(JSON.stringify({ path: p, content: fs.readFileSync(abs, "utf8") }));\n`,
});
fs.writeFileSync(join(String(dir), name), assetContent);

{
await using proc = Bun.spawn({
cmd: [bunExe(), "build", "--target=bun", "./entry.ts", "--outdir=./out"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).not.toContain("error:");
expect(exitCode).toBe(0);
}

await using proc = Bun.spawn({
cmd: [bunExe(), "./out/entry.js"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stdout: stdout.trim(), exitCode }).toEqual({
stdout: expect.stringContaining(`"content":"${assetContent}"`),
exitCode: 0,
});
expect(stderr).not.toContain("SyntaxError");
const emitted = JSON.parse(stdout).path as string;
expect(fs.existsSync(join(String(dir), "out", emitted))).toBe(true);
});
}

for (const [label, name] of [
["double quote injection", 'x";process.exit(42);"y.txt'],
["newline in filename", "nl\nname.txt"],
] as const) {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
test(`compile: ${label}`, async () => {
using dir = tempDir("file-loader-escape-compile", {
"entry.ts":
`import p from ${JSON.stringify("./" + name)} with { type: "file" };\n` +
`console.log(JSON.stringify({ path: p, content: await Bun.file(p).text() }));\n`,
});
fs.writeFileSync(join(String(dir), name), assetContent);
const outfile = join(String(dir), "exe");

{
await using proc = Bun.spawn({
cmd: [bunExe(), "build", "--compile", "./entry.ts", "--outfile", outfile],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).not.toContain("error:");
expect(exitCode).toBe(0);
}

await using proc = Bun.spawn({
cmd: [outfile],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stdout: stdout.trim(), exitCode }).toEqual({
stdout: expect.stringContaining(`"content":"${assetContent}"`),
exitCode: 0,
});
expect(stderr).not.toContain("SyntaxError");
});
}

test.concurrent("bundle: public-path with double quote and backslash", async () => {
using dir = tempDir("file-loader-escape-public-path", {
"entry.ts":
`import p from "./asset.txt" with { type: "file" };\n` + `console.log(JSON.stringify({ path: p }));\n`,
"asset.txt": assetContent,
});

{
await using proc = Bun.spawn({
cmd: [
bunExe(),
"build",
"--target=bun",
'--public-path=";process.exit(42);\\"/',
"./entry.ts",
"--outdir=./out",
],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).not.toContain("error:");
expect(exitCode).toBe(0);
}

await using proc = Bun.spawn({
cmd: [bunExe(), "./out/entry.js"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stdout: stdout.trim(), exitCode }).toEqual({
stdout: expect.stringContaining('{"path":'),
exitCode: 0,
});
expect(stderr).not.toContain("SyntaxError");
expect(JSON.parse(stdout).path).toStartWith('";process.exit(42);\\"/asset-');
});
});

// Count pass and write pass must agree on the substituted bytes; on Windows
// the write pass posix-normalizes `\` -> `/` before escaping, so the count
// pass must too. A subdir in --asset-naming is enough to put a separator in
// dest_path; the output must be parseable and contain no trailing NUL bytes.
itBundled("bun/loader-file-asset-naming-subdir", {
target: "bun",
outdir: "/out",
assetNaming: "assets/[name]-[hash].[ext]",
files: {
"/entry.ts": /* js */ `
import p from "./data.txt" with { type: "file" };
console.log(JSON.stringify({ path: p }));
`,
"/data.txt": "asset-bytes",
},
run: {
validate({ stdout }) {
expect(JSON.parse(stdout).path).toMatch(/^\.\/assets\/data-[a-z0-9]+\.txt$/);
},
},
onAfterBundle(api) {
const out = api.readFile("out/entry.js");
expect(out).not.toContain("\0");
},
});

// Lazy-export modules (JSON, TOML, CSS modules, ...) used to crash the
// printer when bundled with the dev server's module format.
// https://github.com/oven-sh/bun/issues/31943
Expand Down
Loading