-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix(cloakserve): opt-in --auth-token + explicit --host for the CDP surface (#217)#248
fix(cloakserve): opt-in --auth-token + explicit --host for the CDP surface (#217) #248therahul-yo wants to merge 2 commits into
Conversation
...loakHQ#217) The path-traversal half of CloakHQ#217 was fixed in babef04. The remaining gaps were: - /json/*, /devtools/*, and /fingerprint/*/devtools/* served CDP metadata + WebSocket control with no authentication. - Inside a container the auto-detect binds to 0.0.0.0, so docker port-forwards expose the CDP surface to anyone on the network by default — exactly the deployment shape the reporter flagged. This change adds two opt-in knobs and a startup warning: 1. ``--auth-token=<secret>`` (or env ``CLOAKSERVE_AUTH_TOKEN``) installs an aiohttp middleware that requires ``Authorization: Bearer <secret>`` or ``?token=<secret>`` on every /json, /devtools, and /fingerprint/... route. ``/`` (status) stays open so health checks keep working. Comparison is ``hmac.compare_digest`` to avoid timing oracles. 2. ``--host=<addr>`` lets the operator pin the bind address explicitly instead of relying on the dockerenv auto-detect. Default behaviour is unchanged: loopback off-container, 0.0.0.0 in a container. 3. When the resolved bind is non-loopback AND no auth token is set, the server prints a loud WARNING at startup pointing the operator at the new flag. This is the realistic remediation path for the existing deployments that turned up in the report — they keep working but get a clear nudge. Backwards-compatible: no token = no auth = current behaviour. Closes one of the open gaps in CloakHQ#217. Path containment + safe rmtree were already addressed; per-IP rate limiting is left for a follow-up. Tests: 12 new in TestHostAndAuthCli + TestAuthMiddleware; full test_cloakserve.py suite green at 66 passed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@journeytosilius
journeytosilius
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First-pass review. I found two blockers in the query-token auth path:
-
bin/cloakserve:332-359does not excludetokenfromparse_connection_params(). A request like/json/version?token=s3cretauthenticates in the middleware, then the handler parses the same query string and treatstokenas a generic fingerprint parameter, forwarding--fingerprint-token=s3cretthroughextra_argsinto the Chrome launch path. That leaks the auth secret into child process args and may also pass an unsupported flag. Please striptokenfrom the launch-param parser, e.g. by adding it toSPECIAL_PARAMS, and cover this with a regression test. -
bin/cloakserve:487andbin/cloakserve:524-528rewritewebSocketDebuggerUrlwithout preserving the query token. Withauth_tokenconfigured, a client using the advertised?token=compatibility can authenticate/json/versionor/json/list, but the returnedws://.../devtools/...URL has no token, so the new middleware rejects the subsequent WebSocket handshake with 401. That breaks the stated Playwrightconnect_over_cdpURL-string use case. Please propagate the token into the rewritten WebSocket URLs, or otherwise make the discovered WS URL authenticate, and add an end-to-end test for that flow.
Local validation: attempted python3 -m pytest tests/test_cloakserve.py -q, but this environment is missing pytest (and websockets), so I reviewed against the exported PR tree and diff instead.
Addresses both review blockers on CloakHQ#248: 1. ``parse_connection_params`` did not exclude ``token`` from the generic fingerprint passthrough. A request like ``/json/version?token=secret`` authenticated in the middleware, then the same query string was parsed again by the launch path and ``token`` fell through to the catch-all branch — yielding ``--fingerprint-token=secret`` in Chrome's argv. That leaked the auth secret to child process args (visible in ``ps``) and pushed an unsupported flag at the Chrome binary. Add ``token`` to ``SPECIAL_PARAMS`` so the parser silently drops it from launch params. Two new tests cover bare ``?token=`` and the mixed ``fingerprint=...&token=...`` case. 2. The rewritten ``webSocketDebuggerUrl`` in ``/json/version`` and ``/json/list`` was emitted without any auth query, so a client that authenticated via the ``?token=`` compat path (the documented ``connect_over_cdp(URL_STRING)`` flow) followed the URL and got 401 on the WebSocket handshake. The WS handshake cannot carry the same Authorization header that worked on the JSON probe in that flow, so the rewritten URL itself must carry the secret. New ``_append_token_query(url, token)`` helper appends ``?token=<urlquoted-token>`` (or ``&token=...`` if a query already exists) when ``app["auth_token"]`` is configured. Both rewrite sites call it. Tests: - ``TestAppendTokenQuery`` (5 unit cases: no-token no-op, no-query append, with-query append, URL encoding, fingerprint path). - ``TestHandleJsonVersionTokenPropagation`` (2 end-to-end cases against a stub Chrome upstream — one with auth on, one with auth off, asserting the WS URL carries / omits the token correctly). Full suite: 75 passed (66 prior + 9 new). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
therahul-yo
commented
May 16, 2026
Thanks for the careful review @journeytosilius — both blockers are real and now fixed in 91b1230.
Blocker 1 — ?token= leaking into Chrome args.
Added token to SPECIAL_PARAMS so parse_connection_params silently drops it instead of falling through to the generic --fingerprint-{key}={val} branch.
- New test
test_token_query_param_is_not_forwarded_to_chromeexercises the exactfingerprint=1&token=super-secret-token-valueshape from your example and asserts notokensubstring appears anywhere inextra_args. - New test
test_token_query_param_alone_is_silently_consumedcovers the bare?token=abccase.
Blocker 2 — rewritten webSocketDebuggerUrl lost the token.
New _append_token_query(url, token) helper appends ?token=<urlquoted-token> (or &token=... if a query already exists) when app["auth_token"] is configured. Wired into both rewrite sites — handle_json_version at L495 and handle_json_list at L528 — so the Playwright connect_over_cdp(URL_STRING) compat flow round-trips end-to-end.
TestAppendTokenQuery(5 unit cases): no-token no-op, no-query append, with-query append, URL encoding, fingerprint path.TestHandleJsonVersionTokenPropagation(2 end-to-end cases) spins up a stub Chrome/json/versionupstream and asserts the rewritten WS URL carries?token=rev-secretwhen auth is configured, and is clean when it isn't.
Full suite: 75 passed (66 prior + 9 new). To reproduce locally:
pip install -e . pytest pytest-asyncio aiohttp websockets
python -m pytest tests/test_cloakserve.py -v
Cloak-HQ
commented
May 26, 2026
Thanks for the thorough PR! There's a merge conflict with bin/cloakserve from #240 (WebSocket origin guards) that landed on main. Could you rebase on main and resolve it? Let us know if you need a hand.
rafaelfiguereod-stack
commented
Jun 9, 2026
Nice — opt-in --auth-token is the right shape here.
One complementary gap that's orthogonal to auth and might be worth folding in (or as a follow-up): the / status handler returns each session's proxy string verbatim, and the "seed already running" logger.warning(...) logs it too — both include inline user:pass credentials when a session was started with an authenticated proxy (e.g. ?proxy=http://user:pass@host:8080).
--auth-token gates who can reach /, but an authorized operator — and the log file / any log aggregation — still sees plaintext proxy creds, which for paid residential/ISP proxies is a real secret. So it's defense-in-depth alongside, not instead of, the token.
A tiny _redact_proxy() that masks the userinfo (→ http://***@host:8080), applied in handle_root and that warning, closes it with no behavior change. Happy to send a small PR (against this branch or standalone) if that's useful.
Summary
Addresses the remaining gaps in #217 after the path-traversal fix (commit
babef04).The reporter flagged three classes of risk; one was fixed, two remained:
fingerprint=..%2F...→ arbitraryshutil.rmtreebabef04(SAFE_SEED_RE+_safe_rmtree)/json/version,/json/list,/devtools/...,/fingerprint/.../devtools/...--auth-token(Bearer or?token=)cloakserveauto-binds0.0.0.0in containers — Docker port-forward exposes the CDP surface by default--hostflag + startup warning when bound non-loopback with no tokenWhy this shape
connect_over_cdp(...)users keep working unchanged.Authorization: Bearer <secret>(preferred — keeps the secret out of access logs) or?token=<secret>(compat for clients that can't set headers, e.g. Playwrightconnect_over_cdpURL strings).CLOAKSERVE_AUTH_TOKEN) so the secret doesn't have to land in process-list output.hmac.compare_digest— the endpoint can't be used as a length/prefix oracle./(status) stays open so container health checks keep working without leaking the token to every probe.What's NOT in this PR
0.0.0.0-in-container default to keep behaviour stable for current users. The warning gives operators time to opt into--auth-token.Test plan
12 new tests covering CLI parsing and middleware behaviour:
TestHostAndAuthCli(6)--host,--auth-token,CLOAKSERVE_AUTH_TOKEN, CLI-beats-env precedenceTestAuthMiddleware(8)/always open; missing/wrong/correct bearer; query-param token;/devtools/*and/fingerprint/*/devtools/*both protectedFiles
bin/cloakserve—+45 / -3(middleware, two CLI flags, env fallback, startup warning)tests/test_cloakserve.py—+185 / 0(regression coverage)🤖 Generated with Claude Code