fix(tlsnotary): bake wstcp into the image + publish the proxy port range#949
Open
Shitikyan wants to merge 9 commits into
Open
fix(tlsnotary): bake wstcp into the image + publish the proxy port range#949Shitikyan wants to merge 9 commits into
Shitikyan wants to merge 9 commits into
Conversation
TLSNotary verifications failed with nginx 502 / CloseEvent 1006 because the wstcp websocket→TCP proxy could never be reached, on two layers: 1. The runtime image ships no Rust toolchain, so ensureWstcp()'s on-demand `cargo install wstcp` fallback silently failed and the proxy never spawned. Bake the binary in via a dedicated `rust` build stage and copy it to the exact path proxyManager.ts probes ($HOME/.cargo/bin/wstcp). 2. The proxy binds a dynamic port in 55000-57000 inside the container, but docker-compose published none of them — so nginx forwarding /tlsn/<port>/ to 127.0.0.1:<port> hit a dead upstream. Publish a narrow, localhost-bound, env-configurable window; the allocation range (PORT_MIN/MAX, now env-overridable) and the published range read the same vars so they cannot drift. Manually binding one host port was a band-aid that broke on the next session's different port. This fixes both layers durably. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Contributor
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThe Docker image now includes ChangesTLSNotary wstcp runtime and port wiring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Greptile SummaryThis PR fixes two root-cause issues that caused TLSNotary verifications to 502: the
Confidence Score: 5/5Safe to merge — the three-layer fix (binary baked in, env-driven port window, loopback port publishing) directly addresses the confirmed 502 root cause with no changes to existing behavior when defaults are unchanged. All changed paths are infrastructure/config: the Dockerfile multistage addition is contained and the binary lands at the exact path already probed by proxyManager.ts. The env-driven port range change is backward-compatible (defaults preserve historical behavior). The Caddy routing additions are additive and guarded by a port-range regexp that prevents proxying arbitrary node ports. No files require special attention. The two stale range references in portAllocator.ts and proxyManager.ts are cosmetic only. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Browser
participant Caddy
participant Node
participant wstcp
participant TLSTarget
Browser->>Caddy: GET wss://node/tlsn/7047/ (notary)
Caddy-->>Browser: WebSocket upgrade → tlsnotary:7047
Browser->>Node: "POST /api/tlsnotary/proxy {targetUrl}"
Node->>wstcp: spawn /app/.cargo/bin/wstcp --bind-addr 0.0.0.0:55123 target:443
wstcp-->>Node: INFO Starts a WebSocket proxy (port 55123)
Node-->>Browser: "{websocketProxyUrl: wss://node/tlsn/55123/}"
Browser->>Caddy: GET wss://node/tlsn/55123/ (proxy)
Caddy->>wstcp: reverse_proxy node:55123 (WebSocket upgrade)
wstcp->>TLSTarget: TCP relay :443
TLSTarget-->>wstcp: TLS data
wstcp-->>Browser: WebSocket frames
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Browser
participant Caddy
participant Node
participant wstcp
participant TLSTarget
Browser->>Caddy: GET wss://node/tlsn/7047/ (notary)
Caddy-->>Browser: WebSocket upgrade → tlsnotary:7047
Browser->>Node: "POST /api/tlsnotary/proxy {targetUrl}"
Node->>wstcp: spawn /app/.cargo/bin/wstcp --bind-addr 0.0.0.0:55123 target:443
wstcp-->>Node: INFO Starts a WebSocket proxy (port 55123)
Node-->>Browser: "{websocketProxyUrl: wss://node/tlsn/55123/}"
Browser->>Caddy: GET wss://node/tlsn/55123/ (proxy)
Caddy->>wstcp: reverse_proxy node:55123 (WebSocket upgrade)
wstcp->>TLSTarget: TCP relay :443
TLSTarget-->>wstcp: TLS data
wstcp-->>Browser: WebSocket frames
Reviews (8): Last reviewed commit: "fix(tlsnotary): pass TLSNOTARY_PORT to t..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?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: 1
🧹 Nitpick comments (1)
Dockerfile (1)
150-151: 📐 Maintainability & Code Quality | 🔵 TrivialPin the
wstcpversion for reproducible builds.
cargo install wstcpfetches the latest release (currently0.2.1) at build time, making builds non-reproducible. Thewstcpcrate is available on crates.io with versions ranging from0.1.0to0.2.1. To prevent unexpected behavior changes or build failures from upstream updates, pin the specific version and use--locked.♻️ Suggested update
FROM rust:1-slim AS wstcp -RUN cargo install wstcp --root /wstcp +RUN cargo install wstcp --version 0.2.1 --locked --root /wstcp🤖 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 `@Dockerfile` around lines 150 - 151, The wstcp install step is unpinned and can pull a changing latest release, so update the Dockerfile’s wstcp build stage to install a specific crates.io version of wstcp and use --locked for reproducible builds. Keep the change focused on the FROM rust:1-slim AS wstcp stage and the cargo install wstcp command so the image always builds against the same crate release.
🤖 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 `@src/features/tlsnotary/constants.ts`:
- Around line 60-61: `PORT_CONFIG` is duplicated and the allocator is still
using the hardcoded range instead of the env-driven values. Update
`portAllocator.ts` to remove its local `PORT_CONFIG` and import the shared one
from `./constants`, then make `proxyManager.ts` also use `PORT_CONFIG` from
`./constants` so there is a single source of truth for the TLSNotary port range.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 150-151: The wstcp install step is unpinned and can pull a
changing latest release, so update the Dockerfile’s wstcp build stage to install
a specific crates.io version of wstcp and use --locked for reproducible builds.
Keep the change focused on the FROM rust:1-slim AS wstcp stage and the cargo
install wstcp command so the image always builds against the same crate release.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93605c23-fd3e-4cf7-ac95-581870d043e0
📒 Files selected for processing (3)
Dockerfiledocker-compose.ymlsrc/features/tlsnotary/constants.ts
…ocation portAllocator.ts kept its own hardcoded PORT_CONFIG (55000-57000) and proxyManager imported PORT_CONFIG from it, so the env-overridable values in constants.ts were never used for allocation: the allocator would walk past the published window (e.g. 55064) and hand nginx an unreachable port, re-creating the 502 after the first 64 allocations. Point both consumers at constants.ts (the env-driven copy) and drop the duplicate. Addresses CodeRabbit/Greptile review on #949. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`cargo install wstcp` pulled the latest release at build time. Pin 0.2.1 and pass --locked so the image always builds the same wstcp and an upstream release can't change behaviour or break the build. Addresses CodeRabbit review on #949. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`--locked` forces wstcp 0.2.1's bundled Cargo.lock, whose pinned deps no longer compile on the current rust:1-slim toolchain (cargo exit 101). Keep the version pin for reproducibility but let cargo resolve compatible dependency versions. Validated: the stage builds and yields wstcp 0.2.1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The devnet override replaces the node `ports:` list (`!override`), which dropped the proxy-range publish from the base compose — so devnet verifications would still 502. Add a devnet-specific window (55100-55163, +100 offset from mainnet so the two stacks don't collide on host ports), with the allocation env and the published range driven by the same values. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The node advertises every TLSN endpoint under /tlsn/<port>/ (buildWsUrl's default path) — the notary at /tlsn/<TLSNOTARY_PORT>/ and each per-target wstcp proxy at /tlsn/<dynamic-port>/. The subpath Caddy config only routed /tlsnotary/* to the notary, so every proxy (and the notary on remapped hosts) fell through to the RPC catch-all and 502'd — the browser's attest() websocket then closed with CloseEvent 1006. This is why TLSN verification failed on every node, not just dev; node3 only worked because of a manual nginx rule that was never in the repo and is lost on redeploy. - honour $TLSNOTARY_PORT (dev remaps it to 7147; the hard-coded 7047 made the notary leg 502 on remapped hosts) - add /tlsn/<notary-port>/ -> tlsnotary container - add a regex route /tlsn/<digits>/ -> node:<port> for the wstcp proxies (wstcp binds 0.0.0.0, reachable by service name on the compose network) Validated with `caddy validate`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docker-compose.devnet.yml`:
- Around line 57-63: The wstcp proxies are still binding to all interfaces
because proxyManager defaults WSTCP_BIND_HOST to 0.0.0.0, so update the node
service environment to set WSTCP_BIND_HOST to 127.0.0.1 as well. Keep the
existing TLSNOTARY_PROXY_PORT_MIN and TLSNOTARY_PROXY_PORT_MAX settings, and
make sure the change aligns with the spawn logic in proxyManager so only the
reverse proxy can reach the worker ports.
In `@monitoring/caddy/tlsnotary-modes/subpath.caddy`:
- Around line 27-30: The `@tlsnproxy` route in subpath.caddy currently forwards
any numeric /tlsn/<port>/ request directly to reverse_proxy
node:{re.tlsnport.1}, which can expose arbitrary listeners. Tighten the matching
logic in the tlsnport path_regexp/handle block so only ports within the
allocated TLSNOTARY_PROXY_PORT_MIN..MAX range are accepted, and reject or bypass
all other ports before reverse_proxy is reached. Use the existing `@tlsnproxy`,
tlsnport capture, and reverse_proxy directives as the place to enforce this
validation.
- Around line 18-21: The Caddy service is missing the TLSNOTARY_PORT environment
variable, so the placeholder in subpath.caddy never resolves to the devnet port.
Update the caddy service definition in both compose files to pass TLSNOTARY_PORT
through to Caddy, and make sure it matches the tlsnotary upstream port used by
the handle_path /tlsn/{$TLSNOTARY_PORT:7047} matcher so the remap works as
intended.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f788a92-d8c8-45c0-acd4-5b205db4202b
📒 Files selected for processing (3)
Dockerfiledocker-compose.devnet.ymlmonitoring/caddy/tlsnotary-modes/subpath.caddy
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile
…pital Connection: Upgrade gotcha wstcp 0.2.1 checks the Connection header case-sensitively (requires 'Upgrade', capital U). The common nginx 'Connection "upgrade"' (lowercase) makes wstcp reject every handshake -> /tlsn/ 502s -> browser CloseEvent 1006. Document the exact Caddy (repo, automatic) and nginx (hand-maintained) routing so any node can be brought up correctly, plus the wstcp/port prerequisites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/runbooks/tlsn-reverse-proxy.md (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
textlanguage specifier to the error output block.-``` +```text Invalid WebSocket handshake request: assertion failed: `values.any(|v| v.trim() == "Upgrade")`; value="upgrade"🤖 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 `@docs/runbooks/tlsn-reverse-proxy.md` at line 25, Add the text language specifier to the fenced error output block in the TLSN reverse proxy runbook so the literal handshake error is rendered correctly; update the markdown snippet containing the “Invalid WebSocket handshake request” message to use the appropriate fenced block syntax.
🤖 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 `@docs/runbooks/tlsn-reverse-proxy.md`:
- Around line 74-78: Remove the stray closing Markdown fence in the tlsn reverse
proxy runbook so the section renders correctly. Update the runbook content
around the HTTPS RPC note and the `101`/`502` explanation to ensure there is no
unmatched code fence; keep the surrounding prose intact and verify the markdown
block boundaries are balanced in this document.
---
Nitpick comments:
In `@docs/runbooks/tlsn-reverse-proxy.md`:
- Line 25: Add the text language specifier to the fenced error output block in
the TLSN reverse proxy runbook so the literal handshake error is rendered
correctly; update the markdown snippet containing the “Invalid WebSocket
handshake request” message to use the appropriate fenced block syntax.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7f687a3-a279-4eda-9b15-3d2626c669a7
📒 Files selected for processing (1)
docs/runbooks/tlsn-reverse-proxy.md
Address review: the /tlsn/(\d+)/ regex forwarded any digits to node:<port>,
which could expose internal listeners (RPC 53550, MCP 3001, metrics 9090)
under a public /tlsn/ path. Narrow to 5[567]\d{3} — the wstcp allocation
window — in both the Caddy route and the runbook's nginx snippet.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
T-Rex pricing update — T-Rex was free through June 2026. Effective July 1, 2026, T-Rex adds 2 credits on top of the standard 1-credit review (3 total). T-Rex settings |
…fence
The subpath snippet routes the notary leg via {$TLSNOTARY_PORT}, but Caddy reads
that from its own container env, which was never set — so on port-remapped stacks
(devnet notary = 7147) Caddy fell back to /tlsn/7047/ → tlsnotary:7047 where
nothing listens → 502. Set TLSNOTARY_PORT on the caddy service in the base
(${TLSNOTARY_PORT:-7047}) and devnet (7147) compose files. Verified via
`docker compose config`: mainnet caddy=7047, devnet caddy=7147.
Also drop a stray closing ``` fence in the reverse-proxy runbook.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Problem
TLSNotary verifications (e.g. Discord) fail with nginx 502 /
CloseEvent {code: 1006}on the prover. The notary itself is healthy (/tlsnotary/health→ 200, port 7047) — "connected to verifier" succeeds — but the next hop,wss://<node>/tlsn/<port>/, 502s.Root cause is two layers, both confirmed live on node3:
wstcp can't spawn. The TLSNotary flow spawns a
wstcpwebsocket→TCP proxy on demand (proxyManager.ts). The slim runtime image has no Rust toolchain, soensureWstcp()'scargo install wstcpfallback fails silently — nothing ever binds the proxy port. Confirmed:docker exec demos-node→ nowstcp, nocargo.The proxy port isn't reachable from the host. Even with wstcp, the proxy binds a dynamic port in
55000-57000inside the container, butdocker-compose.ymlpublishes none of that range. nginx forwards/tlsn/<port>/→127.0.0.1:<port>→ dead upstream → 502. Manually binding a single port (55688) is a band-aid that breaks on the next session's different port — which is why this kept recurring.Fix
wstcpinto the image via a dedicatedFROM rust:1-slim AS wstcpstage and copy the binary to the exact pathproxyManager.tsprobes ($HOME/.cargo/bin/wstcp,HOME=/app). No toolchain in the runtime image; the proxy spawns instantly. (Validated: the stage builds,wstcp 0.2.1runs.)PORT_MIN/PORT_MAXare now overridable viaTLSNOTARY_PROXY_PORT_MIN/MAX(defaults unchanged) so the allocation range can be narrowed to a host-publishable window.127.0.0.1:55000-55063, env-driven). The allocation range and the published range read the same vars so they can't drift, and binding to127.0.0.1keeps the ports reachable by the host reverse proxy only, not the public internet.Deploy on node3
demos-nodecontainer so the newports:mapping applies. (Immediate unblock without a rebuild:docker cpa prebuiltwstcpinto/app/.cargo/bin/wstcpand add the port-range mapping.)TLSNOTARY_PROXY_PORT_MIN/MAXenv + the127.0.0.1:<min>-<max>port mapping there.Refs:
docs/runbooks/wstcp-reachability-check.md(this makes the containerized default actually reachable instead of "internal only").🤖 Generated with Claude Code
Summary by CodeRabbit
/tlsn/<port>/...to the corresponding per-port proxy.wstcphelper via a multistage build.