Harden script_reader against vulnerabilities - #546
Open
urfeex wants to merge 1 commit into
Open
Conversation
1. Path traversal (.. and absolute-path escape) — `replaceIncludes()` now
rejects absolute include paths outright (avoiding the `path::operator/`
LHS-drop trap) and requires every include's
`weakly_canonical`-normalized target to be lexically inside `root_dir_`
(the directory of the top-level script).
2. Unbounded / circular includes → stack overflow — added `include_stack_`
(set of canonical paths currently being processed) for cycle
detection and `include_depth_` bounded by a fixed threshold of 32. Both
self-references and A→B→A cycles now throw `UrException`, as do chains
beyond the depth cap.
3. `script_path_` state corruption from nested includes — removed the
mutable `script_path_` member. The public `readScriptFile()` is now a thin
wrapper that initializes state, and a new private `readScriptFileImpl()`
save-restores `current_dir_` (and cleans up
`include_stack_`/`include_depth_`) around every recursive include, even
on exceptions.
4. Malformed include regex ['|"] — changed to ['"]. {% include
|target.txt| %} is no longer treated as a valid include.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #546 +/- ##
==========================================
- Coverage 79.17% 79.06% -0.12%
==========================================
Files 115 115
Lines 6801 6856 +55
Branches 3000 3037 +37
==========================================
+ Hits 5385 5421 +36
- Misses 1047 1063 +16
- Partials 369 372 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
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
1 participant
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.
This doesn't change anything in behavior, it merely enforces the behavior as it is described in the docs.
Path traversal (.. and absolute-path escape) —
replaceIncludes()now rejects absolute include paths outright (avoiding thepath::operator/LHS-drop trap) and requires every include'sweakly_canonical-normalized target to be lexically insideroot_dir_(the directory of the top-level script).Unbounded / circular includes → stack overflow — added
include_stack_(set of canonical paths currently being processed) for cycle detection andinclude_depth_bounded by a fixed threshold of 32. Both self-references and A→B→A cycles now throwUrException, as do chains beyond the depth cap.script_path_state corruption from nested includes — removed the mutablescript_path_member. The publicreadScriptFile()is now a thin wrapper that initializes state, and a new privatereadScriptFileImpl()save-restorescurrent_dir_(and cleans upinclude_stack_/include_depth_) around every recursive include, even on exceptions.Malformed include regex ['|"] — changed to ['"]. {% include |target.txt| %} is no longer treated as a valid include.
Note
High Risk
Changes file-read boundaries and include recursion in script loading—a security-sensitive path where prior behavior allowed directory escape and unbounded recursion; legitimate deep include chains beyond 32 levels will now fail.
Overview
Hardens
ScriptReaderinclude processing so nested{% include %}matches documented behavior: includes stay under the top-level script directory, cannot recurse indefinitely, and sibling includes keep resolving from the parent file’s directory.readScriptFile()now canonicalizes the entry path, setsroot_dir_from that file’s parent, and delegates to privatereadScriptFileImpl(). Includes resolve viacurrent_dir_(save/restored per file), reject absolute or rooted paths (including Windows edge cases), normalize withweakly_canonical, and throw if the target is outsideroot_dir_. Cycle detection (include_stack_) and a depth cap (MAX_INCLUDE_DEPTH= 32) replace the old pattern of re-enteringreadScriptFile()through mutablescript_path_.The include directive regex no longer treats
|as a quote character (['"]instead of['|"]). Security regression tests cover traversal, absolute paths, cycles, depth, sibling resolution, pipe quotes, and Windows-only rooted paths.Reviewed by Cursor Bugbot for commit 7ab796b. Bugbot is set up for automated code reviews on this repo. Configure here.