fix(tlsnotary): hostname-based scheme, default /tlsn path for public …#819
fix(tlsnotary): hostname-based scheme, default /tlsn path for public …#819Shitikyan wants to merge 1 commit into
Conversation
…bases
EXPOSED_URL doubles as the public RPC endpoint advertised to peers for
omniprotocol port discovery, so it must stay as http://host:53550 — peers
fail otherwise. The previous protocol-based scheme switch forced
ws://host:7047 for that base, which can't be reached through the reverse
proxy.
Switch buildWsUrl to a hostname-based rule:
- localhost / 127.0.0.1 → ws://<host>:<port> (dev, raw port)
- anything else → wss://<host><path>/<port>/, path defaulting
to /tlsn when not supplied
Behaviour:
http://localhost:53550 → ws://localhost:7047
http://127.0.0.1:53550 → ws://127.0.0.1:7047
http://node2.demos.sh:53550 → wss://node2.demos.sh/tlsn/7047/
https://node2.demos.sh/tlsn → wss://node2.demos.sh/tlsn/7047/
Public bases now always route through TLS — assumes the reverse proxy
terminates HTTPS at hostname:443 (standard for the production deploy).
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
WalkthroughThe PR modifies WebSocket URL construction in the TLS Notary proxy manager. The ChangesWebSocket URL Construction
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (2)
src/features/tlsnotary/proxyManager.ts (2)
177-188: 💤 Low valueConsider treating IPv6 loopback
[::1]as local too.When a developer points
EXPOSED_URL(or a request origin) athttp://[::1]:53550,url.hostnameresolves to[::1](brackets included per WHATWG URL), so the function falls through to the public branch and produceswss://[::1]/tlsn/<port>/— not reachable in local dev. Trivial to include alongsidelocalhost/127.0.0.1.♻️ Suggested adjustment
- const isLocal = - url.hostname === "localhost" || url.hostname === "127.0.0.1" + const isLocal = + url.hostname === "localhost" || + url.hostname === "127.0.0.1" || + url.hostname === "[::1]"🤖 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 `@src/features/tlsnotary/proxyManager.ts` around lines 177 - 188, The buildWsUrl function treats only "localhost" and "127.0.0.1" as local; update its local-check to also recognize the IPv6 loopback by either testing for url.hostname === "[::1]" or by normalizing brackets (e.g. strip leading/trailing '[' ']' and compare to "::1") so addresses like http://[::1]:53550 are treated as local and return a ws:// URL; modify the isLocal condition in buildWsUrl accordingly.
190-195: 💤 Low valueStale JSDoc example for
getPublicUrl.The
@returnsexample still reads"ws://node.demos.sh:55123", but for any non-local base this function now returns awss://<host>/tlsn/<port>/form. Worth updating so the doc reflects the new public-base shape (the local example is still accurate).📝 Suggested doc tweak
/** * Build the public WebSocket URL for the proxy * `@param` localPort - Local port the proxy is listening on * `@param` requestOrigin - Optional request origin for auto-detection - * `@returns` WebSocket URL like "ws://node.demos.sh:55123" + * `@returns` WebSocket URL — `ws://localhost:<port>` for local bases, + * `wss://<host>/tlsn/<port>/` (or `/<basePath>/<port>/`) for public bases */🤖 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 `@src/features/tlsnotary/proxyManager.ts` around lines 190 - 195, Update the stale JSDoc for getPublicUrl to reflect the new public-base URL shape: keep the local example (ws://<host>:<port>) but change the non-local example to show the current form returned for non-local bases, e.g. wss://<host>/tlsn/<port>/, so the `@returns` accurately documents both local and non-local outputs from getPublicUrl.
🤖 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.
Nitpick comments:
In `@src/features/tlsnotary/proxyManager.ts`:
- Around line 177-188: The buildWsUrl function treats only "localhost" and
"127.0.0.1" as local; update its local-check to also recognize the IPv6 loopback
by either testing for url.hostname === "[::1]" or by normalizing brackets (e.g.
strip leading/trailing '[' ']' and compare to "::1") so addresses like
http://[::1]:53550 are treated as local and return a ws:// URL; modify the
isLocal condition in buildWsUrl accordingly.
- Around line 190-195: Update the stale JSDoc for getPublicUrl to reflect the
new public-base URL shape: keep the local example (ws://<host>:<port>) but
change the non-local example to show the current form returned for non-local
bases, e.g. wss://<host>/tlsn/<port>/, so the `@returns` accurately documents both
local and non-local outputs from getPublicUrl.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ca21d65-ee12-4728-a3b1-896c0307fb18
📒 Files selected for processing (1)
src/features/tlsnotary/proxyManager.ts
Greptile SummarySwitches
Confidence Score: 4/5Safe to merge for the described production topology; the edge cases (IPv6 loopback, private-network IPs) only affect non-standard dev environments. The hostname-based switch correctly solves the EXPOSED_URL problem for the documented production setup. The two gaps — IPv6 src/features/tlsnotary/proxyManager.ts — specifically the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["buildWsUrl(base, port)"] --> B["new URL(base)"]
B --> C{"hostname ==\n'localhost' or\n'127.0.0.1'?"}
C -- Yes --> D["ws://hostname:port\n(direct, dev)"]
C -- No --> E["path = pathname || '/tlsn'"]
E --> F["wss://hostname + path + '/' + port + '/'"]
G["getPublicUrl(localPort, requestOrigin?)"] --> H{requestOrigin\nprovided?}
H -- Yes --> I["buildWsUrl(requestOrigin, localPort)"]
H -- No --> J{EXPOSED_URL set?}
J -- Yes --> K["buildWsUrl(EXPOSED_URL, localPort)"]
J -- No --> L["buildWsUrl(sharedState.exposedUrl, localPort)"]
L --> M["Fallback: ws://localhost:port"]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const isLocal = | ||
| url.hostname === "localhost" || url.hostname === "127.0.0.1" |
There was a problem hiding this comment.
isLocal check omits the IPv6 loopback address ::1. When a base URL like http://[::1]:53550 is passed, url.hostname is ::1, which falls through to the public branch and produces wss://::1/tlsn/7047/. That string is an invalid URL (IPv6 literals in URLs require brackets), so any downstream new URL(...) call or WebSocket constructor on the result will throw.
| const isLocal = | |
| url.hostname === "localhost" || url.hostname === "127.0.0.1" | |
| const isLocal = | |
| url.hostname === "localhost" || | |
| url.hostname === "127.0.0.1" || | |
| url.hostname === "::1" |
| const isLocal = | ||
| url.hostname === "localhost" || url.hostname === "127.0.0.1" | ||
|
|
||
| if (isLocal) { | ||
| return `ws://${url.hostname}:${port}` | ||
| } | ||
|
|
||
| const path = url.pathname.replace(/\/+$/, "") || "/tlsn" | ||
| return `wss://${url.hostname}${path}/${port}/` | ||
| } |
There was a problem hiding this comment.
wss://
Any base URL whose hostname is a private-network address (e.g. 192.168.x.x, 10.x.x.x, 172.16–31.x.x) is not localhost/127.0.0.1, so the new code treats it as a public host and emits a wss:// path-routed URL. Without a TLS-terminating reverse proxy on those hosts the resulting WebSocket URL is unreachable, breaking LAN / on-premise development setups. Consider extending isLocal (or a separate isPrivate guard) for RFC-1918 ranges, or documenting that those deployments must also set up a reverse proxy.
1 participant
…bases
EXPOSED_URL doubles as the public RPC endpoint advertised to peers for omniprotocol port discovery, so it must stay as http://host:53550 — peers fail otherwise. The previous protocol-based scheme switch forced ws://host:7047 for that base, which can't be reached through the reverse proxy.
Switch buildWsUrl to a hostname-based rule:
to /tlsn when not supplied
Behaviour:
http://localhost:53550 → ws://localhost:7047
http://127.0.0.1:53550 → ws://127.0.0.1:7047
http://node2.demos.sh:53550 → wss://node2.demos.sh/tlsn/7047/
https://node2.demos.sh/tlsn → wss://node2.demos.sh/tlsn/7047/
Public bases now always route through TLS — assumes the reverse proxy terminates HTTPS at hostname:443 (standard for the production deploy).
Summary by CodeRabbit