-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
feat: add --allow-sys permission flag #16028
Conversation
a69bf27
to
110e915
Compare
110e915
to
21df843
Compare
cc @nayeemrmn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation wise it looks good to me (minus one nipick). I'd like to suggest we use --allow-sys
instead of --allow-sys-info
- that way all permission flags will be two-word and won't require quotes in places like permission definition in Deno.test()
cli/args/flags.rs
Outdated
.takes_value(true) | ||
.use_value_delimiter(true) | ||
.require_equals(true) | ||
.help("Allow access to system info"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now this flag can accept arbitrary values, I suggest to add a validator here that will limit options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added validation of sys kind in 3 places:
allow-sys
arg values are validatedkind
prop ofSysPermissionDescriptor
is now typed as"hostname" | "loadavg" | ...
(in TypeScript)op_{query|revoke|request}_permission
throws TypeError when the kind is invalid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice work
match key { | ||
"hostname" | "osRelease" | "loadavg" | "networkInterfaces" | ||
| "systemMemoryInfo" | "getUid" | "getGid" => {} | ||
_ => { | ||
return Err(format!("unknown system info kind \"{}\"", key)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be de-duplicated with parse_sys_kind
, I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Created a PR #16087
This PR add
--allow-sys
permission flag to control the access to the system infromations such asloadavg
,hostname
,networkInterfaces
,getUid
,getGid
,osRelease
, andsystemMemoryInfo
.Summary of changes
Rust
SysDescriptor(pub String)
typeUnaryPermission<SysDescriptor>
Permissions
structdeserializer
forChildPermissionsArg
create_child_permissions
function--allow-sys
flag handlingJS
TS
SysPermissionDescriptor
typePermissionOptionsObject
to acceptsys
permDoc
@tags
for 7 methods.closes #15986