fix: support TruffleHog exclude patterns - #94
Conversation
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b426ea5. Configure here.
David Larsen (dc-larsen)
left a comment
There was a problem hiding this comment.
Tested this end to end against the YNAB repro before reviewing. Ran the real CLI (socket-basics --secrets --exclude-dir ...) on main and this branch, absolute and relative workspaces, with YNAB's exact 8-directory list, on trufflehog 3.93.8 (the Dockerfile pin) and 3.96.0. Fixture repo had planted secrets in app/, node_modules/pkg/, src/node_modules/, .github/workflows/, .git/, dist/, and sub/tmp/.
The core fix works:
- main reproduces the customer failure exactly:
flag 'exclude-paths' cannot be repeated, empty facts file, and the log still saysConnector trufflehog completed successfully - this branch excludes correctly in every workspace shape I tried, including nested
src/node_modules .gitexcluded while.github/workflowsstill gets scanned- workspace under
/tmpwithtmpin the exclude list no longer wipes the whole scan - temp file cleaned up on success and on the early-return error path
- 199 tests pass
One change requested, one gap to decide on, two small suggestions.
1. Relative workspace regresses reported paths and component IDs (change requested)
_absolute_scan_target makes trufflehog emit absolute paths, but the prefix-stripping in _create_alert and _process_results resolves the workspace root via getattr(workspace, 'path', None) or getattr(workspace, 'root', None). For a relative pathlib.Path, .path doesn't exist and .root is '', so nothing gets stripped.
A/B through the CLI with a relative --workspace ws, no excludes configured, isolating just this change:
| main | this PR | |
|---|---|---|
| component name | app/creds.txt |
/Users/jane/proj/ws/app/creds.txt |
| alert filePath | ws/app/creds.txt |
/Users/jane/proj/ws/app/creds.txt |
| component id | a11692a9... |
48aab2e0... |
That moves name, subpath, manifestFiles[].file, props.filePath, and the sha256 id, so alert identity churns and full host paths land in customer-visible output. The absolute-workspace case (the GH Actions default) is byte-identical to main, IDs included, so this only bites local runs with a relative --workspace. YNAB runs socket-basics locally, so they could hit it.
Two ways out, either works:
- Anchor the patterns to the workspace as passed instead of abspath'ing it, and drop
_absolute_scan_target. I tested this variant: exclusions still correct in all four workspace shapes, and names/filePaths/ids byte-identical to main everywhere._workspace_rootreturnsos.path.normpath(os.fspath(workspace))(with''for.), and the no-root fallback pattern becomes^(?:.*[/\\])?<dir>(?:[/\\]|$)since paths are cwd-relative in that case. - Keep absolute targets and fix the prefix-stripping in
_create_alert/_process_resultsin this PR (stop relying onPath.root).
If you take the first route, test_scan_uses_absolute_targets_for_relative_workspace inverts to assert the target passes through as-is.
2. Excludes still no-op in changed-files mode (your call whether in scope)
In PR-diff mode the connector passes individual files as targets. Trufflehog applies --exclude-paths only while walking directories; an explicitly-passed file target is scanned regardless. Verified on 3.93.8 and 3.96.0: the pattern regex-matches the file path, the same file is skipped when reached by directory walk, and scanned when passed directly as a target.
Repro: real git repo, staged app/staged.txt and node_modules/staged.txt, excludes configured. This branch reports both files. So trufflehog_exclude_dir still does nothing on the diff path, which is the common CI path.
Cheap fix if you want it here: filter changed_files against the same patterns before building the command and drop any file whose target path matches. Fine as a fast-follow too. Either way a code comment documenting the asymmetry would save the next person the investigation.
3. Keep the resolved patterns visible in logs
The log line now shows --exclude-paths /tmp/socket-basics-trufflehog-xxxx.txt and the finally block deletes that file. The old log line is how CE-308 got root-caused from the customer's paste. A logger.debug of the resolved patterns preserves that.
4. Warn when a target falls outside the workspace anchor
Absolute scan_files entries outside the workspace root bypass all excludes (patterns anchor to the workspace, target lives elsewhere, exit 0, nothing excluded, no warning). This predates the PR; a logger.warning when a target doesn't sit under the anchor would keep it visible.
One thing for the changelog: the bug is broader than the issue title. On main, a single exclude dir also kills scanning, since trufflehog reads the value as a filter filename (unable to open filter file: node_modules), and when the name happens to resolve to a real file it silently applies whatever that file contains as patterns. trufflehog_exclude_dir has never excluded anything; this PR is the first time the setting works. Worth a release note since configured customers will see output change.
Separate from this PR: with an absolute workspace, filePath is already Users/jane/repo/app/creds.txt on main (Path.root is /, and relpath against / only strips the leading slash). Every secret alert today carries the full host path minus the leading slash. I'll file that one separately.
Signed-off-by: lelia <2418071+lelia@users.noreply.github.com>
Signed-off-by: lelia <2418071+lelia@users.noreply.github.com>
|
bugbot run |
David Larsen (dc-larsen)
left a comment
There was a problem hiding this comment.
Re-tested the new head (7443e82) the same way as last round: real CLI on main vs this branch, absolute and relative workspaces, YNAB's 8-directory list, a staged changed-files repo, and the new glob forms, on trufflehog 3.93.8 (the Docker pin) and 3.96.
Everything from the last review is fixed and verified:
- Absolute and relative workspace now produce identical output: workspace-relative names and filePaths (
app/creds.txtin both) with identical component IDs. The relative-workspace regression is gone. - Changed-files mode honors excludes now. Staged
node_modules/staged.txtwas skipped with the log line, the remaining file scanned and reported with a clean relative path. - Exclude patterns log at debug, and out-of-workspace targets warn.
- Globs behave exactly as documented, end to end on both trufflehog versions:
config/*.jsonstayed root-relative (src/config/secrets.jsonstill scanned),**/appsettings.*.jsonexcluded at every depth whileappsettings.jsonand a.json.bakvariant still scanned. - Workspace under
/tmpwithtmpin the exclude list stays safe,.gitexcluded while.github/workflowsscans, root-workspace edge covered, 214 tests pass.
The path normalization also fixes the pre-existing host-path leak (main emits Users/jane/repo/app/creds.txt today, this branch emits app/creds.txt). That means every existing secret-scanning user gets a one-time component ID change on upgrade, not just relative-workspace runs. The changelog covers the new stability guarantee; is there anything downstream keyed on component id that should know the whole set re-keys once?
Two nits, neither blocking:
- Entry normalization:
./distbuilds^<root>[/\\]\.[/\\]dist(...)andsub//cachebuilds a double-separator pattern; both can never match a normalized path, so they're silent no-ops. Running each entry throughposixpath.normpathbefore pattern building would cover it. action.ymlnow points atghcr.io/socketdev/socket-basics:2.2.1, which isn't published yet (2.0.3 is the current tag, and what action consumers run today). Assuming the image publishes at merge/release so action users never hit a missing tag; they'll also jump 2.0.3 to 2.2.1 in one step, so the release note matters for them most.
LGTM. Thanks for picking up all four items, and the globs are a nice addition on top.
Signed-off-by: lelia <2418071+lelia@users.noreply.github.com>
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 51e285e. Configure here.
Summary
**/appsettings.*.json.Validation
uv run --with pytest pytest -qpython3 -m py_compile socket_basics/core/connector/trufflehog/__init__.pyFixes CE-308
Ref CE-347
Note
Medium Risk
Changes how secret-scan exclusions are interpreted and which files TruffleHog runs on; misconfigured patterns could skip scans or leave excluded paths scanned until configs are validated.
Overview
Release 2.2.1 bumps package, GitHub Action image, and changelog for TruffleHog exclude and path fixes.
TruffleHog exclusions no longer pass each
trufflehog_exclude_direntry as its own--exclude-pathsargument (which TruffleHog could treat as filenames). All exclusions are compiled into one temporary regex filter file and a single--exclude-pathsflag. Patterns are anchored under the workspace so names liketmpor.gitdo not match parent paths or.github. Entries support case-sensitive literals and globs (e.g.**/appsettings.*.json), with normalization for./, doubled slashes, and filesystem-root workspaces.Scan targeting applies the same exclude logic to changed-file and explicit scan_files targets before invoking TruffleHog, skips the run when everything is excluded or there are no targets, warns when a target lies outside the workspace, and deletes the filter file after the run (including on failure).
Finding output normalizes file paths to workspace-relative posix paths for alerts, components, and SHA256 IDs so facts stay stable across OS, cwd, and absolute vs relative TruffleHog paths.
Parameter descriptions in
action.yml,connectors.yaml, anddocs/parameters.mdreflect glob-capable excludes. Regression tests cover pattern building, scan wiring, and path normalization.Reviewed by Cursor Bugbot for commit 51e285e. Configure here.