Added required secret check file #3419
Conversation
|
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (20.45%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #3419 +/- ##
==========================================
- Coverage 32.57% 31.86% -0.72%
==========================================
Files 14 15 +1
Lines 703 747 +44
Branches 119 126 +7
==========================================
+ Hits 229 238 +9
- Misses 474 509 +35 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a GitHub Action workflow and CLI command to automatically detect fields with secret-like names (containing "key", "token", "secret", "password", or "code") that aren't using type: 'password' for proper security protection. The goal is to prevent accidentally exposing sensitive credentials in destination configurations.
Key changes:
- New CLI command
validate-secret-fieldsto scan destination settings and authentication fields for secret-like patterns - GitHub Actions workflow that runs on PRs, scans for secret fields, and posts warnings if any are found
- Ability to skip the check by marking the security review checkbox in the PR description
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| packages/cli/src/commands/validate-secret-fields.ts | New CLI command to identify fields with secret-like names that aren't marked as password type. Currently only checks destination-level settings/authentication, not action fields. |
| .github/workflows/required-secret-check.yml | GitHub Actions workflow that runs the CLI command and comments on PRs when potential secret fields are detected. Can be bypassed via PR description checkbox. |
| export default class ValidateSecretFields extends Command { | ||
| public static enableJsonFlag = true | ||
|
|
||
| static description = `Returns a list of secret fields for each action in the destination.` |
There was a problem hiding this comment.
The description states the command "Returns a list of secret fields for each action in the destination", but the current implementation only checks destination-level settings and authentication fields, not individual actions. The description should be updated to accurately reflect what the command does, or the implementation should be fixed to match the description.
| } | ||
|
|
||
| destinationMap[destination.name] = { settings: [] } | ||
| const secretFieldPattern = /(^|[^A-Za-z0-9])(key|token|secret|password|code)($|[^A-Za-z0-9])/i |
There was a problem hiding this comment.
The regex pattern /(^|[^A-Za-z0-9])(key|token|secret|password|code)($|[^A-Za-z0-9])/i will incorrectly flag common non-secret fields that contain "code" as a substring, such as:
postalCode(very common for address data)countryCodestatusCodecurrencyCode
The pattern should be more specific or exclude known false positives. Consider using word boundaries or a more selective pattern like /(api.?key|access.?token|auth.?token|secret|password|client.?secret)/i that targets actual secret field naming patterns.
| const secretFieldPattern = /(^|[^A-Za-z0-9])(key|token|secret|password|code)($|[^A-Za-z0-9])/i | |
| const secretFieldPattern = /^(api.?key|access.?token|auth.?token|secret|password|client.?secret)$/i |
| destinationMap[destination.name] = { | ||
| ...destinationMap[destination.name], | ||
| ...actionToSecretFields | ||
| } |
There was a problem hiding this comment.
This line overwrites the destinationMap[destination.name] object that was just populated with settings on line 82-88, replacing it with a shallow merge that includes the empty actionToSecretFields object. This results in the correct structure but is confusing and inefficient.
Instead, initialize the object once with the correct structure:
destinationMap[destination.name] = {
settings: [],
...actionToSecretFields
}Then populate the settings array in the loop.
| if: steps.pr_desc.outputs.skip != 'true' | ||
| uses: actions/github-script@v7 | ||
| with: | ||
| findings: ${{ steps.list.outputs.findings }} |
There was a problem hiding this comment.
The findings input is declared but never used in the script. The script retrieves findings via core.getInput("findings") on line 82, but GitHub Actions doesn't automatically pass declared inputs to the script context unless they're referenced in the with section as ${{ steps.list.outputs.findings }}.
The current approach works because step outputs are available via core.getInput(), but the findings: declaration on line 80 is misleading and should be removed to avoid confusion.
| findings: ${{ steps.list.outputs.findings }} |
| for (const [destination, obj] of Object.entries(parsed)) { | ||
| if (Array.isArray(obj.settings) && obj.settings.length > 0) { | ||
| findings.push({ | ||
| destination, | ||
| settings: obj.settings | ||
| }); | ||
| } |
There was a problem hiding this comment.
The workflow only checks for obj.settings but ignores action-level secret fields. Since the CLI command's description states it returns "secret fields for each action", the workflow should also iterate through action keys in the parsed output to detect secret fields in individual actions, not just destination settings.
This is related to the CLI bug where actions aren't being checked, but even if that's fixed, this workflow logic needs to be updated to handle action-level findings.
| for (const [destination, obj] of Object.entries(parsed)) { | |
| if (Array.isArray(obj.settings) && obj.settings.length > 0) { | |
| findings.push({ | |
| destination, | |
| settings: obj.settings | |
| }); | |
| } | |
| for (const [destination, obj] of Object.entries(parsed)) { | |
| // Check destination-level settings | |
| if (Array.isArray(obj.settings) && obj.settings.length > 0) { | |
| findings.push({ | |
| destination, | |
| settings: obj.settings | |
| }); | |
| } | |
| // Check action-level secret fields | |
| if (obj.actions && typeof obj.actions === "object") { | |
| for (const [actionName, actionObj] of Object.entries(obj.actions)) { | |
| if (Array.isArray(actionObj) && actionObj.length > 0) { | |
| findings.push({ | |
| destination, | |
| action: actionName, | |
| fields: actionObj | |
| }); | |
| } | |
| } | |
| } |
| with: | ||
| script: | | ||
| const pr = context.payload.pull_request; | ||
| const body = (pr?.body || "").toLowerCase(); |
There was a problem hiding this comment.
Converting the PR body to lowercase (line 36) but then testing against case-insensitive regex patterns (with the i flag) is redundant and can cause issues. The pattern on line 42 will never match because it checks for \*\* (literal asterisks for markdown bold) after lowercasing the text, but lowercase conversion doesn't affect asterisks.
Remove the .toLowerCase() call since all patterns already use the i flag for case-insensitive matching:
const body = pr?.body || "";| const body = (pr?.body || "").toLowerCase(); | |
| const body = pr?.body || ""; |
| }) | ||
|
|
||
| const destinationMap: Record<string, Record<string, string[]>> = {} | ||
| const actionToSecretFields: Record<string, string[]> = {} |
There was a problem hiding this comment.
The variable actionToSecretFields is declared but never populated with any values. According to line 10, the command should "Returns a list of secret fields for each action in the destination", but the code only checks settings/authentication fields and never iterates through destination.actions to check action fields. This means action-level fields with secret-like names will not be detected.
To fix this, add logic similar to list-required-fields.ts (lines 89-98) to iterate through actions and check their fields for secret patterns.
| const patterns = [ | ||
| /-\s*\[x\]\s*required secret checkout/i, | ||
| /required secret checkout\s*:\s*true/i, | ||
| // The new checkbox you provided (checked) |
There was a problem hiding this comment.
[nitpick] The comment on line 41 says "The new checkbox you provided (checked)" but doesn't clearly explain what this pattern is checking for. Consider updating the comment to be more descriptive, e.g.:
// Pattern to match the security review checkbox from the PR template| // The new checkbox you provided (checked) | |
| // Pattern to match the security review checkbox from the PR template (checked) |
|
|
||
| ${lines} | ||
|
|
||
| This PR cannot be merged until these are removed or converted to secure secrets. |
There was a problem hiding this comment.
The error message "This PR cannot be merged until these are removed or converted to secure secrets" is unclear and potentially misleading. The fields themselves aren't removed - they need to be marked with type: 'password' to properly protect sensitive data.
Consider a clearer message such as:
⚠️ **Action Required**: The following fields appear to contain sensitive data based on their names but are not using `type: 'password'`. Please review these fields and update them to use `type: 'password'` if they contain API keys, tokens, passwords, or other secrets.| This PR cannot be merged until these are removed or converted to secure secrets. | |
| ⚠️ **Action Required**: The following fields appear to contain sensitive data based on their names but are not using \`type: 'password'\`. Please review these fields and update them to use \`type: 'password'\` if they contain API keys, tokens, passwords, or other secrets. |
| const body = (pr?.body || "").toLowerCase(); | ||
|
|
||
| const patterns = [ | ||
| /-\s*\[x\]\s*required secret checkout/i, |
There was a problem hiding this comment.
[nitpick] The pattern /-\s*\[x\]\s*required secret checkout/i on line 39 doesn't match anything in the documented PR template patterns. This appears to be a legacy pattern that should be removed if it's no longer in use, or documented if it's still valid.
| /-\s*\[x\]\s*required secret checkout/i, |
itsarijitray
left a comment
itsarijitray
left a comment
There was a problem hiding this comment.
LGTM!
Make sure to resolve the field issues before merging this change.
Some of the copilot comments are also good. Try to incorporate them if possible.
Jira ticket: https://twilio-engineering.atlassian.net/browse/STRATCONN-6226
In this PR, I’ve added a GitHub Action check that scans for common security parameters, Failed the Github Action and raises a warning if any of those parameters are not of type password.
Testing
Include any additional information about the testing you have completed to
ensure your changes behave as expected. For a speedy review, please check
any of the tasks you completed below during your testing.
Security Review
Please ensure sensitive data is properly protected in your integration.
type: 'password'