Desktop: broker serves the built SPA (/ + /assets) -- FastAPI off the critical path - #80
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds ChangesSPA Static Serving
Sequence Diagram(s)sequenceDiagram
participant Client
participant Broker as createBroker handler
participant spa as desktop/spa.js
participant FS as dist/ filesystem
Client->>Broker: GET /?t=<token>
Broker->>spa: serveIndex(req, res, sopDir)
spa->>spa: tokenOk(t, expected)
alt token invalid or missing
spa-->>Client: 401 NO_TOKEN_PAGE HTML
else token valid
spa->>FS: readFileSync index.html
spa->>spa: inject window.__SMBOS_TOKEN__ before </head>
spa-->>Client: 200 text/html + cache-control: no-store
end
Client->>Broker: GET /assets/app.js
Broker->>spa: serveAsset(req, res, "app.js")
spa->>spa: lexical containment check
spa->>FS: realpathSync symlink check
alt traversal / symlink escape / missing
spa-->>Client: 404
else safe path
spa->>FS: readFileSync asset bytes
spa-->>Client: 200 + content-type from MIME map
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
Comment |
… critical path
The broker now serves the dashboard page itself instead of forwarding it to FastAPI, the
last piece of the switchover. Mirrors dashboard_app's index + assets routes:
- GET / token-gated (?t=); reads frontend/dist/index.html and injects
window.__SMBOS_TOKEN__ before </head> (the server token, not the ?t= value);
the friendly no-token 401 page byte-for-byte; 503 unbuilt / 500 no </head>.
- GET /assets/<path> the hashed, secret-free bundle (no token), path-contained against
traversal, content-type by extension.
dist defaults to ../frontend/dist, or for a packaged app. With this, the broker
owns the WHOLE surface -- the SPA, every read, the SSE live mirror, and all 11 actions; the
only thing still forwarded to FastAPI is /api/settings (its echoed terminal is env-detected).
Verified end-to-end against the REAL built SPA (/?t= -> 200 with the token injected + the
hashed bundle loads; no token -> 401; encoded traversal + missing asset -> 404) and by a
broker unit test (token gate, injection, asset content-type, traversal containment). 36 node.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Adversarial self-review hardening (folded into the latest push): the traversal containment + token gate were sound, but the adversarial pass found one real divergence -- the JS containment was WEAKER than the FastAPI original. path.resolve is purely lexical (it does not follow symlinks), whereas Python Path.resolve dereferences them, so a symlink inside assets/ pointing outside would pass the lexical startsWith check and readFileSync would follow it. Fixed: serveAsset now re-checks containment after fs.realpathSync (both target and base), matching Python -- a symlink that escapes the realpath base 404s. Added a test (a symlink in assets/ -> outside -> 404). The token-injection, the no-store on the token-bearing HTML, the 401-no-leak, and parity on the error pages were all confirmed clean. |
9966012 to
4642934
Compare
![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: 9966012473
ℹ️ 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".
| const target = path.resolve(base, assetPath) | ||
| if (target !== base && !target.startsWith(base + path.sep)) { // containment: no ../ escape |
There was a problem hiding this comment.
If dist/assets contains a symlink (for example from a copied or packaged asset), this lexical path.resolve check passes while fs.statSync/readFileSync follow the link, so /assets/<link> can serve files outside the asset directory without a token. The FastAPI route this replaces resolved (base / path) before the containment check, which rejected symlink escapes; use realpath/equivalent for both base and target before reading.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already fixed in the latest push (our own adversarial pass flagged the same thing): serveAsset now re-checks containment after fs.realpathSync on BOTH base and target, so a symlink inside assets/ that escapes the realpath base 404s -- matching the FastAPI route which resolved the path (dereferencing symlinks) before its containment check. The lexical path.resolve check stays as a fast pre-reject; the realpath check closes the symlink-follow gap. A test was added (a symlink in assets/ -> outside -> 404).
1 participant
The broker now serves the dashboard page itself instead of forwarding it to FastAPI — the last piece of the switchover. Mirrors
dashboard_app's index + assets routes:GET /— token-gated (?t=); readsfrontend/dist/index.htmland injectswindow.__SMBOS_TOKEN__before</head>(the server token, not the?t=value); the friendly no-token 401 page byte-for-byte;503unbuilt /500no</head>.GET /assets/<path>— the hashed, secret-free bundle (no token), path-contained against traversal, content-type by extension.distdefaults to../frontend/dist, or$SMBOS_DISTfor a packaged app.Milestone
With this, the broker owns the whole surface — the SPA, every read, the SSE live mirror, and all 11 actions. The only thing still forwarded to FastAPI is
/api/settings(its echoed terminal is env-detected). FastAPI is off the critical path.Verification
End-to-end against the real built SPA (
/?t=→ 200 with the token injected + the hashed bundle loads; no token → 401; encoded traversal + missing asset → 404) and a broker unit test (token gate, injection, asset content-type, traversal containment). 36 node.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes