Skip to content

fix(audit): ignore local virtualenv directories - #153

Merged
L4XB merged 1 commit into
SixSentences:mainfrom
hizlidepoo:oss/l2/145-ignore-local-venvs
Sep 15, 2026
Merged

L4XB merged 1 commit into
SixSentences:mainfrom
hizlidepoo:oss/l2/145-ignore-local-venvs

Conversation

@hizlidepoo

Copy link
Copy Markdown
Contributor

Fixes #145.

Summary

  • ignore both .venv and venv when walking the community export tree
  • preserve symlink rejection everywhere outside ignored directories
  • add regression coverage proving local virtualenv symlinks do not change the audited file set and ordinary symlinks are still rejected
  • refresh the committed export manifest for the two changed audited files

Validation

  • documented community export audit — PASS: 406 files, 400 HTTP operations, 1 WebSocket, 89 tables
  • manifest file count before/after — unchanged at 406
  • uv run --project services/api pytest services/api/tests/test_export_manifest.py -q — PASS: 5 passed
  • uv run --project services/api pytest services/api/tests -q — PASS: 2915 passed, 1 skipped (dedicated CI PostgreSQL service), 2 warnings
  • uv run --project services/api ruff check services/api/scripts/audit_community_export.py services/api/tests/test_export_manifest.py — PASS
  • git diff --check — PASS

Exact base: 6fa79ccb56e206ed55e637c4786c56ddb99fa055
Exact head: f02a1624f1ab4dd50d552c7a8954b5a599a8d39e

Signed-off-by: hizlidepoo <307505121+hizlidepoo@users.noreply.github.com>
@hizlidepoo
hizlidepoo requested a review from L4XB as a code owner September 15, 2026 13:31
@github-actions

Copy link
Copy Markdown

Thanks for the pull request. One required step is still open, and it is not one
a maintainer can complete for you.

Contributor License Agreement. This project records acceptance as a public
comment on the pull request itself: there is no external CLA service and no
separately stored token. Read CLA.md and post this exact
sentence as a standalone comment here:

I have read and agree to the SixSentences CLA v1.0.

The CLA / acceptance status is bound to the exact head commit and is
re-evaluated when you post the comment or push again, so nothing else is needed.

Two further gates apply to every contribution and are described in
CONTRIBUTING.md: every commit carries your own
Signed-off-by trailer (git commit -s) and is signed with your own key. A
maintainer must not add either on your behalf.

@hizlidepoo

Copy link
Copy Markdown
Contributor Author

I have read and agree to the SixSentences CLA v1.0.

@L4XB L4XB left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you — this is a good first contribution, and it fixes a real
embarrassment: the audit command CONTRIBUTING.md tells people to run could not
pass on the setup CONTRIBUTING.md tells them to create.

I reviewed it by reproducing rather than reading, and everything you claimed
holds.

The bug, before and after, with a real services/api/.venv present, which is
what uv sync --project services/api leaves behind:

$ python <main's audit_community_export.py> services/api
community export audit failed: symlink:.venv/bin/python

$ python services/api/scripts/audit_community_export.py services/api   # this branch
community export audit passed: 406 files, 400 HTTP operations, 1 WebSocket, 89 tables

406 both before and after the change, so the acceptance criterion about the file
count is met in the strict sense: nothing entered the audited set, the walk
merely stopped aborting. I also created a plain venv/bin/python and re-ran —
also 406, also passing.

Your regression test is a real one. I put main's script back and ran your
tests against it:

FAILED tests/test_export_manifest.py::test_local_virtualenv_directories_are_ignored
E               RuntimeError: symlink:.venv/bin/python
1 failed, 4 passed

It fails without the fix, with exactly the error from the issue, and passes with
it. test_symlink_outside_ignored_directories_is_rejected passes both before and
after, which is the right shape for a guard: it proves the fix did not buy the
pass by weakening the check it was supposed to preserve. The anchored
^symlink:linked\.txt$ is a nice touch — it cannot drift into matching some
other symlink later.

The manifest is exact. I ran --refresh-manifest on your branch and got
no change, with a clean git diff. Two entries, both explained by the two
files you edited, and the 17-byte delta on the script is precisely the two names
you added.

Full suite: 2915 passed, 1 skipped — the same numbers you reported.

The one thing worth writing down

IGNORED_DIRS is matched against every path component, not just the top
level, so this also skips a directory named venv nested anywhere under
services/api/. That is a widening of a fail-closed audit's blind spot, and it
is the one dimension the description does not address.

It is fine, and here is why, so it is on the record rather than in someone's
head: .gitignore carries .venv/ and venv/ without a leading slash, so both
match at any depth. I checked:

$ git check-ignore -v services/api/src/sixsentences_server/venv/evil.py
.gitignore:2:venv/    services/api/src/sixsentences_server/venv/evil.py

Nothing under either name can be committed, so nothing the audit now skips could
ever have reached the published tree. The blind spot grew over territory that
was already unreachable. (git add -f would still get past it, but that is
equally true of data/ today and is a deliberate act in front of a reviewer.)

Covering both names rather than explaining an asymmetry was the better of the
two options the issue offered.

One note for next time, not a change request

Your commit is not signed. CONTRIBUTING.md asks for it alongside the DCO
sign-off, which you did include. It changes nothing here — the squash commit is
signed by GitHub, so main stays fully signed, and no status check verifies it
— but if you contribute again, the one-time setup is in the console block near
the top of CONTRIBUTING.md. That the guide states a requirement nothing checks
is our inconsistency to resolve, not yours to work around.

Merging once the remaining two contexts report. Thanks again for picking up a
maintenance issue rather than a fun one — this is the kind of thing that
silently costs every future contributor twenty minutes, and now it will not.

@L4XB
L4XB merged commit 5f8c7ee into SixSentences:main Sep 15, 2026
23 checks passed
@L4XB

L4XB commented Sep 15, 2026

Copy link
Copy Markdown
Member

Merged as 5f8c7ee, and #145 closed with it. All 23 contexts green.

Thanks again — the next person who follows the setup in CONTRIBUTING.md will just get community export audit passed where they used to get a symlink error in a security audit on their first run. Good first issue to pick, and a clean fix.

One correction to my note about signing, for accuracy rather than because it needs anything from you: the Protect main ruleset carries require_extra_approval_for_unattributed_changes, so an unsigned commit does have a consequence — it turns the merge into one that needs a maintainer approval it would otherwise not need. No status check reports it, which is why it is invisible on the checks list and why I said it verified nothing. It cost you nothing here since the change wanted a review anyway.

If you want another, #152 is the same size and shape.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

audit: the documented local export audit cannot pass on the documented local setup

2 participants