fix(cloudxr): sync and serve webpack lazy chunks for OOB static client#684
fix(cloudxr): sync and serve webpack lazy chunks for OOB static client#684gareth-morgan-nv wants to merge 5 commits into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements manifest-driven lazy-chunk asset delivery for the WebXR web client. The Webpack build now generates Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The changes introduce stateful asset-sync logic with manifest parsing, URL fetching, deduplication, and atomic writes. The launcher's conditional fast/slow paths and fallback behavior warrant close review. WSS proxy file resolution requires path-traversal validation. Large docstring updates across the module increase surface area but are straightforward. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/core/cloudxr/python/oob_teleop_env.py`:
- Around line 247-270: The _manifest_file_names function currently accepts
arbitrary strings from the manifest; update it to validate that each entry is a
safe basename by rejecting any entry that contains path separators ("/" or
"\\"), any parent-segment (".."), or is an absolute path, before appending to
names; ensure validation happens where entries are checked (inside the for entry
in files loop) so only clean basenames are returned and downstream code that
uses names (e.g., the download/destination logic) cannot write outside the
intended directory.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a1de5c26-d3ab-4cff-a609-b89686b81eae
📒 Files selected for processing (9)
deps/cloudxr/webxr_client/webpack.common.jsdeps/cloudxr/webxr_client/webpack.prod.jsdocs/source/getting_started/build_from_source/webxr.rstdocs/source/getting_started/quick_start.rstdocs/source/references/oob_teleop_control.rstsrc/core/cloudxr/python/__main__.pysrc/core/cloudxr/python/oob_teleop_env.pysrc/core/cloudxr/python/wss.pysrc/core/cloudxr_tests/python/test_oob_teleop_env.py
| def _manifest_file_names(payload: object) -> list[str] | None: | ||
| """Parse ``asset-manifest.json`` payload into a deduplicated file list. | ||
|
|
||
| Args: | ||
| payload: Decoded JSON object from ``asset-manifest.json`` (expects a | ||
| top-level ``files`` array of non-empty strings). | ||
|
|
||
| Returns: | ||
| Deduplicated basenames to sync, or ``None`` if *payload* is not a valid | ||
| manifest (wrong shape, empty list, or no usable entries). | ||
| """ | ||
| # Reject non-object JSON (or wrong schema version). | ||
| if not isinstance(payload, dict): | ||
| return None | ||
| files = payload.get("files") | ||
| # Require a non-empty array of basenames. | ||
| if not isinstance(files, list) or not files: | ||
| return None | ||
| # Preserve manifest order; skip duplicates and non-string entries. | ||
| names: list[str] = [] | ||
| for entry in files: | ||
| if isinstance(entry, str) and entry and entry not in names: | ||
| names.append(entry) | ||
| return names or None |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Path traversal via malicious manifest filenames.
_manifest_file_names accepts arbitrary strings from the manifest without validating they're safe basenames. If a compromised or malicious manifest contains entries like ../../../.bashrc, the download loop at line 427 (dest = p / name) would write outside the static directory.
Add validation to reject entries containing path separators or .. segments:
Proposed fix
# Preserve manifest order; skip duplicates and non-string entries.
names: list[str] = []
for entry in files:
- if isinstance(entry, str) and entry and entry not in names:
+ if isinstance(entry, str) and entry and entry not in names:
+ # Reject path traversal attempts and subdirectory paths.
+ if "/" in entry or "\\" in entry or entry == ".." or entry.startswith(".."):
+ continue
names.append(entry)
return names or None🤖 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/core/cloudxr/python/oob_teleop_env.py` around lines 247 - 270, The
_manifest_file_names function currently accepts arbitrary strings from the
manifest; update it to validate that each entry is a safe basename by rejecting
any entry that contains path separators ("/" or "\\"), any parent-segment
(".."), or is an absolute path, before appending to names; ensure validation
happens where entries are checked (inside the for entry in files loop) so only
clean basenames are returned and downstream code that uses names (e.g., the
download/destination logic) cannot write outside the intended directory.
|
Do we want to split bundle.js for lazy loading, which is original behavior and is what I disabled. It is less deterministic than having a single bundle.js. Are we able to keep a single bundle.js but reduce the size by removing not needed packages? |
gareth-morgan-nv
force-pushed
the
gmorgan/MultipleChunks
branch
from
b446f75 to
0949d28
Compare
| 3. Build & Run | ||
| -------------- | ||
|
|
||
| Production build |
There was a problem hiding this comment.
I felt the description here is not needed (hidden from robotics/teleop people)
yanziz-nvidia
left a comment
yanziz-nvidia
left a comment
There was a problem hiding this comment.
Reviewed by yanziz-reviewer-bot
Summary
Removes asyncChunks: false to split the ~9.5 MiB bundle.js into bundle.js + bundle.emulator.js; the guard in chunkFilename that throws on unexpected chunk names is a good defensive pattern, and the _OPTIONAL_WEB_CLIENT_ASSETS fallback path handles older deployments correctly.
Legend: 🚫 Blocker · 💡 Suggestion · 🔍 Nit
| Finding | |
|---|---|
| 🚫 | deps/cloudxr/webxr_client/webpack.common.js:114 — chunkFilename lives in webpack.common.js but chunkOptimization (which renames the chunk to 'emulator') is not applied in webpack.dev.js. In dev mode webpack 5 names the chunk from its module path ('emulate', not 'emulator'), so npm start would throw Unexpected async chunk "emulate". Move both chunkFilename and chunkOptimization into webpack.prod.js, or add a /* webpackChunkName: "emulator" */ annotation on the dynamic import and document the assumption. |
| 💡 | src/core/cloudxr/python/wss.py:344 — except OSError returns HTTP 503; a permanently absent optional file should be 404. |
| 💡 | src/core/cloudxr/python/oob_teleop_env.py:174 — Asset list is hardcoded, not manifest-driven; any new async chunk will silently 404 on OOB hosts until added here. Add a comment noting the limitation. |
| 🔍 | src/core/cloudxr/python/__main__.py:89 — Help string is 119 chars; surrounding lines are ≤80. |
| 🔍 | deps/cloudxr/webxr_client/webpack.chunkNames.js:1 — SPDX-only header; other webpack configs in this directory carry the full Apache-2.0 boilerplate. |
Actionables (for bots — copy-paste-ready for AI)
Fix if it makes sense in context — these are agent-generated suggestions, not human-vetted obligations. Skip anything that's wrong, already addressed, or not worth the churn.
webpack.common.js:114— MovechunkFilename+chunkOptimizationintowebpack.prod.jsso the throw-guard only fires where the cacheGroup guarantees the'emulator'name; restore the default inwebpack.common.js.wss.py:344— Changeexcept OSErrorresponse status from 503 to 404 for missing optional static files.oob_teleop_env.py:174— Add a comment above_OPTIONAL_WEB_CLIENT_ASSETSthat the list is hardcoded and new chunks must be added manually here.__main__.py:89— Wrap the 119-char help string at ≤80 chars to match surrounding lines.webpack.chunkNames.js:1— Expand SPDX-only header to the full Apache-2.0 boilerplate used bywebpack.common.js.
|
LGTM. We should be sure this works on all combinations of browsers/devices (and with public vs private released from Pico) |
2 participants
Description
Production webpack builds of the WebXR client emit a main
bundle.jsplus lazy[id].bundle.jschunks (UIKit msdf text, desktop XR emulation, etc.). OOB modes(
--host-client,--usb-local) only downloaded and servedindex.htmlandbundle.js, so lazy chunks 404'd on the headset and in-VR UI text did notrender (#602).
A temporary workaround inlined all async chunks into one ~9.5 MiB
bundle.js(
asyncChunks: falseinwebpack.prod.js). This change removes that workaroundand fixes static hosting end-to-end:
AssetManifestPluginto writeasset-manifest.json; enableoutput.cleanon prod builds.require_web_client_static_dir()reads the manifest (local orfrom the versioned GitHub Pages client) and downloads every listed asset.
Falls back to
index.html+bundle.jswhen no manifest exists (olderpublished clients).
/client/serves the full static tree withpath-traversal-safe resolution and correct MIME types (not a two-file
whitelist).
missing UI text.
Fixes #602
Type of change
Testing
Unit tests
Covers legacy two-file sync (manifest 404) and manifest-driven download of a
lazy chunk (
553.bundle.js).Web client prod build
Manual / integration (Quest, USB-local OOB)
Verified: static cache includes manifest-listed chunks; headset loads UI with
text after cert acceptance. Tested on Linux host with Quest 3 (adb + Wi‑Fi for
ICE).
Pre-commit (run before merge):
Checklist
SKIP=check-copyright-year pre-commit run --all-filesgit commit -s) per the DCOSummary by CodeRabbit
New Features
Documentation