Skip to content

Confine asset compiler imports consistently across LESS and SCSS - #245

Merged
LukeTowers merged 5 commits into
developfrom
fix/asset-import-confinement
Sep 26, 2026
Merged

LukeTowers merged 5 commits into
developfrom
fix/asset-import-confinement

Conversation

@LukeTowers

@LukeTowers LukeTowers commented Sep 14, 2026 •

Copy link
Copy Markdown
Member

Two gaps in how @import resolution is bounded to the asset tree.

Requires assetic/framework v3.2.3 (assetic-php/assetic#53), released 2026-09-15 — composer.json is bumped accordingly.

LESS — confinement didn't follow the import graph

LessImportResolver bounds resolution by colliding with the path-form import dir less.php auto-adds for the current file's directory. That entry is re-created for every file less.php parses, keyed by that file's own directory — and it's also consulted by data-uri() / image-size(). We only claimed the entry asset's directory, so anything imported from another directory resolved unconfined.

makeResolver() now registers a resolver for each directory it admits, so the collision follows the import graph. The key-normalisation moves into importDirKey() so registration and the initial build share one implementation — worth keeping in one place, since getting it wrong doesn't fail loudly, it just silently stops colliding (and differs on Windows).

SCSS — never wired up at all

LESS, CSS and JavaScript all got the allowed-root policy; ScssCompiler never did. scssphp resolves @import against the configured import paths and the importing file's own directory, both with .. traversal allowed.

ScssCompiler now uses HasAllowedImportRoots and installs a validator through the new assetic hook. Import paths configured on the filter are mirrored into the allowed roots so legitimate cross-tree imports still resolve — the parent stores them privately, hence the local mirror.

Tests

+9, all verified in both directions by reverting each fix and re-running:

LessCompilerTest — testBlocksTraversalFromAnImportedSubdirectoryFile and testBlocksDataUriFileReadFromAnImportedSubdirectoryFile both fail without the resolver change. testAllowsNestedPartialChain guards the legitimate case.

ScssCompilerTest (new, mirrors LessCompilerTest) — the three confinement tests fail without the compiler change; testAllowsLegitimateSameTreePartial, testAllowsNestedPartialChain and testAllowsCrossTreeImportWhenRootIsWhitelisted guard normal use.

Full suite: 837 tests, 3324 assertions, 0 failures (828 before). phpcs clean — includes removing one stray blank line in LessCompilerTest that pre-dates this branch but would now be linted as a changed file.

Summary by CodeRabbit

  • Bug Fixes

    • Restricted LESS and SCSS imports to the asset’s directory and explicitly allowed locations.
    • Blocked path traversal, absolute-path imports, and unintended file reads from nested imported files.
    • Preserved valid same-tree, nested, and explicitly approved cross-directory imports, including imports from the entry asset’s directory.
  • Security

    • Improved protection against unintended access to files outside configured import locations.
  • Tests

    • Added coverage for blocked traversal scenarios and supported nested import workflows.

LessImportResolver confines @import resolution by colliding with the
path-form import dir less.php auto-adds for the current file's directory.
That entry is re-created for every file less.php parses, keyed by that
file's own directory, and it is also consulted by data-uri() and
image-size(). Claiming only the entry asset's directory therefore left
anything imported from another directory resolving unconfined.

makeResolver() now registers a resolver for each directory it admits, so
the collision follows the import graph. The key-normalisation logic moves
into importDirKey() so registration and the initial build share it.
The allowed-root policy applied to the LESS, CSS and JavaScript compilers
was never wired into ScssCompiler, so scssphp resolved @import against the
importing file's own directory with `..` traversal allowed and resolution
was not bounded to the asset tree.

ScssCompiler now uses HasAllowedImportRoots and installs a validator via
ScssphpFilter::setImportValidator(), which requires assetic/framework
^3.2.3. Import paths configured on the filter are mirrored so they count as
allowed roots, preserving legitimate cross-tree imports.

Adds ScssCompilerTest, mirroring the existing LessCompilerTest coverage.
@coderabbitai

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

SCSS string literals treat a backslash as an escape, so embedding a raw
Windows path in the @import statement mangled it -- scssphp never saw a
resolvable target and raised a compile error instead of the import being
refused by the validator. The path is now written with forward slashes.

A refused import also differs by platform: scssphp either emits the
statement verbatim or raises a compile error. Both are refusals, so the
test accepts either and asserts only that the file is not inlined.

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

LESS: the resolver registered for an admitted file's directory kept only the configured roots and dropped the importing resolver's own context directory. A file imported from a subdirectory could therefore no longer import back up into the entry asset's tree (e.g. sub/child.less importing ../variables.less), which the base branch allowed. The context directory is now carried forward.

SCSS: ScssphpFilter::getChildren() recurses through the override for each child, and each nested call reset the allowed root to the child's directory without restoring it. Imports following a subdirectory import were then validated against the wrong root and dropped from dependency extraction and hashAsset(). Only the outermost call now sets the root, matching filterLoad().

Also drops the redundant setAllowedImportRoots() call from the SCSS cross-tree test so it actually covers the configured-import-path mirroring.
@LukeTowers

Copy link
Copy Markdown
Member Author

Re the CodeRabbit nitpick on ScssCompilerTest fixtures: not applying it. PathResolver::standardize() converts to backslashes on Windows, which is exactly what that line avoids (backslashes are escapes in SCSS string literals). file_put_contents() matches LessCompilerTest, and atomic writes add nothing for test fixtures.

@LukeTowers
LukeTowers merged commit e97198d into develop Sep 26, 2026
12 of 13 checks passed
@LukeTowers
LukeTowers deleted the fix/asset-import-confinement branch September 26, 2026 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants