Skip to content

chore(precommit): annotate known false positives so secret scanner passes - #1805

Merged
mirooon merged 3 commits into
mainfrom
fix/pre-commit-false-positive-markers
May 25, 2026
Merged

mirooon merged 3 commits into
mainfrom
fix/pre-commit-false-positive-markers

Conversation

@mirooon

@mirooon mirooon commented May 14, 2026

Copy link
Copy Markdown
Contributor

Which Linear task belongs to this PR?

None — repository chore: the husky pre-commit secret scanner blocked unrelated branch commits because of false positives. No Linear ticket; happy to open one if preferred.

Why did I implement it this way?

The .husky/pre-commit hook already supports an opt-out marker: lines containing pre-commit-checker: not a secret (on the line itself or the line immediately above) are skipped by both the .env-secret check and the Ethereum-private-key regex check. See is_known_false_positive_match and DISABLE_WITH_COMMENT in .husky/pre-commit.

Several long-standing constants in the repo match those patterns but are not credentials. Rather than weakening the scanner, I annotated each false positive with the documented marker, placed adjacent to (or directly above) the matching line so the hook's PREV/CURR window sees it.

Annotated false positives:

  • .github/workflows/deploy-smoke-test.yml
    • mongo:7@sha256:… image digest pin (supply-chain integrity)
    • EXPECTED_SHA256=fd1a34… gum release checksum (publisher-published)
    • ETH_NODE_URI_LOCALANVIL=http://localhost:8545 (local anvil URL written into CI .env)
    • second http://localhost:8545 use inside the anvil readiness probe
  • script/demoScripts/demoPatcher.ts
    • public arbiscan example transaction hash in JSDoc
    • 0xff…ff max-uint256 used as approval amount
  • script/demoScripts/utils/cowSwapHelpers.ts
    • zero bytes32 sentinel for non-EVM receiver (bridging disabled)
    • 0xff…ff max-uint256 used as approval amount
  • script/deploy/safe/timelock-queue.test.ts
    • zero bytes32 test fixture (ZERO_BYTES32)
  • test/solidity/Periphery/Patcher.t.sol
    • three occurrences of the 0x1234…CDEF test fixture marker used to locate the patched word

No runtime behavior changes. The hook now exits with "Secret check passed" (no warnings) and all downstream pre-commit checks (forge build, typechain, tsc, lint-staged) pass.

Checklist before requesting a review

Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)

  • I have checked that any arbitrary calls to external contracts are validated and or restricted
  • I have checked that any privileged calls (i.e. storage modifications) are validated and or restricted
  • I have ensured that any new contracts have had AT A MINIMUM 1 preliminary audit conducted on by <company/auditor>

mirooon and others added 2 commits May 14, 2026 20:12
…comment

The husky pre-commit secret scanner flags several constants that look like
private keys or .env values but are not credentials:

- mongo:7 image SHA-256 digest pin (deploy-smoke-test.yml)
- gum release tarball SHA-256 checksum (deploy-smoke-test.yml)
- localhost:8545 anvil RPC URL written into the CI .env (deploy-smoke-test.yml)
- public on-chain example tx hash in JSDoc (demoPatcher.ts)
- max uint256 (0xff…ff) constants used as approval amounts (demoPatcher.ts, cowSwapHelpers.ts)
- zero bytes32 sentinels used as non-EVM receiver / test fixture (cowSwapHelpers.ts, timelock-queue.test.ts)
- fixed test fixture marker 0x1234…CDEF used to locate the patched word (Patcher.t.sol)

Annotate each occurrence with the documented "pre-commit-checker: not a secret"
marker that the hook already recognizes via is_known_false_positive_match.
No runtime behavior changes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The hook's is_known_false_positive_match only checks PREV_LINE and CURR_LINE,
so a marker placed above the const declaration was not seen for the literal
on the following line. Move the comment directly above the literal so the
remaining "Potential private key" warning clears.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented May 14, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: e1b39e7e-0eac-4b32-8923-a3c882fef5f3

📥 Commits

Reviewing files that changed from the base of the PR and between 61ef8dc and 936ccec.

📒 Files selected for processing (5)
  • .github/workflows/deploy-smoke-test.yml
  • script/demoScripts/demoPatcher.ts
  • script/demoScripts/utils/cowSwapHelpers.ts
  • script/deploy/safe/timelock-queue.test.ts
  • test/solidity/Periphery/Patcher.t.sol

Walkthrough

This PR adds nine inline pre-commit-checker clarification comments across the codebase to prevent false secret detection. Comments are inserted in workflow configuration, deployment scripts, and test files to explicitly mark SHA-256 digests, sentinel values, max constants, and test fixtures as non-credentials.

Changes

Pre-commit Checker Clarification Comments

Layer / File(s) Summary
Pre-commit checker clarification comments
.github/workflows/deploy-smoke-test.yml, script/demoScripts/demoPatcher.ts, script/demoScripts/utils/cowSwapHelpers.ts, script/deploy/safe/timelock-queue.test.ts, test/solidity/Periphery/Patcher.t.sol
Adds inline pre-commit-checker comments to clarify that SHA-256 digests, checksums, zero-byte sentinels, max-value constants, transaction hashes, and test fixtures are not secrets.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

  • lifinance/contracts#1747: Introduces the smoke-test workflow with SHA-256/checksum handling that this PR adds pre-commit-checker comments to.
  • lifinance/contracts#1343: Introduces the .husky/pre-commit hook mechanism that the pre-commit-checker comment markers bypass.
  • lifinance/contracts#1759: Modifies the same Solidity test file sections where this PR inserts fixture marker comments.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: adding annotations for false positives to allow the pre-commit secret scanner to pass.
Description check ✅ Passed The description fully completes the repository template with all required sections, clearly explains the rationale and implementation approach, and provides comprehensive details about the changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/pre-commit-false-positive-markers

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@lifi-action-bot
lifi-action-bot marked this pull request as draft May 14, 2026 18:14
@mirooon
mirooon marked this pull request as ready for review May 14, 2026 18:18
@lifi-action-bot

lifi-action-bot commented May 14, 2026 •

Copy link
Copy Markdown
Collaborator

Test Coverage Report

Line Coverage: 89.97% (3240 / 3601 lines)
Function Coverage: 93.65% ( 502 / 536 functions)
Branch Coverage: 72.18% ( 597 / 827 branches)
Test coverage (89.97%) is above min threshold (83%). Check passed.

@mirooon
mirooon enabled auto-merge (squash) May 14, 2026 18:31
0xDEnYO
0xDEnYO previously approved these changes May 15, 2026
maxklenk
maxklenk previously approved these changes May 15, 2026
@mirooon
mirooon dismissed stale reviews from maxklenk and 0xDEnYO via 936ccec May 22, 2026 22:40
@mirooon
mirooon merged commit 2e335b1 into main May 25, 2026
28 of 29 checks passed
@mirooon
mirooon deleted the fix/pre-commit-false-positive-markers branch May 25, 2026 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants