-
Notifications
You must be signed in to change notification settings - Fork 69
feat: Enhanced secret scan no longer relies on env vars #6333
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
feat: Enhanced secret scan no longer relies on env vars #6333
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This pull request adds or modifies JavaScript ( |
This pull request adds or modifies JavaScript ( |
This pull request adds or modifies JavaScript ( |
// Note: Using the global flag (g) means this regex object maintains state between executions. | ||
// We would need to reset lastIndex to 0 if we wanted to reuse it on the same string multiple times. | ||
const likelySecretRegex = new RegExp( | ||
`(?:["'\`]|^|[=:,]) *(?:${prefixMatchingRegex})[^ "'\`=:,]{${MIN_CHARS_AFTER_PREFIX}}[^ "'\`=:,]*?(?:["'\`]|[ =:,]|$)`, |
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.
Could we use capturing group for token so that we don't have to do massaging after finding a match later in?
const token = match[0].replace(/^["'`=:, ]+|["'`=:, ]+$/g, '')
`(?:["'\`]|^|[=:,]) *(?:${prefixMatchingRegex})[^ "'\`=:,]{${MIN_CHARS_AFTER_PREFIX}}[^ "'\`=:,]*?(?:["'\`]|[ =:,]|$)`, | |
`(?:["'\`]|^|[=:,]) *(?<token>(?:${prefixMatchingRegex})[^ "'\`=:,]{${MIN_CHARS_AFTER_PREFIX}}[^ "'\`=:,]*?)(?:["'\`]|[ =:,]|$)`, |
And later
-let match
+let match: RegExpExecArray | null
while ((match = likelySecretRegex.exec(line)) !== null) {
- const token = match[0].replace(/^["'`=:, ]+|["'`=:, ]+$/g, '')
+ const token = match.groups?.token
This pull request adds or modifies JavaScript ( |
…y-on-value-being-an-env-var
This pull request adds or modifies JavaScript ( |
🎉 Thanks for submitting a pull request! 🎉
Summary
Fixes https://linear.app/netlify/issue/WRFL-2554/enhanced-scan-shouldnt-rely-on-value-being-an-env-var
The users most at risk of shipping secrets to production accidentally are folks who haven't configured env vars yet. This PR switches up the implementation of the enhanced secret scan so that we don't rely on env vars at all.
Previously: all env vars checked in case they are secrets, and just haven't been marked as 'secret' by the user
Now: we check file content directly for anything that looks like a secret (based on length and prefix), and if we find one we fail the build
This PR also:
ENHANCED_SECRETS_SCAN_OMIT_VALUES
andENHANCED_SECRETS_SCAN_ENABLED
to allow users to opt out of the new functionality or safelist values without sacrificing the explicit secrets env var checksFor us to review and ship your PR efficiently, please perform the following steps:
we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
something that`s on fire 🔥 (e.g. incident related), you can skip this step.
your code follows our style guide and passes our tests.
A picture of a cute animal (not mandatory, but encouraged)