Skip to content

Added detect-secrets through pre-commit package#89

Draft
eklee15 wants to merge 3 commits intoqiskit-community:mainfrom
eklee15:detect-secret-yelp
Draft

Added detect-secrets through pre-commit package#89
eklee15 wants to merge 3 commits intoqiskit-community:mainfrom
eklee15:detect-secret-yelp

Conversation

@eklee15
Copy link
Collaborator

@eklee15 eklee15 commented Mar 25, 2026

Description of Change

PR related to this issue (#85)

  1. Added instruction to enable "pre-commit" in README.md
  2. Added pre-commit configuration file .pre-commit-config.yaml in order to enable "detect-secrets"
  3. Initialized .secrets.baseline to baseline existing secrets identified by detect-secrets. This baseline has been manually audited, assuming there are no existing secrets in our repo; subsequent scans will now focus exclusively on new potential secret leaks.

Checklist ✅

  • Have you included a description of this change?
  • Have you updated the relevant documentation to reflect this change?
  • Have you made sure CI is passing before requesting a review?

Ticket

  • Fixes #
  • Is Part of #

@awennersteen
Copy link
Collaborator

awennersteen commented Mar 26, 2026

Edit:
My bad, I now see there was the issue #85 discussing the below already. At least partially.

It looks like this detect-secrets is not actively maintained?
See discussion at
Yelp/detect-secrets#926

I see there are other forks of it, one even by IBM:
https://github.com/IBM/detect-secrets

Should we switch? are there alternatives?

----

### Pre-commit detect-secrets
`detect-secrets` is an open-source, developer-friendly tool designed to scan
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you add to the docs how to run pre-commit manually?

pre-commit run --all-files

for example.

We could also include instructions on how to push without

git commit --no-verify

but perhaps we shouldn't encourage it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added in this commit.
693abf5

@@ -0,0 +1,8 @@
repos:
Copy link
Collaborator

Choose a reason for hiding this comment

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

OK for me to use pre-commit.

I was actually considering setting up up for some forced formatting too.

@@ -0,0 +1,8 @@
repos:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we consider adding this to the CI/CD pipeline?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree

Copy link
Collaborator Author

@eklee15 eklee15 Mar 27, 2026

Choose a reason for hiding this comment

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

I've talked to some of the developers who have used detect-secrets before, and they have noted that detect-secrets occasionally triggers false positives that can be "annoying" for the developers. Once forced, they might have more issues if they did not follow our instructions to install/use detect-secret correctly (maintenance overhead).

So we could keep this as an option, but I would suggest adding a GitHub Actions workflow to serve as a secondary sanity check for the repository - as this process will be executed at the server side, this would be "remediation," rather than the "prevention" method, ensuring our main branch remains "secret free".

Please find the discussion in this issue #85 in the Describe the solution you’d like section.

If agreed, I will create another PR to enforce this mechanism on GHA workflow.
Please let me know what you think.

@OkuyanBoga
Copy link
Collaborator

Looks good to me overall. The main thing I’d still want is CI enforcement, so the check is applied consistently and not only when someone has the hook installed locally. Aside from that, it may be worth documenting how .secrets.baseline should be refreshed and maintained over time.

@eklee15
Copy link
Collaborator Author

eklee15 commented Mar 27, 2026

Edit: My bad, I now see there was the issue #85 discussing the below already. At least partially.

It looks like this detect-secrets is not actively maintained? See discussion at Yelp/detect-secrets#926

I see there are other forks of it, one even by IBM: https://github.com/IBM/detect-secrets

Should we switch? are there alternatives?

@awennersteen I would like to go with the Yelp version because the IBM version flagged a larger number of false positives (75 vs 31), which could become a friction point for developers. Additionally, using the Yelp version keeps this repository vendor-neutral rather than IBM-specific. Of course, we would use detect-secrets with a grain of salt, as it does not guarantee finding all the secrets. If we find significant false positives or false negatives over our development, we could switch to IBM version or something else (https://github.com/bridgecrewio/detect-secrets).

@eklee15
Copy link
Collaborator Author

eklee15 commented Mar 27, 2026

Looks good to me overall. The main thing I’d still want is CI enforcement, so the check is applied consistently and not only when someone has the hook installed locally. Aside from that, it may be worth documenting how .secrets.baseline should be refreshed and maintained over time.

@OkuyanBoga I've added this commit (be9b7bf) to handle false-positive in development time, and how to update the .secrets.baseline. Please let me know if this makes sense.

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.

3 participants