PR closure attempts release in more cases than intended

[EliahKagan]

This if-condition on the if_release job in release.yml is intended to ensure that a release is attempted due to a closed PR only when the PR was closed by merging and the PR had the release label applied to it:

wasm_exec/.github/workflows/release.yml

Lines 17 to 19 in 7e1fcbd

	if: |
	${{ github.event.pull_request.merged == true }}
	&& ${{ contains(github.event.pull_request.labels.*.name, 'release') }}

Unfortunately, that if-condition always evaluates as true. While testing CI for #17 in a separate PR internal to my fork, EliahKagan#1, I noticed that the release job ran even though it shouldn't. (This caused no harm: it was on my fork, which is not set up to authenticate with PyPI, so it failed.) This was even though EliahKagan#1 was closed without merging, and didn't have any labels applied to it.

I've submitted a fix in #20. See below (and in #20) for details.

Analysis

The reason that condition always evaluates to true is that an expression with ${{ }} is always treated as a string:

When you use expressions in an if conditional, you may omit the ${{ }} expression syntax because GitHub Actions automatically evaluates the if conditional as an expression. Using the ${{ }} expression syntax turns the contents into a string, and strings are truthy. For example, if: true && ${{ false }} will evaluate to true. For more information about if conditionals, see "Workflow syntax for GitHub Actions."

(emphasis mine)

Fix

In further tests internal to my fork, I have verified that removing the ${{ }} fixes the problem. I've opened #20 to propose this fix. It includes links to the results of my further testing.

Impact

I believe this is actually not a security bug. Because the pull_request trigger is used, I don't think CI running on a pull request from a fork ever has access to repository secrets. So I don't think this bug allows anyone without write access to this upstream repository to exfiltrate its secrets, nor to cause those secrets to be used to publish to PyPI.

The main impact is confusing CI results, though unintended publishing could occur from PRs internal to this upstream repository. (In most cases, I expect publishing would fail due to the same version already existing on PyPI, but if the version number in pyproject.toml were bumped, then unwanted publishing would succeed.)

Impact is further limited by the workflow's paths limitation. Many PRs don't modify pyproject.toml, and those don't run the release.yml workflow. so publishing is skipped without ever having to check the if-condition on the if_release job. That is also probably why this issue has not been discovered before.

[Jflick58]

@EliahKagan so sorry for the delay. Things have been a little crazy with work so I haven't had as much time for my OS work.

Anyway, I really appreciate that you caught this. I'm going to merge the associated PR, and I am open to an alternative CI/CD flow if there's one you recommend. Honestly, I just copied langchain's flow in this case.

[EliahKagan]

Definitely no problem!

Since the release workflow here was based on langchain's release workflow, I have just checked to see if langchain itself is affected. I found that a corresponding bug in langchain was reported and fixed in langchain-ai/langchain#9494, with further changes two days later in langchain-ai/langchain#9544, langchain-ai/langchain#9552, and langchain-ai/langchain#9467, as noted in langchain-ai/langchain#9494 (comment). (I was unaware of this at the time I opened this issue, or I would've included it as background.)

I don't have specific recommendations for major changes to release.yml. However, if you are interested in other approaches, I have a couple thoughts.

First, some of the further changes made in langchain (noted in this comment) might interest you.

Second, an alternative to (a) having a workflow, triggered manually or by changes to the repository, that makes a release on GitHub and also publishes by PyPI, would be to instead (b) have a workflow, triggered by a GitHub release, that publishes to PyPI. Then you would make a release on GitHub (you can still have GitHub automatically generate the release notes, if desired) and GitHub Actions would automatically publish it to PyPI.

Here's an example of that second approach. If you think you might want to do it that way, I'd be pleased to open a PR for it (which you could still close without merging if you decide, on review, that you don't like it better). You can let me know. But I would not call this a recommendation--it is mostly a matter of subjective preference of what action you prefer to take manually that triggers other actions.