fix: protect remote web access and copy session paths#186
Conversation
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details🔇 Additional comments (1)
📝 WalkthroughWalkthroughRequest token extraction now accepts Basic auth; protected frontend paths are classified and the router enforces rate-limits and auth for non-loopback clients; tests and UI add a session file-path copy action with translations. ChangesWeb-Surface Path Authentication
Session Path Copy UI
Sequence DiagramsequenceDiagram
participant Client
participant WebServer
participant PathClassifier
participant RateLimiter
participant AuthValidator
Client->>WebServer: GET / (with/without Authorization)
WebServer->>PathClassifier: isProtectedWebSurfacePath(requestPath)
PathClassifier-->>WebServer: true
alt remoteAddress is non-loopback
WebServer->>RateLimiter: check client rate
RateLimiter-->>WebServer: pass/fail
alt pass
WebServer->>AuthValidator: assertRequestAuthorized(token)
alt token valid (Bearer or Basic)
AuthValidator-->>WebServer: authorized
WebServer-->>Client: 200 OK (content)
else token invalid/missing
AuthValidator-->>WebServer: unauthorized
WebServer-->>Client: 401 Unauthorized
end
else fail
WebServer-->>Client: 429 Too Many Requests
end
else remoteAddress is loopback
WebServer-->>Client: 200 OK (no auth)
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested Reviewers
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 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 |
![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
🧹 Nitpick comments (2)
tests/unit/web-run-host.test.mjs (2)
971-1033: 💤 Low valueSolid coverage of the deny + loopback-bypass paths. The loopback test in particular nicely asserts auth is never invoked (
authCalled === false) rather than just checking the status.Optional: the protected-path enforcement also has an authorized success branch (auth returns
{ ok: true }→ content is served) and a429rate-limit branch (percli.jsrequest handler) that aren't exercised here. Consider adding a remote request whereauthorizeRequestreturns{ ok: true }to confirm the handler proceeds to serve the asset, locking in the full contract for this security-sensitive change.🤖 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/unit/web-run-host.test.mjs` around lines 971 - 1033, Add a new unit test that exercises the authorized-success branch for remote clients: using createWebServerHarness provide an authorizeRequest that returns { ok: true } (and an htmlReader or asset reader that returns known content), call requestHandler with a remote socket (e.g., remoteAddress '192.0.2.10') and the request URL for root or a web-ui asset, then assert response.statusCode is 200 and response.body equals the reader content and that errors remains empty; reference createWebServerHarness, requestHandler, authorizeRequest, htmlReader/createMockResponse to locate where to add this test.
813-830: 💤 Low valueConsolidate duplicated auth mocks.
isLoopbackRemoteAddress,writeJsonResponse, andisProtectedWebSurfacePathare defined inline here and again as standalonemock*helpers (Lines 888-910) with identical bodies. Reusing the standalone helpers keeps a single source of truth and avoids the two copies drifting apart.Note: the standalone helpers are declared after this function, so either move them above
createWebServerHarnessor rely on function-declaration hoisting (they are referenced only at call time, so hoisting works).♻️ Proposed consolidation
- isLoopbackRemoteAddress(value) { - return value === '127.0.0.1' || value === '::1' || value === '::ffff:127.0.0.1'; - }, + isLoopbackRemoteAddress: mockIsLoopbackRemoteAddress, assertRequestAuthorized: authorizeRequest, - writeJsonResponse(res, statusCode, payload, headers = {}) { - const body = JSON.stringify(payload || {}, null, 2); - res.writeHead(statusCode, { - 'Content-Type': 'application/json; charset=utf-8', - ...headers - }); - res.end(body); - }, - isProtectedWebSurfacePath(requestPath) { - return requestPath === '/' - || requestPath === '/web-ui/index.html' - || requestPath.startsWith('/web-ui/') - || requestPath.startsWith('/res/'); - }, + writeJsonResponse: mockWriteJsonResponse, + isProtectedWebSurfacePath: mockIsProtectedWebSurfacePath,🤖 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/unit/web-run-host.test.mjs` around lines 813 - 830, Remove the duplicated inline helper implementations inside createWebServerHarness and reuse the existing standalone mock helpers instead: replace the inline isLoopbackRemoteAddress, writeJsonResponse, and isProtectedWebSurfacePath with calls to the corresponding mock helpers (mockIsLoopbackRemoteAddress, mockWriteJsonResponse, mockIsProtectedWebSurfacePath). Ensure the standalone mock helper declarations are visible at call time—either move those mock* helper declarations above createWebServerHarness or rely on function-hoisting by confirming they are function declarations (not const arrow functions) so references resolve when tests run.
🤖 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 `@cli.js`:
- Around line 10964-10977: When handling protected web surface requests (in the
isProtectedWebSurfacePath branch) ensure 401 responses trigger a browser
Basic-auth prompt by adding a WWW-Authenticate header; modify the flow that
calls assertRequestAuthorized(req, res) so that if auth.ok is false and the
response status is 401 you call writeJsonResponse(res, 401, { error:
'Unauthorized' }, { 'WWW-Authenticate': 'Basic realm="Restricted"',
'Content-Type': 'application/json' }) (or update assertRequestAuthorized to
include that header on its 401 path) instead of returning a bare 401 JSON
response, preserving existing behavior for other statuses and keeping
rate-limiting logic unchanged.
---
Nitpick comments:
In `@tests/unit/web-run-host.test.mjs`:
- Around line 971-1033: Add a new unit test that exercises the
authorized-success branch for remote clients: using createWebServerHarness
provide an authorizeRequest that returns { ok: true } (and an htmlReader or
asset reader that returns known content), call requestHandler with a remote
socket (e.g., remoteAddress '192.0.2.10') and the request URL for root or a
web-ui asset, then assert response.statusCode is 200 and response.body equals
the reader content and that errors remains empty; reference
createWebServerHarness, requestHandler, authorizeRequest,
htmlReader/createMockResponse to locate where to add this test.
- Around line 813-830: Remove the duplicated inline helper implementations
inside createWebServerHarness and reuse the existing standalone mock helpers
instead: replace the inline isLoopbackRemoteAddress, writeJsonResponse, and
isProtectedWebSurfacePath with calls to the corresponding mock helpers
(mockIsLoopbackRemoteAddress, mockWriteJsonResponse,
mockIsProtectedWebSurfacePath). Ensure the standalone mock helper declarations
are visible at call time—either move those mock* helper declarations above
createWebServerHarness or rely on function-hoisting by confirming they are
function declarations (not const arrow functions) so references resolve when
tests run.
🪄 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: d43f7ee1-bb29-4269-a148-5ce19c042fc3
📒 Files selected for processing (2)
cli.jstests/unit/web-run-host.test.mjs
📜 Review details
🔇 Additional comments (4)
tests/unit/web-run-host.test.mjs (3)
723-728: LGTM!
912-917: LGTM!
1107-1110: LGTM!Also applies to: 1228-1231, 1366-1369, 1548-1551
cli.js (1)
10415-10428: LGTM!Also applies to: 10460-10465
awsl233777
changed the title
|
Updated this PR with the session-browser copy-path action. Changes added in
Validation:
@coderabbitai review |
|
✅ Action performedFull review finished. |
|
Handled the CodeRabbit auth challenge feedback in Fixes:
Validation:
@coderabbitai review |
|
✅ Action performedFull review finished. |
2 participants
Summary
filePathwithout reconstructing it in the browserValidation
npm run test:unit→All 585 tests passednode tests/e2e/run.js→E2E_EXIT:0filePathvalues are absoluteCODEXMATE_HTTP_TOKEN=web-auth-test-tokenserver:/without token →401 Unauthorized/with Bearer token →200 OK/with Basic token →200 OK/web-ui/app.jswith Basic token →200 OK/without token →200 OKNotes
Screenshots for the auth flow were captured locally under
_artifacts/codexmate/auth-web-token-20260603-090421and shared in the chat.Summary by CodeRabbit
New Features
WWW-Authenticatechallenge responsesChores