-
Notifications
You must be signed in to change notification settings - Fork 573
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
fix(detect-secrets): improve how secret detection works #6109
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I have not run this PR on the simulator or device. Happy to do tomorrow. I haven't run eigen since summer so I suspect im out of date as far as deps/tooling goes. Is there something specific I should look out for?
|
||
This happens when you try to commit some code that looks like a secret, a key, a token, etc. | ||
Make sure what you are committing has no sensitive data in it. | ||
If you are sure is it _not_ sensitive data, then you can add an inline comment containing `pragma: allowlist secret`, to signify it is ok to commit. Then try to commit again, and it will work this time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. I will update the playbook to include this suggestion as well.
One thing I recalled when looking at this, there is an open issue when using the next line variant of the inline allowlist (_pragma: allowlist nextline secret _), it does not work in .env
files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we would only need to use the nextline for typescript, since prettier might format things to fit for width. for .env files, i think we are ok with the inline one, and the line can go as long as we want. good to know though.
@@ -0,0 +1,15 @@ | |||
#!/usr/bin/env bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really nice. Moving these commands with exclusions into dedicated scripts keeps things clean and contained. 🙌
@@ -221,7 +221,7 @@ jobs: | |||
working_directory: /usr/src/app | |||
steps: | |||
- checkout | |||
- run: detect-secrets-hook --baseline .secrets.baseline --exclude-secrets '[a-fA-F0-9]{24}' --exclude-lines 'W/"[!#-\x7E]*"' $(git ls-files | grep -v stickerpack) | |||
- run: ./scripts/secrets-check-all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has eigen had any funkiness in CI with this tool? We had to implement a work around (in some projects) for what seems to be a random mutation of the baseline generated_at field. I have never heard eigen having this issue tho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we did have this at some point at the beginning, but it was only while people didnt have their stations set up, so the ci would be the first time this would run. we havent had an issue since.
i dont want to complicate things for now, so if we do end up having this again often, ill definitely try to fix it then.
no need to run eigen for this, especially if you havent touched it in a while. i will show this on next KS and do a demo, but if you wanna try it out, i think the minimum thing is:
extra:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!!
In addition to line comments I have couple higher level questions:
- Do we want to ignore files via
--exclude-files
for some commands and not others? I'm wondering if we should be consistent with which files we're excluding. - Is there an opportunity to DRY up some of the scripts? Seems like most common change is what we're piping in (git ls-files vs git diff --staged --name-only) - we might be able to have fewer scripts and inline that input into the package.json commands
- Do we intend to run the "add/generate" commands regularly? If not, would it make sense to document that process / commands rather than defining these package.json commands?
package.json
Outdated
"secrets-check-all": "scripts/secrets-check-all", | ||
"secrets-add-staged": "scripts/secrets-add-staged", | ||
"secrets-add-all": "scripts/secrets-add-all", | ||
"secrets-generate-once": "scripts/secrets-generate-baseline", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like these commands might benefit from some namespacing. What do you think of a secrets:
command namespace and a ./scripts/secrets/
folder?
"secrets:audit": "detect-secrets audit .secrets.baseline",
"secrets:check:staged": "scripts/secrets/check-staged",
"secrets:check:all": "scripts/secrets/check-all",
"secrets:generate": "scripts/secrets/generate",
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thats a nice idea
requirements.txt
Outdated
detect-secrets >= 1.1 | ||
# commented out for now, and replaced with the brew version | ||
# read more here: https://github.com/artsy/homebrew-formulas/pull/13 | ||
# detect-secrets==1.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: I feel like we might as well git remove this file if we're commenting all of the lines. We can always bring the file back if we need to but chances are that we'll stick with the brew-based installation and this file will remain unused and cluttering the root directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess i needed just a tiny extra push haha. doing it.
removed inline allowlist
|
@dblandin I've been involved with this tool for some time and I have not had to add secrets and I don't immediately recall having to inline exclude any (it may have happened) however I've had to re-generate the baseline on a number of occasions, thought not in a while. This was something that would happen more frequently after we started to incorporate this tool to projects. Just wanted to share my experience. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks great to me, might be worth sharing out in mobile-practice before or after it is merged, good to merge from my perspective
this should be good to go. ill mark it as #squashongreen and we can show it on the next free KS. the last two were taken over by other stuff. |
The type of this PR is: TYPE
This PR resolves []
Description
Secret detection has been added, but it had some small issues with certain filepaths and how we handle these.
There's some context info here https://artsy.slack.com/archives/CP9P4KR35/p1643215901266300?thread_ts=1643198172.246900&cid=CP9P4KR35 but the gist of it is:
'
,(
,)
in them. the original bash script did not handle them, and to account for them we just skipped them. i fixed that by making xargs do better delimiting so the command can run.--verbose
so we can see progress and any wrong running of the command.--exclude-files
.detect-secrets
and used the brew version. (more info: PLATFORM-3565: add detect-secrets formula homebrew-formulas#13)so the idea is:
generate
script at the start. that is done already. (we might need to run it again in the future, but it should be fine for a while, if we dont change many things).check
scripts on precommit hook and on ci, to make sure we dont commit secrets.todo:
PR Checklist (tick all before merging)
To the reviewers 👀
Changelog updates
Changelog updates
Cross-platform user-facing changes
iOS user-facing changes
Android user-facing changes
Dev changes