Skip to content

bug: hardened-YAML guard's venv exclusion is off by one — a tree-root virtualenv yields false positives #2890

Description

@vybe

Summary

tests/unit/test_ent314_hardened_yaml.py::test_no_service_parses_yaml_without_the_shared_loader tries to exclude virtualenvs from its AST scan, but the exclusion never matches a virtualenv sitting at the root of a scanned tree — which is where virtualenvs actually live. Any contributor with src/backend/venv/ gets a false failure naming vendored site-packages files as unguarded yaml.safe_load calls.

Context

At tests/unit/test_ent314_hardened_yaml.py:301-302:

rel = str(path.relative_to(root))
if rel in exempt or "/venv/" in rel or rel.startswith("migrations/"):

rel is relative to root (here src/backend), so a file at src/backend/venv/lib/python3.x/site-packages/starlette/<module>.py yields rel = "venv/lib/..." — with no leading slash. "/venv/" in rel is therefore False and the vendored file gets scanned.

The exclusion only works for a virtualenv nested at least one directory deep (e.g. services/venv/...). The intent is unambiguous; it just misses the common case.

Observed as two false offenders from vendored starlette and uvicorn sources. The directory is already gitignored (.gitignore:19, venv/), so the scan is walking ignored files.

This matters beyond the noise: a guard that cries wolf on a clean checkout trains people to dismiss it, and this one backs a security invariant (Invariant #5, the hardened YAML loader).

Acceptance Criteria

  • A virtualenv at the root of a scanned tree (e.g. src/backend/venv/) is excluded from the scan
  • The guard still fails on a genuine unguarded yaml.safe_load in first-party code (prove with a temporary fixture)
  • Exclusion is path-segment based rather than substring based, so it cannot be defeated by position
  • The same treatment covers other vendored/ignored directories the scan may encounter (.venv, site-packages, node_modules)

Technical Notes

Prefer matching on path parts over substrings:

parts = path.relative_to(root).parts
if {"venv", ".venv", "site-packages", "node_modules"} & set(parts):
    continue

Deriving the candidate file list from git ls-files instead of rglob would sidestep this entire class — the scan would then only ever see tracked, first-party files, which is what it actually means to check.

Worth auditing sibling AST guards that walk the tree with rglob for the same substring-exclusion pattern.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions