Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a secret-scanning step to the pre-commit hook by introducing a new 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #27609 +/- ##
==========================================
+ Coverage 73.17% 73.19% +0.02%
==========================================
Files 1561 1561
Lines 127051 127070 +19
Branches 15383 15394 +11
==========================================
+ Hits 92970 93011 +41
+ Misses 33105 33101 -4
+ Partials 976 958 -18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
9larsons
left a comment
9larsons
left a comment
There was a problem hiding this comment.
Thanks for adding this! I'm annoyed at how much baggage this package brings in given it's all prettifying the console output 😬... but let's give it a try. I made a slight adjustment to the Tinybird regex in testing to cover some more cases.
cmraible
force-pushed
the
chris-ny-1247-add-a-git-pre-commit-hook-to-check-for-secrets-in-staged
branch
from
629b711 to
fc25e84
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/hooks/pre-commit.bash:
- Around line 23-44: The scan_staged_secrets function can mask upstream failures
from git show because pipelines (e.g., git show | grep or git show | pnpm exec
secretlint) don't propagate git show exit codes; enable strict pipe failure or
explicitly check git show status to ensure read errors cause the hook to fail:
add "set -o pipefail" at the top of the function (or globally in the script) or
capture git show output into a variable/temporary file and verify git show exit
code before piping into grep/secretlint, updating scan_status to 1 on any git
show failure so the hook returns non-zero.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d72db31c-48d1-454b-bc9d-3ddadcd0ead6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
.github/hooks/pre-commit.bash.secretlintrc.jsonpackage.json
✅ Files skipped from review due to trivial changes (2)
- .secretlintrc.json
- package.json
cmraible
force-pushed
the
chris-ny-1247-add-a-git-pre-commit-hook-to-check-for-secrets-in-staged
branch
from
fc25e84 to
7b2ecae
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/hooks/pre-commit.bash (1)
23-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
git showfailures are still silently masked in the scan pipeline (previously flagged, unresolved)Without
set -o pipefail, agit show ":$file"failure at line 34 causes secretlint to receive empty/partial input and exit 0, soscan_statusstays 0 and the hook proceeds as if the file was clean. The suggested fix from the prior review is still applicable.🔧 Proposed fix
#!/usr/bin/env bash +set -o pipefail # Modified from https://github.com/chaitanyagupta/gitutils🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/hooks/pre-commit.bash around lines 23 - 44, The scan_staged_secrets function currently pipes git show ":$file" into secretlint which can mask git show failures; enable strict pipe failure handling or explicitly check git show's exit status before invoking secretlint. Update scan_staged_secrets to either set -o pipefail at the start of the function or run git show ":$file" into a temporary buffer/variable, check its exit code, and only then pass the content to pnpm exec secretlint --format=compact --stdinFileName="$file"; ensure scan_status is set to 1 if git show fails and that secretlint is only invoked when git show succeeded (references: scan_staged_secrets, git show ":$file", pnpm exec secretlint --format=compact --stdinFileName="$file").
🧹 Nitpick comments (1)
.github/hooks/pre-commit.bash (1)
40-44: ⚡ Quick winAdd a success confirmation message when files are scanned clean.
Secretlint's documented behavior is: "If you got empty result and exit status is 0, your project is secure" — meaning
--format=compactproduces zero output on a clean scan. Combined with the absence of any completion echo here, a developer staging several files will seeScanning staged files for secrets (pre-commit hook)followed by complete silence before the hook moves on. This is especially jarring for a security gate where visible confirmation builds trust that the scan actually ran to completion.✅ Proposed improvement
if [ $files_scanned -eq 0 ]; then echo "No staged text files to scan, continuing..." + else + echo -e "${green}✅ Secret scanning passed${no_color} (${files_scanned} file(s) scanned)" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/hooks/pre-commit.bash around lines 40 - 44, The hook currently prints a message only when no staged files are scanned and otherwise returns $scan_status silently; add a clear success confirmation when files were scanned and the scan exited successfully by checking files_scanned and scan_status (e.g., if [ $files_scanned -gt 0 ] && [ $scan_status -eq 0 ]), and echo a concise message like "Secret scan passed for X staged files" before returning $scan_status so developers see visible confirmation that secretlint ran and found no issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/hooks/pre-commit.bash:
- Around line 23-44: The scan_staged_secrets function currently pipes git show
":$file" into secretlint which can mask git show failures; enable strict pipe
failure handling or explicitly check git show's exit status before invoking
secretlint. Update scan_staged_secrets to either set -o pipefail at the start of
the function or run git show ":$file" into a temporary buffer/variable, check
its exit code, and only then pass the content to pnpm exec secretlint
--format=compact --stdinFileName="$file"; ensure scan_status is set to 1 if git
show fails and that secretlint is only invoked when git show succeeded
(references: scan_staged_secrets, git show ":$file", pnpm exec secretlint
--format=compact --stdinFileName="$file").
---
Nitpick comments:
In @.github/hooks/pre-commit.bash:
- Around line 40-44: The hook currently prints a message only when no staged
files are scanned and otherwise returns $scan_status silently; add a clear
success confirmation when files were scanned and the scan exited successfully by
checking files_scanned and scan_status (e.g., if [ $files_scanned -gt 0 ] && [
$scan_status -eq 0 ]), and echo a concise message like "Secret scan passed for X
staged files" before returning $scan_status so developers see visible
confirmation that secretlint ran and found no issues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ddee1064-91e6-4757-9520-96dda077b67a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
.github/hooks/pre-commit.bash.secretlintrc.jsonpackage.json
✅ Files skipped from review due to trivial changes (1)
- .secretlintrc.json
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
|
Haven't merged this yet because I have a (potentially) better idea that I'm working on, which would render this unnecessary. Keeping it open for now as I may still merge this. |
ref https://linear.app/tryghost/issue/NY-1247/add-a-git-pre-commit-hook-to-check-for-secrets-in-staged Secret scanning should run from the package-managed toolchain so contributors get protection after pnpm install without installing a separate binary.
- bare tokens in source (const t = "p.eyJ...") and code comments now match - case variants of the URL/env prefix (Token=, tinybird_token=) now match - pinned on the universal p.eyJ shape verified against real local-host samples
cmraible
force-pushed
the
chris-ny-1247-add-a-git-pre-commit-hook-to-check-for-secrets-in-staged
branch
from
7b2ecae to
61179ad
Compare
ref https://linear.app/tryghost/issue/NY-1247/ The pre-commit secret scan should fail closed when staged file content cannot be read from the Git index, instead of letting pipeline exit behavior mask the read failure.
ref https://linear.app/tryghost/issue/NY-1247/ The pre-commit hook should explain how to recover when secretlint is unavailable, instead of showing pnpm's generic command-not-found output.
cmraible
deleted the
chris-ny-1247-add-a-git-pre-commit-hook-to-check-for-secrets-in-staged
branch
closes https://linear.app/ghost/issue/BER-3616 When a recipient redeems a gift subscription, Ghost sends staff a notification email. Today it reuses the *paid subscription started* copy and the *New paid members* email-preference toggle. We've decided to differentiate the gift redemption staff notification more clearly from the new paid members one. This PR changes the copy of the gift redemption staff notification and moves it under the "Gift subscriptions" email-preference toggle. ## Changes **Email copy** — `notifyGiftSubscriptionStarted`: - Subject: `🎁 Paid subscription started: <name>` → `🎁 Gift subscription redeemed: <name>` - Headline: `You have a new paid subscriber` → `A gift subscription was redeemed` - Plaintext body line updated to match - Preview text updated to match **Notification preference** — gift redemptions now go through the existing **Gift subscriptions** toggle (Stripe + `giftSubscriptions` feature flag), instead of *New paid members*: - Toggle label: `Gift subscription purchases` → `Gift subscriptions` - Description: `Every time someone purchases a gift subscription` → `Every time someone purchases or redeems a gift subscription` **Pre-commit hygiene:** - Added a targeted `secretlint-disable-next-line` on the random-password-generator default in `user.js` — the new secret-scanning hook (added in #27609) flags `password: security.identifier.uid(50)` as a credential-assignment false-positive, blocking any future change to this file. The line generates a random password placeholder; it isn't a real credential.
2 participants
ref https://linear.app/tryghost/issue/NY-1247/add-a-git-pre-commit-hook-to-check-for-secrets-in-staged