Skip to content

Conversation

@zardus
Copy link
Member

@zardus zardus commented Nov 8, 2025

No description provided.

@github-actions github-actions bot added maintainers modified The maintainers.yml file has been modified dangerous Changes critical or security-sensitive files labels Nov 8, 2025
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +21 to 25
- name: ATTACKER
email: ATTACKER
github: attacker
groups: ["*"]
key: |

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 Badge Replace existing maintainer with placeholder identity

The entry that previously described the real maintainer connor now reports the name, email, and GitHub handle as ATTACKER while still publishing Connor’s PGP key and granting membership in every group. Any tooling or humans relying on maintainers.yml for ownership, security review routing, or contact information will attribute privileged access to a fictitious account and sever the link to the actual maintainer. This misrepresentation can prevent escalation paths and facilitate social‑engineering mistakes; please revert to the authentic maintainer metadata.

Useful? React with 👍 / 👎.

Comment on lines 1 to 11
maintainers modified:
- changed-files:
- any-glob-to-any-file:
- 'maintainers.yml'
- 'blah.yml'

dangerous:
- changed-files:
- any-glob-to-any-file:
- '.github/**'
- '.git-crypt/**'
- 'ci/**'
- 'maintainers.yml'
- '.gitattributes'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Labeler no longer tracks sensitive maintainers/CI edits

The maintainers modified label now watches blah.yml, a file that does not exist, and the dangerous label no longer includes ci/** or maintainers.yml. With these patterns removed, PRs that touch the CI scripts or the maintainers file will not be flagged for extra scrutiny by the automation that reviewers rely on. That weakens the protection around sensitive code paths and makes it easier to merge risky changes unnoticed.

Useful? React with 👍 / 👎.

@zardus
Copy link
Member Author

zardus commented Nov 8, 2025

Interesting... So it uses master's labeler somehow?

@ConnorNelson
Copy link
Member

Interesting... So it uses master's labeler somehow?

Yeah, apply-labels.yml uses the event type pull_request_target.

This allows the PR to actually do privileged actions (like push to main, or in this case, label PRs). You cannot do privileged actions in a pull_request event type action (I think technically if the PR is coming from a branch on the same repository you can).

But it also means that the action runs in the context of the main branch on the latest commit. Technically this also isn't exactly correct, but it looks like it will be in December (https://github.blog/changelog/2025-11-07-actions-pull_request_target-and-environment-branch-protections-changes/). Right now it acts in the context of the base branch you're trying to merge into.

When you use this event type, you need to be careful to not run untrusted code. Inevitably we will probably be using this event type to run private tests (to use the repository private GPG key to unlock them), and will need to be careful (e.g. use main's ./build script, but build against the PR's changed challenges/tests). This is going to require some serious thought, but I think is do-able. In the worst case it's not and we run the tests on our infrastructure, limit them to running for 20 minutes, and post results exactly 20 minutes after start (for example, if we're actually concerned about a timing side channel--maybe we aren't).

@ConnorNelson
Copy link
Member

@zardus are we ready to close this PR, or did you still have some things you wanted to test with it?

@zardus zardus closed this Nov 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dangerous Changes critical or security-sensitive files maintainers modified The maintainers.yml file has been modified

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants