permission: fix spawnSync permission check

[RafaelGSS]

It was missing in the original PR #44004.

Fixes: https://github.com/nodejs-private/node-private/issues/394

[cjihrig]

LGTM. Please also include:

Fixes: https://github.com/nodejs-private/node-private/issues/394

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/50232/

[RafaelGSS]

CI https://ci.nodejs.org/job/node-test-pull-request/50242/ ✅

[github-actions]

Fast-track has been requested by @RafaelGSS. Please 👍 to approve.

[RafaelGSS]

Requesting fast-track because I'll work on the v19.8.0 proposal tomorrow and we need to have this patch.

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/46975
✔  Done loading data for nodejs/node/pull/46975
----------------------------------- PR info ------------------------------------
Title      permission: fix spawnSync permission check (#46975)
Author     Rafael Gonzaga  (@RafaelGSS)
Branch     RafaelGSS:fix/add-permission-spawn-check -> nodejs:main
Labels     child_process, c++, fast-track, needs-ci
Commits    1
 - permission: fix spawnSync permission check
Committers 1
 - RafaelGSS 
PR-URL: https://github.com/nodejs/node/pull/46975
Fixes: https://github.com/nodejs-private/node-private/issues/394
Reviewed-By: Colin Ihrig 
Reviewed-By: Anna Henningsen 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/46975
Fixes: https://github.com/nodejs-private/node-private/issues/394
Reviewed-By: Colin Ihrig 
Reviewed-By: Anna Henningsen 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - permission: fix spawnSync permission check
   ℹ  This PR was created on Mon, 06 Mar 2023 16:46:11 GMT
   ✔  Approvals: 2
   ✔  - Colin Ihrig (@cjihrig) (TSC): https://github.com/nodejs/node/pull/46975#pullrequestreview-1326742875
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/46975#pullrequestreview-1327221779
   ℹ  This PR is being fast-tracked
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-03-06T22:59:26Z: https://ci.nodejs.org/job/node-test-pull-request/50242/
- Querying data for job/node-test-pull-request/50242/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/4356000066

[RafaelGSS]

Landed in b164038

[tniessen]

Requesting fast-track because I'll work on the v19.8.0 proposal tomorrow and we need to have this patch.

Maybe we should consider letting it bake on main for a while longer, given that this kind of bug would otherwise trigger the security release workflow?

[RafaelGSS]

Maybe we should consider letting it bake on main for a while longer, given that this kind of bug would otherwise trigger the security release workflow?

I don't see much value in it, at least for this particular feature. It's useful when it's a significant change or notable change being released in an LTS release. @cjihrig found the bug as a Node.js maintainer, I don't see other Node.js contributors finding it as they are unlikely to use it. Errors like this will be encountered (if any) by users and as you stated, would be fixed in a security release -- but, I don't see the baking time avoiding it though.

However, if you feel strongly about the baking time of this feature, I can postpone it to v19.9.0. Just let me know.

[tniessen]

That's a good point. I don't feel strongly about it. I am only bringing this up because I think it is very likely that this feature will lead to vulnerability reports shortly after being released, simply because it significantly increases the attack surface and has only been reviewed by very few people so far. Letting it bake might lead to more issues like this being found that can be fixed publicly, but it might just as well be unlikely.

[RafaelGSS]

Well, skipping it for v19.8.0 won't hurt. Let's release the permission model in v19.9.0 then. I will include the blocked label.