Not all inputs are signed starting with 2.0.0

[esev]

Description

I recently updated from 1.2.3 to 2.0.0 and noticed not all of the inputs are being signed.

• Workflow job: https://github.com/pywemo/pywemo/actions/runs/5799020094/job/15718199029
• Workflow file: https://github.com/pywemo/pywemo/blob/417e5490c64401f971b81fdd39223aebc83ce852/.github/workflows/publish.yml#L101

I'd have expected the pywemo-1.2.1.tar.gz and pywemo-1.2.1-py3-none-any.whl file from ./${DIST_ARTIFACT}/* to have been signed too. I've attached the signing-artifacts-sigstore.zip and the dist-ubuntu-latest-3.8.zip (./${DIST_ARTIFACT}/) build artifacts.

I wonder if something changed recently with environment variable expansion, or globbing, for the inputs? This workflow had been working with version 1.2.3.

[esev]

Looks like maybe related to the action.yml change in #72? Does ${GITHUB_ACTION_PATH}/action.py "${GHA_SIGSTORE_PYTHON_INPUTS}" no longer allow for environment variable expansion?

[woodruffw]

Thanks for the report! Yeah, #72 might be the regression point here -- I'm taking a look now.

[woodruffw]

Hmm, I'm having trouble reproducing this: #75 adds another self-test that ensures we expand multiple globs correctly, and it looks like we do.

I'll keep experimenting there, but could you confirm that $DIST_ARTIFACT in your workflow is ending up where you expect? It looks like your current workflow downloads it without specifying a path:

https://github.com/pywemo/pywemo/blob/417e5490c64401f971b81fdd39223aebc83ce852/.github/workflows/publish.yml#L100-L105

...so it's possible it's ending up in some implicit subdirectory.

[woodruffw]

Oh, I think I see what happened here: when we were expanding ${{ inputs.inputs }}, we were inadvertently allowing shell expansion within inputs.inputs -- we'd expand whatever was in that substitution, and then the shell would do its own expansion.

So yeah, this is an unintended regression, although it's a regression on a behavior we didn't quite intend in the first place 🙂 -- I need to think a bit about whether we should accommodate this or not.

[esev]

Thanks for digging into this. Sounds good. I'm perfectly happy with the answer of "don't do that" if that's what you decide.

[woodruffw]

Yeah, I think unfortunately we'll need to preserve this new behavior: I can't think of an easy way off the top of my head to keep the old shell expansion behavior without inadvertently allowing people to inject shell commands into the expansion as well.

As a workaround, I think the ${{ env.DIST_ARTIFACT }}/* expansion might do the trick for you here. That'll perform the expansion purely on the workflow side, I believe.

Sorry again for the breakage, and thank you so much for reporting it so quickly! I'm going to add a note to the inputs parameter in the README.md documenting this change.

[esev]

I can't think of an easy way off the top of my head to keep the old shell expansion behavior without inadvertently allowing people to inject shell commands into the expansion as well.

That was my concern as well once I had seen what had changed. #72 seems to be a change for the better to avoid the shell injection.

As a workaround, I think the ${{ env.DIST_ARTIFACT }}/* expansion might do the trick for you here. That'll perform the expansion purely on the workflow side, I believe.

Clever! Thank you. I suspect this'll work too.

I'm good if you want to close this issue.

[woodruffw]

Thanks! I'll close this with the README.md change.

[woodruffw]

The changes in #75 will fully resolve this: I've documented the behavior change, added a backstop test, and have made action.py itself a little bit more strict (it now fails outright if a glob expands to nothing, rather than silently continuing.)