fix(og): constrain inlined assets to trusted roots - #2172
Conversation
|
/bigbonk review for issues |
commit: |
|
Review: fix(og): constrain inlined assets to trusted rootsI reviewed the security hardening in A few points worth considering, none blocking: 1.
|
|
Review posted to PR #2172. SummaryI reviewed the OG asset containment hardening. The fix is sound: it canonicalizes paths with I raised 4 non-blocking points:
No correctness blockers found. |
|
/bigbonk review for issues |
Review: fix(og): constrain inlined assets to trusted rootsI re-reviewed after the second commit ( A few observations, none blocking: 1.
|
|
Review posted to PR #2172. SummaryI reviewed the OG asset containment fix, including the second commit ( Verification: Conclusion: no correctness or security blockers. The containment design is sound — every asset is independently canonicalized and confined with I raised 3 non-blocking refinements:
|
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(og): constrain inlined assets to trusted roots
I reviewed the current HEAD (43fb13a), which matters here: the third commit reverted the boundary approach. The two earlier ask-bonk reviews on this PR describe commit cbda50c's isNodeModulesPath + findPackageRoot design — but 43fb13a test(og): cover canonical boundary intersections removed isNodeModulesPath and restored a getNodeModulesPackageRoot (now also handling module paths outside the project root via path.parse().root). So the prior "this already resolved most feedback" reviews are stale against what's actually shipping.
I re-verified the current code against the security goal (an asset is inlined only if its canonical realpath sits inside a trusted root). The containment guard is sound: every asset is independently canonicalized with realpathNative and confined via isPathInside(realAssetRoot, realPath) (og-assets.ts:179-183), and realAssetRoot is always either the canonical project root, the canonicalized node_modules package dir, or a workspace package root. I traced symlink-to-secret, .env exfiltration, pnpm/nested node_modules, root-inside-node_modules, and symlinked-module-file cases with standalone scripts — every escape attempt fails closed. vp test run tests/og-inline.test.ts (27 pass) and vp check on both files are green.
No correctness or security blockers. Findings below are refinements / fail-safe limitations worth documenting.
1. Stale prior reviews vs. current code (process note)
The current getNodeModulesPackageRoot (og-assets.ts:77-95) is not the code described by the two earlier reviews. Whoever reads those reviews should re-check against 43fb13a. The reverted-to function keys the boundary off path string segments, then canonicalizes the result with realpathNative(packageRoot) (og-assets.ts:156-161) — a different shape than the findPackageRoot-only approach that was reviewed and approved.
2. getNodeModulesPackageRoot reasons over non-canonical paths (og-assets.ts:154)
getNodeModulesPackageRoot(projectRoot, modulePath) is called with the raw, non-canonical projectRoot/modulePath, and its segment math (isPathInside, path.relative, lastIndexOf("node_modules")) runs on those raw strings. The result is then canonicalized. Meanwhile the project-boundary branch (line 162) and asset checks use realpaths. This interleaves canonical and non-canonical reasoning in the boundary decision. I confirmed it can only over/under-constrain realAssetRoot, never widen it past the asset realpath guard — so it's not an escape — but computing the package-root decision from the canonical pair would make the invariant auditable in one place.
3. lastIndexOf("node_modules") narrows the boundary for deps with an internal node_modules (og-assets.ts:85)
For a dependency that ships a literal node_modules inside its published files (node_modules/foo/dist/node_modules/bar/...), the boundary resolves to the inner bar package, so foo's own dist/ assets would be rejected. Rare, only over-constrains (fails safe), but undocumented. A one-line comment on the lastIndexOf assumption would help.
4. Symlinked module file (dir not symlinked) rejects ./-relative assets
When the module file is a symlink whose target lives outside its node_modules/<pkg> dir, realModuleDir (symlink target) diverges from realAssetRoot (the canonicalized package dir), so a legitimate ./asset next to the real module file is rejected. Fail-safe, but a behavioral gap vs. inlining the asset. No test covers it; worth a comment noting the package dir — not the resolved module file location — is the boundary.
5. findPackageRoot walks to the nearest ancestor package.json
For an out-of-root module with no package.json in its own dir, the workspace branch (og-assets.ts:164-167) can resolve realAssetRoot to a wide ancestor. Intentional for linked workspace deps and still asset-realpath-confined, but a comment that this is "nearest package.json, not strictly the package dir" would document the looser-than-node_modules boundary.
Overall: solid defense-in-depth with strong adversarial coverage. The above are refinements and documentation, not correctness blockers. The one thing I'd genuinely flag is finding #1 — the prior approvals on this PR reviewed code that no longer exists.
|
Review posted to PR #2172. SummaryI reviewed the current HEAD ( Verification: Conclusion: no correctness or security blockers. Every asset is independently canonicalized with Findings raised (1 process, 4 refinements — none blocking):
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(og): constrain inlined assets to trusted roots
Re-reviewed at HEAD 1589ecd ("support file-symlinked dependency modules"), the fourth commit. This commit landed after the last ask-bonk review and directly closes the gap that review flagged.
What changed since the last review (43fb13a): commit 1589ecd splits the node_modules branch (og-assets.ts:159-175) — it canonicalizes the package dir to realPackageRoot, and when the canonical module file falls outside that dir (a file-level symlink into a workspace package) it now confines assets to findPackageRoot(realModuleDir) instead of failing. It also adds the documentation comments the prior reviews asked for: the lastIndexOf("node_modules") "narrows the boundary and fails safe" note (og-assets.ts:85-87) and the "linked workspace" intent comment (og-assets.ts:179-180). This resolves prior findings #3, #4, and #5.
Verification: vp test run tests/og-inline.test.ts (28 pass) and vp check on both files are green. I traced the boundary logic with standalone scripts across: symlink-to-secret, .env exfiltration, pnpm/nested/scoped node_modules, root-inside-node_modules, symlinked project root, file-symlinked module, and sibling-prefix paths (/a/b vs /a/bc). Every escape attempt fails closed.
Conclusion: no correctness or security blockers. The containment guard is sound. readAsBase64 (og-assets.ts:192-211) is the single chokepoint: every asset is canonicalized via realpathNative and confined with isPathInside(realAssetRoot, realPath) (line 199) — a canonical-on-canonical comparison. The only way to escape would be a too-wide realAssetRoot, and all four assignment branches resolve to either the canonical project root, the canonical node_modules package dir, or findPackageRoot of a canonical dir (with null → bail). isPathInside correctly rejects parent escapes and sibling-prefix collisions.
Test coverage now covers the prior gaps
- File-symlinked dependency module (
tests/og-inline.test.ts:251) — exercises the newog-assets.ts:168-175branch. - Parent-relative application asset under a symlinked root that escapes (
tests/og-inline.test.ts:354) — locks in the intersection the prior review wanted. - Symlinked-root with
../application asset that stays inside (tests/og-inline.test.ts:337).
Minor refinements (non-blocking, all carried over from prior reviews — fine to defer)
-
getNodeModulesPackageRootstill reasons over raw, non-canonical paths (og-assets.ts:157) before its result is canonicalized at line 162, while the subsequentisPathInsidechecks userealModulePath. I re-confirmed this can only over/under-constrainrealAssetRoot, never widen it past the line-199 asset guard, so it is not an escape. Computing the package-root decision from the canonical pair would keep all boundary reasoning canonical in one place. -
realModulePathkeeps a non-canonical basename in the fallback path (og-assets.ts:148-151). When the full-pathrealpathfails, the fallback canonicalizes only the dir and rejoinspath.basename(modulePath). Harmless for containment (the asset is independently realpathed), but a one-line note that the basename is intentionally left unresolved would help future readers. -
findPackageRootresolves to the nearest ancestorpackage.json(og-assets.ts:63-75), a looser boundary than the exactnode_modules/<pkg>dir for the workspace/file-symlink branches. Intentional for linked workspace deps and still asset-realpath-confined; the new comment at line 179 covers the workspace branch but not the file-symlink branch at line 172.
Overall: solid defense-in-depth with strong adversarial coverage, and the prior review's one substantive gap (file-symlinked module) is now closed and tested.
|
Review posted to PR #2172. SummaryI reviewed the OG asset containment fix at the current HEAD Key finding (process + substance): the prior reviews described commit Verification: Conclusion: no correctness or security blockers. The single chokepoint Test coverage now includes the previously-missing file-symlinked module case ( I carried over 3 non-blocking refinements (canonical/raw asymmetry in |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(og): constrain inlined assets to trusted roots
Re-reviewed at HEAD 30ed1f3 ("docs(og): clarify logical asset boundary resolution"), the fifth commit. It is comments-only (17 insertions / 6 deletions in og-assets.ts, no logic change vs 1589ecd): it documents the deliberate use of logical Vite paths in getNodeModulesPackageRoot before realpath resolution (lines 81-83) and the lastIndexOf("node_modules") "narrows the boundary and fails safe" assumption (lines 91-93). This closes the documentation items prior reviews raised (findings #2/#3).
Verification: vp test run tests/og-inline.test.ts (28 pass). I independently traced the boundary logic with standalone scripts across: out-of-root node_modules module, scoped pnpm/nested layouts, project-root-inside-node_modules, package with an internal node_modules dir, and the findPackageRoot→/ worst case.
Conclusion: no correctness or security blockers. The single chokepoint is readAsBase64 (og-assets.ts:203-222): every asset is canonicalized via realpathNative and confined with isPathInside(realAssetRoot, realPath) (line 210), a canonical-on-canonical comparison. All four realAssetRoot branches resolve to a canonical trusted root or bail with null, and every symlink/parent-escape attempt I constructed fails closed.
One concern worth weighing before merge (not a blocker): findPackageRoot can climb past the package dir
findPackageRoot (og-assets.ts:63-75) walks to the nearest ancestor package.json. It is reached on two paths — the file-symlinked-module branch (:183) and the out-of-both-roots workspace branch (:192). In both, the boundary becomes whatever ancestor first carries a manifest, which is looser than the exact node_modules/<pkg> dir that getNodeModulesPackageRoot pins.
I confirmed the meaningful failure mode: if the resolved realAssetRoot is wide, the guard weakens proportionally, because isPathInside(root, target) is satisfied by any descendant of root (isPathInside("/", "/etc/passwd") → true). It does not reach / in practice — findPackageRoot returns null at the filesystem root (:72), so a missing manifest fails closed. But in a monorepo whose root carries a package.json, a dependency module that lacks its own package-dir manifest (or a file-symlink target placed above the package dir) would have realAssetRoot resolve to the monorepo root, letting that one module inline assets from sibling packages it shouldn't reach. That is a narrower trust boundary than the PR title implies ("the dependency package that owns the transformed module"), even though it is not a full FS escape.
Severity is low (requires a manifest-less package dir or an unusual symlink target, and stays within the monorepo), so it is fine to defer — but it is the one behavioral gap I'd flag explicitly, and there is no test asserting the boundary stays at the package dir when an ancestor package.json exists above it. A negative test (dependency package with no own package.json, sibling asset under a monorepo-root manifest → expect null) would lock down the intended boundary.
Minor (carried over, fine to defer)
getNodeModulesPackageRootreasons over raw, non-canonical paths (:84-103) before the result is canonicalized at:171. Re-confirmed this can only over/under-constrainrealAssetRoot, never widen past the:210asset guard — now documented by the new comment.- Fallback
realModulePathkeeps a non-canonical basename (:160). Harmless for containment (asset is independently realpathed); the inline comment at:157-159now explains the intent.
Overall: solid defense-in-depth with strong adversarial coverage (symlink-to-secret, .env exfiltration, pnpm/nested/scoped layouts, symlinked root, file-symlinked module). The findPackageRoot boundary-width note is the only thing I'd consider tightening or testing before merge.
|
Review posted to PR #2172. SummaryI reviewed PR #2172 (OG asset containment hardening) at the current HEAD Verification: Conclusion: no correctness or security blockers. The single chokepoint The one concern I'd weigh before merge (non-blocking): I also carried over 2 minor doc-only items (canonical/raw asymmetry in |
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(og): constrain inlined assets to trusted roots
Re-reviewed at HEAD 27d36ad ("pin symlinked modules to matching packages"), the sixth commit, which landed after the last ask-bonk review. This commit directly targets the findPackageRoot boundary-width concern the prior review flagged as the one thing worth weighing before merge.
What changed since 30ed1f3: findPackageRoot gained an optional expectedPackageName argument (og-assets.ts:63-83). The file-symlinked-module branch (og-assets.ts:197-200) now passes the owning package name derived from the logical node_modules path via the new getNodeModulesPackageName helper, so the walk to a manifest only succeeds if that manifest's name matches. A new negative test (tests/og-inline.test.ts:276) proves a file-symlink whose ../../ asset escapes to a monorepo-root manifest now returns null. This closes the file-symlink half of the prior boundary-width finding.
Verification: vp test run tests/og-inline.test.ts (29 pass) and vp check on both files are green. I also traced the boundary logic with standalone scripts across out-of-root, scoped/pnpm/nested node_modules, project-root-inside-node_modules, internal-node_modules, file-symlinked-module, and npm-alias cases.
Conclusion: no correctness or security blockers. readAsBase64 (og-assets.ts:220-239) remains the single chokepoint — every asset is canonicalized via realpathNative and confined with isPathInside(realAssetRoot, realPath) (:227), a canonical-on-canonical comparison. Every escape attempt I constructed fails closed (returns null), and getNodeModulesPackageName derives the expected name correctly for plain, scoped, and pnpm layouts.
1. The fix is asymmetric: the out-of-both-roots workspace branch still climbs to an ancestor manifest (og-assets.ts:206-211)
This commit name-pins the file-symlink branch but leaves the sibling workspace branch unguarded:
} else {
// Modules outside both the project and node_modules are treated as
// linked workspace packages and confined to their nearest manifest.
const workspacePackageRoot = await findPackageRoot(realModuleDir); // <-- no expectedPackageName
if (workspacePackageRoot === null) return null;
realAssetRoot = workspacePackageRoot;
}This is the same "climb to nearest ancestor package.json" risk the prior review flagged, and it is reached for a linked workspace module (file: dep / a resolve.alias to a path outside the project that is not under any node_modules). I reproduced the widening: a workspace module at mono/packages/ui/dist/index.js with no package.json in its own package dir resolves realAssetRoot to the monorepo root (mono/, which carries a package.json), making mono/packages/other/secret.txt inlineable:
findPackageRoot resolved boundary: /tmp/.../mono
Would sibling secret be inside boundary? true
It is not a full FS escape (findPackageRoot returns null at the FS root, failing closed), and it stays within the monorepo, so severity is low — but it is a narrower trust boundary than the PR title implies ("the dependency package that owns the transformed module"), and it is now inconsistent with the file-symlink branch that this very commit hardened. The existing linked-workspace tests (:208) only pass because their package dirs each carry their own package.json; there is no test for the manifest-less workspace-package-dir case. Either pass an expectedPackageName here too (deriving it from the logical path when available), or add a comment + negative test documenting that this branch intentionally trusts the nearest ancestor manifest.
2. getNodeModulesPackageName trusts the node_modules alias, not the canonical manifest name (og-assets.ts:114-118, :199)
The expected name is derived from the node_modules/<dir> basename, but with an npm alias ("og-helper": "npm:real-pkg@1") the directory is og-helper while the target manifest's name is real-pkg. findPackageRoot(realModuleDir, "og-helper") then walks past the real package dir (name mismatch) and returns null. This only over-constrains (a legitimate aliased dependency's ./asset stops inlining and falls back to a runtime fetch), so it fails safe — but it is a behavioral gap with no test. A one-line comment noting that aliased packages are intentionally not matched here would help future readers.
Minor (carried over, fine to defer)
getNodeModulesPackageRootstill reasons over raw, non-canonical paths (og-assets.ts:88-112) before its result is canonicalized at:185. Re-confirmed this can only over/under-constrainrealAssetRoot, never widen past the:227asset guard; the new comment at:89-91documents the intent.- The fallback
realModulePathkeeps a non-canonical basename (og-assets.ts:174); harmless for containment and documented inline.
Overall: solid defense-in-depth with strong adversarial coverage (symlink-to-secret, .env exfiltration, pnpm/nested/scoped layouts, symlinked root, file-symlinked module, and now the file-symlink-escape negative case). The one thing I'd genuinely tighten before merge is finding #1 — applying the same name-pinning to the workspace branch so the fix is symmetric, or explicitly testing/documenting why that branch trusts the nearest ancestor manifest.
|
Review posted to PR #2172. SummaryI reviewed PR #2172 (OG asset containment hardening) at the current HEAD What this commit does: It adds an optional Verification: Conclusion: no correctness or security blockers. The single chokepoint Findings raised (none blocking):
Plus 2 carried-over doc-only items. The one thing I'd tighten before merge is finding #1, to make the hardening symmetric across both |
|
Review posted to PR #2172. SummaryI reviewed PR #2172 (OG asset containment) at the current HEAD Verification: Conclusion: no correctness or security blockers. The optimization replaces a flat
The security boundary is unchanged: I could not construct a within-monorepo sibling-leak through any alias shape, and the recurring sibling-leak class from the PR's history remains closed. I raised 2 minor non-blocking notes (empty-string |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(og): constrain inlined assets to trusted roots
Re-reviewed at HEAD 6a787e8 ("refactor(og): extract asset ownership policy"), the seventeenth commit, which landed after the last ask-bonk review.
What this commit does: it is a pure refactor with no intended behavior change. The ~311 lines of containment logic that lived inline in createOgInlineFetchAssetsPlugin are extracted into a typed OgAssetOwnership class in a new module (packages/vinext/src/plugins/og-asset-ownership.ts), and the plugin now delegates to it through four seams: configure (was configResolved's body), reset (was buildStart's linkedPackageRoots.clear()), shouldTrackImport + recordResolvedImport (was resolveId's body), resolveModuleBoundary (was the transform boundary computation), and resolveContainedAsset (was the readAsBase64 realpath/isPathInside guard). A focused unit test file (tests/og-asset-ownership.test.ts) covers the class directly. This is exactly the layering AGENTS.md asks for — runtime behavior in an importable, unit-tested server/plugin module rather than inline in plugin glue.
Verification: vp test run tests/og-asset-ownership.test.ts tests/og-inline.test.ts (51 pass) and vp check on all three changed files are green. I diffed the extracted class against the inline implementation at c4e06ae line-by-line and reproduced the boundary logic through the new class.
Equivalence confirmed. Each method is a faithful move of the prior inline code:
resolveModuleBoundary(og-asset-ownership.ts:215-273) reproduces the oldtransformboundary block, including the dual-realpath fallback that retains the non-canonical basename (:228), thegetNodeModulesPackageRoot→ canonicalize → file-symlink (declared-namefindPackageRoot) path, the project-root branch, and thelinkedPackageRootslongest-match selection. It now returns{ assetRoot, moduleDir }wheremoduleDir = path.dirname(realModulePath)— identical to the oldrealModuleDirthat fed relative asset resolution.resolveContainedAsset(:275-282) is the unchanged chokepoint:realpathNative(assetPath)thenisPathInside(assetRoot, realPath), canonical-on-canonical.recordResolvedImport/resolveAliasPackageRoot(:188-213,:298-341) preserve the alias capture/directory/file branching and thegetAliasedPackageName,packageOwnsAliasFile,getPackageNameFromSpecifierhelpers verbatim.
Security conclusion: no change, no escape. The asset guard is still a canonical-on-canonical isPathInside chokepoint, findPackageRoot fails closed at the FS root, and the alias directory/file branches still narrow to the resolved module's owning package. I reproduced the previously-flagged escape shapes through the new class and they fail closed — e.g. the realistic monorepo layout (every workspace package carries its own package.json, as all package managers produce) narrows @myorg/ui to mono/packages/ui and rejects a sibling package's ../../other/secret.txt.
Notes (none blocking)
-
The known
findPackageRootresidual carries over unchanged. I reproduced the long-standing low-severity case the prior reviews repeatedly noted: a manifest-less linked package dir whose nearest ancestor manifest'snameequals the import's package name, with the project living outside that monorepo, resolvesassetRootup to the monorepo root and lets a sibling package's asset inline. It requires a layout no package manager produces (a leaf package with no ownpackage.jsonunder a root manifest named after that leaf) and stays within the monorepo tree (not a full FS escape). This is purely inherited — the refactor moved the code without altering it — so it's fine to keep deferred, consistent with the prior reviews. -
Minor redundant work on the gated path.
og-assets.ts'sresolveIdnow callsshouldTrackImport(source)(which runsfindAlias+getExpectedPackageName) and thenrecordResolvedImport(source, ...)re-runs both. The old inline code computed them once. It's only on the already-gated alias/dependency path and the expensivethis.resolve/realpath/walk is unchanged, so the cost is negligible — butshouldTrackImportcould return the resolved{alias, expectedPackageName}(orrecordResolvedImportaccept them) to avoid the double lookup. -
Carried-over doc-only items (fine to defer):
getNodeModulesPackageRootreasons over raw, non-canonical paths (og-asset-ownership.ts:80-101) before its result is canonicalized inresolveModuleBoundary— can only over/under-constrain, never widen past theresolveContainedAssetguard; the fallbackrealModulePathkeeps a non-canonical basename (:228), harmless for containment.
Overall: a clean, behavior-preserving extraction that improves maintainability and adds direct unit coverage for the ownership policy, matching AGENTS.md's "keep entry/plugin glue thin, put runtime behavior in importable typed modules" guidance. The containment design remains sound and the recurring sibling-leak class stays closed for real-world layouts. No correctness or security blockers.
|
Review posted to PR #2172. SummaryI reviewed PR #2172 (OG asset containment) at the current HEAD What this commit does: a pure refactor — the ~311 lines of containment logic that lived inline in Verification: Conclusion: no correctness or security blockers. The asset guard remains a canonical-on-canonical Notes raised (all non-blocking):
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(og): constrain inlined assets to trusted roots
Re-reviewed at HEAD 299b29d ("test(og): cover linked package fonts in e2e"), the commit added after the last ask-bonk review. It adds an e2e fixture: a tests/fixtures/og-font-package package whose index.js does fetch(new URL("./noto-sans.ttf", import.meta.url)), wired into app-basic and app-router-cloudflare via a string resolve.alias ("@test/og-font" -> tests/fixtures/og-font-package), plus a new og-image.spec.ts case that runs across app-router (Vite dev), cloudflare-dev, and cloudflare-workers (real wrangler/miniflare build).
The intent — end-to-end coverage of the resolveId provenance path that the prior 10 commits built — is exactly what earlier reviews asked for. But I reproduced the new fixture's shape through the real plugin in a real build() and found a blocking problem: the alias-based provenance machinery is unreachable in a real Vite pipeline, so this new e2e exercises Rollup's built-in asset handling, not vinext — and for a realistically-sized font it produces the exact Invalid URL-on-Workers failure this PR exists to prevent.
1. (blocking) String/regex resolve.alias imports never reach vinext's resolveId, so the linked-package boundary is never recorded
vinext's OG plugin is enforce: "pre", but Vite's built-in alias plugin runs before all user enforce: "pre" resolveId hooks. So for an aliased import, vinext's resolveId never sees the bare specifier (@test/og-font) — it only sees the already-resolved absolute path. shouldTrackImport(absolutePath) is false (findAlias can't match an absolute path against the alias find, and getPackageNameFromSpecifier returns null for a /-prefixed string), so recordResolvedImport never runs, linkedPackageRoots stays empty, and resolveModuleBoundary returns null for the package module.
I traced this through createOgInlineFetchAssetsPlugin() in a real build() with the exact e2e fixture shape (@test/og-font → an external dir):
ALL resolveId sources seen by plugin: ["<app>/entry.js", "<pkgdir>"] // never "@test/og-font"
recordResolvedImport: never called
resolveModuleBoundary(<pkg>/index.js) -> null
The same is true for the regex-alias shape ({ find: /^ui\/(.*)$/, replacement: "<pkg>/$1" }): vinext's resolveId only ever observes the resolved absolute path, never ui/index.js. All of resolveAliasPackageRoot / getAliasBoundaryPath / packageOwnsAliasFile / the alias-bucketing perf work is dead code in a real build — it's only reachable via the unit tests, which hand-call resolveId.call(..., source, ...) (resolveLinkedPackage, og-inline.test.ts:29-51, and the hand-driven resolveId.call(...) at :427-433) with a bare specifier that Vite never actually delivers.
The node_modules dependency path is unaffected and works correctly — Vite does not alias-rewrite a bare node_modules import, so getExpectedPackageName matches and the IIFE is produced (I confirmed atob(-IIFE output for a real node_modules dep). The gap is specific to resolve.alias-linked packages — which is exactly what the new fixture uses.
2. (blocking, consequence of #1) The new e2e will hit Invalid URL on Workers for a real-sized font
Because vinext does nothing for the aliased package, the fetch(new URL("./noto-sans.ttf", import.meta.url)) is left for Rollup/Vite to handle. The fixture's noto-sans.ttf is ~27 KB — above Vite's 4 KB assetsInlineLimit — so Rollup emits it as a separate hashed asset and rewrites the call to:
fetch(new URL("/assets/noto-sans-BVfvdXyH.ttf", "" + import.meta.url))(reproduced through the real plugin + build() with a 27 KB font). On Cloudflare Workers import.meta.url === "worker", so "" + import.meta.url === "worker" and new URL("/assets/noto-sans-…ttf", "worker") throws TypeError: Invalid URL — the precise failure mode the whole plugin exists to prevent. So the cloudflare-workers project of this new test should fail (or the route 500s). With a tiny font, Rollup inlines a data: URL instead (fetch(new URL("data:font/ttf;base64,…", "" + import.meta.url))), which workerd happens to accept — which is why a small fixture would pass green while still proving nothing about vinext. The header comment's claim that this "exercises resolveId provenance and package-scoped asset containment" does not hold: I verified resolveModuleBoundary returns null for this fixture, so vinext's containment path is never taken.
Suggested fix: make the boundary resolution reachable for aliased imports. Two complementary options:
- In
resolveId, whenthis.resolve(source, importer)yields an id whose bare-specifier form differs, keyshouldTrackImport/recordResolvedImportoff the resolved id as well (e.g. recognize that the resolved absolute path lives under a configured alias target and recordfindPackageRootof it). I.e. don't rely on seeing the bare specifier. - Or, since
transformalready has the resolved module id, haveresolveModuleBoundary's out-of-both-roots branch fall back tofindPackageRoot(realModuleDir)(name-pinned against the owning manifest) whenlinkedPackageRootshas no hit — the same self-contained path the pre-8708478code used — so a missingresolveIdobservation degrades to inlining rather than silently dropping the asset.
Either way, add a real build() test that asserts the vinext IIFE is emitted (code.includes("atob(") / Promise.resolve(), not just that the base64 appears anywhere — the existing "real Vite build" tests (og-inline.test.ts:315, :352) pass only incidentally: I confirmed their assertion is satisfied by Rollup's own data: URL (fetch(new URL(\data:font/ttf;base64,…`, …))), with resolveModuleBoundaryreturningnullandrecordResolvedImport` never firing. They would still pass even if the vinext plugin were deleted.
Minor (carried over, fine to defer)
getNodeModulesPackageRootreasons over raw, non-canonical paths before its result is canonicalized — can only over/under-constrain, never widen past theresolveContainedAssetguard; documented inline.- The fallback
realModulePathkeeps a non-canonical basename (og-asset-ownership.ts:228); harmless for containment.
Net
The readAsBase64/resolveContainedAsset chokepoint remains a sound canonical-on-canonical isPathInside guard, and the node_modules dependency path is correct and well-covered — no security/escape regression. But the new e2e was meant to prove the linked-package (resolve.alias) path works end-to-end, and instead reveals that path is inert in a real Vite build: Vite resolves aliases before vinext's resolveId, so vinext never records the boundary, and a realistic-sized aliased font falls back to fetch(new URL("/assets/…", "worker")) → Invalid URL on Workers. I'd treat #1/#2 as blocking for the linked-package use case this PR claims to support, and verify the new cloudflare-workers e2e actually passes (with the 27 KB fixture font) before merge.
|
Review posted to PR #2172. SummaryI reviewed PR #2172 at the current HEAD Verification: Key finding — blocking for the linked-package use case (no security regression): The alias-based provenance machinery built across this PR's 10+ commits is unreachable in a real Vite pipeline. Vite's built-in alias plugin resolves
I also showed the existing "real Vite build" tests ( The |
|
/bigbonk review for issues |
2 similar comments
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(og): constrain inlined assets to trusted roots
Re-reviewed at HEAD b162bc2 ("fix(og): recognize resolved package aliases"), the commit added after the last ask-bonk review. It directly targets that review's blocking finding (the alias-based provenance was dead code in a real Vite build, so the new linked-package e2e would hit Invalid URL on Workers).
Verification: vitest run tests/og-asset-ownership.test.ts tests/og-inline.test.ts (53 pass) and vp check on the four changed files are green. I reproduced the boundary logic through the OgAssetOwnership class and, crucially, through real build() runs matching the e2e fixture shape.
The prior blocking finding is genuinely fixed (string-alias path)
The new resolveConfiguredAliasRoot (og-asset-ownership.ts:300-301, 372-420) resolves the boundary off the resolved module id rather than the bare specifier, so it no longer depends on resolveId observing the bare import (which Vite's alias plugin consumes before any enforce: "pre" hook — I confirmed resolveId only ever sees the resolved absolute path). I reproduced the exact e2e fixture shape in a real build() (alias @test/og-font → pkg/lib, package.json at pkg/ root, 27,748-byte noto-sans.ttf):
contains atob( (vinext IIFE): true
contains /assets/noto (rollup asset): false
contains font base64: true
leftover fetch(new URL(...): false
So the font is inlined and there is no residual fetch(new URL(..., import.meta.url)) to throw Invalid URL on Workers. The two real-build tests were also strengthened (20 KB fonts + expect(code).toContain("atob(") + expect(code).not.toContain("/assets/font-")), which correctly proves the path is not dead code. The string-alias cloudflare-workers e2e should now pass. No security/escape regression: resolveContainedAsset (:304-311) remains a canonical-on-canonical isPathInside chokepoint, findPackageRoot fails closed at the FS root, and the adversarial sibling-leaks I constructed (string alias to a non-package parent dir; @scope/* -> packages/$1 reaching a sibling package) all fail closed.
1. (blocking for the regex-alias use case) Capture-alias @scope/* -> packages/$1 over-rejects legitimate scoped packages → Invalid URL on Workers
The fallback resolveConfiguredAliasRoot is now solely responsible for capture (regex) aliases, because — exactly as the new code acknowledges — Vite resolves them before resolveId. But its capture branch (og-asset-ownership.ts:381-394) rejects a legitimate, manifest-carrying scoped package linked via the most common monorepo mapping. For a regex find, packageName is forced to null (:378-379: only string finds derive a name), so packageOwnsAliasDirectory(packageRoot, null, staticRoot) (:389) can only succeed via staticRoot === packageRoot or a main-field match. For @scope/* -> packages/$1, staticRoot is <packages> (the parent of all packages) while packageRoot is <packages>/ui, so both clauses fail and the boundary is null.
I reproduced this in a real Vite build with a legitimate @scope/ui package (its own package.json, main: index.js) that fetches its own 27 KB ./font.ttf:
resolve.alias: [{ find: /^@scope\/(.*)$/, replacement: "<packages>/$1" }]
atob( (inlined): false <- NOT inlined
/assets asset emitted: true
leftover fetch(new URL: true <- throws "Invalid URL" on Workers
This is the exact failure mode the PR exists to prevent, now hitting a mainstream layout. The non-separator shape (packages/pkg-$1, dir pkg-ui) over-rejects identically. It is fail-safe for security (it only over-rejects, never widens — no leak), but it defeats inlining for @scope/* -> packages/*, which is the standard tsconfig/Vite monorepo alias.
The tell is the asymmetry with the recordResolvedImport path: when resolveId does see the bare specifier (e.g. a node_modules dep), resolveAliasPackageRoot correctly resolves the boundary to the owning package via findPackageRoot(path.dirname(realResolvedPath)). The fallback should do the same — derive the boundary from findPackageRoot of the resolved module dir and require isPathInside(staticRoot/aliasBoundary, packageRoot) + isPathInside(packageRoot, realModulePath) (the dual check resolveAliasPackageRoot already uses for directory aliases), instead of packageOwnsAliasDirectory against a null package name. That narrows to the owning package (closing the sibling-leak) and accepts the legitimate scoped package. Add a positive test: @scope/* -> packages/$1 with @scope/ui (own package.json) inlining ./font.ttf → expect atob( in a real build; and a negative sibling test → expect null.
The new e2e only exercises the string alias (@test/og-font), so this regex-capture gap is untested.
Minor (carried over, fine to defer)
getNodeModulesPackageRootreasons over raw, non-canonical paths (og-asset-ownership.ts:80-101) before its result is canonicalized inresolveModuleBoundary— can only over/under-constrain, never widen past theresolveContainedAssetguard; documented inline.- The fallback
realModulePathkeeps a non-canonical basename (:252); harmless for containment.
Net
The string-alias linked-package path — what the new e2e fixture uses — now works end-to-end in a real build, closing the prior review's blocking finding, with strengthened real-build tests. The one thing I'd fix before merge is finding #1: the fallback's capture branch rejects legitimate scoped packages under the common @scope/* -> packages/$1 alias, reproducing the Invalid URL-on-Workers failure for that layout. Reuse the resolved-module owning-package narrowing (as resolveAliasPackageRoot already does) and add real-build positive/negative tests for the regex-capture shape.
|
Review posted to PR #2172. SummaryI reviewed PR #2172 at the current HEAD Verification: Conclusion: the prior blocking finding is genuinely fixed for the string-alias path. The new New blocking finding (#1): The fallback's regex-capture branch over-rejects legitimate scoped packages under the mainstream Plus 2 carried-over doc-only items. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(og): constrain inlined assets to trusted roots
Re-reviewed at HEAD ce1cd86 ("fix(og): support regex package aliases"), the commit added after the last ask-bonk review. It directly targets that review's blocking finding #1 (the regex-capture fallback over-rejected legitimate scoped packages under the mainstream @scope/* -> packages/$1 mapping, producing Invalid URL on Workers).
Verification: vp test run tests/og-asset-ownership.test.ts tests/og-inline.test.ts (58 pass) and vp check on both plugin files are green. I reproduced the boundary logic through the OgAssetOwnership class and a new real-build test exercises the previously-broken shape.
The prior blocking finding is genuinely fixed
The capture branch (og-asset-ownership.ts:381-399) was rewritten. Instead of deriving the boundary from the alias replacement's static prefix and matching against a null package name, it now:
- takes
packageRoot = findPackageRoot(path.dirname(realModulePath))— the resolved module's owning package, - reads its declared manifest name (
readPackageName), - requires the regex to match that declared name (
aliasMatches), - round-trips the declared name back through the alias (
applyAlias) and confirms the resulting target directory/file contains/equals the module, - returns the narrow
packageRoot(not the alias parent).
I reproduced through the OgAssetOwnership class:
@scope/* -> packages/$1, legitimate@scope/ui(ownpackage.json,main), own./font.ttf: INLINED — the prior over-rejection is closed. The matching real-build test (og-inline.test.ts:390, 20 KB font) assertsatob(is emitted and no/assets/font-Rollup asset, so this is proven end-to-end, not incidentally.- Sibling-leak attempt (
@scope/uimodule reading../other/secret.txt):assetRootnarrows topackages/ui, sibling rejected — no leak. - Separator-terminated
packages/$1/lib: own asset inlined, sibling rejected. - Name-spoof (an
evilpackage inpackages/evildeclaringname:"@scope/ui"soapplyAliasmaps to the victimpackages/ui):null— the round-trip containment check (isPathInside(realAliasTarget, realModulePath)) rejects it because the module lives inevil, not the spoofed target. Good defense.
The resolveContainedAsset chokepoint (og-asset-ownership.ts:304-311) is unchanged (canonical-on-canonical isPathInside), and findPackageRoot fails closed at the FS root. I could not construct a within-monorepo sibling-leak through any alias shape at this HEAD, and the dead findExistingAliasDirectory helper is correctly removed.
Conclusion: the recurring sibling-leak class is closed and the prior over-rejection is fixed. No correctness or security blockers.
1. (low severity, inherited — fine to defer) Manifest-less leaf package under a name-matching monorepo root still widens to the monorepo root
The capture branch's findPackageRoot(path.dirname(realModulePath)) walks to the nearest ancestor package.json. I reproduced the long-standing residual the prior reviews repeatedly noted: a manifest-less leaf package dir (mono/packages/ui/ with no own package.json) whose nearest ancestor manifest (mono/package.json, name:"@scope/ui") matches the alias capture resolves assetRoot up to mono, letting a sibling (mono/packages/other/secret.txt) be inlined.
I confirmed this is not a full FS escape — an asset outside the monorepo (../../outside/.env) is rejected; it stays bounded within the nearest ancestor manifest's tree. It requires a layout no package manager produces (a leaf package with no own package.json under a root manifest named after the imported package), so severity is low and it is purely inherited (the node_modules branch uses the same findPackageRoot primitive). Consistent with prior reviews, fine to keep deferred — though a name-pin against the leaf's own resolved package (or a comment + negative test) would lock the boundary to the package dir.
Minor (carried over, fine to defer)
getNodeModulesPackageRootreasons over raw, non-canonical paths (og-asset-ownership.ts:80-101) before its result is canonicalized inresolveModuleBoundary— can only over/under-constrain, never widen past theresolveContainedAssetguard; documented inline.- The fallback
realModulePathkeeps a non-canonical basename (:252); harmless for containment.
Net
ce1cd86 closes the last review's blocking finding for the regex-capture alias shape by reusing the declared-manifest-name approach (round-tripped through the alias) and narrowing to the resolved module's owning package, with a strong real-build positive test plus negative sibling/spoof tests. The string-alias e2e path remains intact. The only residual is the inherited, low-severity, within-monorepo manifest-less-leaf case. No blockers.
|
Review posted to PR #2172. SummaryI reviewed PR #2172 (OG asset containment) at the current HEAD Verification: The prior blocking finding is genuinely fixed. The capture branch now derives
Conclusion: no correctness or security blockers. The recurring sibling-leak class stays closed and the chokepoint is unchanged. I raised one low-severity, inherited residual (manifest-less leaf package under a name-matching monorepo root widens to the monorepo root — not a full FS escape, requires a layout no package manager produces) plus 2 carried-over doc-only items, all fine to defer. |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(og): constrain inlined assets to trusted roots
Re-reviewed at HEAD 4f98cc4 ("test(og): isolate linked font fixture") — the commit added after the last ask-bonk review (ce1cd86). It is a test-only change: it removes the app-basic/Vite-dev half of the new linked-package e2e (deletes tests/fixtures/app-basic/app/api/og-linked-font/route.tsx and the @test/og-font alias from tests/fixtures/app-basic/vite.config.ts) and skips the app-router (Vite dev) Playwright project for the linked-font test (tests/e2e/og-image.spec.ts:169), leaving it as "Cloudflare-only". The plugin source (og-asset-ownership.ts, og-assets.ts) is unchanged since ce1cd86.
Verification: vp test run tests/og-asset-ownership.test.ts tests/og-inline.test.ts (58 pass) and vp check on both plugin files are green. I reproduced the exact e2e fixture shape in a real build() and ran adversarial cases through the real plugin.
The string-alias linked-package path works end-to-end (the prior blocking finding stays closed)
I reproduced the precise fixture layout — alias @test/og-font → og-font-package/lib (a directory), package.json one level up at og-font-package/ with name:"@test/og-font", module lib/index.js fetching ./noto-sans.ttf (27,748 bytes, above the 4 KB inline limit) — in a real Vite build:
contains atob( (vinext inlined): true
contains font base64: true
leftover /assets/*.ttf: false
leftover fetch(new URL(...): false
So cloudflare-workers (real wrangler/miniflare) and cloudflare-dev should pass — and those are the projects where import.meta.url === "worker" would actually throw Invalid URL, so the meaningful coverage is retained (og-image.spec.ts runs in cloudflare-workers/cloudflare-dev per playwright.config.ts:124-146).
Security boundary holds. Adversarial cases through the real plugin:
- Module escaping beyond the owning package (
../../outside-secret.ttf) → rejected (no inline,fetchleft). - Symlink inside
lib/pointing outside the package → rejected (realpath check confines). - Separator regex alias
@scope/* -> packages/$1reading a sibling package's secret → rejected.
resolveContainedAsset (og-asset-ownership.ts:304-311) remains the canonical-on-canonical isPathInside chokepoint, and findPackageRoot fails closed at the FS root. No correctness or security blockers.
One nuance worth recording about the directory-alias branch (resolveAliasPackageRoot, og-asset-ownership.ts:345-356): for this fixture the boundary resolves to the whole owning package (og-font-package/), not just the alias-target dir lib/. So a module under lib/ can inline a file at og-font-package/secret.ttf via ../secret.ttf. I confirmed this is within the PR's stated boundary ("the dependency package that owns the transformed module") — it's the package reading its own files, not a sibling-package leak — and it cannot escape the package root. Not a blocker; noting it because the boundary is the package, not the alias target.
1. (test coverage regression, worth confirming before merge) The commit drops Vite-dev coverage and the describe-block comment is now stale
The header comment still claims the test "exercises resolveId provenance and package-scoped asset containment through Vite dev, Cloudflare dev, and the built Workers bundle" (og-image.spec.ts:161-165), but the test now test.skip(... === "app-router", "linked package fixture is Cloudflare-only") (:169) — i.e. Vite dev is no longer exercised, contradicting the comment.
This matters because Vite dev is exactly where this path is most fragile, and the skip looks like it's masking a real dev-mode limitation rather than trimming redundant coverage:
- In dev, a bare-imported dependency (
@test/og-font) is a candidate for the dep optimizer, which uses its own rolldown pipeline — not the Vite plugin pipeline — so vinext'svinext:og-inline-fetch-assetstransformnever runs on the optimized dep (see the dedicateddepOptimizeAliasPlugin/optimizeDeps.rolldownOptionswiring inindex.ts:2213-2239). If the linked package gets pre-bundled, thefetch(new URL(...))survives un-inlined. - At the plugin-unit level the dev path looks fine — I simulated serve mode where
resolveIdonly sees the resolved absolute path (as Vite's built-in alias plugin delivers in dev) andresolveModuleBoundary'sresolveConfiguredAliasRootfallback still inlines. So the boundary logic isn't the problem; the optimizer bypass (or Nodefetchnot supportingfile://) is the likely reason theapp-routerproject had to be skipped.
I'd ask the author to (a) state in the skip reason / comment why Vite dev can't run this fixture (optimizer bypass? Node fetch file://?), and (b) update the stale "through Vite dev" claim in the describe comment so it matches the skip. If the underlying cause is the optimizer bypassing the inline transform for linked deps in dev, that's a known dev-mode limitation worth an explicit optimizeDeps.exclude for OG-fetching linked packages or at least a documented caveat — silently skipping the only Vite-dev assertion hides it.
Minor (carried over from prior reviews, doc-only, fine to defer)
getNodeModulesPackageRootreasons over raw, non-canonical paths (og-asset-ownership.ts:80-101) before its result is canonicalized inresolveModuleBoundary— can only over/under-constrain, never widen past theresolveContainedAssetguard; documented inline.- The fallback
realModulePathkeeps a non-canonical basename (og-asset-ownership.ts:252); harmless for containment. - The long-standing low-severity
findPackageRoot"nearest ancestor manifest" residual (manifest-less leaf under a name-matching root, project outside the monorepo) carries over unchanged — a layout no package manager produces, stays within the monorepo tree, fine to keep deferred.
Net
The plugin code is unchanged and sound: the string-alias linked-package fixture inlines end-to-end in a real build, every escape attempt I constructed fails closed, and the security-critical cloudflare-workers/cloudflare-dev coverage is retained. The one thing I'd resolve before merge is finding #1 — the commit silently drops the Vite-dev assertion while leaving a comment that claims Vite-dev coverage; confirm whether dev is skipped due to the dep-optimizer bypass (a real limitation worth documenting) and fix the stale comment either way. No security blockers.
|
Review posted to PR #2172. SummaryI reviewed PR #2172 (OG asset containment) at the current HEAD Verification: Conclusion: no correctness or security blockers.
One finding raised (non-blocking, worth confirming before merge):
I also noted that the directory-alias boundary resolves to the whole owning package (not just the alias-target dir) — confirmed within the PR's intended boundary, not a sibling leak — plus the 2 carried-over doc-only items. |
1 participant
Summary
Testing
vp test run tests/og-asset-ownership.test.ts tests/og-inline.test.tsCI=1 PLAYWRIGHT_PROJECT=cloudflare-dev npx playwright test tests/e2e/og-image.spec.ts -g "linked-package font"CI=1 PLAYWRIGHT_PROJECT=cloudflare-workers npx playwright test tests/e2e/og-image.spec.ts -g "linked-package font"vp check packages/vinext/src/plugins/og-asset-ownership.ts tests/og-asset-ownership.test.ts tests/og-inline.test.ts tests/e2e/og-image.spec.ts examples/app-router-cloudflare/vite.config.ts examples/app-router-cloudflare/app/api/og-linked-font/route.tsx tests/fixtures/og-font-package/lib/index.js tests/fixtures/og-font-package/lib/index.d.ts