Build public remote tunnel toolkit#1
Conversation
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the Remote-SSH-Tunnel project, featuring a Node.js loopback-only relay and a bounded Python MessagePack RPC client to interoperate with VS Code Remote Tunnels. The review feedback highlights several critical improvement opportunities: addressing a potential event listener leak on the abort signal in the Node.js client, replacing fragile strict key-matching checks in the Python RPC protocol parser with robust presence checks to prevent future compatibility failures, and reconfiguring standard output streams in the CLI to prevent crashes on non-UTF-8 terminal encodings.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const outcome = await new Promise((resolve) => { | ||
| const abort = () => resolve(); | ||
| signal.addEventListener("abort", abort, { once: true }); | ||
| finalDisconnect.disconnected.then(resolve).finally(() => { | ||
| signal.removeEventListener("abort", abort); | ||
| }); | ||
| }); | ||
| if (outcome instanceof Error && !signal.aborted) { | ||
| throw outcome; | ||
| } |
There was a problem hiding this comment.
There is a potential event listener leak on the signal object. If the signal is aborted before finalDisconnect.disconnected resolves, the promise returned by waitForDisconnect resolves and the function returns, but the 'abort' event listener is never removed because the .finally block is attached to finalDisconnect.disconnected.then(...), which remains pending. To prevent memory leaks in long-running supervisor loops, ensure the event listener is cleaned up immediately after the promise resolves.
let cleanup;
const outcome = await new Promise((resolve) => {
const abort = () => resolve();
signal.addEventListener("abort", abort, { once: true });
cleanup = () => signal.removeEventListener("abort", abort);
finalDisconnect.disconnected.then(resolve);
});
cleanup?.();
if (outcome instanceof Error && !signal.aborted) {
throw outcome;
}| if set(params) != {"version", "protocol_version"}: | ||
| raise ProtocolError("RPC hello has unexpected version fields") |
There was a problem hiding this comment.
Strict key matching (set(params) != {"version", "protocol_version"}) is fragile and violates the Robustness Principle. If Microsoft updates the VS Code Remote RPC protocol to include any new or extra fields in the version handshake payload, the client will fail with a ProtocolError. It is safer to only verify that the required fields are present.
| if set(params) != {"version", "protocol_version"}: | |
| raise ProtocolError("RPC hello has unexpected version fields") | |
| if "version" not in params or "protocol_version" not in params: | |
| raise ProtocolError("RPC hello is missing required version fields") |
|
|
||
| expected_fields = {"stream"} if method == "stream_ended" else {"stream", "segment"} | ||
| if set(params) != expected_fields: |
There was a problem hiding this comment.
Strict key matching on stream notification parameters (set(params) != expected_fields) can cause the client to crash if any extra or new fields are added to the protocol in the future. It is more robust to only check for the presence of the required fields.
| expected_fields = {"stream"} if method == "stream_ended" else {"stream", "segment"} | |
| if set(params) != expected_fields: | |
| if "stream" not in params or (method == "stream_data" and "segment" not in params): | |
| raise ProtocolError("stream notification is missing required fields") |
| if not isinstance(result, dict) or set(result) != {"exit_code", "message"}: | ||
| raise ProtocolError("RPC response has an invalid result shape") |
There was a problem hiding this comment.
Strict key matching on the command result map (set(result) != {"exit_code", "message"}) will cause a crash if the remote host adds any new fields (such as resource usage, signal info, etc.) to the result dictionary. Only verify that the required fields are present.
| if not isinstance(result, dict) or set(result) != {"exit_code", "message"}: | |
| raise ProtocolError("RPC response has an invalid result shape") | |
| if not isinstance(result, dict) or "exit_code" not in result or "message" not in result: | |
| raise ProtocolError("RPC response has an invalid result shape") |
| stdout = OutputTarget(sys.stdout, sys.stdout.buffer, raw=args.raw_output) | ||
| stderr = OutputTarget(sys.stderr, sys.stderr.buffer, raw=args.raw_output) |
There was a problem hiding this comment.
If the local terminal or console uses a non-UTF-8 encoding (such as CP1252 or CP437 on Windows) and the remote command outputs characters that cannot be represented in that encoding, writing to sys.stdout or sys.stderr will raise a UnicodeEncodeError and crash the CLI. Reconfigure sys.stdout and sys.stderr to use backslashreplace or replace error handlers to ensure the CLI remains robust across different terminal encodings.
if hasattr(sys.stdout, "reconfigure"):
sys.stdout.reconfigure(errors="backslashreplace")
if hasattr(sys.stderr, "reconfigure"):
sys.stderr.reconfigure(errors="backslashreplace")
stdout = OutputTarget(sys.stdout, sys.stdout.buffer, raw=args.raw_output)
stderr = OutputTarget(sys.stderr, sys.stderr.buffer, raw=args.raw_output)![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51b07917c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| text = self._decoder.decode(data, final=final) | ||
| rendered: list[str] = [] | ||
| for character in text: | ||
| if character in "\t\r\n": |
There was a problem hiding this comment.
When an untrusted remote command emits \r, safe mode writes it through unchanged, so a terminal returns the cursor to the start of the current line and subsequent bytes can overwrite or hide earlier output (for example, FAILED\rOK \n). That leaves a terminal-control path in the default renderer despite the surrounding code escaping other Cc controls; escape \r as well or normalize it instead of passing it through.
Useful? React with 👍 / 👎.
2 participants
Summary
assets/vscode-interface/with pinned-source attribution and an exact CC-BY-4.0 license copy; no extension code, binaries, or product assets are redistributedVerification
git diff --checkSecurity impact
This creates a new authenticated tunnel client and private-protocol parser. The security invariants are fail-closed tunnel selection, connect-only service scope, no token in argv/config/logs, environment-token scrubbing, filter-before-connect on every fresh SDK client, no wildcard listener, bounded MessagePack and output state, no implicit shell, opt-in environment forwarding, safe output rendering by default, and full cleanup with an SDK cancellation token plus a post-disposal exit fallback for an observed upstream idle socket handle.
Compatibility and caveats