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.

Sign up for GitHub

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

Jump to bottom

feat: add --allow-import flag #25469

Merged
merged 43 commits into from
Sep 26, 2024
Merged

feat: add --allow-import flag #25469

merged 43 commits into from
Sep 26, 2024

Conversation

bartlomieju
Copy link
Member

@bartlomieju bartlomieju commented Sep 5, 2024 •
edited by dsherret
Loading

This replaces --allow-net for import permissions and makes the security sandbox stricter by also checking permissions for statically analyzable imports.

By default, this has a value of --allow-import=deno.land:443,jsr.io:443,esm.sh:443,raw.githubusercontent.com:443,gist.githubusercontent.com:443, but that can be overridden by providing a different set of hosts.

Additionally, when no value is provided, import permissions are inferred from the CLI arguments so the following works because fresh.deno.dev:443 will be added to the list of allowed imports:

deno run -A -r https://fresh.deno.dev

Closes #8266

@bartlomieju bartlomieju mentioned this pull request Sep 6, 2024
Closed
@lowlighter lowlighter mentioned this pull request Sep 8, 2024
Closed
@bartlomieju bartlomieju mentioned this pull request Sep 8, 2024
Merged
@bartlomieju
Copy link
Member Author

bartlomieju commented Sep 14, 2024 •
edited
Loading

Closing in favor of #25508

Ooops, wrong PR.

