feat: add --tunnel=cloudflare flag with embedded frontend#145
feat: add --tunnel=cloudflare flag with embedded frontend#145S4tyendra wants to merge 1 commit intoekzhang:mainfrom
Conversation
![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: a9141e9bd1
ℹ️ 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".
|
|
||
| /// The SvelteKit static build, embedded at compile time. | ||
| /// Ensure `npm run build` has been run from the workspace root before compiling. | ||
| static BUILD_DIR: Dir<'static> = include_dir!("$CARGO_MANIFEST_DIR/../../build"); |
There was a problem hiding this comment.
Embedding ../../build with include_dir! makes Rust compilation depend on frontend artifacts that are not checked into git (git ls-tree for this commit has no build/ entries), so a clean checkout now fails unless npm run build is run first. This regresses existing Rust-only flows (e.g., .github/workflows/ci.yaml rust jobs run cargo test/cargo clippy without a web build step), and it also breaks source installs in environments that only expect Cargo to be required.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR adds a self-hosted “quick tunnel” mode that can expose a locally spawned sshx-server via Cloudflare, and switches the server’s web UI to be served from an embedded (compile-time) frontend build.
Changes:
- Add a
--tunnel <provider>CLI flag (initiallycloudflare) that starts a localsshx-serverand acloudflaredquick tunnel. - Introduce a new
sshx::tunnelmodule that manages the local server + cloudflared process lifecycle. - Replace filesystem-based static serving in
sshx-serverwith embedded static assets (plus precompressed.br/.gzcontent negotiation).
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| package-lock.json | Updates lockfile entries (npm dependency graph change). |
| crates/sshx/src/tunnel.rs | New tunnel orchestration (local server + cloudflared) and guard lifecycle. |
| crates/sshx/src/main.rs | Adds --tunnel flag and routes controller to local server when tunneling. |
| crates/sshx/src/lib.rs | Exposes the new tunnel module. |
| crates/sshx/Cargo.toml | Adds sshx-server as a dependency for the CLI crate. |
| crates/sshx-server/src/web.rs | Serves embedded frontend build with gzip/brotli negotiation + SPA fallback. |
| crates/sshx-server/Cargo.toml | Adds include_dir + mime_guess for embedded static serving. |
| Cargo.lock | Locks new Rust dependencies and adds sshx-server to sshx deps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// The SvelteKit static build, embedded at compile time. | ||
| /// Ensure `npm run build` has been run from the workspace root before compiling. | ||
| static BUILD_DIR: Dir<'static> = include_dir!("$CARGO_MANIFEST_DIR/../../build"); | ||
|
|
There was a problem hiding this comment.
include_dir!("$CARGO_MANIFEST_DIR/../../build") requires the build/ directory to exist at compile time, but the repo .gitignore excludes /build. On a clean clone, cargo build / cargo test for sshx-server will fail unless a manual npm run build was run first. Consider adding a build.rs to generate (or at least validate and emit a clear error about) the frontend build, or gating the embedded-asset path behind a feature and keeping the previous filesystem ServeDir fallback for dev builds.
| pub fn app() -> Router<Arc<ServerState>> { | ||
| let root_spa = ServeFile::new("build/spa.html") | ||
| .precompressed_gzip() | ||
| .precompressed_br(); | ||
|
|
||
| // Serves static SvelteKit build files. | ||
| let static_files = ServeDir::new("build") | ||
| .precompressed_gzip() | ||
| .precompressed_br() | ||
| .fallback(root_spa); | ||
|
|
||
| Router::new() | ||
| .nest("/api", backend()) | ||
| .fallback_service(get_service(static_files)) | ||
| .fallback(serve_static) | ||
| } | ||
|
|
||
| /// Routes for the backend web API server. | ||
| fn backend() -> Router<Arc<ServerState>> { | ||
| Router::new().route("/s/{name}", any(socket::get_session_ws)) | ||
| } | ||
|
|
||
| /// Serve an embedded static file with content-negotiation for precompressed variants. | ||
| /// | ||
| /// Resolution order for a request path `P`: | ||
| /// 1. `P` with brotli encoding (`P.br`) if client accepts `br` | ||
| /// 2. `P` with gzip encoding (`P.gz`) if client accepts `gzip` | ||
| /// 3. `P` raw | ||
| /// 4. SPA fallback: `spa.html` (same compression priority) for unknown paths | ||
| async fn serve_static(req: Request) -> Response { | ||
| let path = req.uri().path().trim_start_matches('/'); | ||
|
|
||
| // Empty path → "index.html", which SvelteKit puts at root. | ||
| let path = if path.is_empty() { "index.html" } else { path }; | ||
|
|
There was a problem hiding this comment.
Router::fallback(serve_static) will route all HTTP methods (POST/PUT/…) to the SPA/static handler. Previously, the static handler was wrapped in get_service(...), which limited it to GET/HEAD behavior. To avoid surprising behavior and potential security footguns, consider returning 405 Method Not Allowed for non-GET/HEAD requests in serve_static (or route static handling via method-specific routing).
| fn build_response( | ||
| body: &'static [u8], | ||
| content_type: &str, | ||
| encoding: Option<&str>, | ||
| cache_control: &str, | ||
| ) -> Response { | ||
| let mut builder = Response::builder() | ||
| .status(StatusCode::OK) | ||
| .header(header::CONTENT_TYPE, content_type) | ||
| .header(header::CACHE_CONTROL, cache_control); | ||
|
|
||
| if let Some(enc) = encoding { | ||
| builder = builder.header(header::CONTENT_ENCODING, enc); | ||
| } |
There was a problem hiding this comment.
Static responses are content-negotiated on Accept-Encoding, but the response does not set Vary: Accept-Encoding. Without Vary, intermediaries (CDNs/proxies/browsers) can incorrectly cache and serve a brotli/gzip-encoded response to clients that don't support it. Add a Vary: Accept-Encoding header (at least when a compressed variant is served, and ideally always for these static routes).
| impl Drop for TunnelGuard { | ||
| fn drop(&mut self) { | ||
| self.server_task.abort(); | ||
| } | ||
| } |
There was a problem hiding this comment.
TunnelGuard::drop aborts the server_task, but sshx_server::Server::listen spawns additional background tasks (e.g. listen_for_transfers / close_old_sessions) that are not tied to the join handle and will keep running unless Server::shutdown() is called. This can leak tasks after the guard is dropped. Consider keeping a shutdown handle (e.g., store an Arc<Server> inside TunnelGuard) and calling server.shutdown() in Drop, letting the server task exit cleanly (optionally with a timeout before aborting).
No description provided.