Code scanning: model dw/security.py's validators, fix the two real findings

.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

.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"

.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(_)
+    )
+  }
+}

.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

.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: "*"

.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 }}"

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)

dw/server/app.py

@@ -345,9 +345,19 @@
     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 @@
             )
         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:

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):