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.