Code scanning: model dw/security.py's validators, fix the two real findings - #58
Merged
Conversation
…ndings 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 <noreply@anthropic.com>
| confined = validate_path(candidate, source.root, allow_create=False) | ||
| except SecurityError: | ||
| confined = None | ||
| if confined is not None and os.path.isfile(confined): |
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.
The 27 open code scanning alerts were 26 copies of one false positive and one real finding.
Why there were 26
Every flagged filesystem access already reaches the disk through
validate_path, which resolves withos.path.realpathand then raises unless the result is the base directory or a descendant of it — exactly the normalize-then-check shapepy/path-injectionlooks for. But that query recognizes the check only as a local barrier guard. A validator that lives in another module and returns the safe path rather than guarding a branch is invisible to it, so every route touching a file gets flagged. Dismissing the alerts one by one would not stop the next one.What this does
Teaches the query about the validators, in a local query pack (
.github/codeql/dw-security/) that models them as sanitizers, with the built-in query it replaces filtered out. Loading a pack needs advanced setup, so scanning moves off GitHub's default setup to.github/workflows/codeql.yml— same three languages (actions,javascript-typescript,python), same weekly schedule.It is deliberately not a blanket suppression:
validate_path(path, base)is a barrier only whenbaseis notNone; withNoneit is modeled asPathNormalization, so the path stays reportable until something checks it.CLAUDE.mdnow says so.The trade is that
dw/security.pybecomes trusted by the query rather than checked by it — documented at the top ofDwPathSanitizers.qll, where it points attests/test_security.pyas the place containment is actually established.The two genuine findings, fixed rather than modeled
resolve_workflow_referencecalledos.path.isfileon the submitted path before any containment check, so aworkflow_pathcould probe for files anywhere on disk. Containment now comes first, and is re-applied to the path that is returned rather than trusted fromsource_for_path's answer about it./api/validatereturnedstr(e)of an unexpected validator exception to the client (py/stack-trace-exposure). It now logs the detail and reports the category, as the handler 15 lines above it already did. The test that pinned the old behaviour now pins the new contract: generic message out,"boom"only in the log.Verification
Run locally with the CodeQL CLI 2.26.4 against a database built from this tree:
py/path-injectiondw/path-injectionpy/stack-trace-exposure: 1 before → 0 after. Full test suite: 3293 passed, 5 skipped.black --checkclean.Caveat worth knowing
Advanced setup means we own the workflow file — GitHub's automatic query-suite and CLI updates stop being invisible, and
github/codeql-actionneeds Dependabot to stay current. That is the cost of custom queries; there is no way to load a pack under default setup.Default setup is already disabled
It had to be: GitHub rejects SARIF from an advanced configuration while default setup is enabled (
Code Scanning could not process the submitted SARIF file: CodeQL analyses from advanced configurations cannot be processed when the default setup is enabled), which is what the first red checks on this PR were. Turned off viaPATCH /code-scanning/default-setup; re-enabling is one API call if this is ever reverted.With it off, all three languages upload cleanly, and the python job's log shows the pack doing its job in CI, not just locally:
After merge: the 27 existing alerts close as fixed once master is analyzed.
🤖 Generated with Claude Code