fix(attack): error handling for secrets exfiltration #238
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| else: | ||
| Output.error("Unexpected run artifact structure!") | ||
| for k, v in extracted.items(): | ||
| print(f"{k}={v}") |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
In general, to fix clear-text logging of secrets, you must avoid printing or logging the actual secret values. Options include: omitting them entirely, logging only metadata (like key names), or logging redacted forms (such as lengths or masked strings). The goal is that any logs or captured stdout do not contain usable secret material, while still providing helpful information to the user.
For this specific code in gatox/attack/secrets/secrets_attack.py, the problematic behavior is the print(f"{k}={v}") inside the loop over extracted.items(). We can keep informing the user which secrets were exfiltrated and that secret values are available, without printing their contents. A minimal, behavior-preserving change is to print the secret name and a placeholder/redacted indication instead of the raw value, e.g. print(f"{k}=<redacted>") or include the length of v if useful. This change alters only line 262 and keeps the rest of the flow intact.
Concretely:
- In the
elseblock that currently doesfor k, v in extracted.items(): print(f"{k}={v}"), replace the print statement with a version that:- Does not include
vin clear text. - Optionally includes
len(str(v))or some basic info that isn’t itself secret.
- Does not include
- No new imports are needed; we just change the format string.
- All related alert variants are fixed simultaneously because they all refer to the same sink.
| @@ -259,7 +259,7 @@ | ||
| ) | ||
| else: | ||
| for k, v in extracted.items(): | ||
| print(f"{k}={v}") | ||
| print(f"{k}=<redacted>") | ||
| if delete_action and ( | ||
| not finegrain_scopes or "actions:write" in finegrain_scopes | ||
| ): |
There was a problem hiding this comment.
Pull request overview
This PR enhances error handling for GitHub Actions secrets exfiltration by addressing edge cases where workflow artifacts may not be immediately available and when no extractable secrets are present despite enumeration showing accessible secrets.
Key Changes:
- Implements retry logic with 30-second timeout for artifact retrieval to handle asynchronous availability
- Adds informative warning when only the default
github_tokenis extracted, indicating org secrets limitations - Refactors conditional logic to improve code clarity by eliminating nested if-else statements
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not extracted: | ||
| Output.warn( | ||
| "No secrets were extracted. Org secrets are not " | ||
| "accessible to public repos on the free plan." |
There was a problem hiding this comment.
The error message states that "Org secrets are not accessible to public repos on the free plan" but according to the PR description, this issue occurs with private repositories attached to organizations with a free plan. The message should be updated to accurately reflect that this limitation applies to private repos on free organization plans, not just public repos.
| "accessible to public repos on the free plan." | |
| "accessible to private repos on free organization plans." |
| for _ in range(30): | ||
| res = await self.api.retrieve_workflow_artifact( | ||
| target_repo, workflow_id | ||
| ) | ||
| if res and "output_updated.json" in res and "lookup.txt" in res: | ||
| break | ||
| await asyncio.sleep(1) |
There was a problem hiding this comment.
The retry loop uses a hard-coded 30 second timeout (30 iterations × 1 second sleep). Consider making this configurable or adding a comment explaining why 30 seconds is appropriate. This could lead to a poor user experience if artifacts consistently fail to upload or if they typically become available much sooner.
| if not extracted: | ||
| Output.warn( | ||
| "No secrets were extracted. Org secrets are not " | ||
| "accessible to public repos on the free plan." | ||
| ) |
There was a problem hiding this comment.
The new warning message for empty secrets (lines 255-259) lacks test coverage. Consider adding a test case that verifies this warning is displayed when only the github_token is present in the extracted secrets, to ensure the filtering and warning logic works as expected.
| for _ in range(30): | ||
| res = await self.api.retrieve_workflow_artifact( | ||
| target_repo, workflow_id | ||
| ) | ||
| if res and "output_updated.json" in res and "lookup.txt" in res: | ||
| break | ||
| await asyncio.sleep(1) |
There was a problem hiding this comment.
The artifact retrieval retry logic (lines 227-233) lacks test coverage. Consider adding test cases that verify: 1) successful retrieval after retries, 2) timeout after all retries are exhausted, and 3) immediate success on first attempt. This ensures the retry mechanism works correctly in different scenarios.
Pull Request Overview
Inform user of reason for empty secrets if repo shows accessible org secrets but the repository is private and attached to an organization with a free plan.
What does this PR add/solve?
Clarify behavior.
Type of change
Areas affected
Testing Steps
Manual Testing
Automated Testing
Testing Commands
Security Considerations
Documentation
Checklist
Additional Notes
Screenshots (if applicable)
Performance Impact
Breaking Changes
Related Issues/PRs
Closes #
Related to #