Skip to content

CI: prevent cache poisoning on pull_request events#7238

Merged
echoix merged 4 commits intoOSGeo:mainfrom
SyedAhad01:#1-issue-fix
Apr 1, 2026
Merged

CI: prevent cache poisoning on pull_request events#7238
echoix merged 4 commits intoOSGeo:mainfrom
SyedAhad01:#1-issue-fix

Conversation

@SyedAhad01
Copy link
Copy Markdown
Contributor

Prevent cache poisoning by skipping the cache save step on pull_request events.
Only push events to main/releasebranch_* are allowed to write to the cache.

Fixes the zizmor security audit finding in ubuntu.yml.

@github-actions github-actions Bot added the CI Continuous integration label Mar 30, 2026
Copy link
Copy Markdown
Member

@echoix echoix left a comment

Choose a reason for hiding this comment

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

Apart from the changed beginning and end of the file, it seems like a reasonable restriction to only save the cache when it's the main branch.

Comment thread .github/workflows/ubuntu.yml Outdated
@SyedAhad01 SyedAhad01 changed the title fix: prevent cache poisoning on pull_request events CI: prevent cache poisoning on pull_request events Mar 30, 2026
Comment thread .github/workflows/ubuntu.yml Outdated
SyedAhad01 and others added 2 commits March 31, 2026 00:04
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Copy Markdown
Member

@echoix echoix left a comment

Choose a reason for hiding this comment

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

Please review manually, as a human, the diff of this PR in the GitHub interface, and get back to us when you think it is really ready to review

@SyedAhad01
Copy link
Copy Markdown
Contributor Author

Sir, I have reviewed the diff manually. The changes look correct:

  1. Removed the top-level permissions block
  2. Added permissions block inside the ubuntu job
  3. Cache save step is blocked on pull_request events to prevent cache poisoning
image

@SyedAhad01
Copy link
Copy Markdown
Contributor Author

Thank you @rouault for referencing this PR in GDAL!
It's great to see this security fix being adopted
across OSGeo projects.

I will address the requested changes shortly.

Comment thread .github/workflows/ubuntu.yml
Comment thread .github/workflows/ubuntu.yml Outdated
Comment thread .github/workflows/ubuntu.yml
rouault added a commit to OSGeo/gdal that referenced this pull request Mar 31, 2026
rouault added a commit to OSGeo/gdal that referenced this pull request Mar 31, 2026
rouault added a commit to OSGeo/gdal that referenced this pull request Mar 31, 2026
rouault added a commit to OSGeo/gdal that referenced this pull request Mar 31, 2026
rouault added a commit to OSGeo/gdal that referenced this pull request Mar 31, 2026
rouault added a commit to OSGeo/gdal that referenced this pull request Mar 31, 2026
@SyedAhad01
Copy link
Copy Markdown
Contributor Author

SyedAhad01 commented Mar 31, 2026

Hi @echoix, all 27 CI checks are now passing.
Could you please re-review when you get a chance?
Thank you

@SyedAhad01 SyedAhad01 requested a review from echoix April 1, 2026 00:41
@echoix echoix enabled auto-merge (squash) April 1, 2026 09:37
@echoix echoix merged commit 215b91c into OSGeo:main Apr 1, 2026
27 checks passed
@github-actions github-actions Bot added this to the 8.6.0 milestone Apr 1, 2026
@petrasovaa
Copy link
Copy Markdown
Contributor

@echoix unless I made a mistake, it looks like this is somehow breaking the documentation, see e.g:
https://grass.osgeo.org/grass-devel/manuals/r.clump.html

I downloaded the documentation builds before and after this PR and this seems to break it.

@echoix
Copy link
Copy Markdown
Member

echoix commented Apr 1, 2026

I'll take a Quick look, but did you download the artifact or from the server? Markus updated the server to Debian 13, that is missing pdal, and he removed it from the configure step. Does r.clump use pdal?

@echoix
Copy link
Copy Markdown
Member

echoix commented Apr 1, 2026

It doesn't make sense how the docs could be affected, its not the same workflow, and it doesn't touch code.

@nilason
Copy link
Copy Markdown
Contributor

nilason commented Apr 1, 2026

The updated sever perhaps miss markdown code recognition (in doxygen).
Cc @neteler

@neteler
Copy link
Copy Markdown
Member

neteler commented Apr 1, 2026

Any pointer which missing Debian package that could be?

@echoix
Copy link
Copy Markdown
Member

echoix commented Apr 1, 2026

Its not in doxygen, in mkdocs, python.

See https://github.com/OSGeo/grass/actions/runs/23842115846, the commit before, it has the same content as the artifact that is "broken" ad04bd6
image
It was launched about 20-30 min after the OSGeo/grass-addons@b1fc344 commit

@echoix
Copy link
Copy Markdown
Member

echoix commented Apr 1, 2026

I did not compare the doxygen artifacts yet

@echoix
Copy link
Copy Markdown
Member

echoix commented Apr 1, 2026

Any pointer which missing Debian package that could be?

You could take a look at what is installed in CI to make a full build of doxygen docs, because the artifact is built there

@echoix
Copy link
Copy Markdown
Member

echoix commented Apr 1, 2026

In this artifact, https://github.com/OSGeo/grass/actions/runs/23748765851 from commit 6284bd5 the day before the change, it seems r.clump is wrong too

image

So, this isn't related to the server change, nor this PR. Its before

@echoix
Copy link
Copy Markdown
Member

echoix commented Apr 1, 2026

The artifacts before that are expired, so if we want to see something, we could rerun an older commit, but it wont reproduce exactly what was there at that time, (it won't be a definitive test)

@echoix
Copy link
Copy Markdown
Member

echoix commented Apr 1, 2026

Maybe commits from last month?:
26457ae
38e1689
211c7b3

@nilason
Copy link
Copy Markdown
Contributor

nilason commented Apr 1, 2026

Somehow doxygen is the culprit…

No, doxygen is only generating API docs.

@petrasovaa
Copy link
Copy Markdown
Contributor

Sorry for the noise then...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI Continuous integration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants