fix(engine): contain a relative sub-workflow path before it is stat'ed - #105
Merged
Merged
Conversation
The dw/path-injection scan flagged one stat in resolve_sub_workflow: `os.path.isfile(candidate)` ran on `normpath(join(base_dir, name))` with nothing checking that the result was inside anything. Every caller does refuse the path afterwards - create_step_action validates the resolved path against its confinement, and the run's own error for an escaping reference is unchanged - so no caller read it. But resolution is not the enforcement point, and that left a real hole one frame up: realize.read_sub_workflow is handed the run's workflow_dir, which is None for a bare CLI run, and validate_workflow_path(path, None) normalizes without confining. An unconfined run therefore *read* a sub-workflow outside its catalog (the run then refused it, and realization digested it first). Containment now happens before the stat, against the root the candidate would be handed back with: the caller's confinement when it named one, else the catalog root the run itself would confine to. That second rule moved to workflow_sources.catalog_root so the resolver and workflow.catalog_root_dir cannot drift apart - the latter now delegates. Two details the surrounding design already insisted on, kept: - normpath before the validator, because validate_path refuses a '..' outright - so a climb that stays inside the root is collapsed first and '../models/x.json', the form every template uses, still resolves. - a climb out is a PathTraversalError, not a SubWorkflowNotFound: a refusal, not a name that was absent, which is the distinction the not-found message is careful about. Tests: the resolver refuses a climb out (confined and unconfined) and still resolves a climb to a sibling catalog folder; the unconfined read now answers None rather than the file's bytes, while the sibling climb stays readable. Co-Authored-By: Claude <noreply@anthropic.com>
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.
Fixes the one real code-scanning finding of the three open
dw/path-injectionalerts.The finding
dw/workflow_sources.py:251—os.path.isfile(candidate)onnormpath(join(base_dir, name)), with nothing checking that the result stayed inside anything.No caller read that path:
create_step_actionvalidates the resolved path against its confinement afterwards, and a run's error for an escaping reference is unchanged. But resolution was not the enforcement point, and the stat was one frame short of one:realize.read_sub_workflowis handed the run'sworkflow_dir, which isNonefor a bare CLI run — andvalidate_workflow_path(path, None)normalizes without confining. So an unconfined run read a sub-workflow from outside its catalog (the run then refused it; realization digested it first):The fix
Containment before the stat, against the root the candidate would be handed back with: the caller's confinement when it named one, else the catalog root the run itself would confine to. That second rule moved to
workflow_sources.catalog_rootso the resolver andworkflow.catalog_root_dircannot drift apart — the latter now delegates to it.Two things the surrounding design already insisted on, kept:
normpathbefore the validator.validate_pathrefuses a..outright, so a climb that stays inside the root is collapsed first —"../models/x.json", the form every template uses (workflows/templates/describe-and-regenerate.json), still resolves.PathTraversalError, not aSubWorkflowNotFound— a refusal, not a name that was absent.tests/test_workflow.pyasserts that distinction for the not-found message, and an earlier draft of this fix that reported the climb as absent broke it.The validator's return value is what gets stat'ed, which is also the shape
dw-path-injectionmodels as sanitized — the same "guardrail the scanner could not see" pattern the pack in.github/codeql/dw-security/exists for, here with the guard moved where the query can see it.Tests
Six new, and two of them were watched failing first:
Nonerather than the file's bytes; the sibling climb stays readablepytest tests/test_workflow_sources.py tests/test_realize.py tests/test_workflow.py tests/test_security.py tests/test_catalog_structure.py tests/test_template_subfolders.py tests/test_runs.py tests/test_validate_arguments.py tests/test_library_sources.py— 967 passed.Verification of the alert itself
The CodeQL CLI is not installed locally, so the scan this PR triggers (
codeql.ymlruns onpull_request) is the evidence that the finding clears, rather than my say-so.Not in this PR
The other two alerts are false positives and want dismissing, not code changes:
dw/workflow_sources.py:241—source.contains(candidate) and os.path.isfile(candidate).WorkflowSource.containsisvalidate_path(candidate, self.root)in atry/exceptreturning bool, andandshort-circuits, so the stat only runs when contained. The pack sanitizes a validator's return value, andcontainsconsumes that value as a boolean — so the sanitizer never attaches tocandidate. Fixable in the pack by modellingcontainsas aBarrierGuard, if it is worth it rather than dismissing.dw/workflow.py:457—os.path.isfile(resolved)whereresolved = join(builtin_root(), builtin_name)and the guard above rejects any/or\, so the name is a single path segment. Every traversal-shaped name is refused before the stat (probed). It is an inline check rather than a validator call, so the pack has nothing to hook;safe_join_pathwould make it legible if we would rather not dismiss it.