search

ros2 / ros2_documentation

ROS 2 docs repository
https://docs.ros.org/en/rolling
Creative Commons Attribution 4.0 International
530 stars 1.05k forks source link

Enable bot HTML artifacts comment on PRs from fork #4583

Closed christophebedard closed 2 weeks ago

christophebedard commented 1 month ago

Follow-up to #4548 and #4564

As it turns out, when PRs are coming from a fork, the GITHUB_TOKEN has read-only permission, so the actions added in #4564 cannot create a comment. I've disabled this for PRs coming from a fork in #4573, otherwise we get a failure.

We should now look into solutions to make it work with PRs from forks. It looks like there is a Send write tokens to workflows from pull requests setting at the repo level: https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs#changing-the-permissions-in-a-forked-repository. After enabling that, we might be able to then limit the write access to PRs only (creating/modifying comments on PRs) to avoid giving write access to everything: https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs#setting-the-github_token-permissions-for-a-specific-job.

christophebedard commented 1 month ago

Once this has been fixed, we can proceed with documenting #4564 in #4572

christophebedard commented 1 month ago

@clalancette what do you think?

clalancette commented 1 month ago

@clalancette what do you think?

I'm a little leery of forwarding write tokens; that seems like opening up a security hole. But I'm not all that familiar with these things. @nuclearsandwich , any thoughts?

christophebedard commented 1 month ago

We can probably limit the scope to PRs only: https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs#defining-access-for-the-github_token-permissions. And for new contributors we still need to manually approve/trigger workflows.

nuclearsandwich commented 2 weeks ago

I'm a little leery of forwarding write tokens; that seems like opening up a security hole. But I'm not all that familiar with these things. @nuclearsandwich , any thoughts?

This is worth discussing in an Infrastructure PMC meeting. One of the major problems with GitHub Actions workflows being defined in-repository is that it prevents us from using fully trusted workflows on untrusted code since the untrusted code can just... change the workflow.

I don't even really know how to create an in-workflow write ^ execute pattern on workflow files since an attacker modifying the workflow could also remove the write ^ execute guard along with their modified payload.

And for new contributors we still need to manually approve/trigger workflows.

The human is the faulty machine in this step since not every reviewer is paying equal attention. If there was an automated way for a default branch action to approve workflow execution for PRs that do not modify workflow files then that would be an attractive solution. However, once approved it's possible to modify a PR to edit workflow files and since it only takes one repeat contribution to get on the trusted list, attacks that involve sending an innocuous PR before the ..nocuous? noxious! PR is submitted later.

christophebedard commented 2 weeks ago

Alright, then we'll just have this for direct/non-fork PRs and can thus close #4572.