fix: resolve all CodeRabbit vulnerabilities and harden protocol#18
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR implements Intent Bus v7.61 with ephemeral cryptographic claim tokens for securing job state mutations. Changes span the protocol, server implementation, worker scripts, tests, and documentation to enforce token-based mutations, strict HMAC canonicalization, and replay protection. ChangesIntent Bus v7.61 Claim Token Enforcement
🎯 4 (Complex) | ⏱️ ~75 minutes
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_server_basic.py (1)
83-260:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd tests for
POST /extend_claim/<iid>claim_tokenvalidation
tests/test_server_basic.py(andtests/test_server_advanced.py) has no coverage for/extend_claim.- The endpoint requires
claim_tokenin the POST JSON body and returns:
- 200 when the
claim_tokenmatches an active claim for the intent- 404 for an invalid/expired/overwritten
claim_token(updates only whenstatus='claimed'andclaim_tokenmatches)- 400 when
claim_tokenis missing (invalid_request: "Missing claim_token.")- Add the lifecycle test steps: claim an intent → extend with the extracted token (200) → extend with
"invalid_token"(404) → extend withoutclaim_token(400).🤖 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 `@tests/test_server_basic.py` around lines 83 - 260, Add tests covering POST /extend_claim/<iid> claim_token validation by mirroring the claim/fulfill lifecycle: in tests/test_server_basic.py (and similarly in tests/test_server_advanced.py) create an intent via client.post("/intent", headers={"X-API-KEY": TEST_API_KEY}, json=...), claim it via client.post("/claim?goal=...&namespace=...") to extract claim_token and intent_id, then POST to f"/extend_claim/{intent_id}" with JSON {"claim_token": <token>} and assert 200; next POST with {"claim_token":"invalid_token"} and assert 404; finally POST with no claim_token and assert 400 and that the JSON error message equals or contains "Missing claim_token."; follow the existing test patterns (e.g., test_publish_and_claim_lifecycle, test_claim_token_required) and reuse the client and TEST_API_KEY headers.
🧹 Nitpick comments (5)
worker.sh (1)
162-168: 💤 Low valueConsider using jq for JSON payload construction.
String interpolation for JSON payloads is fragile—if
$RESULTever contains quotes or backslashes (e.g., from dynamic task output), the JSON will be malformed or vulnerable to injection. The other workers (logger.sh,discord_worker.sh) correctly usejq -nfor safe construction.Currently safe since
RESULTis hardcoded, but this pattern invites bugs if the script is extended.♻️ Suggested fix using jq
- FULFILL_RESPONSE=$(curl -sS \ - --max-time 10 \ - -X POST "$BASE_URL/fulfill/$INTENT_ID" \ - -H "X-API-KEY: $API_KEY" \ - -H "Content-Type: application/json" \ - -d "{\"claim_token\":\"$CLAIM_TOKEN\",\"result\":\"$RESULT\",\"result_type\":\"$RESULT_TYPE\"}" \ - -w "\n%{http_code}") + FULFILL_PAYLOAD=$(jq -n \ + --arg t "$CLAIM_TOKEN" \ + --arg r "$RESULT" \ + --arg rt "$RESULT_TYPE" \ + '{claim_token:$t,result:$r,result_type:$rt}') + + FULFILL_RESPONSE=$(curl -sS \ + --max-time 10 \ + -X POST "$BASE_URL/fulfill/$INTENT_ID" \ + -H "X-API-KEY: $API_KEY" \ + -H "Content-Type: application/json" \ + -d "$FULFILL_PAYLOAD" \ + -w "\n%{http_code}")Note: This would require adding jq as a dependency (currently optional for this script).
🤖 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 `@worker.sh` around lines 162 - 168, The current curl payload construction in the FULFILL_RESPONSE assignment uses fragile string interpolation for JSON (the -d "{\"claim_token\":\"$CLAIM_TOKEN\",\"result\":\"$RESULT\",\"result_type\":\"$RESULT_TYPE\"}") which will break if $RESULT contains quotes/backslashes; update the code that builds the POST body for the curl call (the block that sets FULFILL_RESPONSE and posts to "$BASE_URL/fulfill/$INTENT_ID") to construct JSON safely using jq -n (e.g., build an object with jq -n --arg claim_token "$CLAIM_TOKEN" --arg result "$RESULT" --arg result_type "$RESULT_TYPE" '{claim_token:$claim_token, result:$result, result_type:$result_type}') and pipe that into curl via --data `@-` (or similar), and document that jq is required as a dependency for this script.tests/test_server_advanced.py (1)
101-101: ⚡ Quick winStrengthen the claim_token validation.
The assertion only checks for key presence but doesn't validate the token is a non-empty string, which is a security requirement.
🔒 Proposed fix to validate token value
assert res3.status_code == 200 assert res3.json["goal"] == "process_video" - assert "claim_token" in res3.json + claim_token = res3.json.get("claim_token") + assert isinstance(claim_token, str) and len(claim_token) > 0🤖 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 `@tests/test_server_advanced.py` at line 101, The test currently only checks the presence of "claim_token" in res3.json; update the assertion in the test (the assertion referencing res3 and the "claim_token" key) to also validate that res3.json["claim_token"] is a non-empty string (e.g., assert it's an instance of str and that len(res3.json["claim_token"].strip()) > 0 or truthy) so the token value is not empty or invalid.tests/test_server_basic.py (1)
109-111: ⚡ Quick winStrengthen the claim_token assertion.
The current assertion
assert claim_tokenonly checks truthiness, which would pass for empty strings or other falsy values. Consider validating both type and content.🔒 Proposed fix to strengthen validation
- claim_token = claim_res.json["claim_token"] - - assert claim_token + claim_token = claim_res.json.get("claim_token") + assert isinstance(claim_token, str) and len(claim_token) > 0🤖 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 `@tests/test_server_basic.py` around lines 109 - 111, The assertion for claim_token is too weak; update the test to validate that claim_res.json["claim_token"] is a non-empty string (and optionally matches the expected token format) rather than just truthy. Locate the usage of claim_token (variable claim_token and claim_res.json access) and replace the simple truthiness check with explicit checks such as asserting isinstance(claim_token, str) and asserting len(claim_token) > 0 (or a regex match if a specific token pattern is expected)..flake8 (2)
2-3: 💤 Low valueConfiguration clarification: E501 ignore makes max-line-length ineffective.
Setting
max-line-length = 120changes the threshold for E501 violations, but since E501 is in the ignore list, Flake8 won't enforce any line length limit. This configuration is contradictory.Choose one approach:
- To enforce 120-character lines: Remove
E501from the ignore list and keepmax-line-length = 120- To disable line length checking entirely: Remove the
max-line-lengthsetting and keepE501in ignore🤖 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 @.flake8 around lines 2 - 3, The Flake8 config currently ignores E501 while also setting max-line-length = 120, which is contradictory; decide whether to enforce or disable line-length checks and update the .flake8 accordingly: either remove E501 from the ignore list so max-line-length = 120 is applied, or remove the max-line-length setting and keep E501 in ignore; update the ignore line (E501) or the max-line-length entry to reflect your chosen policy.
4-4: ⚡ Quick winConsider linting test files to maintain code quality.
Excluding
tests/from linting can hide code quality issues in test code. Tests should follow the same coding standards as production code for maintainability and readability.♻️ Suggested fix
-exclude = .git,__pycache__,venv,env,.venv,tests/ +exclude = .git,__pycache__,venv,env,.venv🤖 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 @.flake8 at line 4, The .flake8 configuration currently excludes tests/ from linting via the "exclude" setting; remove the tests/ entry from the exclude value in .flake8 so test files are linted (e.g., change the exclude list to omit tests/), then run flake8 against the repo to confirm no new violations; update any test-specific lint exceptions via per-file-ignores in .flake8 if necessary rather than excluding the entire tests/ directory.
🤖 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 `@Examples/discord_worker.sh`:
- Around line 1-6: The script uses a POSIX shebang (#!/bin/sh) but also uses the
Bash-only setting "set -uo pipefail"; either change the shebang to a Bash
interpreter (e.g., update the shebang line to /bin/bash) or make the settings
POSIX-safe by removing "pipefail" and keeping "set -uo" (or implement a portable
pipe-failure workaround); locate the lines containing the shebang and the "set
-uo pipefail" statement in Examples/discord_worker.sh (look for the exact token
"set -uo pipefail") and apply one of these two fixes so the script runs on the
intended shell.
In `@flask_app.py`:
- Around line 1466-1468: The SQL predicate currently grants a 2-second
post-expiry grace by comparing COALESCE(claim_expires_at, claimed_at + ?) > (? -
2); remove that subtraction so the condition strictly requires
COALESCE(claim_expires_at, claimed_at + ?) > ? (i.e., compare directly to
now()). Update both occurrences referenced by the /fail and /fulfill handlers
(the queries using iid, g.api_key, claim_token, DEFAULT_CLAIM_TIMEOUT, now()) so
the SQL and its parameter list remain consistent but no longer subtract 2
seconds.
- Around line 251-255: The current auth logic returns False when an
Authorization header starts with "Bearer " but the token check fails, preventing
admin_auth_ok() from being considered; change the flow in the block that reads
auth_header (and uses METRICS_TOKEN and hmac.compare_digest) so that if
auth_header startswith "Bearer " you return True when the token matches and
otherwise do not short-circuit—call and return admin_auth_ok() as the fallback;
ensure you still respect the METRICS_TOKEN truthiness check when comparing the
token.
In `@logger.sh`:
- Around line 1-2: The script uses a non-POSIX option: replace or fix the
shebang/flags around the line with "set -uo pipefail" in logger.sh; either
change the shebang from "/bin/sh" to "/bin/bash" and update the accompanying
comment on lines describing the shell, or remove "pipefail" and keep a strictly
POSIX-safe "set -uo" invocation so the script runs under dash/ash—modify the
"set -uo pipefail" invocation accordingly and update the script header comment
to reflect the chosen shell.
In `@tests/test_server_advanced.py`:
- Around line 55-350: Add a new test (e.g., test_cross_attempt_token_isolation)
that publishes an intent via POST /intent, claims it once via POST /claim and
saves claim_token as token1, fails it via POST /fail to return it to the queue,
claims it again via POST /claim and saves claim_token as token2, then attempt to
fulfill/complete the intent using token1 and assert the server returns 404 (or
an appropriate token-not-found response) and does not succeed, and finally
fulfill using token2 and assert success; reference the endpoints and JSON fields
used in existing tests (POST /intent, POST /claim, POST /fail, claim_token) and
ensure the assertions mirror the style in test_dead_letters_and_backoff (status
codes and response body checks).
- Around line 247-294: The test must assert that each claim produces a fresh
ephemeral token: after obtaining token1 from the first claim (claim1) and token2
from the second claim (claim2), add an assertion that token1 != token2 to ensure
tokens are unique between attempts; reference the claim response parsing
(token1, token2) and the fail handlers (fail1, fail2) so the assertion is placed
after token2 is extracted and before using token2 in the second fail call.
In `@tests/test_server_basic.py`:
- Line 165: The test currently allows 403 for a missing claim_token but the
/fulfill/<iid> endpoint returns a 400 via api_error("invalid_request", "Missing
claim_token."); update the assertion in test_claim_token_required to assert
fulfill_res.status_code == 400 (and optionally assert
fulfill_res.json()["error"] == "invalid_request" and the message contains
"Missing claim_token.") so the test matches the actual behavior.
---
Outside diff comments:
In `@tests/test_server_basic.py`:
- Around line 83-260: Add tests covering POST /extend_claim/<iid> claim_token
validation by mirroring the claim/fulfill lifecycle: in
tests/test_server_basic.py (and similarly in tests/test_server_advanced.py)
create an intent via client.post("/intent", headers={"X-API-KEY": TEST_API_KEY},
json=...), claim it via client.post("/claim?goal=...&namespace=...") to extract
claim_token and intent_id, then POST to f"/extend_claim/{intent_id}" with JSON
{"claim_token": <token>} and assert 200; next POST with
{"claim_token":"invalid_token"} and assert 404; finally POST with no claim_token
and assert 400 and that the JSON error message equals or contains "Missing
claim_token."; follow the existing test patterns (e.g.,
test_publish_and_claim_lifecycle, test_claim_token_required) and reuse the
client and TEST_API_KEY headers.
---
Nitpick comments:
In @.flake8:
- Around line 2-3: The Flake8 config currently ignores E501 while also setting
max-line-length = 120, which is contradictory; decide whether to enforce or
disable line-length checks and update the .flake8 accordingly: either remove
E501 from the ignore list so max-line-length = 120 is applied, or remove the
max-line-length setting and keep E501 in ignore; update the ignore line (E501)
or the max-line-length entry to reflect your chosen policy.
- Line 4: The .flake8 configuration currently excludes tests/ from linting via
the "exclude" setting; remove the tests/ entry from the exclude value in .flake8
so test files are linted (e.g., change the exclude list to omit tests/), then
run flake8 against the repo to confirm no new violations; update any
test-specific lint exceptions via per-file-ignores in .flake8 if necessary
rather than excluding the entire tests/ directory.
In `@tests/test_server_advanced.py`:
- Line 101: The test currently only checks the presence of "claim_token" in
res3.json; update the assertion in the test (the assertion referencing res3 and
the "claim_token" key) to also validate that res3.json["claim_token"] is a
non-empty string (e.g., assert it's an instance of str and that
len(res3.json["claim_token"].strip()) > 0 or truthy) so the token value is not
empty or invalid.
In `@tests/test_server_basic.py`:
- Around line 109-111: The assertion for claim_token is too weak; update the
test to validate that claim_res.json["claim_token"] is a non-empty string (and
optionally matches the expected token format) rather than just truthy. Locate
the usage of claim_token (variable claim_token and claim_res.json access) and
replace the simple truthiness check with explicit checks such as asserting
isinstance(claim_token, str) and asserting len(claim_token) > 0 (or a regex
match if a specific token pattern is expected).
In `@worker.sh`:
- Around line 162-168: The current curl payload construction in the
FULFILL_RESPONSE assignment uses fragile string interpolation for JSON (the -d
"{\"claim_token\":\"$CLAIM_TOKEN\",\"result\":\"$RESULT\",\"result_type\":\"$RESULT_TYPE\"}")
which will break if $RESULT contains quotes/backslashes; update the code that
builds the POST body for the curl call (the block that sets FULFILL_RESPONSE and
posts to "$BASE_URL/fulfill/$INTENT_ID") to construct JSON safely using jq -n
(e.g., build an object with jq -n --arg claim_token "$CLAIM_TOKEN" --arg result
"$RESULT" --arg result_type "$RESULT_TYPE" '{claim_token:$claim_token,
result:$result, result_type:$result_type}') and pipe that into curl via --data
`@-` (or similar), and document that jq is required as a dependency for this
script.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f27a24a-7a1e-4f51-a3c2-0578ad9c2626
📒 Files selected for processing (15)
.flake8CHANGELOG.mdCONTRIBUTING.mdExamples/README.mdExamples/discord_worker.shREADME.mdSECURITY.mdSPEC.mdWORKER_SECURITY.mdflask_app.pylogger.shserver-requirements.txttests/test_server_advanced.pytests/test_server_basic.pyworker.sh
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 5 file(s) based on 7 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 5 file(s) based on 7 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Security
Documentation
Tests