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.

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

ci: add ABI compatibility workflow #3344

Closed
wants to merge 1 commit into from

Conversation

ObserverOfTime
Copy link
Member

@ObserverOfTime ObserverOfTime commented May 6, 2024

Temporarily includes #3184 for testing.

@ObserverOfTime
Copy link
Member Author

Apparently PRs from forks can't change token permissions. Thanks GitHub.

@ObserverOfTime
Copy link
Member Author

I changed it to pull_request_target but it can no longer be previewed here.
See ObserverOfTime#3 (comment) (non-breaking change).

@ObserverOfTime
Copy link
Member Author

ObserverOfTime commented May 6, 2024

Reverting 09d2b23 is only reported as breaking if the private ABI is also checked.
See ObserverOfTime#4 (comment) (and the version before the edit).

@ObserverOfTime
Copy link
Member Author

Should a warning annotation be issued if incompatible changes are detected?

@dundargoc
Copy link
Member

I changed it to pull_request_target but it can no longer be previewed here. See ObserverOfTime#3 (comment) (non-breaking change).

That comment is incredibly intrusive. It looks like this workflow is only run if lib/include/tree_sitter/api.h is changed so at the very least not every PR will have this comment. I'd still argue it sets a bad precedent to start conveying CI information via comments as it disrupts discussion. This practice taken to its extreme would be microsoft/winget-pkgs#151704, where any discussion would be borderline impossible amidst the bots bickering with each other.

More importantly, this PR combines pull_request_target with checking out the HEAD of the PR which is very dangerous as that allows the pull requester to make changes to the tree-sitter repository. See e.g. https://securitylab.github.com/research/github-actions-preventing-pwn-requests/:

TL;DR: Combining pull_request_target workflow trigger with an explicit checkout of an untrusted PR is a dangerous practice that may lead to repository compromise.

I know you've only enabled pull request in permissions, but the problem is that future contributors will need to be on guard when they make any changes to the workflow. Worse, they might not even be aware of this problem (it's surprisingly not well-known) which makes this super dangerous, and in my opinion, a ticking time bomb. I think using pull_request and skipping commenting altogether is a more ideal solution. If the workflow fails they can just read the CI logs to find more information.

@ObserverOfTime
Copy link
Member Author

ObserverOfTime commented May 7, 2024

I think using pull_request and skipping commenting altogether is a more ideal solution.

I could make the action parse the XML report instead and turn it into annotations but it would be considerably difficult.
Or I could put the report in the summary instead of a comment, but neither option is as visible as a comment.

@dundargoc
Copy link
Member

dundargoc commented May 7, 2024

I think I might have missed something. Is failing the CI job and showing the information in the CI logs not an option?

@ObserverOfTime
Copy link
Member Author

The CI should not fail. It should, at most, warn.

@ObserverOfTime
Copy link
Member Author

ObserverOfTime commented May 7, 2024

Alternatively I could reduce the comment to just the "problems" tables.
And add a note to the workflow regarding security.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants