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 3 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? |
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