Skip to content

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

Open
superm1 wants to merge 3 commits into
mainfrom
fix/model-viewer-integrity
Open

fix(security): pin vendored model-viewer bundle to verified upstream#2709
superm1 wants to merge 3 commits 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 Outdated
@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.

Comment thread src/app/src/renderer/vendor/README.md Outdated
@superm1
superm1 force-pushed the fix/model-viewer-integrity branch from 0e6e28b to b61b0fe Compare August 16, 2026 16:35
@superm1
superm1 requested a review from fl0rianr August 16, 2026 16:35

@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, this is much closer. I still see one blocker though: model-viewer.min.js itself is unchanged from the base branch, so the PR currently documents and hashes the existing blob as pristine 4.3.1 without actually replacing it.

Could you also make the update snippet fail closed (set -euo pipefail, ideally extract into a fresh temp dir), and change the workflow comment from “equivalent of SRI” to “drift guard” to match the README?

Once those are fixed, this looks good to me.

@superm1
superm1 requested a review from fl0rianr August 16, 2026 20:52

@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, the previous points look good now. One small thing remains: please use mktemp -d for the extraction directory as well, so the update flow is fully isolated and fail-closed even after an interrupted previous run.

It would also be helpful to note that the verified 4.3.1 artifact produces the existing 283b06… SHA-256, which explains why the vendored bundle itself does not appear in the diff.

Merge conflicts, rebase or main sync is due.

With that, this looks good to me.

superm1 and others added 3 commits August 17, 2026 11:28
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.
…h in README

Address final review feedback from fl0rianr:

- Use mktemp -d for extraction directory instead of hardcoded /tmp path
  so the update flow is fully isolated and fail-closed even after an
  interrupted previous run.
- Add a note explaining that the verified 4.3.1 artifact produces the
  existing 283b06… SHA-256, which is why the bundle itself does not
  appear in the diff.
@superm1
superm1 force-pushed the fix/model-viewer-integrity branch from 0e5f0e8 to 34e848a Compare August 17, 2026 16:28
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