fix(sandboxed-web-e2e): probe explicit port 0, not the scheme default - #1467
Conversation
require_unoccupied_readiness_port() derived the probe port with `parsed.port or (443 if https else 80)`. urllib.parse's .port returns the int 0 for a URL with an explicit :0 port, and `0 or X` evaluates to X in Python, so an explicitly-requested port 0 was silently replaced with the scheme's default port (80/443) instead of actually being probed. Devin's review on PR #1347 flagged this pattern but it was out of scope for that PR's authorized task. Switch to an explicit None check so port 0 is honored, and add a regression test that monkeypatches socket.create_connection to record the probed address and assert it names port 0. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
📝 WalkthroughWalkthrough
Changes샌드박스 웹 E2E 보정
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🔵 Low · up to The change fixes explicit port-0 readiness probing without altering production interfaces or deployment behavior. Merge readiness is low risk, but the repository’s lint checks may reject the two sandbox mount paths unless they receive narrowly scoped exemptions. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Bandit B108 correctly began running after the central file-detection repair and identified the two literal /tmp arguments passed to bubblewrap. These are tmpfs mount targets inside a new isolated namespace, not host temporary-file paths. Add B108-only waivers with adjacent rationale and a regression that pins both the scope and count, preserving every other Bandit rule and host-path finding.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@opencode-agent review Exact-head request: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
CodeRabbit ran `ruff check --select S108` against this file and found the two bubblewrap tmpfs `"/tmp"` mount-target arguments still fail Ruff's own insecure-temp-path rule -- `# nosec B108` only silences Bandit, not Ruff. Add `# noqa: S108` alongside each existing waiver and extend the regression test that already pins the Bandit waiver's exact text/count/rationale to also pin the Ruff waiver, so this scoped exception can't silently broaden to other paths or rules. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
|
Fixed CodeRabbit's finding: Verified on the exact new head:
Generated by Claude Code |
|
@opencode-agent review Exact-head request: |
2 participants
Bug
require_unoccupied_readiness_port()inscripts/ci/sandboxed_web_e2e.pyderived the probe port with:urllib.parse's.portreturns the int0for a URL with an explicit:0port (e.g.http://127.0.0.1:0/health), and0 or Xevaluates toXin Python — so an explicitly-requested port 0 was silently replaced with the scheme's default port (80/443) instead of actually being probed. This means the pre-start "is this readiness port already occupied?" check would connect to the wrong port entirely whenever a caller (deliberately or accidentally) passed a:0readiness URL.This was flagged by an external reviewer (Devin) during review of #1347, but was out of scope for that PR's authorized task, so it was left unfixed.
Fix
An explicit
is not Nonecheck distinguishes "no port in the URL" (None, defaulting as before) from "port 0 was requested" (probed as 0).I checked the rest of the file (
wait_for_urland every other URL-handling helper) for the sameX.port or <default>shape —require_unoccupied_readiness_portis the only call site that manually derives a port;wait_for_urlhands the full URL straight tourllib.request's opener rather than deriving a port, so it does not have this bug.Tests
Added
test_require_unoccupied_readiness_port_probes_explicit_port_zerototests/test_sandboxed_web_e2e.py: it monkeypatchessocket.create_connectionto record the addressrequire_unoccupied_readiness_portactually probes forhttp://127.0.0.1:0/health, and asserts it is("127.0.0.1", 0)rather than("127.0.0.1", 80).Verified locally:
python -m pytest tests/test_sandboxed_web_e2e.py -q→ 67 passedcoverage run -m pytest tests && coverage report --show-missing→ 2091 passed, 1 skipped, 21 subtests passed;scripts/ciat 100% coverage (no missing lines/branches)interrogate→ 100% docstring coverageScope
Single targeted fix + regression test, no unrelated changes. Not merging this myself — leaving it for the normal OpenCode review → merge-scheduler pipeline per this repo's governance model.
Generated by Claude Code
Bandit operational follow-up
After #1464 made the central Python detector execute reliably, its exact-head hosted Bandit job exposed two B108 findings on this file's literal
"/tmp"values. Both values are bubblewrap--tmpfsmount targets created inside a new isolated namespace, not host temporary-file paths.This owner branch now carries exactly two paired
# nosec B108/# noqa: S108waivers, each immediately preceded by that rationale. The regression pins the exact waiver text, count, and rationale, so neither Bandit nor Ruff exceptions can broaden to other rules or arbitrary paths.Exact head:
ef75ce93732e5706aa51f73a5f466d65aef2c1ffIndependent exact-tree verification:
68 passed.2,092 passed · 1 skipped · 21 subtests.10,455/10,455, branches4,164/4,164; docstrings 100%.--select S108: all checks passed.