search

nodejs / security-wg

Node.js Ecosystem Security Working Group
MIT License
488 stars 121 forks source link

Ping TSC on deps update not from GithubBot #1329

Open marco-ippolito opened 1 month ago

marco-ippolito commented 1 month ago

We receive dependency update such as https://github.com/nodejs/node/pull/53406 that are created by users that might be hard and risky to review. We should probably ping TSC whenever we receive changes in the deps folder not from a GithubBot

UlisesGascon commented 1 month ago

I strongly agree

RafaelGSS commented 1 month ago

Context: https://openjs-foundation.slack.com/archives/C03Q9MS3KFB/p1718290908194829?thread_ts=1718036013.366919&cid=C03Q9MS3KFB

Uzlopak commented 1 month ago

To create a comment in a PR in nodejs/node repository, even when providing a PR from a fork you need to listen on a pull_request_target event . This can have security implications. Maybe @lirantal wants to comment on that.

I dont think we need to write a new action.

So something like the following could be enough? Untested(!)

on:
  pull_request_target:
    paths:
      - 'deps/**'

jobs:
  ping-tsc:
    runs-on: ubuntu-latest
    name: Ping the TSC
    steps:
      - name: Ping the TSC
        if: >
          github.event.pull_request.user.login != 'nodejs-github-bot'
        uses: thollander/actions-comment-pull-request@v2
        with:
          message: Pinging @tsc because of an irregular change in the deps folder
          comment_tag: ping-tsc
Uzlopak commented 1 month ago

Maybe it makes also sense to add a "blocked" label to the PR.

richardlau commented 1 month ago

We should probably ping TSC whenever we receive changes in the deps folder not from a GithubBot

Pinging the TSC for changes to deps should be easy (add them as a CODEOWNER and then our bot should then ping the team in a comment to the PR) but we have no logic AFAIK for checking who originated the PR.

RedYetiDev commented 1 month ago

I can make a workflow if you'd like

RedYetiDev commented 1 month ago

https://github.com/nodejs/node/pull/53149 migrates the requesting reviews to GitHub actions, and also implements this in the process

mhdawson commented 4 weeks ago

PR - to add guidance to contributing docs - https://github.com/nodejs/node/pull/53499

mhdawson commented 3 weeks ago

Thinking about this a bit, instead of a ping to the TSC, I think adding a comment to the PR with a warning to collaborators that updates to dependencies should generally be generated by the automation and pointing to https://github.com/nodejs/node/blob/main/doc/contributing/maintaining/maintaining-dependencies.md#updating-dependencies for more information would be more beneficial.

RedYetiDev commented 3 weeks ago

I can modify my PR if you'd like