Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d5904168c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
continue-on-error suppressed every non-zero exit from the TruffleHog action, so a scan that failed to start reported a pass. TruffleHog exits 183 for a finding and 1 for an operational error, so warn-only mode now appends --no-fail to suppress only the 183 exit. Also scan this repository with a local workflow ref, so a change to secretScan.yml is checked by the version it proposes.
The trufflesecurity/trufflehog action hardcodes --fail and exposes no exit code, and kingpin rejects a repeated flag with "flag 'fail' cannot be repeated", so --no-fail cannot override it. TruffleHog exits 183 for a finding and 1 for an operational error. Reading the exit code is the only way to let warn-only mode suppress a finding while still failing the job when the scan itself breaks. Pin the image by digest and derive the scan range from the triggering event.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7198ccc294
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The scan reads file:///repo, so it cannot reach the remote. Two events
did not meet that requirement:
- pull_request_target checks out the base repository, so an external
fork's head commit is absent.
- A force push can leave the previous head unreachable from every
remaining ref, so --since-commit cannot resolve.
Neither event has a caller, so drop both rather than fetch the missing
objects. Any unlisted event now fails with an explicit error.
Also check that each commit in the scan range resolves before starting.
TruffleHog aborts on a missing commit without saying why.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83951f7878
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The note ran under if: success(), which is true in warn-only mode even when TruffleHog exits 183, so a run that found a credential claimed the scan was clean. Move the note into the exit-code-0 branch.
|
@roryabraham and @neil-marcellini Requesting your reviews as the top contributors to this repo please 🙏 |
|
|
||
| Scans a repository for committed credentials using [TruffleHog](https://github.com/trufflesecurity/trufflehog). | ||
|
|
||
| The scan scope follows the event that triggered the calling workflow, so a repository needs two callers for full coverage: |
There was a problem hiding this comment.
Hmmmm I'm wondering if we should use Rulesets to run this on all PRs across the org, following the example of verifyPeerReview and validateActions.yml. That way, each repo does not need to wire in its own secret-scan.yml, and this repo does not need to wire in its own secretScanSelf.yml.
We would lose the weekly scan since running workflows on PRs via Rulesets only supports a few event triggers, but we could replace it with a weekly Bedrock job that scans the whole GitHub org using the Trufflehog CLI: trufflehog github --org=trufflesecurity --exclude-archived
Separately, I'm wondering if just the weekly scan is sufficient? Because once a secret has been pushed in any commit, blocking the pull request from being merged doesn't really do anything: at that point the secret should be considered compromised and be rotated.
There was a problem hiding this comment.
Agreed on rulesets; that will drop 57 repos of wiring. I'll rework this into a ruleset-triggered workflow, delete secretScanSelf.yml, and close the Salt PR.
On the weekly scan, I'd drop it rather than move it to Bedrock, It was probably misguided in the first place since history is immutable and the version is pinned, so a recurring full-history scan reports identical result every week forever.
There was a problem hiding this comment.
I was thinking the one gap that might be filled by the full-repo weekly scan would be if someone pushes a token to a branch but never opens a pull request from that branch.
There was a problem hiding this comment.
Unfortunately not. Given that our end goal isn't necessarily blocking merge, but alerting someone who then should rotate the compromised secret in question, I think the webhook + manually running the TruffleHog CLI is ultimately a better tool for this than GitHub Actions.
|
I'm going to drop off of the review here since it sounds like this will go in a different direction anyways, and I'm surprised I'm a top contributor to this repo because I'm still pretty new to GH actions haha. |
3 participants
Details
Adds a reusable workflow that scans a repository for committed credentials using TruffleHog. We currently have no automated secret scanning anywhere in the org. This is the shared half of that work; each repository then adds a short caller workflow.
The scan scope follows the event that triggered the calling workflow, because the TruffleHog action derives its own
--since-commitand--branchfromgithub.event_name. Apull_requestcaller scans only the commits in the PR; ascheduleorworkflow_dispatchcaller scans the full history. So a repository needs two callers for full coverage, which is why this is one reusable workflow rather than two.Inputs are
fail_on_findings(defaultfalse),exclude_paths_file, andrunner.Three details in here are non-obvious and are worth a reviewer's attention:
The container tag has no leading
v. The action's release tag isv3.97.4but the ghcr image tag is3.97.4. I confirmed this against the registry:3.97.4returns HTTP 200 andv3.97.4returns HTTP 404. Pinningv3.97.4would fail every run.--resultsis deliberately omitted. We scan with--no-verification, because verification authenticates each candidate against its live provider and a burst of failed authentication attempts from CI is indistinguishable from credential stuffing in CloudTrail. The consequence is that every finding is then classifiedunverified. TruffleHog's own README recommends--results=verified,unknown, which combined with--no-verificationreports nothing at all. The default (verified,unverified,unknown) is the value we want. There is a comment in the workflow saying so, because this failure mode is a silently green check.The workflow runs the container directly instead of using
trufflesecurity/trufflehog. TruffleHog exits 183 for a finding and 1 for an operational error, and warn-only mode has to suppress the first without hiding the second. The action hardcodes--fail, exposes no exit code, and cannot be overridden — kingpin rejects a repeated flag withflag 'fail' cannot be repeated, which I confirmed in CI. Reading the exit code is the only way to express this, so the workflow derives the scan range from the triggering event itself and branches on the exit code. The image is pinned by digest, which is stronger than the tag pin the action allows.Default is warn-only so each repository can measure its false-positive rate before enforcing.
Related Issues
https://github.com/Expensify/Expensify/issues/681714
Manual Tests
This is a workflow definition, so the meaningful test is a real run. I ran this repository's own three CI validation scripts locally against the new file:
I also added
secretScanSelf.yml, so this repository scans itself through a local workflow ref. A pull request that changessecretScan.ymlis therefore checked by the version it proposes. That caught a real bug in this PR before review: an earlier attempt to pass--no-failfailed in 8 seconds withflag 'fail' cannot be repeated.The exit-code branching is the part worth testing, so I extracted the scan step's shell and ran it against a stubbed
dockeracross every branch:fail_on_findingspull_requestfalsepull_requestfalsepull_requesttruepull_requestfalseschedulefalsepushto new branchfalse--branchonly, no--since-commitpushto existing branchfalse--since-commitand--branchreleasefalseThe fourth row is the one that matters: a broken scan fails the job even in warn-only mode.
validation logs
To verify end to end after merge:
@main.SaltPR, which adds the caller workflows.Saltand confirm theSecret Scan / TruffleHogcheck runs and passes.SaltActions tab, selectSecret Scan (Full History), and chooseRun workflow.Linked PRs
Enable TruffleHog secret scanning in CI — on HOLD until this PR merges