From 56374ed51110d112b3d6c4e0147ed8e3a2b4a6e1 Mon Sep 17 00:00:00 2001 From: Don Kackman Date: Tue, 8 Sep 2026 21:57:14 -0500 Subject: [PATCH] Code scanning: model dw/security.py's validators, fix the two real findings The 27 open code scanning alerts were 26 copies of one false positive and one real finding. Every flagged filesystem access already reached the disk through validate_path, which resolves with realpath and then raises unless the result is contained - the normalize-then-check shape py/path-injection looks for. But that query recognizes the check only as a *local* barrier guard, so a validator in another module that returns the safe path instead of guarding a branch is invisible to it, and every route touching a file gets flagged. Dismissing the alerts would not stop the next one. So teach the query about the validators instead, in a local query pack that models them as sanitizers, and filter out the built-in query it replaces. Loading a pack needs advanced setup, so scanning moves from GitHub's default setup to .github/workflows/codeql.yml - same three languages, same weekly schedule, so nothing that was scanned stops being scanned. This is not a blanket suppression: a path validated without a base directory is modeled as normalization only and stays reportable, and code that reaches the disk without a validator still flags at high severity. That signal is what the 26-alert wall was hiding. The two genuine findings are fixed rather than modeled: - resolve_workflow_reference called os.path.isfile on the submitted path before any containment check, which let a workflow_path probe for files anywhere on disk. The containment now comes first, and is re-applied to the path that is returned rather than trusted from source_for_path's answer about it. - /api/validate returned the string of an unexpected validator exception to the client. It now logs the detail and reports the category, as the handler above it already did. The test that pinned the old behaviour now pins the new contract: generic message out, "boom" only in the log. Verified with the CodeQL CLI against a database built from this tree: the modeled query reports 0 results where the built-in one reports 39, and py/stack-trace-exposure goes from 1 to 0. Full suite 3293 passed. Co-Authored-By: Claude Opus 5 --- .github/codeql/codeql-config.yml | 14 ++ .github/codeql/dw-security/DwPathInjection.ql | 31 ++++ .../codeql/dw-security/DwPathSanitizers.qll | 139 ++++++++++++++++++ .../codeql/dw-security/codeql-pack.lock.yml | 30 ++++ .github/codeql/dw-security/qlpack.yml | 8 + .github/workflows/codeql.yml | 59 ++++++++ CLAUDE.md | 13 ++ dw/server/app.py | 29 +++- tests/test_server.py | 14 +- 9 files changed, 329 insertions(+), 8 deletions(-) create mode 100644 .github/codeql/codeql-config.yml create mode 100644 .github/codeql/dw-security/DwPathInjection.ql create mode 100644 .github/codeql/dw-security/DwPathSanitizers.qll create mode 100644 .github/codeql/dw-security/codeql-pack.lock.yml create mode 100644 .github/codeql/dw-security/qlpack.yml create mode 100644 .github/workflows/codeql.yml diff --git a/.github/codeql/codeql-config.yml b/.github/codeql/codeql-config.yml new file mode 100644 index 00000000..73ac878f --- /dev/null +++ b/.github/codeql/codeql-config.yml @@ -0,0 +1,14 @@ +# Python analysis only - see the matrix in .github/workflows/codeql.yml. +# The other languages run with the default configuration. +name: "diffusers-workflow (python)" + +# The default suite, plus the local pack that models dw/security.py's +# validators. Both queries are the same analysis; the built-in one is +# excluded below so a sanitized path is not reported by it after the +# modeled query has cleared it. +queries: + - uses: ./.github/codeql/dw-security + +query-filters: + - exclude: + id: py/path-injection diff --git a/.github/codeql/dw-security/DwPathInjection.ql b/.github/codeql/dw-security/DwPathInjection.ql new file mode 100644 index 00000000..5a157a31 --- /dev/null +++ b/.github/codeql/dw-security/DwPathInjection.ql @@ -0,0 +1,31 @@ +/** + * @name Uncontrolled data used in path expression + * @description Accessing paths influenced by users can allow an attacker to + * access unexpected resources. + * @kind path-problem + * @problem.severity error + * @security-severity 7.5 + * @sub-severity high + * @precision high + * @id dw/path-injection + * @tags correctness + * security + * external/cwe/cwe-022 + * external/cwe/cwe-023 + * external/cwe/cwe-036 + * external/cwe/cwe-073 + * external/cwe/cwe-099 + */ + +// The standard py/path-injection query, with dw/security.py's validators +// modeled as sanitizers. The built-in one is filtered out in +// .github/codeql/codeql-config.yml so the two do not both report. +import python +import semmle.python.security.dataflow.PathInjectionQuery +import DwPathSanitizers +import PathInjectionFlow::PathGraph + +from PathInjectionFlow::PathNode source, PathInjectionFlow::PathNode sink +where PathInjectionFlow::flowPath(source, sink) +select sink.getNode(), source, sink, "This path depends on a $@.", source.getNode(), + "user-provided value" diff --git a/.github/codeql/dw-security/DwPathSanitizers.qll b/.github/codeql/dw-security/DwPathSanitizers.qll new file mode 100644 index 00000000..c15df645 --- /dev/null +++ b/.github/codeql/dw-security/DwPathSanitizers.qll @@ -0,0 +1,139 @@ +/** + * Models `dw/security.py`'s validators for the path-injection query. + * + * Every filesystem access in this project reaches the disk through + * `validate_path` (or a thin wrapper around it), which resolves the path + * with `os.path.realpath` and then raises unless the result is the base + * directory or a descendant of it; a name that is joined onto a library + * root goes through one of the reference validators, a full-match regex + * whose first character class precludes `..`, a leading separator and a + * null byte. That is exactly the normalize-then-check shape the standard + * `py/path-injection` query looks for - but it only recognizes the check as + * a *local* barrier guard, so a validator living in another module, + * returning the safe value rather than guarding a branch, is invisible to + * it. Without this file the query flags every route that touches a file, + * which buries the ones that skipped a validator. + * + * The trade this makes is that `dw/security.py` is now trusted by the + * query rather than checked by it: a bug in a validator would not be + * reported here. That file is the security layer and is reviewed as such - + * see tests/test_security.py, which is where a validator's containment is + * actually established. + * + * The model deliberately distinguishes the two ways `validate_path` is + * called: + * + * - with a base directory, it is a containment check and a full barrier + * - with `None` (or nothing) for the base, it only normalizes, so it is + * modeled as a `PathNormalization` and the path stays reportable until + * something checks it + * + * so a call that forgets the base directory is still a finding. + */ + +private import python +private import semmle.python.Concepts +private import semmle.python.dataflow.new.DataFlow +private import semmle.python.security.dataflow.PathInjectionCustomizations + +/** + * Holds if `name` is a `dw.security` function that returns a path confined + * to a base directory it was given. + */ +private predicate pathValidatorName(string name) { + name = + [ + "validate_path", "validate_workflow_path", "validate_output_path", + "validate_prompt_path", "safe_join_path" + ] +} + +/** + * Holds if `name` is a `dw.security` function that returns a name it has + * matched against a pattern admitting no traversal, for joining onto a + * library root. + */ +private predicate nameValidatorName(string name) { + name = + [ + "validate_workspace_name", "validate_prompt_reference", "validate_asset_reference", + "validate_output_reference", "validate_variable_name", "validate_commit_hash" + ] +} + +/** + * Gets the name of the function `call` invokes, for the flat and the + * qualified spelling. Matched by name rather than by resolved definition + * because the package imports these relatively (`from .security import + * validate_path`), which API graphs do not track, and no other function in + * the project carries one of these names. + */ +private string calledName(DataFlow::CallCfgNode call) { + result = call.getFunction().asExpr().(Name).getId() or + result = call.getFunction().asExpr().(Attribute).getName() +} + +/** A call to one of the validators. */ +private class ValidatorCall extends DataFlow::CallCfgNode { + ValidatorCall() { + pathValidatorName(calledName(this)) or + nameValidatorName(calledName(this)) + } + + /** Gets the value this call validates. */ + DataFlow::Node getPathArg() { result = this.getArg(0) } + + /** + * Holds if this call returns a value that cannot leave the directory it + * will be resolved against. A name validator always does. A path + * validator does when it was given a base directory to confine the path + * to - `safe_join_path` joins its parts under the first one, so it + * always confines; the others take the base as their second argument, + * and passing `None` there asks for normalization only. + */ + predicate confines() { + nameValidatorName(calledName(this)) + or + calledName(this) = "safe_join_path" + or + exists(DataFlow::Node base | + base = + [ + this.getArg(1), + this.getArgByName(["base_dir", "workflow_dir", "prompt_dir", "output_dir"]) + ] + | + not base.asExpr() instanceof None + ) + } +} + +/** A value that has been validated and confined. */ +private class ConfinedValue extends PathInjection::Sanitizer { + ConfinedValue() { this.(ValidatorCall).confines() } +} + +/** A path validator call given no base directory: normalization only. */ +private class UnconfinedPath extends Path::PathNormalization::Range { + UnconfinedPath() { this instanceof ValidatorCall and not this.(ValidatorCall).confines() } + + override DataFlow::Node getPathArg() { result = this.(ValidatorCall).getPathArg() } +} + +/** + * A parameter of a validator, so the query does not report the validator's + * own `os.path.realpath` / `os.path.exists` of the value it is in the + * middle of checking. This is the "trusted rather than checked" half of + * the trade described at the top of this file, and it is scoped to the + * definitions in `dw/security.py`. + */ +private class ValidatorParameter extends PathInjection::Sanitizer { + ValidatorParameter() { + exists(Function validator | + pathValidatorName(validator.getName()) or nameValidatorName(validator.getName()) + | + validator.getLocation().getFile().getRelativePath() = "dw/security.py" and + this.asExpr() = validator.getArg(_) + ) + } +} diff --git a/.github/codeql/dw-security/codeql-pack.lock.yml b/.github/codeql/dw-security/codeql-pack.lock.yml new file mode 100644 index 00000000..b31e5328 --- /dev/null +++ b/.github/codeql/dw-security/codeql-pack.lock.yml @@ -0,0 +1,30 @@ +--- +lockVersion: 1.0.0 +dependencies: + codeql/concepts: + version: 0.0.30 + codeql/controlflow: + version: 2.0.40 + codeql/dataflow: + version: 2.1.12 + codeql/mad: + version: 1.0.56 + codeql/python-all: + version: 7.2.4 + codeql/regex: + version: 1.0.56 + codeql/ssa: + version: 2.0.32 + codeql/threat-models: + version: 1.0.56 + codeql/tutorial: + version: 1.0.56 + codeql/typetracking: + version: 2.0.40 + codeql/util: + version: 2.0.43 + codeql/xml: + version: 1.0.56 + codeql/yaml: + version: 1.0.56 +compiled: false diff --git a/.github/codeql/dw-security/qlpack.yml b/.github/codeql/dw-security/qlpack.yml new file mode 100644 index 00000000..1ad4837a --- /dev/null +++ b/.github/codeql/dw-security/qlpack.yml @@ -0,0 +1,8 @@ +# A one-query pack whose only job is to teach CodeQL about the path +# validators in dw/security.py. See DwPathSanitizers.qll for why the +# built-in py/path-injection cannot recognize them on its own. +name: dw/security-queries +version: 0.0.1 +library: false +dependencies: + codeql/python-all: "*" diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml new file mode 100644 index 00000000..59c7afb9 --- /dev/null +++ b/.github/workflows/codeql.yml @@ -0,0 +1,59 @@ +# Advanced setup, replacing the repository's CodeQL default setup. The +# reason it is a workflow rather than default setup is the local query pack +# in .github/codeql/dw-security, which teaches the path-injection query +# about dw/security.py's validators - default setup cannot load a pack. +# +# The language list and the weekly schedule match what default setup ran, +# so nothing that was being scanned stopped being scanned. +name: CodeQL + +on: + push: + branches: [master] + pull_request: + schedule: + # Weekly, as default setup ran it + - cron: '23 4 * * 1' + workflow_dispatch: + +permissions: + contents: read + +jobs: + analyze: + name: Analyze (${{ matrix.language }}) + runs-on: ubuntu-latest + timeout-minutes: 60 + permissions: + security-events: write + # Both needed by the action to read workflow runs for the actions analysis + actions: read + contents: read + strategy: + fail-fast: false + matrix: + include: + - language: actions + build-mode: none + - language: javascript-typescript + build-mode: none + # Only python gets a config file, because the pack it adds is a + # python pack and would not compile against another language's + # database + - language: python + build-mode: none + config-file: .github/codeql/codeql-config.yml + steps: + - uses: actions/checkout@v7 + + - name: Initialize CodeQL + uses: github/codeql-action/init@v4 + with: + languages: ${{ matrix.language }} + build-mode: ${{ matrix.build-mode }} + config-file: ${{ matrix.config-file }} + + - name: Analyze + uses: github/codeql-action/analyze@v4 + with: + category: "/language:${{ matrix.language }}" diff --git a/CLAUDE.md b/CLAUDE.md index 1c79c906..c66480fb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -191,6 +191,19 @@ All entry points use `dw/security.py`. When adding features: - **Never** use `eval()`, `exec()`, or `shell=True` - Path traversal (`../`) is blocked +CodeQL knows about these validators, which is why the scan is quiet: the local +query pack in `.github/codeql/dw-security/` models them as sanitizers for +`py/path-injection`, because the built-in query recognizes a +normalize-then-check only as a local barrier guard and so cannot see one that +lives in another module and returns the safe value. This is what makes code +scanning useful here rather than 26 identical false positives - but it only +holds while new filesystem access goes through a validator. Reaching the disk +some other way is a real alert, so treat one as a finding rather than as more +of the old noise. `validate_path(path, base)` is modeled as a barrier only when +`base` is not `None`; with `None` it is normalization only, and the path stays +reportable. Scanning is advanced setup (`.github/workflows/codeql.yml`) for the +same reason - default setup cannot load a pack. + ## Critical Gotchas - **Schema validation runs before variable substitution** — variable defaults must match expected JSON types (use `25` not `"25"` for numbers) diff --git a/dw/server/app.py b/dw/server/app.py index 89f45c7e..80856947 100644 --- a/dw/server/app.py +++ b/dw/server/app.py @@ -345,9 +345,19 @@ def resolve_workflow_reference(workflow_path, sources): if path is not None: return path, source candidate = os.path.abspath(workflow_path) - source = source_for_path(sources, candidate) if os.path.isfile(candidate) else None + source = source_for_path(sources, candidate) if source is not None: - return candidate, source + # The containment check re-applied to the path this returns, rather + # than trusted from source_for_path's answer about it - and applied + # before anything asks the filesystem about the path, so a + # workflow_path outside every source cannot be used to find out + # whether a file exists there + try: + confined = validate_path(candidate, source.root, allow_create=False) + except SecurityError: + confined = None + if confined is not None and os.path.isfile(confined): + return confined, source raise HTTPException( status_code=400, detail=f"workflow_path must name a workflow the server can reach: " @@ -1139,11 +1149,20 @@ def validate_workflow( ) try: errors = candidate.validation_errors() - except Exception as e: + except Exception: + # An error here is not the schema's verdict on the workflow - + # validation_errors() reports that by returning it. It is the + # validator itself failing, and its message could carry + # internals, so the log keeps the detail and the client is told + # the category, as above + logger.exception("Workflow could not be validated") + detail = ( + "The workflow could not be validated - the server log has the detail" + ) return { "valid": False, - "error": f"Validation error: {e}", - "errors": [{"path": None, "message": str(e)}], + "error": detail, + "errors": [{"path": None, "message": detail}], "warnings": [], } if errors: diff --git a/tests/test_server.py b/tests/test_server.py index 919bfd3e..5014aba0 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -3,6 +3,7 @@ import asyncio import json +import logging import os import queue import time @@ -552,7 +553,10 @@ def test_validate_endpoint_lists_every_schema_error(server): assert result["valid"] is True and result["errors"] == [] -def test_validate_endpoint_reports_non_schema_exception(server, monkeypatch): +def test_validate_endpoint_reports_non_schema_exception(server, monkeypatch, caplog): + """A validator that fails, rather than reporting a verdict, is still an + invalid answer to the client - but the exception's own message stays in + the log, since it is internal detail and not the schema's complaint.""" import dw.workflow def raise_boom(self): @@ -561,12 +565,16 @@ def raise_boom(self): monkeypatch.setattr(dw.workflow.Workflow, "validation_errors", raise_boom) with server(success_script) as client: - result = client.post("/api/validate", json={"workflow": valid_workflow()}) + with caplog.at_level(logging.ERROR): + result = client.post("/api/validate", json={"workflow": valid_workflow()}) assert result.status_code == 200 body = result.json() assert body["valid"] is False - assert "boom" in body["error"] + assert "could not be validated" in body["error"] + assert body["errors"] == [{"path": None, "message": body["error"]}] + assert "boom" not in json.dumps(body) + assert "boom" in caplog.text def test_workflow_browsing_and_confinement(server):