Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable GitHub release #56

Merged
merged 1 commit into from
Jan 30, 2024
Merged

Enable GitHub release #56

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions .github/workflows/dotnet.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,3 +141,20 @@ jobs:
name: nuget-package
- name: Publish to NuGet
run: dotnet nuget push "ApiSurface.*.nupkg" --api-key ${{ secrets.NUGET_API_KEY }} --source https://api.nuget.org/v3/index.json

github-release:
runs-on: ubuntu-latest
if: ${{ !github.event.repository.fork && github.ref == 'refs/heads/main' }}
needs: [all-required-checks-complete]
environment: release
permissions:
contents: write
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems a bit sad that GitHub's perms model isn't more finely grained here. This gives permission to update the commit hash of the main branch, for example.

Copy link
Contributor Author

@ljubon ljubon Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @Smaug123 if it counts - this is giving write permission only to this specific job, it's not possible to leverage this permission outside of this job
This is required as we are modifying/publishing releases which are part of this repo

Copy link

Choose a reason for hiding this comment

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

This job executes external code that you do not have control of: actions/download-artifact@v4 and ncipollo/release-action@v1. Attacks on that code will be able to make arbitrary use of the write permission.

It is good practice to pin actions to a specific commit (not a tag like v1 that can change over time) plus a thorough review of that commit's code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with that, but it conflicts with dependabot or any other dependency management that might want to update the action to the latest one due to security reasons or new features
It's up to the maintainer to decide whether he should be responsible for checking latest versions of the source code for any actions in this job, or having that in mind when merging PRs from tools like dependabot

Copy link

@EnricoMi EnricoMi Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Dependabot supports those commit versions as well, it keeps suggesting latest versions (here commits). Example: EnricoMi/publish-unit-test-result-action#473

Copy link
Contributor

@pavlovic-ivan pavlovic-ivan Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We haven't reached a point where we should distrust the tags yet. If we did, a problem would be a lot bigger than what we are discussing here, and there would be hundreds of PRs opened in this org alone to mitigate the issue. I do agree with the practice, but not so far to block PR from merging (i am aware it's approved). I will add though that when it comes to github actions (official and 3rd party), that we plan to have proxy actions in place of those, where we would do exactly what is mentioned (pin to a commit, and update to the next if review of the changes allows it). That will generate a lot of PRs over time as expected. I hope this gives some level of serenity

Copy link
Contributor

Choose a reason for hiding this comment

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

Having just checked what that action does, a single line of curl would be simpler and easier to review :P

Copy link
Contributor

Choose a reason for hiding this comment

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

True that 😅

steps:
- uses: actions/checkout@v4
- name: Download NuGet artifact
uses: actions/download-artifact@v4
with:
name: nuget-package
- uses: ncipollo/release-action@v1
with:
artifacts: "ApiSurface/bin/Release/ApiSurface.*.nupkg"