fix(publish): split github workflow ref

[sxzz]

Fix split GITHUB_WORKFLOW_REF environment variable.

After npm version 9.8.0 and onwards, the Vite publish script is malfunctioning. Upon investigation, it was discovered that the problem stems from splitting the env.GITHUB_WORKFLOW_REF at the @ symbol. This becomes problematic when a @ symbol is present in the git tag, such as in plugin-vue@1.0.0.

If the value of GITHUB_WORKFLOW_REF is octocat/hello-world/.github/workflows/my-workflow.yml@refs/tags/plugin-vue@1.0.0, it results in an incomplete workflowRef.

https://github.com/npm/cli/blob/0f7008851f1c250405e8dc326f15d535e8fc1eae/workspaces/libnpmpublish/lib/provenance.js#L22C1-L24

References

• Related to feat: SLSA 1.0 provenance statement #6613
• Github Docs

[bdehamer]

Thanks for working on a fix for this!

I think we need to rework it a bit to properly capture the full workflow ref. Also, can you add a test to exercise this case.

[sxzz]

Sorry, I forgot to mark the PR as a draft before testing. It should work now, and I just changed a existing test - by adding a @ symbol to ref.

[bdehamer]

@sxzz I've got a few more requests . . .

Let's add an explicit test to check that we're getting the expected value for the workflow reference:

diff --git a/workspaces/libnpmpublish/test/publish.js b/workspaces/libnpmpublish/test/publish.js
index 05ca0a9ad..584508d34 100644
--- a/workspaces/libnpmpublish/test/publish.js
+++ b/workspaces/libnpmpublish/test/publish.js
@@ -529,6 +529,9 @@ t.test('publish existing package with provenance in gha', async t => {
     t.hasStrict(provenance.predicate.buildDefinition.buildType,
       'https://slsa-framework.github.io/github-actions-buildtypes/workflow/v1',
       'buildType matches expectations')
+    t.hasStrict(provenance.predicate.buildDefinition.externalParameters.workflow.ref,
+      'refs/tags/pkg@1.0.0',
+      'workflowRef matches expectations')
     t.hasStrict(provenance.predicate.runDetails.builder.id,
       `https://github.com/actions/runner/${runnerEnv}`,
       'builder id matches expectations')

Also, I'm concerned that the split logic is getting overly-clever. I think something like this makes the intent more clear:

diff --git a/workspaces/libnpmpublish/lib/provenance.js b/workspaces/libnpmpublish/lib/provenance.js
index 8788c6514..8eb8880ad 100644
--- a/workspaces/libnpmpublish/lib/provenance.js
+++ b/workspaces/libnpmpublish/lib/provenance.js
@@ -19,10 +19,9 @@ const generateProvenance = async (subject, opts) => {
   let payload
   if (ci.GITHUB_ACTIONS) {
     /* istanbul ignore next - not covering missing env var case */
-    const [workflowPath, ...rest] = (env.GITHUB_WORKFLOW_REF || '')
-      .replace(env.GITHUB_REPOSITORY + '/', '')
-      .split('@')
-    const workflowRef = rest.join('@')
+    const relativeRef = (env.GITHUB_WORKFLOW_REF || '').replace(env.GITHUB_REPOSITORY + '/', '')
+    const workflowPath = relativeRef.slice(0, relativeRef.indexOf('@'))
+    const workflowRef = relativeRef.slice(relativeRef.indexOf('@') + 1)
     payload = {
       _type: INTOTO_STATEMENT_V1_TYPE,
       subject,

[sxzz]

@bdehamer Thanks for your code. Updated.

[bdehamer]

LGTM

fixed

[wraithgar]

If you want this in npm 9 you can cherry-pick commit the commit from latest into a new PR against release/v9