diff --git a/CHANGELOG.md b/CHANGELOG.md index 74f1334..7384498 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,21 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). carrying an unresolvable debug-id `missing_sym`, which is what prompts an upload — an unstamped bundle is silently unsymbolicated instead. +- **`debug-files upload --strip-sources-content`** (`--type sourcemaps`) — upload each map without + its embedded original source. `sourcesContent` carries your code verbatim, and it is what lets a + symbolicated crash show source lines; stripping it keeps file/line/column resolution and drops the + snippet, for teams who would rather their source did not leave the build machine. + + The map on disk is never modified — only the copy that is uploaded — and the declared `hash` + describes the stripped bytes rather than the file that was read. A map that carries no + `sourcesContent` (or is not the JSON object we expect) is uploaded byte-for-byte unchanged: this + is a privacy preference, not a validator. An INDEXED map (spec §Index-Map) keeps its source inside + `sections[].map`, and those are stripped too — removing only the top-level key would have shipped + the source while reporting success. Rejected (exit 20) for every other `--type`, since no + other symbol format embeds source. + + Measured on a real esbuild bundle: 253 → 181 bytes uploaded, local map untouched. + ### Fixed - **`debug-files upload --type sourcemaps --dry-run` no longer fails on a map that has no debug-id.** A dry run sends nothing, so an un-keyed map cannot register the unfindable symbol the diff --git a/README.md b/README.md index 6959e80..0efa85b 100644 --- a/README.md +++ b/README.md @@ -50,6 +50,7 @@ bugsee-cli debug-files upload ... \ [--force] # re-upload even if the server already has it (dsym/pdb/rust/il2cpp-linemap/sourcemaps) [--concurrency N] # sourcemaps only: ceiling on uploads in flight, 1..=32 (default: scaled) [--allow-empty] # sourcemaps only: "nothing to upload" is success, not exit 10 + [--strip-sources-content] # sourcemaps only: upload maps without the embedded source [--dry-run] ``` @@ -221,6 +222,13 @@ rather than fatal there (`unkeyed` in the completion log) — the whole flow can freshly built directory, where `sourcemaps inject --dry-run` has deliberately written nothing yet. A REAL run still refuses such a map (exit 11): uploading it would register a symbol nothing can find. +`--strip-sources-content` uploads each map WITHOUT its `sourcesContent` — including the copies an +indexed map keeps inside `sections[].map` — for teams who would rather their source did not leave +the build machine. Symbolication still resolves file, line and column; +what is lost is the source snippet shown beside a crash frame. The map on disk is never modified — +only the copy that is uploaded — and the declared `hash` describes the stripped bytes. A map that +carries no `sourcesContent` is uploaded byte-for-byte unchanged. + `--allow-empty` turns "nothing to upload" into success (exit 0) instead of exit 10 — a monorepo package built without maps, or a framework whose server output has none, is a legitimate no-op rather than a reason to fail the build. @@ -228,8 +236,9 @@ A path that does not exist is an error regardless (`path does not exist:

`, exit 10) — including when other paths do hold maps — so a typo or a build that never ran cannot half-upload a build's symbols. -Both flags apply to `--type sourcemaps` only, and are rejected (exit 20) for any -other type rather than accepted and ignored. +`--concurrency`, `--allow-empty` and `--strip-sources-content` apply to +`--type sourcemaps` only, and are rejected (exit 20) for any other type rather +than accepted and ignored. `--exclude ` (repeatable) keeps `inject` out of part of a build output — `--exclude '**/node_modules/**'` leaves vendored third-party code inside a server bundle untouched, `--exclude @@ -247,7 +256,7 @@ unsymbolicated instead. Measured on a stock `next build` with browser source map ``` bugsee-cli sourcemaps inject ... [--exclude ]... [--dry-run] bugsee-cli debug-files upload --type sourcemaps ... --version --build \ - [--concurrency N] [--allow-empty] + [--concurrency N] [--allow-empty] [--strip-sources-content] ``` ### `xcode upload-dsyms` diff --git a/scripts/e2e_flows.py b/scripts/e2e_flows.py index 834ec54..429c26a 100644 --- a/scripts/e2e_flows.py +++ b/scripts/e2e_flows.py @@ -449,6 +449,36 @@ def main(): os.path.join(fix, "mapping.txt")] + v) results["sourcemaps_upload"] = run(binpath, "sourcemaps", ["debug-files", "upload", "--type", "sourcemaps", os.path.join(fix, "dist", "app.js.map")] + v) + + # --strip-sources-content: the uploaded copy must not carry the source, and the file on disk + # must be untouched — it is the user's build output. + sc_map = os.path.join(fix, "with-sources", "app.js.map") + os.makedirs(os.path.dirname(sc_map), exist_ok=True) + with open(sc_map, "w") as f: + json.dump({"version": 3, "debug_id": "77777777-7777-7777-7777-777777777777", + "sources": ["app.ts"], "sourcesContent": ["const secret = 1;"], + "names": [], "mappings": "AAAA"}, f) + before = open(sc_map, "rb").read() + results["sourcemaps_strip_sources_content"] = run( + binpath, "sourcemaps_strip", + ["debug-files", "upload", "--type", "sourcemaps", "--strip-sources-content", sc_map] + v) + try: + import zipfile as _zf + uploaded = None + for name in os.listdir(STATE["cap"]): + if name.startswith("sourcemaps_strip__") and name.endswith(".bin"): + with _zf.ZipFile(os.path.join(STATE["cap"], name)) as z: + uploaded = json.loads(z.read(z.namelist()[0])) + results["sourcemaps_strip_uploads_no_source"] = ( + uploaded is not None + and "sourcesContent" not in uploaded + and uploaded.get("mappings") == "AAAA" + and open(sc_map, "rb").read() == before) + if not results["sourcemaps_strip_uploads_no_source"]: + print(f" [warn] uploaded={uploaded!r}") + except Exception as e: + print(" [warn] could not verify the strip flow:", e) + results["sourcemaps_strip_uploads_no_source"] = False results["elf"] = run(binpath, "elf", ["debug-files", "upload", "--type", "elf", os.path.join(fix, "native-debug-symbols.zip"), "--uuid", "11111111-2222-3333-4444-555555555555"] + v) diff --git a/src/cli/debug_files.rs b/src/cli/debug_files.rs index a2eeae3..9c6dd15 100644 --- a/src/cli/debug_files.rs +++ b/src/cli/debug_files.rs @@ -117,6 +117,15 @@ pub enum DebugFilesCommand { #[arg(long)] allow_empty: bool, + /// Upload source maps WITHOUT their embedded original source. `--type sourcemaps` only. + /// + /// A map's `sourcesContent` carries your source verbatim, which is how a symbolicated crash + /// shows source lines. Stripping it keeps file/line/column symbolication and drops the + /// snippet, for teams who would rather their code did not leave the build machine. The map + /// on disk is NOT modified — only the copy that is uploaded. + #[arg(long)] + strip_sources_content: bool, + /// Dry-run — discover and pack files but skip the HTTP upload. #[arg(long)] dry_run: bool, @@ -210,6 +219,7 @@ pub async fn dispatch( force, concurrency, allow_empty, + strip_sources_content, dry_run, } => { let kind = r#type.unwrap_or(DebugFileType::Proguard); @@ -260,6 +270,11 @@ pub async fn dispatch( "--allow-empty is only valid for --type sourcemaps — every other type treats an empty input as a configuration mistake", )); } + if kind != DebugFileType::Sourcemaps && strip_sources_content { + return Err(config_invalid( + "--strip-sources-content is only valid for --type sourcemaps — no other symbol format embeds source", + )); + } if kind != DebugFileType::Sourcemaps && concurrency.is_some() { return Err(config_invalid( "--concurrency is only valid for --type sourcemaps — no other type uploads a batch of independent files", @@ -370,6 +385,7 @@ pub async fn dispatch( concurrency.map(usize::from), allow_empty, dry_run, + strip_sources_content, ) .await; } @@ -1399,6 +1415,9 @@ struct SourcemapUploadCtx<'a> { build: &'a str, strategy: Strategy, force: bool, + /// Drop `sourcesContent` from the COPY that is uploaded. The file on disk is the caller's build + /// output and is never rewritten. + strip_sources_content: bool, } /// Pack ONE `.map` and register + PUT it. `None` when this was a dry run (packed, never sent). @@ -1420,6 +1439,30 @@ async fn upload_one_sourcemap( .file_name() .and_then(|n| n.to_str()) .unwrap_or("bundle.js.map"); + + // `--strip-sources-content`: pack a COPY without the embedded original source. The file on disk + // is the caller's build output — rewriting it would take away their own debugging, and the + // bundler plugins delete these maps after a successful upload anyway. + // + // On the blocking pool for the same reason the packing is: parsing and re-serializing a + // multi-megabyte map costs more than the zstd does, and every upload future is polled by ONE + // task. + let stripped = { + let (owned_path, tmp_owned, strip) = ( + map_path.to_path_buf(), + tmpdir.path().to_path_buf(), + ctx.strip_sources_content, + ); + tokio::task::spawn_blocking(move || { + stripped_copy_without_sources(&owned_path, &tmp_owned, strip) + }) + .await + .map_err(|e| anyhow::anyhow!("stripping {} failed to run: {e}", map_path.display()))?? + }; + let (map_path, identity) = match stripped.as_ref() { + Some((path, id)) => (path.as_path(), id), + None => (map_path, identity), + }; // zstd-11 over a source map is real CPU work (~6 ms for a typical map, ~22 ms for a 1.6 MB // one). All these futures are polled by ONE task, so packing inline would serialise across // concurrent uploads and stall the in-flight requests' I/O while it ran. @@ -1508,6 +1551,7 @@ async fn run_sourcemap_upload( concurrency: Option, allow_empty: bool, dry_run: bool, + strip_sources_content: bool, ) -> anyhow::Result<()> { // A path that does not exist is a typo, not an empty build: it must not be swallowed by // `--allow-empty`, and naming it beats the "no .map files found under …" it used to produce. @@ -1686,6 +1730,7 @@ async fn run_sourcemap_upload( build, strategy, force, + strip_sources_content, }, &map_path, &identity, @@ -1739,6 +1784,70 @@ fn is_stylesheet_or_declaration_map(path: &Path) -> bool { .any(|suffix| name.ends_with(suffix)) } +/// Remove every `sourcesContent` a source map can carry; `true` when one was there. +/// +/// Not just the top-level key: an INDEXED map (spec §Index-Map) keeps its source inside +/// `sections[i].map`, and those sections nest. Stripping only the top level left the source in the +/// upload while reporting success — the flag's whole promise, failing silently. +fn remove_sources_content(map: &mut serde_json::Map) -> bool { + let mut removed = map.remove("sourcesContent").is_some(); + if let Some(serde_json::Value::Array(sections)) = map.get_mut("sections") { + for section in sections { + if let Some(serde_json::Value::Object(inner)) = section.get_mut("map") { + removed |= remove_sources_content(inner); + } + } + } + removed +} + +/// A copy of `map_path` with `sourcesContent` removed, plus its recomputed identity — or `None` +/// when nothing needs stripping (flag off, no `sourcesContent`, or the map is not the JSON object we +/// expect, which is a privacy preference's business to ignore rather than to fail over). +/// +/// The identity is recomputed because the declared `hash` must describe what is UPLOADED. The +/// debug-id is carried over untouched: it is the key, and re-deriving it here would break the pair +/// with the bundle that `sourcemaps inject` stamped. +fn stripped_copy_without_sources( + map_path: &Path, + tmpdir: &Path, + strip: bool, +) -> anyhow::Result> { + if !strip { + return Ok(None); + } + let bytes = std::fs::read(map_path)?; + let Ok(serde_json::Value::Object(mut map)) = serde_json::from_slice(&bytes) else { + return Ok(None); + }; + if !remove_sources_content(&mut map) { + return Ok(None); + } + let stripped = serde_json::to_vec(&map)?; + let name = map_path + .file_name() + .map(Path::new) + .unwrap_or_else(|| Path::new("bundle.js.map")); + let out = tmpdir.join(name); + std::fs::write(&out, &stripped)?; + tracing::info!( + path = %map_path.display(), + before_bytes = bytes.len(), + after_bytes = stripped.len(), + "stripped sourcesContent from the uploaded copy" + ); + let identity = sourcemap::SourcemapIdentity { + debug_id: crate::inject::read_debug_id(&out)?, + content_sha1_hex: { + use sha1::Digest; + let digest: [u8; 20] = sha1::Sha1::digest(&stripped).into(); + hex::encode(digest) + }, + size_bytes: stripped.len() as u64, + }; + Ok(Some((out, identity))) +} + /// Pack a `.dSYM` bundle's entries into a temp zip with the chosen strategy. /// Returns the tempdir guard (keep it alive until the PUT finishes), the zip /// path, and the zip size. Called only once the server has confirmed it wants @@ -2015,6 +2124,7 @@ mod sourcemap_upload_tests { None, false, false, + false, ) .await .unwrap(); @@ -2102,6 +2212,7 @@ mod sourcemap_upload_tests { None, false, false, + false, ) .await .unwrap_err(); @@ -2133,6 +2244,7 @@ mod sourcemap_upload_tests { None, false, true, + false, ) .await .unwrap(); @@ -2223,6 +2335,7 @@ mod sourcemap_upload_tests { concurrency: Option, allow_empty: bool, dry_run: bool, + strip_sources_content: bool, } async fn upload_paths( @@ -2242,6 +2355,7 @@ mod sourcemap_upload_tests { tweak.concurrency, tweak.allow_empty, tweak.dry_run, + tweak.strip_sources_content, ) .await } @@ -2378,6 +2492,7 @@ mod sourcemap_upload_tests { None, false, false, + false, ) .await .unwrap_err(); @@ -2414,6 +2529,7 @@ mod sourcemap_upload_tests { None, false, dry_run, + false, ) .await .unwrap_err(); @@ -2603,6 +2719,7 @@ mod sourcemap_upload_tests { None, false, false, + false, ) .await .unwrap(); @@ -2643,6 +2760,7 @@ mod sourcemap_upload_tests { None, false, false, + false, ) .await .unwrap(); @@ -2852,6 +2970,7 @@ mod sourcemap_upload_tests { Some(8), false, false, + false, ) .await .unwrap(); @@ -3019,9 +3138,237 @@ mod sourcemap_upload_tests { None, false, true, + false, + ) + .await + .unwrap(); + } + + /// What we actually PUT: the single `.map` entry, decompressed out of the uploaded zip. The + /// only way to assert on the bytes that leave the machine rather than on the ones on disk. + async fn uploaded_map_bytes(server: &MockServer) -> Vec { + let requests = server.received_requests().await.unwrap(); + let put = requests + .iter() + .find(|r| r.method.as_str() == "PUT") + .expect("no PUT"); + let mut archive = + zip::ZipArchive::new(std::io::Cursor::new(put.body.clone())).expect("not a zip"); + assert_eq!(archive.len(), 1, "one entry: just the map"); + let mut entry = archive.by_index(0).unwrap(); + let mut body = Vec::new(); + std::io::Read::read_to_end(&mut entry, &mut body).unwrap(); + body + } + + async fn uploaded_map_json(server: &MockServer) -> serde_json::Value { + let requests = server.received_requests().await.unwrap(); + let put = requests + .iter() + .find(|r| r.method.as_str() == "PUT") + .expect("no PUT"); + let mut archive = + zip::ZipArchive::new(std::io::Cursor::new(put.body.clone())).expect("not a zip"); + assert_eq!(archive.len(), 1, "one entry: just the map"); + let mut entry = archive.by_index(0).unwrap(); + let mut body = String::new(); + std::io::Read::read_to_string(&mut entry, &mut body).unwrap(); + serde_json::from_str(&body).expect("uploaded map is not JSON") + } + + /// `sourcesContent` embeds the ORIGINAL SOURCE in the map, which is how symbolication shows + /// source lines — and also means the upload ships the customer's code to the backend. Teams who + /// would rather it did not can strip it; they keep file/line/column symbolication and lose the + /// source snippet. + #[tokio::test] + async fn strip_sources_content_removes_the_source_from_what_is_uploaded() { + let tmp = tempfile::tempdir().unwrap(); + let original = br#"{"version":3,"file":"app.js","debug_id":"11111111-1111-1111-1111-111111111111","sources":["src/app.ts"],"sourcesContent":["const secret = 'hunter2';\n"],"names":[],"mappings":"AAAA"}"#; + write(tmp.path(), "app.js.map", original); + let server = collector(&[]).await; + + run_sourcemap_upload( + &[tmp.path().to_path_buf()], + &server.uri(), + "TKN", + "1.0", + "1", + None, + Strategy::Zstd(11), + false, + None, + false, + false, + true, + ) + .await + .unwrap(); + + let uploaded = uploaded_map_json(&server).await; + assert!( + uploaded.get("sourcesContent").is_none(), + "sourcesContent must not leave the machine: {uploaded}" + ); + // Everything symbolication needs is still there. + assert_eq!(uploaded["mappings"], "AAAA"); + assert_eq!(uploaded["sources"], serde_json::json!(["src/app.ts"])); + assert_eq!(uploaded["debug_id"], "11111111-1111-1111-1111-111111111111"); + + // The user's own file is untouched — it is their build output, and their local debugging. + let on_disk = std::fs::read(tmp.path().join("app.js.map")).unwrap(); + assert_eq!(on_disk, original, "the map on disk must not be rewritten"); + } + + /// The declared `hash` must describe what was UPLOADED, not the file we read. + #[tokio::test] + async fn stripping_updates_the_declared_hash() { + let tmp = tempfile::tempdir().unwrap(); + write( + tmp.path(), + "app.js.map", + br#"{"version":3,"debug_id":"22222222-2222-2222-2222-222222222222","sources":["a.ts"],"sourcesContent":["x"],"names":[],"mappings":"AAAA"}"#, + ); + let server = collector(&[]).await; + + run_sourcemap_upload( + &[tmp.path().to_path_buf()], + &server.uri(), + "TKN", + "1.0", + "1", + None, + Strategy::Zstd(11), + false, + None, + false, + false, + true, ) .await .unwrap(); + + let requests = server.received_requests().await.unwrap(); + let post: serde_json::Value = serde_json::from_slice( + &requests + .iter() + .find(|r| r.method.as_str() == "POST") + .unwrap() + .body, + ) + .unwrap(); + let uploaded = uploaded_map_json(&server).await; + let expected = { + use sha1::Digest; + let digest: [u8; 20] = + sha1::Sha1::digest(serde_json::to_vec(&uploaded).unwrap()).into(); + hex::encode(digest) + }; + assert_eq!(post["hash"].as_str().unwrap(), expected); + } + + /// Without the flag, the source rides along exactly as before — that is what makes the + /// symbolicated frame show a source line. + #[tokio::test] + async fn sources_content_is_kept_by_default() { + let tmp = tempfile::tempdir().unwrap(); + write( + tmp.path(), + "app.js.map", + br#"{"version":3,"debug_id":"33333333-3333-3333-3333-333333333333","sources":["a.ts"],"sourcesContent":["const x = 1;"],"names":[],"mappings":"AAAA"}"#, + ); + let server = collector(&[]).await; + + upload_dir(tmp.path(), &server.uri()).await.unwrap(); + + assert_eq!( + uploaded_map_json(&server).await["sourcesContent"], + serde_json::json!(["const x = 1;"]) + ); + } + + /// An INDEXED map (spec §Index-Map: `{"version":3,"sections":[{"offset":…,"map":{…}}]}`) keeps + /// its source inside each section, not at the top level. Removing only the top-level key left + /// the source in the upload while reporting success — the flag's entire promise, failing + /// silently. Webpack emits these for `devtool: 'source-map'` with certain plugins, and the + /// worker symbolicates them. + #[tokio::test] + async fn strip_reaches_inside_an_indexed_map() { + let tmp = tempfile::tempdir().unwrap(); + write( + tmp.path(), + "app.js.map", + br#"{"version":3,"debug_id":"55555555-5555-5555-5555-555555555555","sections":[ + {"offset":{"line":0,"column":0},"map":{"version":3,"sources":["a.ts"],"sourcesContent":["const password = 'hunter2';"],"names":[],"mappings":"AAAA"}}, + {"offset":{"line":9,"column":0},"map":{"version":3,"sources":["b.ts"],"sourcesContent":["const other = 2;"],"names":[],"mappings":"AACA"}} + ]}"#, + ); + let server = collector(&[]).await; + + run_sourcemap_upload( + &[tmp.path().to_path_buf()], + &server.uri(), + "TKN", + "1.0", + "1", + None, + Strategy::Zstd(11), + false, + None, + false, + false, + true, + ) + .await + .unwrap(); + + let body = String::from_utf8(uploaded_map_bytes(&server).await).unwrap(); + assert!( + !body.contains("hunter2") && !body.contains("sourcesContent"), + "source must not leave the machine in a section either: {body}" + ); + // …and the map is still usable: sections, offsets, mappings and the key survive. + let uploaded: serde_json::Value = serde_json::from_str(&body).unwrap(); + assert_eq!(uploaded["sections"].as_array().unwrap().len(), 2); + assert_eq!(uploaded["sections"][1]["offset"]["line"], 9); + assert_eq!(uploaded["sections"][0]["map"]["mappings"], "AAAA"); + assert_eq!(uploaded["debug_id"], "55555555-5555-5555-5555-555555555555"); + } + + /// A map that carries no `sourcesContent` — or is not the JSON we expect — uploads unchanged + /// rather than failing: the flag is a privacy preference, not a validator. + #[tokio::test] + async fn stripping_a_map_with_no_sources_content_changes_nothing() { + let tmp = tempfile::tempdir().unwrap(); + write( + tmp.path(), + "app.js.map", + br#"{"version":3,"debug_id":"44444444-4444-4444-4444-444444444444","sources":["a.ts"],"names":[],"mappings":"AAAA"}"#, + ); + let server = collector(&[]).await; + + run_sourcemap_upload( + &[tmp.path().to_path_buf()], + &server.uri(), + "TKN", + "1.0", + "1", + None, + Strategy::Zstd(11), + false, + None, + false, + false, + true, + ) + .await + .unwrap(); + + // BYTE-identical: with nothing to strip there is no reason to re-serialize the map, and + // doing so would change the bytes (and the hash) of a file we were asked to leave alone. + assert_eq!( + uploaded_map_bytes(&server).await, + std::fs::read(tmp.path().join("app.js.map")).unwrap() + ); } /// A build step that legitimately produces no maps — a monorepo package built without them, a @@ -3135,6 +3482,7 @@ mod sourcemap_upload_tests { None, false, true, + false, ) .await .unwrap_err(); diff --git a/tests/debug_files_flags.rs b/tests/debug_files_flags.rs index c7f65e3..5f119e2 100644 --- a/tests/debug_files_flags.rs +++ b/tests/debug_files_flags.rs @@ -171,6 +171,17 @@ fn sourcemap_only_flags_are_rejected_for_other_types() { "--concurrency is only valid for --type sourcemaps — no other type uploads a \ batch of independent files", )); + + upload( + kind, + &["--strip-sources-content", tmp.path().to_str().unwrap()], + ) + .assert() + .code(CONFIG_INVALID) + .stderr(contains( + "--strip-sources-content is only valid for --type sourcemaps — no other symbol format \ + embeds source", + )); } }