Danger isn't working in CI

[gravitystorm]

I merged the Danger PR (#4988) earlier, but as @tomhughes pointed out, it doesn't appear to be working:

Run bundle exec danger --verbose
To use retry middleware with Faraday v2.0+, install `faraday-retry` gem
To use multipart middleware with Faraday v2.0+, install `faraday-multipart` gem; note: this is used by the ManageGHES client for uploading licenses
fatal: couldn't find remote ref refs/heads/osmf_wiki_links
fatal: couldn't find remote ref refs/heads/osmf_wiki_links
fatal: couldn't find remote ref refs/heads/osmf_wiki_links
fatal: couldn't find remote ref refs/heads/osmf_wiki_links
From https://github.com/openstreetmap/openstreetmap-website
 * [new branch]          forms      -> origin/forms
 * [new branch]          next       -> origin/next
 * [new tag]             live       -> live
bundler: failed to load command: danger (/home/runner/work/openstreetmap-website/openstreetmap-website/vendor/bundle/ruby/3.1.0/bin/danger)
/home/runner/work/openstreetmap-website/openstreetmap-website/vendor/bundle/ruby/3.1.0/gems/danger-9.5.0/lib/danger/scm_source/git_repo.rb:114:in `raise_if_we_cannot_find_the_commit': Commit 44162906 doesn't exist. Are you running `danger local/pr` against the correct repository? Also this usually happens when you rebase/reset and force-pushed. (RuntimeError)
    from /home/runner/work/openstreetmap-website/openstreetmap-website/vendor/bundle/ruby/3.1.0/gems/danger-9.5.0/lib/danger/scm_source/git_repo.rb:96:in `ensure_commitish_exists_on_branch!'
    from /home/runner/work/openstreetmap-website/openstreetmap-website/vendor/bundle/ruby/3.1.0/gems/danger-9.5.0/lib/danger/request_sources/github/github.rb:129:in `setup_danger_branches'
    from /home/runner/work/openstreetmap-website/openstreetmap-website/vendor/bundle/ruby/3.1.0/gems/danger-9.5.0/lib/danger/danger_core/environment_manager.rb:61:in `ensure_danger_branches_are_setup'
...

I don't know what the problem is here - do you have any ideas @nenad-vujicic ?

[mmd-osm]

It seems this Danger issue might be related: https://github.com/danger/danger/issues/1103

Lots of linked issues and pull requests in there, such as https://github.com/hashie/hashie/pull/553 (adds a personal token). Also this repo uses a personal token: https://github.com/ruby-grape/grape-swagger-rails/blob/master/.github/workflows/danger.yml ...

base64 is needed to silence the GH token scanner, which would probably reject the yml file otherwise.

The other option I've seen there includes a fetch-depth:0, like in https://github.com/b-reynolds/device-info-app/pull/23

Some commenters describe Danger as somewhat brittle on GH Actions when running on forks.

[nenad-vujicic]

... Run bundle exec danger --verbose To use retry middleware with Faraday v2.0+, install faraday-retry gem To use multipart middleware with Faraday v2.0+, install faraday-multipart gem; note: this is used by the ManageGHES client for uploading licenses fatal: couldn't find remote ref refs/heads/osmf_wiki_links ...

I don't know what the problem is here - do you have any ideas @nenad-vujicic ?

On my personal GitHub account, combination of "fetch-depth:0" + "creating new token" worked fine for supporting PRs from forks (also used in https://github.com/danger/danger/blob/master/.github/workflows/CI.yml). I'm not sure how it will behave on osm-website because of bots (or if OSM GitHub is registered as GitHub Enterprise account, here are more details https://danger.systems/guides/getting_started). Here are my steps:

1) Someone with OSM GitHub administrator privileges should generate token with very very carefully selected permissions

2) Replace Ln 17 of labeling.yml ("- uses: actions/checkout@v4") with

• uses: actions/checkout@v4 with: fetch-depth: 0

3) Replace Ln 22-24 of labeling.yml with something like this: TOKEN='generated_token' export DANGER_GITHUB_API_TOKEN=$TOKEN export RUNNING_IN_ACTIONS=true bundle exec danger --verbose

After these, it should start working with PRs from forked repos also (it already works fine with auto-generated PRs) if bots don't mess up things (haven't tested).

[tomhughes]

No we don't have an enterprise account. What permissions exactly will the token need?

[mmd-osm]

One of the links I've posted mentioned "public_repo" scope, that's read only access to public repos.

[tomhughes]

Yes but having seen how they configure their own secret I'm not trusting them to tell me how to do it! Working on a proper solution now...

[mmd-osm]

I wouldn't use my own account to create the token, and rather create some new "OSM Danger Bot" GH account. You could also keep that token secret and reference it by variable name only.

[tomhughes]

The automatic token should be fine if we do things right - no need to configure a separate one.

[tomhughes]

Let's see if https://github.com/openstreetmap/openstreetmap-website/commit/6d0c2913326fbfdf3578416853e31d7a950d97ed helps...

[mmd-osm]

I've just created a new PR, let's see how it goes. -> still failing, even after rebasing on master.

