fix(permissions): don't require --allow-run for process.kill on self#34382
Conversation
…low-run Sending a signal to the current process is equivalent to terminating yourself, which never required permissions in the first place (Deno.exit, abort, etc.). Tools like signal-exit re-raise the original signal on process.pid so the parent shell sees the correct exit, which forced users to grant --allow-run just to Ctrl-C a process. Skip the run permission check in op_node_process_kill and op_kill when the target pid equals the current process pid. Killing other pids still requires --allow-run. Refs #34344
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
No issue found. The self-PID carveout is applied in both op_node_process_kill and op_kill before check_run_all, while every other PID still requires --allow-run. The spec covers the non-terminating signal-0 path through both process.kill(process.pid, 0) and Deno.kill(Deno.pid, 0).
Holding approval until CI is green.
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
CI found a real failure on this head: every failing unit shard hits tests/unit/process_test.ts killPermissions, where Deno.kill(Deno.pid, "SIGCONT") no longer throws without run permission after this change.
- Update
killPermissionsto match the new self-PID behavior and keep coverage that non-self PIDs still require--allow-run, or narrow the self-PID carveout so the existing permission contract remains true.
Addressed in commit d51b70e: killPermissions now asserts against a non-self pid (Deno.pid + 1) so the run-permission contract for other pids is preserved, while the self-pid carveout is exercised by the new killSelfNoPermissions unit test and the process_kill_self spec. CI is green on the current head.
bartlomieju
left a comment
bartlomieju
left a comment
There was a problem hiding this comment.
Reviewed the current head (merge of main into the branch). Clean, minimal, and well-tested; the security reasoning holds.
Correctness / security
- The carveout is
pid != std::process::id(), exempting only the exact own pid. Process-group targets stay protected:pid == 0(caller's group),-1(all processes), and negative group pids never equalstd::process::id(), so they still require--allow-run. No permission hole. - Applied symmetrically in both
op_node_process_killandop_kill. - The stance is sound: self-signaling is already reachable via
Deno.exit/abort and is equivalent to self-termination; it grants no capability over any other process. No escalation. as i32of the u32 pid is safe; real pids are well withini32on all supported platforms.- Signal
0is handled cross-platform (UnixNoneliveness probe, Windowsprocess_is_alive), so the spec test works on Windows too.
Tests
killSelfNoPermissions(unit) andprocess_kill_self(spec) cover the self path through bothprocess.killandDeno.kill.killPermissionsretains the contract for non-self pids viaDeno.pid + 1; the permission check runs beforekill(), so the signal is never delivered regardless of whether that pid is live. Good.
No blocking issues. The earlier stale CHANGES_REQUESTED (made against the pre-fix commit 3c1e37e, where killPermissions still broke) has been dismissed since the fix landed in d51b70e. LGTM once CI on the merge head is green.
2 participants
Sending a signal to the current process is functionally equivalent to
terminating yourself, which never required permissions in the first
place (
Deno.exit, abort, etc.). Tools likesignal-exit(used byvite and many others) re-raise the original signal on
process.pidsothe parent shell sees the correct exit status, which forced users to
grant blanket
--allow-runjust to be able to Ctrl-C a process.This skips the run-permission check in
op_node_process_killandop_killwhen the target pid is the current process's pid. Signallingany other pid still requires
--allow-runas before.Fixes #34344