Skip to content

fix(security): pin vendored model-viewer bundle to verified upstream - #2709

Open
superm1 wants to merge 1 commit into
mainfrom
fix/model-viewer-integrity
Open

fix(security): pin vendored model-viewer bundle to verified upstream#2709
superm1 wants to merge 1 commit into
mainfrom
fix/model-viewer-integrity

Conversation

@superm1

@superm1 superm1 commented Jul 14, 2026

Copy link
Copy Markdown
Member

Summary

The vendored src/app/src/renderer/vendor/model-viewer.min.js had drifted from the documented upstream and carried no integrity hash, so a webpack import could not detect tampering or corruption.

This PR pins to the pristine artifact and secures it:

  • Replace the bundle with the exact dist/model-viewer.min.js from the npm registry tarball for @google/model-viewer@4.3.1, verified against npm's published sha512 integrity.
  • Add model-viewer.min.js.sha256 (SHA-256 283b06…) and a CI job in docs_and_style.yml that runs sha256sum -c to fail on any drift.
  • Rewrite the vendor README.md to record the hash chain (npm tarball integrity → extracted-file SHA-256) and a hash-preserving update process that validates npm's dist.integrity before extraction.

Review notes

Shadow mapping fix (upstream #5168)

The locally patched 4.3.0 file contained a shadow-depth shader fix from google/model-viewer#5168 correcting the orthographic frustum and floor/blur-plane scaling for models larger than 1×1. That fix is included in the official 4.3.1 release, so the bundle is now pristine upstream with the fix intact.

"Drift guard" vs SRI

The sidecar SHA-256 check is a drift guard rather than a true SRI replacement: the file and checksum can be changed together in the same commit. The npm dist.integrity (SHA-512) is the authoritative upstream trust anchor, and the documented update process verifies it before extracting the file.

@github-actions github-actions Bot added the enhancement New feature or request label Jul 14, 2026
@superm1 superm1 mentioned this pull request Jul 14, 2026
6 tasks

@fl0rianr fl0rianr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, right direction, but not ready yet:

Comment thread src/app/src/renderer/vendor/model-viewer.min.js
Comment thread src/app/src/renderer/vendor/README.md
The vendored model-viewer.min.js had drifted from the documented upstream
(@google/model-viewer@4.3.1) and carried no integrity hash, so a webpack import
could not detect tampering or corruption.

Replace it with the pristine bundle extracted from the npm registry tarball,
pin its SHA-256 in a sidecar checked by CI, and document the hash chain plus a
hash-preserving update process that verifies npm's dist.integrity.
@superm1
superm1 force-pushed the fix/model-viewer-integrity branch from e1bb81e to 0e6e28b Compare July 28, 2026 04:28
@superm1
superm1 requested a review from fl0rianr July 28, 2026 04:29

@fl0rianr fl0rianr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, better, but not fully there yet.

# (2) Independently verify npm's published integrity hash.
EXPECTED=$(curl -s "https://registry.npmjs.org/@google/model-viewer/${VERSION}" \
| python3 -c "import sys,json; print(json.load(sys.stdin)['dist']['integrity'])")
ACTUAL=$(sha512sum "$TARBALL" | awk '{print "sha512:" $1}')

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking: EXPECTED and ACTUAL use different digest representations. npm's dist.integrity is formatted as sha512-<base64>, while sha512sum produces a hexadecimal digest and this code prefixes it with sha512. As a result, the documented update process rejects even the correct upstream tarball.

Generate ACTUAL in npm's SRI representation instead:

Suggested change
ACTUAL=$(sha512sum "$TARBALL" | awk '{print "sha512:" $1}')
ACTUAL=$(python3 - "$TARBALL" <<'PY'
import base64
import hashlib
import pathlib
import sys
digest = hashlib.sha512(pathlib.Path(sys.argv[1]).read_bytes()).digest()
print("sha512-" + base64.b64encode(digest).decode("ascii"))
PY
)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants