X Tutup
The Wayback Machine - https://web.archive.org/web/20210819204800/https://github.com/nodejs/node/pull/37863
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Canonicalize URLs prior to Policy specifier matching #37863

Closed
wants to merge 1 commit into from

Conversation

@bmeck
Copy link
Member

@bmeck bmeck commented Mar 22, 2021

Currently policies preempt the resolution process entirely and work off of raw specifiers. If file:///app/component.cjs requires require("./util.js") or imports import("./util.js") policies only look for an exact match of "./util.js" in "dependencies". This was found as a usability issue by @giltayar while comparing with / looking at import maps compatibility. In particular, with import maps the specifiers are always canonicalized prior to performing matching, both the specifiers in the map that can be matched and the specifier from the call site. This allows a mapping table to cover all ways to get a hold of file:///app/util.js in a more concise manner.

Consider this as the source texts :

// `file:///app/component.cjs`
require('./dir/foo.cjs');
require('./bar.cjs');
 // `file:///app/dir/foo.cjs`
require('./bar.cjs');

and a policy at file:///policy.json containing:

{
  "scopes": {
    "file:": {
      "cascade": true,
      "integrity": true,
      "dependencies": {
         "./bar.cjs": "file:///app/bar.js"
      }
    }
  }
}

This policy will intercept both the loads from file:///app/dir/foo.cjs and file:///app/component.cjs. And have both resolve to file:///app/bar.js.

Canonicalizing early would mean that it only intercepts resolution that PRIOR TO NODE RESOLUTION would point to file:///bar.cjs regardless of the file containing the load. This actually is a breaking change to policies and one would have to alter some data as seen in the tests in this PR but it would greatly simplify some cases so that instead of needing to specify all routes to dependencies one only needs to specify the eventual target if resolved prior to node resolving. This must be done prior to node resolving so that things like fs and react are still properly able to be intercepted. Due to the number of closures this uses we likely should add a benchmark and migrate to be less closure heavy and pass around objects instead.

lib/internal/policy/manifest.js Outdated Show resolved Hide resolved
lib/internal/policy/manifest.js Outdated Show resolved Hide resolved
lib/internal/policy/manifest.js Outdated Show resolved Hide resolved
lib/internal/policy/manifest.js Outdated Show resolved Hide resolved
@bmeck bmeck force-pushed the bmeck:policy-canonicalize branch from 78bb5e7 to 574381b Aug 3, 2021
bmeck added a commit to bmeck/node that referenced this pull request Aug 4, 2021
PR-URL: nodejs#37863
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@bmeck bmeck force-pushed the bmeck:policy-canonicalize branch from 924edc3 to 64c9851 Aug 4, 2021
@bmeck
Copy link
Member Author

@bmeck bmeck commented Aug 4, 2021

rebased, @aduh95 can you take a look again, your comments should be resolved

doc/api/errors.md Outdated Show resolved Hide resolved
lib/internal/policy/manifest.js Outdated Show resolved Hide resolved
lib/internal/policy/manifest.js Outdated Show resolved Hide resolved
bmeck added a commit to bmeck/node that referenced this pull request Aug 5, 2021
PR-URL: nodejs#37863
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: James M Snell <jasnell@gmail.com>
@bmeck bmeck force-pushed the bmeck:policy-canonicalize branch from 12ac5ea to 5ee3aa6 Aug 5, 2021
@bmeck
Copy link
Member Author

@bmeck bmeck commented Aug 9, 2021

I'd like to land this in the next day or so and if no requests for changes occur I plan to do so.

PR-URL: #37863
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: James M Snell <jasnell@gmail.com>
@bmeck bmeck force-pushed the bmeck:policy-canonicalize branch from 5ee3aa6 to 17e0da5 Aug 19, 2021
bmeck added a commit that referenced this pull request Aug 19, 2021
PR-URL: #37863
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: James M Snell <jasnell@gmail.com>

Reviewed-By: Guy Bedford <guybedford@gmail.com>
@bmeck
Copy link
Member Author

@bmeck bmeck commented Aug 19, 2021

Landed in 90e2e78

@bmeck bmeck closed this Aug 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
X Tutup