Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Talisman pre-commit hook secrets scanning #233

Merged
merged 5 commits into from
Dec 19, 2024

Conversation

peppescg
Copy link
Contributor

co-author @kantord

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@peppescg peppescg marked this pull request as ready for review December 19, 2024 11:49
@peppescg peppescg requested a review from a team as a code owner December 19, 2024 11:49
@peppescg peppescg marked this pull request as draft December 19, 2024 11:49
allow if {
some repo_id
repo_data := parsed_data.repos[repo_id]
endswith(repo_data["repo"], "https://github.com/thoughtworks/talisman")
Copy link
Contributor

Choose a reason for hiding this comment

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

I see we're checking for the hook coming from this repo to be set. Do we want to check the hook entries as well though?

I mean:

      - id: talisman-commit
        entry: cmd --githook pre-commit

We can iterate the hooks and verify those.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, this is a good idea...

I was wondering though. If we add such complex business logic for each rule concerning pre-commit, and then copy paste the exact same code for every imaginable pre-commit hook, then we are creating a maintenance nightmare 🤔

so I guess we could add a helper function in minder for this (like we have for github actions), or else we could convert this rule into a parametric rule where you have to specify which hook you want to enforce?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mm yep, maybe makes sense to match the mandatory fields right

Copy link
Contributor

Choose a reason for hiding this comment

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

convert this rule into a parametric rule where you have to specify which hook you want to enforce?

which kinda raises the question if we can somehow support "partial application" of rule types, i. e. we make a generic rule type that takes 1 argument as required configuration, and we build other rule types that re-use that parametric rule type, but don't require any parameters 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

this is a really good idea. I haven't really figured out how this would look like in terms of implementation, but I can definitely see us supporting libraries... Let me give this some thought. Can you add an entry in the Gaps tracker?

Copy link
Contributor

@kantord kantord Dec 19, 2024

Choose a reason for hiding this comment

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

Gaps tracker?

what is that

edit: nevermind, I realized that it's in the spreadsheet

@peppescg peppescg marked this pull request as ready for review December 19, 2024 15:36
@JAORMX JAORMX merged commit 5d6f64d into mindersec:main Dec 19, 2024
3 of 4 checks passed
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.

5 participants