fix(security): resolve OWASP scanner bypass via argument poisoning#89
fix(security): resolve OWASP scanner bypass via argument poisoning#89SHAURYASANYAL3 wants to merge 8 commits into
Conversation
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughThis PR adds recursive argument flattening to the OWASP scanner and comprehensive test coverage. The ChangesOWASP Security Argument Flattening
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agentwatch/security/owasp.py`:
- Around line 144-157: The _flatten_values method can infinite-recurse on cyclic
structures; modify it to track seen object ids and skip already-visited
containers: add an optional parameter (e.g., seen: set[int] | None = None)
inside _flatten_values, initialize seen if None, check id(data) and return [] if
already in seen, add id(data) to seen before recursing into
dicts/lists/tuples/sets, and pass seen through all recursive calls so
self-referential payloads are ignored and RecursionError is avoided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 058502d4-aac3-4c0e-9d00-8f2d8653fd1a
📒 Files selected for processing (8)
CONTRIBUTING.mdREADME.mdagentwatch/api/server.pyagentwatch/security/owasp.pydocs/adapters/claude-code.mddocs/adapters/langchain.mddocs/getting-started.mdtests/test_owasp_security.py
Adds issues: write permission required to post comments on pull requests.
sreerevanth
left a comment
sreerevanth
left a comment
There was a problem hiding this comment.
Great catch on the OWASP argument-poisoning bypass and the added test coverage.
Before merge, please address one blocker:
_flatten_values() performs recursive traversal without cycle detection. Self-referential dict/list structures can trigger infinite recursion and raise RecursionError, causing scans to fail unexpectedly.
Suggested approach:
Track visited object IDs during traversal.
Skip already-visited containers.
Preserve current behavior for normal nested payloads.
Once cycle protection is added, this looks ready for final review.
sreerevanth
added
bug
SHAURYASANYAL3
force-pushed
the
fix/owasp-argument-bypass
branch
from
85a2073 to
312848c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/test-on-pr.yml (1)
19-19: ⚡ Quick winPin third-party actions to commit SHAs.
actions/checkout@v4,actions/setup-python@v5, andactions/github-script@v7are pinned to mutable tags. Static analysis flags this against the repo's pinning policy, and pinning to a full commit SHA is especially important here because this workflow runs with write permissions. Pin eachuses:to a SHA (optionally with the version in a trailing comment).Also applies to: 27-27, 51-51
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test-on-pr.yml at line 19, Replace mutable action tags with immutable commit SHAs for every "uses:" entry shown (e.g., replace actions/checkout@v4, actions/setup-python@v5, and actions/github-script@v7) so the workflow uses exact commit SHAs; keep the human-readable version as an optional trailing comment if desired. Locate the three "uses:" occurrences in the workflow file (the ones currently referencing actions/checkout, actions/setup-python, and actions/github-script) and update each to the corresponding full commit SHA from the action's repository release commit, ensuring the SHA is the exact full-length commit hash.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test-on-pr.yml:
- Around line 4-5: The workflow uses the privileged pull_request_target trigger
while checking out and running PR-controlled code (e.g., actions/checkout of the
PR head, pip install -e ".[dev]", pytest, ruff), which allows token
exfiltration; fix it by splitting into two workflows: 1) a "PR Tests" workflow
triggered on pull_request (not pull_request_target) with permissions.contents:
read that checks out the PR, runs pip install -e ".[dev]", pytest and ruff, and
uploads results as an artifact; and 2) a privileged "PR Test Comment" workflow
triggered by workflow_run for the "PR Tests" workflow that never checks out PR
code, has permissions.pull-requests: write and issues: write, downloads the
uploaded artifact and posts the comment—ensure actions/checkout is removed from
the privileged workflow and artifacts are used to pass results.
- Around line 18-24: The checkout step uses actions/checkout@v4 with the default
credential persistence, which leaves GITHUB_TOKEN in .git/config; update the
Checkout step (actions/checkout@v4) to set persist-credentials: false to prevent
persisting credentials for this PR-test job since the job does not push and
should not expose GITHUB_TOKEN. Ensure the changed Checkout configuration is
applied where ref: ${{ github.event.pull_request.head.sha }} is used.
In `@tests/test_safety.py`:
- Around line 368-385: The test uses the typing symbol Any in annotations inside
test_flatten_values_cycle_detection (and other tests) but there is no import;
add the missing import "from typing import Any" to the test module so
annotations in test_flatten_values_cycle_detection, and any uses of Any
elsewhere, resolve and CI (F821) no longer fails; locate the
tests/test_safety.py file and insert the import near other typing imports or at
top of the file.
---
Nitpick comments:
In @.github/workflows/test-on-pr.yml:
- Line 19: Replace mutable action tags with immutable commit SHAs for every
"uses:" entry shown (e.g., replace actions/checkout@v4, actions/setup-python@v5,
and actions/github-script@v7) so the workflow uses exact commit SHAs; keep the
human-readable version as an optional trailing comment if desired. Locate the
three "uses:" occurrences in the workflow file (the ones currently referencing
actions/checkout, actions/setup-python, and actions/github-script) and update
each to the corresponding full commit SHA from the action's repository release
commit, ensuring the SHA is the exact full-length commit hash.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d8bb7c4-0ebb-480c-8e90-61e351c5d90f
📒 Files selected for processing (3)
.github/workflows/test-on-pr.ymlagentwatch/security/owasp.pytests/test_safety.py
🚧 Files skipped from review as they are similar to previous changes (1)
- agentwatch/security/owasp.py
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| ref: ${{ github.event.pull_request.head.sha }} | ||
| # Note: pull_request_target runs in the context of the base repo. | ||
| # Checking out the PR head is necessary to test the changes, | ||
| # but use caution as this runs with a write-capable GITHUB_TOKEN. |
There was a problem hiding this comment.
Disable credential persistence on checkout.
Since this job runs untrusted PR code under a privileged trigger, the default persist-credentials: true leaves the GITHUB_TOKEN in .git/config where the executed code can read and exfiltrate it. Set persist-credentials: false (you don't push from this job). This hardens the checkout even after restructuring the trigger.
🛡️ Proposed fix
- name: Checkout
uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.head.sha }}
+ persist-credentials: false🧰 Tools
🪛 zizmor (1.25.2)
[warning] 18-24: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 19-19: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test-on-pr.yml around lines 18 - 24, The checkout step
uses actions/checkout@v4 with the default credential persistence, which leaves
GITHUB_TOKEN in .git/config; update the Checkout step (actions/checkout@v4) to
set persist-credentials: false to prevent persisting credentials for this
PR-test job since the job does not push and should not expose GITHUB_TOKEN.
Ensure the changed Checkout configuration is applied where ref: ${{
github.event.pull_request.head.sha }} is used.
sreerevanth
left a comment
sreerevanth
left a comment
There was a problem hiding this comment.
❌ Changes Requested
Thanks for addressing the original blocker. The cycle-detection fix in _flatten_values() resolves the recursion concern and the security regression tests are appreciated.
Before merge, two additional issues need to be addressed:
- Critical workflow security issue
The workflow now uses pull_request_target while checking out and executing PR-controlled code (pip install, pytest, etc.). This introduces a privileged-code-execution path and is a higher-severity security concern than the OWASP scanner issue being fixed.
Please revert to a safe workflow design (e.g. unprivileged pull_request testing plus a separate reporting workflow) or otherwise eliminate execution of untrusted PR code under a privileged token context.
- Resolve merge conflicts
GitHub currently reports an unresolved conflict in:
- docs/adapters/langchain.md
Please rebase on the latest main, resolve conflicts, and push the updated branch.
Once the workflow security issue and merge conflict are resolved, this can be reviewed again.
2 participants
Summary
This PR fixes a security gap in
OwaspScannerwhere tool call arguments were not included in heuristic analysis. As a result, malicious payloads embedded within nested JSON structures could bypass safety checks that were only inspecting raw command strings.What Changed
Recursive Argument Flattening
_flatten_values()toOwaspScanner.Expanded Scan Coverage
_blob_of()to include flattened argument values in the generated scan payload.Improved Threat Detection
The scanner can now detect:
even when malicious content is embedded deep within structured tool arguments rather than exposed in the top-level command text.
Validation
Added the following test cases in
tests/test_owasp_security.py:test_owasp_detects_exfil_in_poisoned_argumentstest_owasp_detects_prompt_injection_in_argumentstest_owasp_detects_nested_unsafe_codeAll three tests pass with this change and fail against the previous implementation, confirming the vulnerability and validating the fix.
Summary by CodeRabbit
Release Notes
Tests
Refactor