Any PR with "dangerous" edits should be considered dangerous

[peterbe]

Security scenario: Someone makes a PR that covers lots and lots of files. They might all look like harmless typo fixes in various index.html files. But they could also try to sneak in a change to .github/workflows/pr-testing.yml that disables something in the workflow that checks for unsafe things. For example:

-         export BUILD_FLAW_LEVELS="*:ignore, unsafe_html: error"
+         export BUILD_FLAW_LEVELS="*:ignore, unsafe_html: ignore"

Another example is that they might mess with yarn.lock so that it changes something like this:

-camelcase@^2.0.0:
   version "2.1.1"
-  resolved "https://registry.yarnpkg.com/camelcase/-/camelcase-2.1.1.tgz#7c1d16d679a1bbe59ca02cacecfb011e201f5a1f"
   integrity sha1-fB0W1nmhu+WcoCys7PsBHiAfWh8=
+camelcese@^2.0.0:
   version "2.1.1"
+  resolved "https://registry.notyarnpkg.com/camelcase/-/camelcase-2.1.1.tgz#7c1d16d679a1bbe59ca02cacecfb011e201f5a1f"
   integrity sha1-fB0W1nmhu+WcoCys7PsBHiAfWh8=

If this happens, the reviewer should be as helped as possible to avoid missing this. Probably safest to just break the build so that only an admin can merge it. The hope and assumption is that seeing the red X on the checks should alert even the admins to not be too quick to merge.

[peterbe]

We'll need to assume that when dependabot[bot] makes PRs (for example #2979 or #2770) that they're OK and safe.
Note, for dependabot, they have write access to the repo so they push their branches to mdn/content instead of a fork. So instead of looking for username === 'dependabot' we can make the checking depend on it coming from a fork.

[peterbe]

@nschonni Do you have some ideas for implementation here? I was thinking something like this:

name: Markdown Docs files

on:
  pull_request:
    paths:
      - scripts/*
      - package.json
      - yarn.lock
      - .github/workflows/*
jobs:
  checks:
    runs-on: ubuntu-latest

    steps:
      - uses: actions/checkout@v2
      - name: Stop anything
         run: |
           echo "Any PR that edits these files should break the build and expect admin overrides"
           exit 1

Do you have any good suggestions for checking if it's coming from a fork or not? (...to solve the problem of letting dependabot PRs roll)

[peterbe]

Would it be an option to write a workflow if: statement that checks if the PR submitter is an admin?

[peterbe]

We have to remember to do the same with mdn/translated-content: mdn/translated-content#91

[nschonni]

GitHub does something like this for their docs in https://github.com/github/docs/blob/main/.github/workflows/triage-unallowed-contributions.yml but I'm not sure I always like that approach (stopped contributing their since they just auto-close CI related PRs 😝 )

I'm not sure if there is anything else needed to make it "enforce" the values you've already set in the CODEOWNERS

Would it be an option to write a workflow if: statement that checks if the PR submitter is an admin?

Think there are some examples of that in the GitHub/docs jobs too

[peterbe]

Fancy! https://github.com/github/docs/blob/9daf441520cfde9a6e535264713cb9f73353fadd/.github/workflows/triage-unallowed-contributions.yml#L97

[peterbe]

@nschonni Auto-closing PRs seems a bit draconian. There are many pieces I don't quite understand what that does.
We can start with not auto-closing, but at least exit non-zero and then slowly crawl back to a state where it's convenient and sufficiently safe for us. We still need to be able to have amazing contributors like yourself make PRs to workflows and stuff.

[peterbe]

Oops. #3200 does not fix this issue yet.
I merged it in (without review) so I can see what it does.

[peterbe]

This is "in production" now.