@bartlomieju bartlomieju deleted the allow_imports branch September 14, 2024 00:25
@bartlomieju bartlomieju restored the allow_imports branch September 14, 2024 00:25
@bartlomieju bartlomieju reopened this Sep 14, 2024
@dsherret
merge in main
9f947cc
@dsherret
Merge branch 'main' into allow_imports
ef0ebc9
@dsherret
updates
d040407
@dsherret
Merge branch 'main' into allow_imports
ba90b1a
@dsherret
Starting to use permissions container more
9d42419
@dsherret
Starting to use permissions container more
eaff962
@dsherret
Saving for revert
05a39e9
@dsherret
Revert "Saving for revert"
f99c39c 
This reverts commit 05a39e9.
@dsherret
add permissions for check and cache. Allow reading from the file syst…
282dab6 
…em on the first pass
@dsherret
Start adding tests and fix it not working for run
479d046
@dsherret
fix up flags
2b58d88
@dsherret
add 0.0.0.0
b0c74e0
@dsherret
test jsr_host_from_url
9e48c18
@dsherret
add gist.githubusercontent.com
240e2c0
@dsherret
update test because localhost is allowed by default (though maybe it …
3f88938 
…should not be?)
@dsherret
improve naming
43f4624
@dsherret
Merge branch 'main' into allow_imports
4e6b5d2
@dsherret
Clippy
5630383
@dsherret
Merge branch 'main' into allow_imports
d90489f
@dsherret dsherret changed the title feat: Add --allow-imports flag feat: Add --allow-import flag Sep 24, 2024
bartlomieju
bartlomieju commented Sep 25, 2024
View reviewed changes
cli/args/flags.rs Outdated Show resolved Hide resolved
bartlomieju
bartlomieju commented Sep 25, 2024
View reviewed changes
cli/args/flags.rs Outdated Show resolved Hide resolved
bartlomieju
bartlomieju commented Sep 25, 2024
View reviewed changes
cli/graph_util.rs Outdated
Comment on lines 253 to 256
// skip type checking when there are any external remote modules
// because those will be surfaced as errors later and would cause
// type checking to crash
graph.modules().any(|module| match module {
Copy link
Member Author

Choose a reason for hiding this comment

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

This seems strange to me - do we just silently skip checking and there's no info surfaced about it? Or is it getting surfaced somewhere else?

Copy link
Member

Choose a reason for hiding this comment

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

I'm going to update this comment. If the graph contains any of these then it's invalid.

bartlomieju
bartlomieju commented Sep 25, 2024
View reviewed changes
cli/lsp/language_server.rs
Comment on lines +3618 to +3619
// allow remote import permissions in the lsp for now
allow_import: Some(vec![]),
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe open an issue about this one and ask @nayeemrmn to add a setting in vscode_deno to pass an allow list?

bartlomieju
bartlomieju commented Sep 25, 2024
View reviewed changes
cli/standalone/mod.rs Outdated Show resolved Hide resolved
bartlomieju
bartlomieju commented Sep 25, 2024
View reviewed changes
Copy link
Member Author

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

Can you please update the PR description to mention that this flag replaces --allow-net for imports/workers?

Also note to self to update the docs page with new flag.

dsherret
dsherret approved these changes Sep 26, 2024
View reviewed changes
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM

@dsherret dsherret enabled auto-merge (squash) September 26, 2024 01:03
@dsherret dsherret merged commit 5504ace into denoland:main Sep 26, 2024
17 checks passed
@nayeemrmn nayeemrmn mentioned this pull request Sep 26, 2024
Closed
@rivy
Copy link
Contributor

rivy commented Sep 28, 2024 •
edited
Loading

@bartlomieju , @dsherret , I'm hitting new, unexpected permission requests based on this change.

Can "cdn.jsdelivr.net:443" and "cdn.skypack.dev:443" be added to the defaults?
Both are well-known and frequently used CDNs.

@dsherret
Copy link
Member

Probably the defaults will be left as-is. We can't guarantee the security of all hosts.

@bartlomieju bartlomieju deleted the allow_imports branch September 28, 2024 23:46
@rivy
Copy link
Contributor

rivy commented Sep 28, 2024 •
edited
Loading

@dsherret , thanks for the reply.

Just to discuss a bit more... JSDelivr is one of the largest (if not the largest) NPM and GitHub CDNs; SkyPack is more niche but I think similar to ESM.sh in being one of the first on the scene for ESM conversion and hosting. SkyPack seems to be able to generate types for more packages as well.

And, at least JSDelivr should be a minimal security issue as it's sponsored the biggies, Cloudflare and Fastly, and delivering more than 250 billion requests monthly (compared to only 450 million per month for esm). Using only "esm.sh" seems like a weird choice, in comparison.

Since it looks like this just landed in RC7, I hope you'll look at these and pass them along to the other devs and reconsider adding the two.

Thanks for the consideration.

rivy added a commit to rivy/deno.dxx that referenced this pull request Sep 29, 2024
@rivy
refactor ~ replace JSDelivr/SkyPack imports with 'esm.sh'
4a24413 
* Deno-v2.0.0-rc7+ is now requiring "import" permissions
  - JSDelivr and SkyPack are not on the Deno-2.0 default allowlist
  - esm.sh *is* included in the "allow-import" default domain list
  - ref: <denoland/deno#25469>

to comply with Deno-2.0 'import' permission defaults
@rivy
Copy link
Contributor

rivy commented Sep 29, 2024 •
edited
Loading

@dsherret , another data point is that some places where using JSDelivr, as a CDN, simply downloads and succeeds, esm.sh is doing some translation and fails.

For example, deno install --global -Af "https://cdn.jsdelivr.net/gh/rivy/deno.dxx@fabe08a2e7/eg/args.ts" succeeds, but using esm.sh (deno install --global -Af "https://esm.sh/gh/rivy/deno.dxx@fabe08a2e7/eg/args.ts") fails with "error: Import 'https://esm.sh/v135/jsr:@latest/denonext/[email protected]/read-all.js' failed: 400 Bad Request when it attempts to translates the file to JS. And deno install --global -Af "https://raw.esm.sh/gh/rivy/deno.dxx@fabe08a2e7/eg/args.ts" fails with "error: Module not found "https://raw.esm.sh/v135/gh/rivy/deno.dxx@fabe08a2e7/vendor/@types/[email protected]".

Using https://raw.githubusercontent.com/rivy/deno.dxx/fabe08a2e7/eg/args.ts succeeds similarly to JSDelivr, but it has historically had rate limits which are too small to use as a public CDN. Whereas, JSDelivr is designed to serve worldwide at scale and it's perceptibly faster than github or esm as well.

rivy added a commit to rivy/deno.dxx that referenced this pull request Sep 29, 2024
@rivy
refactor ~ replace JSDelivr/SkyPack imports with 'esm.sh'
412017a 
* Deno-v2.0.0-rc7+ is now requiring "import" permissions
  - JSDelivr and SkyPack are not on the Deno-2.0 default allowlist
  - esm.sh *is* included in the "allow-import" default domain list
  - ref: [Proposal/discussion](denoland/deno#8266)
  - ref: [Merged solution](denoland/deno#25469)
@dsherret
Copy link
Member

dsherret commented Oct 3, 2024

@rivy #26013

@rivy
Copy link
Contributor

rivy commented Oct 3, 2024 •
edited
Loading

@rivy #26013

@dsherret , Thank you for the follow-up and your advocacy!

I was going to open an issue requesting this addition.
Thanks for beating me to it!
😄

rivy added a commit to rivy/deno.dxx that referenced this pull request Oct 6, 2024
@rivy
refactor ~ replace JSDelivr/SkyPack imports with 'esm.sh'
09b36a6 
* Deno-v2.0.0-rc7+ is now requiring "import" permissions
  - JSDelivr and SkyPack are not on the Deno-2.0 default allowlist
  - esm.sh *is* included in the "allow-import" default domain list
  - ref: [Proposal/discussion](denoland/deno#8266)
  - ref: [Merged solution](denoland/deno#25469)
rivy added a commit to rivy/deno.dxx that referenced this pull request Oct 6, 2024
@rivy
refactor ~ replace JSDelivr/SkyPack imports with 'esm.sh'
2e5b6cb 
* Deno-v2.0.0-rc7+ is now requiring "import" permissions
  - JSDelivr and SkyPack are not on the Deno-2.0 default allowlist
  - esm.sh *is* included in the "allow-import" default domain list
  - ref: [Proposal/discussion](denoland/deno#8266)
  - ref: [Merged solution](denoland/deno#25469)
@lionel-rowe lionel-rowe mentioned this pull request Oct 10, 2024
Open
rivy added a commit to rivy/deno.dxx that referenced this pull request Oct 13, 2024
@rivy
refactor ~ replace JSDelivr/SkyPack imports with 'esm.sh'
a72a959 
* Deno-v2.0.0-rc7+ is now requiring "import" permissions
  - JSDelivr and SkyPack are not on the Deno-2.0 default allowlist
  - esm.sh *is* included in the "allow-import" default domain list
  - ref: [Proposal/discussion](denoland/deno#8266)
  - ref: [Merged solution](denoland/deno#25469)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsherret dsherret dsherret approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Proposal: add --allow-import=<allow_list> for checking dynamic imports/workers
3 participants
@bartlomieju @rivy @dsherret