google / google/closure-compiler
Robustness: malformed sourceMappingURL URI aborts the whole compile (uncaught RuntimeException); deps.js addDependency path and manifest lines written unescaped
- Dominant language
- JavaScript
- Stars
- 7.7k
- Forks
- 1.2k
- Avg merge
- 2d 12h
- Merged PRs (30d)
- 6
Description
While reading the source-map resolution and output-emission paths (in the
area of #4322), I collected three small robustness gaps. None of these is a
security finding on its own — as #4322's discussion establishes, this project
does not treat compiled input as untrusted, and I am not contesting that
premise. They are ordinary-bug / output-hygiene items; bundling them because
they share the same input-provenance shape (names and paths that originate
from the compiled sources or their container) and the same fix spirit: parse
or emit such strings defensively or reject them loudly.
## 1. A malformed `sourceMappingURL` URI aborts the whole compile
`src/com/google/javascript/jscomp/SourceMapResolver.java:83-89`:
```java
private static boolean isAbsolute(String url) {
try {
return new URI(url).isAbsolute() || url.startsWith("/");
} catch (URISyntaxException e) {
throw new RuntimeException("Sourcemap url was invalid: " + url, e);
}
}
```
An input file ending in e.g. `//# sourceMappingURL=http://a b/` (the space
makes it an invalid URI) kills the entire compilation with an uncaught
`RuntimeException` — no diagnostic, no error count, just a stack trace from
inside the compiler. This is reachable with default flags
(`resolveSourceMapAnnotations` defaults to true), and it fires in
`CompilerInput.parse`, so the failure happens before any pass that would
normally report problems with the input. A malformed annotation in one file
also hides all ordinary diagnostics the run would otherwise have produced.
Suggested shape: catch the `URISyntaxException` and report it as a
compiler diagnostic on that input (the same way other malformed-input
conditions are surfaced), or treat the annotation as absent. This is in the
same file as the #4322 root cause, so whichever way #4322 lands (containment
check or default flip), the exception path is worth converting to a
diagnostic at the same time.
## 2. `goog.addDependency` path is interpolated into an executed JS string literal unescaped
`src/com/google/javascript/jscomp/deps/DependencyInfo.java:166-175` —
`DependencyInfo.Util.writeAddDependency` interpolates
`info.getPathRelativeToClosureBase()` into a single-quoted JS string literal
with no escaping. The neighboring emitters escape their fields:
`writeJsArray` escapes `'` for provides/requires, `writeJsObject` escapes
the load flags — the path gets neither.
A path containing `'` or `\` therefore produces broken or injected
JavaScript in the generated `deps.js`, which Closure Library loads and
executes in uncompiled/dev mode. Where such paths can come from: CLI
`--input` values, ZIP entry names (`SourceFile.fromZipFile`,
`SourceFile.java:485-506`, filtered only by `endsWith(".js")`), and paths
round-tripped from previously parsed `.deps.js` files (DepsFileRegexParser).
The practical case is ordinary paths with quotes or backslashes producing a
`deps.js` that throws on load; the injected-JS shape additionally requires
an attacker-influenced path, which is the #4322 premise — recorded here for
completeness, not claimed as an impact.
Suggested shape: escape the path the way `writeJsArray` escapes its
elements — the codebase already has the machinery
(`SourceCodeEscapers.appendWithJavascriptEscaper`), so this is a one-line
reuse.
## 3. Output manifest lines are written unescaped (newline injection from ZIP entry names)
`AbstractCommandLineRunner.printManifestTo` (around line 2200) writes each
input name followed by `\n` with no escaping. ZIP entry names can contain
embedded newlines and still pass the `.js` filter (e.g. an entry named
`x.js\ninjected.js`), so one input can contribute extra manifest lines.
Consumers treat manifests as file lists, so the practical effect today is
confusion in downstream tooling rather than a capability change — but the
manifest is a machine-consumed format, and one input producing multiple
lines violates its one-line-per-input contract.
Suggested shape: reject or escape newlines (and CR) in manifest lines the
same way the compiler rejects them in other emitted names.
---
All three are happy-path-visible with small inputs; items 2 and 3 can be
verified by reading the cited lines and constructing the example inputs
above (a ZIP with a newline in an entry name for item 3; a path containing
`'` for item 2 via the library API). Item 1 reproduces with any compile of a
JS file whose trailing annotation has a space in the URL. Happy to send a PR
for any subset — the escaping reuses in items 2/3 look mechanical.
Contributor guide
Assessment
This issue has not been assessed yet.