feat(permissions): allow flags should override deny when more specific #31224

dsherret · 2025-11-07T22:55:02Z

Allow flags that are more specific than deny flags should take precedence.

Some examples:

--allow-read=./data/sub --deny-read=./data
--allow-env=NODE_ENV --deny-read=NODE_*

Extracted out of #31187 -- This behaviour will be more important when the --ingore-env flag is added in order to make the feature more powerful.

Co-authored-by: Copilot <[email protected]> Signed-off-by: David Sherret <[email protected]>

Copilot

Pull Request Overview

This PR refactors the permission system to improve handling of granular allow and deny rules. The main change replaces separate HashSet-based storage for granted, flag-denied, and prompt-denied descriptors with a unified sorted vector-based approach, enabling more precise permission resolution with partial denials.

Changed data structure from HashSet-based to sorted Vec-based descriptor storage
Added DeniedPartial state to distinguish between full and partial denials
Implemented comparison traits for descriptor ordering to support binary search

Reviewed Changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
runtime/permissions/lib.rs	Core refactoring: unified descriptor storage, added sorting/comparison logic, implemented partial denial handling
runtime/permissions/Cargo.toml	Added test features for sys_traits dependency
runtime/ops/permissions.rs	Updated PermissionStatus to use static string for state and handle DeniedPartial
libs/config/deno_json/permissions.rs	Added helper method and simplified empty check logic
cli/args/mod.rs	Renamed variable from `no_op` to `identity` for clarity
Cargo.toml	Updated fqdn dependency from 0.3.4 to 0.4.6
Cargo.lock	Updated fqdn package checksum and version

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

runtime/permissions/lib.rs

dsherret · 2025-11-07T22:57:04Z

runtime/ops/permissions.rs