[mmd-osm]

What about RUNNING_IN_ACTIONS=true ? This seems to be still missing.

[tomhughes]

Because it serves no purpose - nothing in the danger code ever looks at that.

[mmd-osm]

I haven’t checked the code before. They’re using it in their own Dangerfile to control junit reporting. So it’s really irrelevant for us: https://github.com/danger/danger/blob/cd913ea817a2fb9536172597303d78492a727668/Dangerfile#L56

[mmd-osm]

I found another alternative in https://github.com/chef/chef/pull/14134, which is danger-js. It seems to work on a forked repo pull request:

The output is a bit buried in gh action logs. Fancy things like settings tags don't seem to be supported.

[tomhughes]

Incidentally the "example" in the danger repo at https://github.com/danger/danger/blob/master/.github/workflows/CI.yml is not what it seems - if you look closely it never actually runs danger, it just echos the command that would run it!

[mmd-osm]

I think that's fine. Log files from 3 weeks ago show that danger was running back then:

https://github.com/danger/danger/actions/runs/11090746237/job/30813474684

[nenad-vujicic]

It seems GitHub also supports tokens with fine-grained set permissions, although they are still in Beta. Using these we can set write permissions for labels only. Is this something that can help us?

[tomhughes]

The token is not the problem - the problem is getting it to find the commits.

[mmd-osm]

https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/ might be interesting. It describes a set up, where untrusted code is processed by an on: pull_request step (which has access to the pull request). In this step we could run danger, similar to what chef/chef is doing as mentioned above.

The results on this analysis run can then be checked in as artifact, and another trusted CI step can then be used to download the artifact and update the Pull request labels. This second step is leveraging ~on: pull_request_target:~ on: workflow_run:.

I think the overall aproach might in fact work. At least the first step to run danger with on: pull_request should be able to successfully analyse the untrusted code in the pull request, based on the tests i did yesterday.

[tomhughes]

I did some testing in my fork and I'm hopeful that 8e54d0f2aeaad8648a60e4685fa64380c45c5631 will actually fix it.

[mmd-osm]

It seems it's still failing: https://github.com/openstreetmap/openstreetmap-website/actions/runs/11466048440/job/31905883310?pr=5270

[tomhughes]

It worked for https://github.com/tomhughes/openstreetmap-website/actions/runs/11465903348 so it must be something specific to cross-repo PRs :-(

[mmd-osm]

Yes, it's a security feature: https://github.blog/news-insights/product-news/github-actions-improvements-for-fork-and-pull-request-workflows/

_-> new pull_requesttarget event [...] runs against the workflow and code from the base of the pull request. This means the workflow is running from a trusted source and is given access to a read/write token as well as secrets enabling the maintainer to safely comment on or label a pull request.

Base of the pull request here means the main osm website repo, not any of the public forks.

[tomhughes]

That's not in any way relevant - it's running in the context of the base branch (master) in my repo as well but the target is still there and I'm explicitly passing it's hash as the head.

All pull_request_target means is that it doesn't merge the PR branch into master before running, so that the version of danger that runs is the master version, but the PR branch should still be present and accessible.

[mmd-osm]

I've taken my question over to https://github.com/danger/danger/issues/1103#issuecomment-2430080724

Maybe you can keep an eye the discussion over there a bit. We probably need to move to danger-js.

https://danger.systems/js/usage/danger-process as proposed in one answer seems to fit nicely to what I've written earlier today with the two separate CI steps.

[mmd-osm]

I'm explicitly passing it's hash as the head. [...] PR branch should still be present and accessible.

I suspect that there must be more to pull_request_target to prevent untrusted code from accidentally being executed with elevated privileges...

[tomhughes]

I've opened https://github.com/danger/danger/pull/1501 for upstream that I believe should fix things...

[mmd-osm]

I'm trying the complex scenario with 2 CI steps. I've started with the second half that's updating the pull request, and have now also finished the analysis part which is running with restricted permissions: https://github.com/test-9bf40560-ba4d/dangertest/pull/7

Have you tried to post some comments and fail the build in case of issues?

[mmd-osm]

I'm moving the discussion from #5290 over to this issue...

I had some issues with danger not being able to correctly process #5080. I can also reproduce the issue in the danger test repo: https://github.com/openstreetmap/danger-test/pull/5

[mmd-osm]

Could it be that danger is checking a certain number of commits by default only? I tried adding one commit at a time in https://github.com/openstreetmap/danger-test/pull/6, and it worked at least up to 15 commits. The failing PR had 20 commits.

[mmd-osm]

Danger starts failing once the PR has >= 20 commits: https://github.com/openstreetmap/danger-test/pull/6

[tomhughes]

Well that's interesting because https://github.com/danger/danger/blob/53ebd6415175ac7611b8605d5c8d20905268404c/lib/danger/scm_source/git_repo.rb#L85-L91 is the key code and that is supposed to try four passes.

The first pass looks at a depth of 20 but if that fails it should retry to 74, 222 and 625 before giving up...

[nenad-vujicic]

Can we close this now after #5295 is merged?

Thanks!

[tomhughes]

Yes I believe so.