[BUG] Fix #510: validate_owasp crashes on malformed JWT tokens#523
[BUG] Fix #510: validate_owasp crashes on malformed JWT tokens#523adityakryadav wants to merge 5 commits into
Conversation
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesJWT validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🧪 PR Test Results
Python 3.12 · commit 5f103f4 |
…ltiagent.py import sort
|
@adityakryadav is attempting to deploy a commit to the sreerevanth's projects Team on Vercel. A member of the Team first needs to authorize it. |
…flicts polymorphically
sreerevanth
left a comment
sreerevanth
left a comment
There was a problem hiding this comment.
Thanks for working on this! I think the fix should remain within the existing validate_owasp(events) API. This PR changes the function signature, mixes JWT validation with OWASP scanning, and changes the return type, which could break existing callers. Could you instead identify where malformed JWTs are causing the crash and handle that case while preserving the current API and return type?
|
@adityakryadav please reply bro either dc or here so i would know youre currently working |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
agentwatch/security/owasp.py (2)
136-136: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
DecodeErroris a subclass ofInvalidTokenError— catching both is redundant.In PyJWT 2.8.0,
DecodeErrorinherits fromInvalidTokenError, soexcept (ValueError, jwt.exceptions.InvalidTokenError)already coversDecodeError. This is harmless but slightly misleading.🤖 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 `@agentwatch/security/owasp.py` at line 136, Remove jwt.exceptions.DecodeError from the exception tuple in the surrounding exception handler, leaving ValueError and jwt.exceptions.InvalidTokenError handled by the existing except clause. Preserve the current error-handling behavior.
127-150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNested function
check_dict_keysis redefined on every loop iteration.
check_dict_keysis defined inside thefor event in eventsloop, so it's recreated on each iteration. Extract it to module level (or at least outside the loop) and passeventandscanas parameters.♻️ Suggested refactor: extract check_dict_keys
def validate_owasp(events: list[AgentEvent]) -> OwaspScan: scan = OwaspScan() for event in events: # Check for malformed JWT tokens in the event arguments if event.tool_call and event.tool_call.arguments: - def check_dict_keys(d: dict): - for k, v in d.items(): - if isinstance(v, str): - is_token_key = any(tk in k.lower() for tk in ["token", "jwt", "auth", "credential"]) - is_jwt_structure = re.match(r"^[A-Za-z0-9-_=]+\.[A-Za-z0-9-_=]+\.[A-Za-z0-9-_=]*$", v) is not None - if is_token_key or is_jwt_structure: - try: - import jwt - jwt.decode(v, options={"verify_signature": False}) - except (ValueError, jwt.exceptions.DecodeError, jwt.exceptions.InvalidTokenError) as exc: - if not any(f.event_id == event.event_id and f.detail.startswith("Malformed JWT token") for f in scan.findings): - scan.findings.append( - OwaspFinding( - vector=OwaspVector.TRUST_BOUNDARY, - severity="high", - detail=f"Malformed JWT token detected: {exc}", - event_id=event.event_id, - ) - ) - except Exception: # noqa: S110 - pass - elif isinstance(v, dict): - check_dict_keys(v) - check_dict_keys(event.tool_call.arguments) + _check_jwt_in_args(event.tool_call.arguments, event, scan) + return scan + + +def _check_jwt_in_args(d: dict, event: AgentEvent, scan: OwaspScan) -> None: + for k, v in d.items(): + if isinstance(v, str): + is_token_key = any(tk in k.lower() for tk in ["token", "jwt", "auth", "credential"]) + is_jwt_structure = re.match(r"^[A-Za-z0-9-_=]+\.[A-Za-z0-9-_=]+\.[A-Za-z0-9-_=]*$", v) is not None + if is_token_key or is_jwt_structure: + try: + import jwt + jwt.decode(v, options={"verify_signature": False}) + except (ValueError, jwt.exceptions.DecodeError, jwt.exceptions.InvalidTokenError) as exc: + if not any(f.event_id == event.event_id and f.detail.startswith("Malformed JWT token") for f in scan.findings): + scan.findings.append( + OwaspFinding( + vector=OwaspVector.TRUST_BOUNDARY, + severity="high", + detail=f"Malformed JWT token detected: {exc}", + event_id=event.event_id, + ) + ) + except Exception: # noqa: S110 + pass + elif isinstance(v, dict): + _check_jwt_in_args(v, event, scan) + elif isinstance(v, list): + for item in v: + if isinstance(item, dict): + _check_jwt_in_args(item, event, scan)🤖 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 `@agentwatch/security/owasp.py` around lines 127 - 150, Move the nested check_dict_keys function outside the for event in events loop, then update its signature to accept event and scan explicitly and pass those values through recursive calls. Preserve its existing JWT detection, finding deduplication, and nested-dictionary traversal behavior, and invoke it with each event’s tool-call arguments.
🤖 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 148-149: Update check_dict_keys to traverse list values in
addition to dict values, recursively invoking the existing dictionary inspection
for each dictionary item so JWTs nested in list-based structures such as headers
are detected.
- Around line 134-136: Update the JWT handling try/except around jwt.decode so
the exception tuple never references jwt when its inner import fails. Move the
jwt import outside the try that catches decode errors, or otherwise use guarded
exception handling, while preserving handling for ValueError, DecodeError, and
InvalidTokenError.
---
Nitpick comments:
In `@agentwatch/security/owasp.py`:
- Line 136: Remove jwt.exceptions.DecodeError from the exception tuple in the
surrounding exception handler, leaving ValueError and
jwt.exceptions.InvalidTokenError handled by the existing except clause. Preserve
the current error-handling behavior.
- Around line 127-150: Move the nested check_dict_keys function outside the for
event in events loop, then update its signature to accept event and scan
explicitly and pass those values through recursive calls. Preserve its existing
JWT detection, finding deduplication, and nested-dictionary traversal behavior,
and invoke it with each event’s tool-call arguments.
🪄 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: 2793e0ee-57bb-449b-88b4-333ca1af725c
📒 Files selected for processing (2)
agentwatch/security/owasp.pytests/test_owasp_security.py
…lled, support traversal in lists
sreerevanth
left a comment
sreerevanth
left a comment
There was a problem hiding this comment.
fix the ci its good to merge
2 participants
Summary
Closes #510.
Calling
validate_owaspwith a payload containing a malformed JWT token raised an unhandledValueError(orjwt.DecodeError), crashing the caller with a raw traceback.Changes
agentwatch/security/owasp.py— Addedvalidate_owasp(payload: dict) -> dict:tokenfield in the payload usingPyJWT(signature verification disabled — format-only check).ValueErrorand any other exception from the JWT library.{"status": "error", "error": "<message>"}on failure,{"status": "success"}on pass.How to test