-        state.to_string()
+      state: match state {
+        PermissionState::Granted | PermissionState::GrantedPartial => "granted",
+        PermissionState::DeniedPartial | PermissionState::Denied => "denied",


This DeniedPartial isn't used at the moment, but it's useful granularity to have. I'll use it in a future PR that I extract out of #31187

Co-authored-by: Copilot <[email protected]> Signed-off-by: David Sherret <[email protected]>

bartlomieju · 2025-11-12T14:34:32Z

libs/config/deno_json/permissions.rs


 impl PermissionsObject {
  /// Returns true if the permissions object is empty (no permissions are set).
  pub fn is_empty(&self) -> bool {


I probably asked this in the past - but aren't we calling this in a hot path? Maybe this value should be memoized?

No, this is only used in a few select places (I think one place). If it were in runtime/permissions then yeah it could be in a hot path.

bartlomieju · 2025-11-12T14:37:15Z

runtime/permissions/lib.rs

      PermissionState::GrantedPartial => f.pad("granted-partial"),
      PermissionState::Prompt => f.pad("prompt"),
-      PermissionState::Denied => f.pad("denied"),
+      PermissionState::Denied | PermissionState::DeniedPartial => {


Why not discriminate denied-partial?

Seems this is not used so I will remove it.

runtime/permissions/lib.rs

bartlomieju · 2025-11-12T14:39:58Z

runtime/permissions/lib.rs

+      UnaryPermissionDesc::FlagDenied(_) => 0,
+      UnaryPermissionDesc::PromptDenied(_) => 1,
+      UnaryPermissionDesc::Granted(_) => 2,


Why does "flag denied" have a higher precedence than "prompt denied"?

One needed to be higher than the other and flag seems a little more higher precedence than prompt.

Co-authored-by: Bartek Iwańczuk <[email protected]> Signed-off-by: David Sherret <[email protected]>

bartlomieju · 2025-11-12T14:43:49Z

runtime/permissions/lib.rs

+    {
+      let perms = Permissions::from_options(
+        &parser,
+        &PermissionsOptions {
+          allow_read: Some(svec!["/foo/specific"]),
+          deny_read: Some(svec!["/foo"]),
+          allow_write: Some(svec!["/foo/specific"]),
+          deny_write: Some(svec!["/foo"]),
+          allow_ffi: Some(svec!["/foo/specific"]),
+          deny_ffi: Some(svec!["/foo"]),
+          ..Default::default()
+        },
+      )


Can you add another test case, that flips the values (ie. allow_read: Some(svec!["/foo"]), deny_read: Some(svec!["/foo/specific"])) - just to be extra sure that reading /foo/bar is allowed but /foo/specific is not

Added in 8185f89

bartlomieju · 2025-11-12T14:44:31Z

runtime/permissions/lib.rs

+      assert_eq!(
+        perms.env.query(Some("PREFIX_TEST")),
+        PermissionState::Denied
+      );
+      assert_eq!(
+        perms.env.query(Some("PREFIX_ALLOWED_TEST")),
+        PermissionState::Granted
+      );
+      assert_eq!(
+        perms.env.query(Some("PREFIX_EXPLICIT_ALLOWED")),
+        PermissionState::Granted
+      );


Copilot

Pull Request Overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

runtime/permissions/lib.rs

runtime/ops/permissions.rs

denoland#31224)

dsherret and others added 30 commits November 4, 2025 10:06

feat: denied env vars to none

a737159

update

9a755aa

update set and delete

8858b66

fix

b706ffd

better

83c98fc

Merge branch 'main' into feat_allow_env_deny_to_none

be118da

small fixes

ffc2506

update

efacd4d

add partial denied

f6ddea8

add partial denied

f9f9e71

fix

e88b7dd

update

511c88c

remove allocation

09e3e81

Merge branch 'main' into feat_allow_env_deny_to_none

33b3ed4

fix logic for deny

21f51a3

fix logic for deny

b8f33e7

Update cli/schemas/config-file.v1.json

cd23acc

Co-authored-by: Copilot <[email protected]> Signed-off-by: David Sherret <[email protected]>

fixes

75f2a00

fix bug in permissions

db08877

fix build error

db6881b

fix another issue

5bd064f

saving work

1cb5f7c

adding single descriptors collection

499b691

working

b8affe1

use a vec

b13d975

add test for env prefix

dd6de11

more tests

f296335

make more internals private of descriptors

f658c29

reverting

4aaff4d

more revert

bb108a4

Copilot AI review requested due to automatic review settings November 7, 2025 22:55

Copilot AI reviewed Nov 7, 2025

View reviewed changes

runtime/permissions/lib.rs Outdated Show resolved Hide resolved

dsherret commented Nov 7, 2025

View reviewed changes

dsherret and others added 2 commits November 7, 2025 23:57

Update runtime/permissions/lib.rs

9ff07e4

Co-authored-by: Copilot <[email protected]> Signed-off-by: David Sherret <[email protected]>

fix windows

f3a1cc5

dsherret requested review from bartlomieju and nathanwhit November 10, 2025 15:21

bartlomieju reviewed Nov 12, 2025

View reviewed changes

runtime/permissions/lib.rs Outdated Show resolved Hide resolved

bartlomieju reviewed Nov 12, 2025

View reviewed changes

Update runtime/permissions/lib.rs

013795a

Co-authored-by: Bartek Iwańczuk <[email protected]> Signed-off-by: David Sherret <[email protected]>

bartlomieju reviewed Nov 12, 2025

View reviewed changes

dsherret added 2 commits November 12, 2025 09:52

remove this display because it is not used

92c517b

add test

8185f89

dsherret requested a review from Copilot November 12, 2025 18:41

Copilot started reviewing on behalf of dsherret November 12, 2025 18:41 View session

Copilot finished reviewing on behalf of dsherret November 12, 2025 18:42

Copilot AI reviewed Nov 12, 2025

View reviewed changes

bartlomieju approved these changes Nov 12, 2025

View reviewed changes

dsherret and others added 5 commits November 12, 2025 14:46

add more tests and fix bug

583d9d2

Add more sorting tests.

c19f8a6

Fix NetDescriptor ordering and then add some tests

e92a509

add way more tests

eba93f7

Merge branch 'main' into improved_handling_allow_deny

5376b0b

dsherret enabled auto-merge (squash) November 12, 2025 20:39

dsherret merged commit a664ebe into denoland:main Nov 12, 2025
19 checks passed

dsherret deleted the improved_handling_allow_deny branch November 12, 2025 22:10

bartlomieju pushed a commit to bartlomieju/deno that referenced this pull request Nov 17, 2025

feat(permissions): allow flags should override deny when more specific (

51acfd2

denoland#31224)

feat(permissions): allow flags should override deny when more specific #31224

feat(permissions): allow flags should override deny when more specific #31224

Uh oh!

Conversation

dsherret commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dsherret commented Nov 7, 2025 •

edited

Loading