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

fix(cli/permissions): Fix CWD and exec path leaks #5642

Merged
merged 7 commits into from May 29, 2020

Conversation

nayeemrmn
Copy link
Collaborator

@nayeemrmn nayeemrmn commented May 19, 2020

Fixes #5627.
Fixes #5742.
Fixes #5786.

  • Add permissions.check_read_blind(path, display) for Deno.cwd() and Deno.execPath(). It allows you to get errors like:
    • PermissionDenied: read access to <CWD>, run again with the --allow-read flag
    • PermissionDenied: read access to <exec_path>, run again with the --allow-read flag
  • For FS functions that accept relative paths, only display the resolved path in permission messages if the CWD is allowed. Otherwise use the input path. Example for permission requests:
    • await Deno.permissions.request({ name: "read", path: "../abc" });
    • deno run --unstable temp.ts:
      ⚠️ Deno requests read access to "../abc". Grant? [g/d (g = grant, d = deny)]
    • deno run --unstable --allow-read=. temp.ts:
      ⚠️ Deno requests read access to "/mnt/c/Users/Nayeem/projects/abc". Grant? [g/d (g = grant, d = deny)]
  • For Deno.realPath(), check if the CWD is allowed when a relative path is given.
  • Add tests.

@nayeemrmn nayeemrmn changed the title WIP: fix(cli/permissions): Don't leak the CWD for relative paths WIP: fix(cli/permissions): Fix CWD and exec path leaks May 19, 2020
@nayeemrmn nayeemrmn changed the title WIP: fix(cli/permissions): Fix CWD and exec path leaks fix(cli/permissions): Fix CWD and exec path leaks May 20, 2020
@bartlomieju bartlomieju added the cli related to cli/ dir label May 20, 2020
@nayeemrmn nayeemrmn force-pushed the cwd-leak branch 5 times, most recently from 48e0137 to c969196 Compare May 21, 2020 12:08
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

Hey @nayeemrmn - sorry for the slow review - I just now saw this patch. I wish I had gotten it in v1.0.2!

cli/permissions.rs Show resolved Hide resolved
cli/state.rs Show resolved Hide resolved
cli/tests/056_make_temp_file_write_perm.ts Outdated Show resolved Hide resolved
@ry
Copy link
Member

ry commented May 28, 2020

@nayeemrmn Gosh - sorry for the slow review again. This looks good to me. Could you please draft a commit message that I can use when squashing this?

@nayeemrmn
Copy link
Collaborator Author

Commit message:

fix(cli/permissions): Fix CWD and exec path leaks

- fix: Add permissions.check_read_blind() to check for read access for a
  directory while anonymizing it in error messages.
- fix: For ops which affect relative paths, only display the resolved
  path in permission messages if the CWD is allowed. Otherwise display
  the input path.
- fix: Deno.realPath(), check if the CWD is allowed when a relative path
  is given.
- refactor: Factor out all path resolution and normalization to
  cli/permissions.rs. This includes paths given as whitelists as well as
  op arguments.

@SyrupThinker SyrupThinker mentioned this pull request May 29, 2020
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM

@bartlomieju bartlomieju merged commit 8e39275 into denoland:master May 29, 2020
@nayeemrmn nayeemrmn deleted the cwd-leak branch May 29, 2020 15:49
bartlomieju pushed a commit to bartlomieju/deno that referenced this pull request Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli related to cli/ dir
Projects
None yet
3 participants