Confine JavaScript importer paths to allowed roots (GHSA-2223-f22x-24cq) - #237
Open
LukeTowers wants to merge 5 commits into
Open
Confine JavaScript importer paths to allowed roots (GHSA-2223-f22x-24cq)#237LukeTowers wants to merge 5 commits into
LukeTowers wants to merge 5 commits into
Conversation
The JS combiner `=include`/`=require` directives resolved their target with `realpath()` and no confinement check, so a `.js` asset containing `=include ../../../.env` inlined arbitrary server-readable files into combined output that `System\Classes\SystemController@combine` serves unauthenticated — an authenticated local file disclosure (APP_KEY, DB credentials, etc.). This is the JavaScript sibling of the LESS import flaw hardened for GHSA-58fp-mcx6-7qf9; that fix gated the LESS filter but left `JavascriptImporter` exploitable. Harden `JavascriptImporter::directiveInclude()` with two layers: - Restrict inlined files to the `.js` extension, rejecting `.env`/`.php`/`.log` and the like before any path math. Extension-less includes are unaffected — they are still resolved to `.js` first — and this mirrors the CSS importer, which only inlines `.css`. - Confine the resolved path to the including file's own directory subtree or a caller-configured allowed root, via a new shared `ImportGuard` policy. Extract the confinement decision shared with the LESS importer into `ImportGuard::isAllowed()`, and the allowed-roots storage into a `HasAllowedImportRoots` trait, so both filters enforce one identical policy. `System\Classes\CombineAssets` configures the roots (themes/plugins/modules). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The import confinement check was a single static method on a dedicated ImportGuard class. Fold it into PathResolver as a general-purpose `withinAny($path, $directories)` — a natural sibling of `within()` — which removes the one-method class and is reusable beyond the asset importers. The "source directory always allowed" semantics become just another entry in the directory list: callers pass the context dir (which may be null — non-string and empty entries are ignored) alongside the configured roots. JavascriptImporter and LessImportResolver call it directly; no behaviour change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The combiner's CSS import confinement relies on `CssImportFilter::setImportValidator()`, added in assetic/framework v3.2.1. Raise the minimum so the method is guaranteed present. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
withinAny() was only exercised transitively through the JS/LESS importer tests. Add a dedicated test covering multi-directory matching, the no-match and empty-list cases, and the null/empty/non-string skipping that lets callers pass a nullable context directory alongside the roots list. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Make the path-confinement guard in JavascriptImporter::directiveInclude() honour $required: a mandatory =require into a disallowed path now throws a RuntimeException, matching the not-found and disallowed-extension guards. The optional =include still degrades to an error comment. Also hoist the allowed-roots array out of the per-include loop. Adds a regression test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses GHSA-2223-f22x-24cq by hardening the JavaScript asset combiner’s =include / =require directives against local file disclosure via traversal and unconstrained realpath() resolution. It introduces a shared path-confinement helper and allowed-roots plumbing so import-capable filters can restrict imports to the including file’s subtree and caller-approved roots.
Changes:
- Harden
JavascriptImporterwith a.jsextension allowlist and path confinement (including stricter behavior for mandatory=require). - Add shared primitives for confinement and configuration (
PathResolver::withinAny()andHasAllowedImportRoots), and refactor LESS import gating to use them. - Add regression tests for the new confinement behavior and helper.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Parse/Assetic/JavascriptImporterTest.php | Adds regression tests covering traversal blocking, extension gating, allowed-root imports, and =require failure modes. |
| tests/Filesystem/PathResolverTest.php | Adds coverage for PathResolver::withinAny() including skipping null/empty entries. |
| src/Parse/Assetic/Filter/LessImportResolver.php | Refactors LESS import confinement check to use PathResolver::withinAny(). |
| src/Parse/Assetic/Filter/LessCompiler.php | Switches to shared allowed-roots storage via HasAllowedImportRoots. |
| src/Parse/Assetic/Filter/JavascriptImporter.php | Enforces .js-only inlining and confines resolved paths to allowed roots. |
| src/Parse/Assetic/Filter/HasAllowedImportRoots.php | Introduces shared trait for storing/configuring additional allowed import roots. |
| src/Filesystem/PathResolver.php | Adds withinAny() helper to check containment against multiple roots. |
| composer.json | Updates assetic/framework requirement to ^3.2.1. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the local file disclosure reported in GHSA-2223-f22x-24cq. The JavaScript combiner
=include/=requiredirectives (Winter\Storm\Parse\Assetic\Filter\JavascriptImporter) resolved their target withrealpath()and no confinement check, so a.jsasset containing=include ../../../.envinlined arbitrary server-readable files into combined output thatSystem\Classes\SystemController@combineserves unauthenticated (APP_KEY, DB credentials, etc.).This is the JavaScript sibling of the LESS import flaw hardened for GHSA-58fp-mcx6-7qf9; that fix gated the LESS filter but left
JavascriptImporter(and the CSS importer) exploitable.Changes
JavascriptImporternow enforces two layers indirectiveInclude():.js-only extension gate — rejects.env/.php/.logetc. before any path math. Extension-less includes are unaffected (still resolved to.jsfirst), matching the CSS importer which only inlines.css.=requireinto a disallowed path now throws (consistent with the not-found and disallowed-extension guards); the optional=includestill degrades to a comment.PathResolver::withinAny()(new) — the shared confinement primitive: returns true if a path lies within any of a list of roots, skipping null/empty entries so callers can pass an optional context dir alongside a roots list without pre-filtering. Used by both the JS importer andLessImportResolver.HasAllowedImportRoots(new trait) — shared allowed-roots storage/setter forJavascriptImporterandLessCompiler.LessImportResolver/LessCompilerrefactored onto the shared primitive + trait (no behaviour change).Callers configure the roots;
System\Classes\CombineAssetssupplies themes/plugins/modules.Tests
New
JavascriptImporterTestcovering traversal blocking, the extension gate, allowed-root includes, legitimate same-tree includes, and both=requirefailure modes (disallowed extension and out-of-root path). NewPathResolverTest::testWithinAny(). Full filter suite: 25/25 pass (9 new + 16 existing LESS).Known follow-up — SCSS
ScssCompiler(scssphp) is not covered by this change and remains the last import-capable combiner filter without confinement (CombineAssetswires allowed roots into JS, CSS and LESS but registersScssCompilerbare). Impact is narrower than JS/LESS — scssphp only imports.scss/.sass/.csstargets and the content must parse as Sass, so.env/keys can't be disclosed — but a.scssfile outside the asset tree can still be inlined via@import '../../../x.scss'. scssphp resolves imports internally with no post-resolution hook (source-dir- and importPath-relative traversal is resolved before any callable importer runs), so a correct fix requires subclassing scssphp'sCompiler(overridefindImport) and swapping it into the filter'sfilterLoad— an invasive change to a shared filter that warrants its own PR + tests. Tracked separately.Related
wip/ghsa-2223-js-importer-lfi(wires the allowed roots intoCombineAssets+ adds the CSS validator)wip/ghsa-2223-css-import-validator(opt-inCssImportFiltervalidator)🤖 Generated with Claude Code