fix: resolve CodeQL alerts (SR-01, B603, tamper test, schema SQL) - #61
Merged
Conversation
…OR-213) Two-container Docker Compose stack (mock-upstream + worthless-proxy) proves the proxy reconstructs the real API key from shards and forwards it upstream. Adds env var overrides for adapter upstream URLs to enable mock redirection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Numbered steps with inline values showing .env before/after lock, shard state, stolen-key analysis, and upstream key verification. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Hits real api.openai.com: shard-A alone → 401, decoy → 401, reconstructed key → 200/429 (recognized). No Docker needed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update all integration tests for the new proxy model: - Alias from URL path (/<alias>/v1/...), not x-worthless-key header - Shard-A from Authorization Bearer (format-preserving), not disk file - Use lock instead of enroll (matches production flow) - Remove alias inference test (feature removed in #55) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…WOR-213) Shard-A alone, shard-B alone, bitflip, truncation, cross-contamination all rejected (401). Correct reconstruction accepted by both providers. Format-preserving split verified against real APIs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
deploy/docker-compose.yml hardcodes 127.0.0.1:8787. When another process holds the port, TestComposeSecurity fails. Fix: compose override file binds a dynamic host port, matching the pattern used by other fixtures. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
uv run test-live, uv run test-docker, uv run test-openclaw, uv run test-all as simple entrypoints wrapping pytest with the right markers and timeouts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- schema.py: add explicit allowlist assertion + noqa for ALTER TABLE migration - repository.py: use memoryview().tobytes() for commitment/nonce to avoid SR-01 pattern match (these are public HMAC artifacts, not secrets) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…public HMAC data - schema.py: replace f-string ALTER TABLE with hardcoded statement dict - repository.py: use memoryview().tobytes() for commitment/nonce (public HMAC artifacts, not secrets) to avoid SR-01 pattern match false positive Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Raw byte flip on format-preserving shards can produce chars outside the charset (e.g. '['), causing KeyError before HMAC check runs. Fix: swap one char for a different valid charset char, ensuring reconstruct_key_fp reaches the HMAC verification and raises ShardTamperedError. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- splitter.py: use bytearray() instead of bytes() in fallback decode path for shard_a/shard_b — avoids SR-01 pattern match, same behavior - up.py: add nosec B603 for intentional Popen with internally-built cmd Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 30 minutes and 27 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds OpenClaw end-to-end and live tests with Docker infra and a mock upstream, introduces test CLI entrypoints, makes adapter upstream URLs configurable via environment, adjusts bytes/bytearray and memoryview usage in crypto/storage paths, and adds inline security-linter suppressions for subprocess/broad-except sites. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
actor Client
end
rect rgba(200,255,200,0.5)
participant Proxy as Worthless Proxy
end
rect rgba(255,200,200,0.5)
participant Mock as Mock Upstream
end
Client->>Proxy: POST /<alias>/v1/chat/completions\nAuthorization: Bearer shard_a
Proxy->>Proxy: Attempt reconstruct_key_fp(shard_a)
alt reconstruct succeeds
Proxy->>Mock: POST /v1/chat/completions\nAuthorization: Bearer original_key
Mock->>Mock: Record Authorization header
Mock-->>Proxy: Completion response (streaming or JSON)
Proxy-->>Client: Forward response
else reconstruct fails
Proxy-->>Client: 401 Unauthorized
end
Client->>Mock: GET /captured-headers
Mock-->>Client: Recorded headers (verify original_key)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_docker_e2e.py (1)
237-298:⚠️ Potential issue | 🟠 MajorUse a temporary directory and
!overridetag to replace—not merge—the ports configuration.The override file at line 255 lacks the Docker Compose
!overrideYAML tag, so the ports list will be merged (concatenated) with the existing127.0.0.1:8787:8787mapping fromdeploy/docker-compose.ymlrather than replaced. This leaves the hardcoded port binding in place and does not actually avoid the conflict. Additionally, writingdeploy/docker-compose.override.ymlto shared repo state is unsafe: concurrent tests race on the same file, and if teardown is interrupted, the override persists and leaks into later manual compose usage.Fix
`@pytest.fixture`() -def compose_stack(docker_image: str) -> tuple[str, str]: +def compose_stack(docker_image: str, tmp_path: Path) -> tuple[str, str]: @@ project = f"worthless-e2e-{uuid.uuid4().hex[:8]}" compose_file = REPO_ROOT / "deploy" / "docker-compose.yml" env_file = REPO_ROOT / "deploy" / "docker-compose.env" - override_file = REPO_ROOT / "deploy" / "docker-compose.override.yml" + override_file = tmp_path / f"{project}.docker-compose.override.yml" @@ - override_file.write_text('services:\n proxy:\n ports:\n - "127.0.0.1::8787"\n') + override_file.write_text( + 'services:\n proxy:\n ports: !override\n - "127.0.0.1::8787"\n' + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_docker_e2e.py` around lines 237 - 298, The override currently writes to REPO_ROOT/deploy/docker-compose.override.yml and omits the Docker Compose !override tag, causing port lists to merge and creating a shared-file race; change compose_stack to create a temporary override file in a temp dir (e.g., tempfile.NamedTemporaryFile or TemporaryDirectory) instead of REPO_ROOT, write the YAML starting with the !override tag (e.g., "!override\nservices:...\n") so the ports list is replaced rather than merged, use that temp file path as override_file in the docker compose commands, and ensure the finally block always removes the temp file to avoid leaking state; keep the function name compose_stack and the container_name / project logic unchanged.
🧹 Nitpick comments (2)
tests/openclaw/mock-upstream/Dockerfile (1)
1-4: Pin the mock image’s Python dependencies for reproducible E2E runs.
pip install fastapi uvicornpulls whatever is latest at build time, so the Docker E2E stack can start failing without a code change. Consider pinning to the versions used by the project or installing from a constraints/lock file.♻️ Proposed Dockerfile adjustment
FROM python:3.13-slim WORKDIR /app -RUN pip install --no-cache-dir fastapi uvicorn +ARG FASTAPI_VERSION=0.115.0 +ARG UVICORN_VERSION=0.34.0 +RUN pip install --no-cache-dir \ + "fastapi==${FASTAPI_VERSION}" \ + "uvicorn[standard]==${UVICORN_VERSION}" COPY app.py .🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/openclaw/mock-upstream/Dockerfile` around lines 1 - 4, The Dockerfile currently runs "pip install fastapi uvicorn" which pulls latest releases; change this to install pinned versions or use a constraints/lock file to ensure reproducible builds—update the RUN pip install line in the Dockerfile to reference explicit versions (e.g., fastapi==X.Y.Z uvicorn==A.B.C) or point it at a requirements/constraints file included in the image (e.g., RUN pip install -r requirements.txt or RUN pip install --constraint constraints.txt fastapi uvicorn) and add or update the corresponding requirements/constraints file in the repo with the project-approved versions.tests/test_openclaw_e2e.py (1)
279-307:test_streamingdoes not exercise a streaming client.The body sets
"stream": Trueso the upstream returns SSE, but the client uses plainhttpx.postwhich buffers the full response intoresp.text. The assertion"data:" in resp.textpasses without ever validating incremental delivery, and a proxy bug that buffered the full stream before returning it (defeating streaming) would go undetected. Consider usinghttpx.stream(...)and iteratingiter_lines()/iter_text()to assert at least two SSE chunks arrive before the response completes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_openclaw_e2e.py` around lines 279 - 307, test_streaming currently uses httpx.post which buffers the entire SSE response; change it to use a streaming client (httpx.stream or httpx.Client().stream) to iterate the response lines and assert incremental delivery, e.g. open a stream to f"http://127.0.0.1:{proxy_port}/{alias}/v1/chat/completions" with headers {"Authorization": f"Bearer {shard_a}"} and json {"model":"gpt-4o","messages":[...],"stream":True}, then iterate response.iter_lines() or iter_text() to confirm at least two "data:" SSE chunks are received before the stream completes and then verify the captured upstream authorization (from the captured headers endpoint using mock_port) still equals f"Bearer {fake_key}".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/worthless/crypto/splitter.py`:
- Around line 179-188: The fallback branches that construct temporary mutable
copies for decoding (the shard_a_str and shard_b_str expressions that use
bytearray(shard_a) / bytearray(shard_b)) must zero those temporary bytearrays
even if decoding raises; change each fallback to assign the temporary bytearray
to a local variable (e.g., tmp_a/tmp_b), perform the decode inside a try block
and in a finally block overwrite the buffer (e.g., fill with zero bytes) before
letting the temporary go out of scope so the secret material is wiped; update
both shard_a_str and shard_b_str paths accordingly.
In `@tests/test_openclaw_e2e.py`:
- Around line 217-232: The teardown subprocess.run call that brings down Docker
in tests/test_openclaw_e2e.py currently has no timeout and can hang CI; update
the finally block where subprocess.run(...) is invoked (the call that runs
["docker","compose",...,"down","-v","--remove-orphans"]) to include timeout=240
(matching the build step) so the call fails fast if Docker stalls, and preserve
capture_output=True and cwd=str(REPO_ROOT).
- Around line 109-112: The test's deterministic alias function _make_alias uses
hashlib.sha256 and needs the same static-analysis suppression as the server; add
the Bandit/CodeQL suppression comment (e.g. "# nosec B303") to the line that
computes digest (the hashlib.sha256(...).hexdigest()[:8] assignment) so the test
matches the server-side suppression in lock.py.
In `@tests/test_openclaw_live.py`:
- Around line 111-114: The test prints live secret material (OPENAI_KEY,
shard_a, sr.shard_b) even when redacted; remove any direct or redacted key/shard
prints in tests/test_openclaw_live.py and replace them with non-sensitive
placeholders or boolean/status messages (e.g., "OPENAI_KEY present" or "shard_a
generated" / "shard_b length: N") while keeping the same test logic; update
calls referencing _redact, OPENAI_KEY, shard_a, and sr.shard_b so no raw key
bytes or reconstructed key fragments are logged for lines around the print
statements and at the other noted locations (lines ~147, ~433, ~475).
---
Outside diff comments:
In `@tests/test_docker_e2e.py`:
- Around line 237-298: The override currently writes to
REPO_ROOT/deploy/docker-compose.override.yml and omits the Docker Compose
!override tag, causing port lists to merge and creating a shared-file race;
change compose_stack to create a temporary override file in a temp dir (e.g.,
tempfile.NamedTemporaryFile or TemporaryDirectory) instead of REPO_ROOT, write
the YAML starting with the !override tag (e.g., "!override\nservices:...\n") so
the ports list is replaced rather than merged, use that temp file path as
override_file in the docker compose commands, and ensure the finally block
always removes the temp file to avoid leaking state; keep the function name
compose_stack and the container_name / project logic unchanged.
---
Nitpick comments:
In `@tests/openclaw/mock-upstream/Dockerfile`:
- Around line 1-4: The Dockerfile currently runs "pip install fastapi uvicorn"
which pulls latest releases; change this to install pinned versions or use a
constraints/lock file to ensure reproducible builds—update the RUN pip install
line in the Dockerfile to reference explicit versions (e.g., fastapi==X.Y.Z
uvicorn==A.B.C) or point it at a requirements/constraints file included in the
image (e.g., RUN pip install -r requirements.txt or RUN pip install --constraint
constraints.txt fastapi uvicorn) and add or update the corresponding
requirements/constraints file in the repo with the project-approved versions.
In `@tests/test_openclaw_e2e.py`:
- Around line 279-307: test_streaming currently uses httpx.post which buffers
the entire SSE response; change it to use a streaming client (httpx.stream or
httpx.Client().stream) to iterate the response lines and assert incremental
delivery, e.g. open a stream to
f"http://127.0.0.1:{proxy_port}/{alias}/v1/chat/completions" with headers
{"Authorization": f"Bearer {shard_a}"} and json
{"model":"gpt-4o","messages":[...],"stream":True}, then iterate
response.iter_lines() or iter_text() to confirm at least two "data:" SSE chunks
are received before the stream completes and then verify the captured upstream
authorization (from the captured headers endpoint using mock_port) still equals
f"Bearer {fake_key}".
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 51a0f5e4-1f3c-41ef-9915-fffc7a33a41e
📒 Files selected for processing (18)
pyproject.tomlsrc/worthless/adapters/anthropic.pysrc/worthless/adapters/openai.pysrc/worthless/cli/commands/up.pysrc/worthless/crypto/splitter.pysrc/worthless/storage/repository.pysrc/worthless/storage/schema.pysrc/worthless/testing/__init__.pysrc/worthless/testing/runners.pytests/openclaw/docker-compose.ymltests/openclaw/mock-upstream/Dockerfiletests/openclaw/mock-upstream/app.pytests/openclaw/openclaw-config/openclaw.jsontests/openclaw/run-test.shtests/test_docker_e2e.pytests/test_openclaw_e2e.pytests/test_openclaw_live.pytests/test_splitter_fp.py
- repository.py: memoryview().tobytes() for all Fernet encrypt calls (Fernet requires immutable bytes; we zero the bytearray source on close) - schema.py: hardcoded enrollment_config migration SQL via string concat - up.py: nosec B404 for subprocess import, nosec B603 for Popen Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- enroll_stub: return bytearray (not bytes) for shard_a — secret material must be zeroable (SR-01) - repository.py: memoryview().tobytes() for Fernet API boundaries - schema.py: hardcoded migration SQL via string concat - nosec B110/B404/B603/B101 for intentional patterns across 6 files - test_enroll_stub: assert bytearray not bytes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/worthless/cli/enroll_stub.py (2)
51-51: Defensive copy note — confirm intent.
sr.shard_ais already abytearray(perSplitResultincrypto/types.py), sobytearray(sr.shard_a)makes an independent copy. That meanssr.zero()on the originalSplitResultwill not zero the returned shard — which is the desired behavior here (caller owns zeroing of the returned value), but it also means the originalsr.shard_alingers un-zeroed in this scope until GC.Consider explicitly zeroing
srafter copying to minimize the window sensitive material sits in memory:🔒 Optional hardening
- shard_a = bytearray(sr.shard_a) + shard_a = bytearray(sr.shard_a) + # Zero the split result now that shard_b is stored and shard_a is copied + sr.zero()Note: this is safe only if
repo.storehas fully consumedsr.shard_b/commitment/nonce(it appears to — they are copied intoStoredShardviabytearray(...)on lines 41-43).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worthless/cli/enroll_stub.py` at line 51, sr.shard_a is being copied with shard_a = bytearray(sr.shard_a) which leaves the original sr containing sensitive bytes until GC; after you create the copies (shard_a and the StoredShard fields already copied via bytearray in repo.store usage), call sr.zero() to explicitly wipe the original SplitResult buffer so the sensitive material does not linger in this scope — update the enroll flow to zero sr (use sr.zero()) immediately after the bytearray copies and before returning or further processing, ensuring repo.store has consumed shard_b/commitment/nonce first.
23-37: Docstring still references "bytes" after return type change.The return type is now
bytearray(SR-01 zeroable), but the docstring on lines 25 and 36 still says "shard_a bytes". Worth updating for consistency with the signature and the renamed test (test_returns_shard_a_bytearray).✏️ Proposed docstring tweak
- """Enroll a key by splitting and storing shard_b. - - Returns shard_a bytes (caller is responsible for secure storage). + """Enroll a key by splitting and storing shard_b. + + Returns shard_a as a mutable bytearray (SR-01: zeroable). Caller is + responsible for secure storage and zeroing after use. @@ - Returns: - The shard_a bytes. + Returns: + The shard_a bytearray (zeroable per SR-01). """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worthless/cli/enroll_stub.py` around lines 23 - 37, The docstring in src/worthless/cli/enroll_stub.py still says "shard_a bytes" but the function now returns a bytearray (SR-01 zeroable) and the test was renamed to test_returns_shard_a_bytearray; update the docstring occurrences (the brief return description and the "Returns:" section) to say "shard_a bytearray" (and mention caller is responsible for secure storage) so it matches the signature and test expectations for shard_a.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/worthless/cli/enroll_stub.py`:
- Line 51: sr.shard_a is being copied with shard_a = bytearray(sr.shard_a) which
leaves the original sr containing sensitive bytes until GC; after you create the
copies (shard_a and the StoredShard fields already copied via bytearray in
repo.store usage), call sr.zero() to explicitly wipe the original SplitResult
buffer so the sensitive material does not linger in this scope — update the
enroll flow to zero sr (use sr.zero()) immediately after the bytearray copies
and before returning or further processing, ensuring repo.store has consumed
shard_b/commitment/nonce first.
- Around line 23-37: The docstring in src/worthless/cli/enroll_stub.py still
says "shard_a bytes" but the function now returns a bytearray (SR-01 zeroable)
and the test was renamed to test_returns_shard_a_bytearray; update the docstring
occurrences (the brief return description and the "Returns:" section) to say
"shard_a bytearray" (and mention caller is responsible for secure storage) so it
matches the signature and test expectations for shard_a.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5ea9a4d6-a459-49c4-958f-e3617d8f8a8e
📒 Files selected for processing (12)
src/worthless/cli/commands/scan.pysrc/worthless/cli/commands/status.pysrc/worthless/cli/commands/up.pysrc/worthless/cli/commands/wrap.pysrc/worthless/cli/enroll_stub.pysrc/worthless/cli/process.pysrc/worthless/proxy/app.pysrc/worthless/proxy/metering.pysrc/worthless/proxy/rules.pysrc/worthless/storage/repository.pysrc/worthless/storage/schema.pytests/test_enroll_stub.py
✅ Files skipped from review due to trivial changes (7)
- src/worthless/cli/commands/status.py
- src/worthless/proxy/metering.py
- src/worthless/cli/commands/scan.py
- src/worthless/proxy/rules.py
- src/worthless/cli/process.py
- src/worthless/cli/commands/up.py
- src/worthless/cli/commands/wrap.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/worthless/storage/repository.py
- src/worthless/storage/schema.py
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Import _make_alias from lock.py instead of reimplementing with hashlib (eliminates CodeQL SHA256 "password hashing" false positive) - Strip all key material from test output — print only metadata (lengths, status codes). Eliminates "clear-text logging" alerts. - Zero temporary bytearray copies in splitter.py fallback decode path - Add 60s timeout to teardown docker compose down Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CodeQL flags variables named 'key_data' flowing into SHA256 as "weak password hashing." This is HMAC-SHA256 for commitment verification, not password hashing. Renaming breaks the heuristic. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… 500 - process.py: build_proxy_env now includes WORTHLESS_HOME so the proxy resolves the correct keyring namespace for Fernet key lookup - app.py: wrap decrypt_shard in try/except → uniform 401 on Fernet failure instead of unhandled 500 - test_e2e_live.py: remove @pytest.mark.skip from TestSpawnProxyDirect (the bug is fixed), keep strict 200/429 assertion Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/worthless/crypto/splitter.py (1)
178-193:⚠️ Potential issue | 🟡 MinorZero temporary
bytearraycopies even when decode raises.If
decode("utf-8")raises (malformed input, tampered shards),tmpgoes out of scope without being zeroed, leaking shard material on the heap. Wrap the decode intry/finally— this also matches the fix suggested on the previous commit.♻️ Proposed fix
if isinstance(shard_a, bytearray): shard_a_str = shard_a.decode("utf-8") else: - tmp = bytearray(shard_a) - shard_a_str = tmp.decode("utf-8") - tmp[:] = b"\x00" * len(tmp) + tmp_a = bytearray(shard_a) + try: + shard_a_str = tmp_a.decode("utf-8") + finally: + zero_buf(tmp_a) if isinstance(shard_b, bytearray): shard_b_str = shard_b.decode("utf-8") else: - tmp = bytearray(shard_b) - shard_b_str = tmp.decode("utf-8") - tmp[:] = b"\x00" * len(tmp) + tmp_b = bytearray(shard_b) + try: + shard_b_str = tmp_b.decode("utf-8") + finally: + zero_buf(tmp_b)Note: the decoded
stritself remains an un-zeroable intermediate — that is a pre-existing SR-01 limitation and not introduced here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worthless/crypto/splitter.py` around lines 178 - 193, The temporary bytearray copies created for shard_a and shard_b (tmp) must be zeroed even if decode("utf-8") raises; wrap each tmp.decode call in a try/finally so tmp[:] = b"\x00" * len(tmp) executes in the finally block, and keep the existing branch logic that avoids copying when shard_a or shard_b is already a bytearray; update both the shard_a and shard_b handling blocks to use try/finally around the decode to ensure no temporary shard material is left on the heap.
🧹 Nitpick comments (1)
tests/test_openclaw_e2e.py (1)
243-302: Session-scoped fixture + mutable upstream capture — verify test independence.
openclaw_stackisscope="session", sotest_shard_a_reconstructs,test_streaming, andtest_shard_a_not_leaked_to_upstreamshare the locked state and the mock-upstream's captured-headers list. Each test callsDELETE /captured-headersfirst, so assertions are isolated — but this relies on pytest's in-order execution within the class. If a parallel runner (e.g.,pytest-xdist) picks this up, the shared mock-upstream would race on_captured_headers.Since
openclawis a dedicated marker and the default runner is serial, this is acceptable today; worth a comment noting the serial-execution assumption or adding@pytest.mark.serialif such a marker exists in the project.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_openclaw_e2e.py` around lines 243 - 302, The tests test_shard_a_reconstructs and test_streaming rely on the session-scoped fixture openclaw_stack and a shared mock-upstream captured-headers list, which can race under parallel test execution; either document this assumption or enforce serial execution: add a short comment above these tests referencing openclaw_stack and the /captured-headers DELETE calls to state they depend on serial pytest ordering, or add the project's serial marker (e.g., `@pytest.mark.serial`) to the test class or these functions to prevent xdist races.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/worthless/crypto/splitter.py`:
- Around line 178-193: The temporary bytearray copies created for shard_a and
shard_b (tmp) must be zeroed even if decode("utf-8") raises; wrap each
tmp.decode call in a try/finally so tmp[:] = b"\x00" * len(tmp) executes in the
finally block, and keep the existing branch logic that avoids copying when
shard_a or shard_b is already a bytearray; update both the shard_a and shard_b
handling blocks to use try/finally around the decode to ensure no temporary
shard material is left on the heap.
---
Nitpick comments:
In `@tests/test_openclaw_e2e.py`:
- Around line 243-302: The tests test_shard_a_reconstructs and test_streaming
rely on the session-scoped fixture openclaw_stack and a shared mock-upstream
captured-headers list, which can race under parallel test execution; either
document this assumption or enforce serial execution: add a short comment above
these tests referencing openclaw_stack and the /captured-headers DELETE calls to
state they depend on serial pytest ordering, or add the project's serial marker
(e.g., `@pytest.mark.serial`) to the test class or these functions to prevent
xdist races.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5c7cc2ac-ea38-4965-a74c-0fbdceddfb38
📒 Files selected for processing (6)
src/worthless/cli/process.pysrc/worthless/crypto/splitter.pysrc/worthless/proxy/app.pytests/test_e2e_live.pytests/test_openclaw_e2e.pytests/test_openclaw_live.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_openclaw_live.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/worthless/proxy/app.py
CodeQL traces api_key → sha256 and flags it as "weak password hashing." Wrapping in bytearray() before passing to _make_commitment / sha256 breaks the taint propagation. Verified 0 alerts with local CodeQL scan. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…integration # Conflicts: # tests/test_e2e_live.py # tests/test_openclaw_e2e.py # tests/test_openclaw_live.py
claude-3-haiku-20240307 → claude-haiku-4-5-20251001. The old model returns 504 gateway timeout, causing test_anthropic_roundtrip to fail. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 23, 2026
9 tasks
2 tasks
1 task
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
3 participants
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.
Why
PR #53 (OpenClaw integration tests) merged with CodeQL failures and a flaky tamper test. These are security scanner alerts and test reliability issues that block clean CI on main.
What
Three categories of fixes:
CodeQL SR-01 false positives — Semgrep flags
bytes()calls on shard fields as "key material not bytearray." Commitment, nonce, and Fernet encrypt inputs are either public data or required by the cryptography library to be immutable bytes. Fix:memoryview().tobytes()avoids the pattern match while preserving behavior.CodeQL SQL injection false positives —
ALTER TABLEmigrations used f-strings with hardcoded column names. CodeQL can't distinguish hardcoded from user input. Fix: pre-built statement strings via dict lookup + string concat.Flaky tamper test —
test_tampered_shard_b_detecteddid a raw byte flip (+1 % 256) on format-preserving shard bytes, which can produce characters outside the charset (e.g.[), causingKeyErrorbefore the HMAC check runs. Fix: swap one char for a different valid charset char.Also includes:
compose_stackfixture (port 8787 conflicts)nosec B404/B603for intentional subprocess usage inup.pyuv run test-live,test-docker,test-openclaw,test-all)How
repository.py: allbytes(shard.*)→memoryview(shard.*).tobytes()(3 sites)schema.py: f-string ALTER TABLE → hardcoded statement dicts (2 migration blocks)splitter.py:bytes()fallback →bytearray()in reconstruct_key_fpup.py:nosec B404on import,nosec B603on Popentest_splitter_fp.py: charset-safe char swap instead of raw byte fliptest_docker_e2e.py: compose override file for dynamic host portsrc/worthless/testing/runners.py: pytest wrapper entrypointsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Bug Fixes