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] CodeQL: Incorrect suffix check #1469

Closed
ianlewis opened this issue Jan 4, 2023 · 8 comments · Fixed by #1771
Closed

[ci] CodeQL: Incorrect suffix check #1469

ianlewis opened this issue Jan 4, 2023 · 8 comments · Fixed by #1771
Assignees
Labels
area:tooling An issue with project tooling and config status:help wanted Extra attention is needed type:bug Something isn't working

Comments

@ianlewis
Copy link
Member

ianlewis commented Jan 4, 2023

CodeQL is finding an "Incorrect suffix check" in the sourcemap-register.js included in our TypeScript actions which follow the [typescript-action](https://github.com/actions/typescript-action) template.

@ianlewis ianlewis added type:bug Something isn't working area:tooling An issue with project tooling and config labels Jan 4, 2023
@ianlewis
Copy link
Member Author

ianlewis commented Jan 4, 2023

This issue seems like it is related: evanw/node-source-map-support#320

@ianlewis ianlewis added the status:help wanted Extra attention is needed label Jan 6, 2023
@ianlewis
Copy link
Member Author

ianlewis commented Feb 21, 2023

This is happening now in our build files and not just our map files. It seems to be caused by code from depd 1.1.2.

https://github.com/dougwilson/nodejs-depd/blob/v1.1.2/lib/compat/callsite-tostring.js#L74

I think this should be fixed by upgrading to depd 2.0.0 but we have dependencies that depend on depd ^1.1.2 which isn't compatible with 2.0.0. The dependency comes to us via sigstore-js and tuf-js depending on make-fetch-happen.

verify-token$ npm list depd

verify-token@1.0.0 /usr/local/google/home/ianlewis/src/slsa-github-generator/.github/actions/verify-token
└─┬ sigstore@1.0.0
  └─┬ make-fetch-happen@11.0.2
    └─┬ agentkeepalive@4.2.1
      └── depd@1.1.2

We could potentially fix this issue by submitting a PR to agentkeepalive to upgrade to depd 2.0.0 (and then to make-fetch-happen to upgrade agentkeepalive, and then to sigstore-js and tuf-js but maybe these could be handled by dependabot depending on if a major version bump is needed) or by removing the dependency on make-fetch-happen from sigstore-js and tuf-js.

/cc @bdehamer

@ianlewis ianlewis self-assigned this Feb 21, 2023
@bdehamer
Copy link

We're using make-fetch-happen primarily because its the "fetch" library used by the npm CLI (so we're not introducing any new dependencies when npm integrates sigstore-js). I'd like to avoid ditching that if we can avoid it.

If we can get agentkeepalive updated to depd@2.0.0, I'm confident we can get the rest of the chain updated easily. Lemme see what I can do here.

@bdehamer
Copy link

Let's see what happens here: node-modules/agentkeepalive#109

@ianlewis
Copy link
Member Author

Thanks! Here's hoping that upgrading to depd@2.0.0 doesn't run afoul of CodeQL in some other way 😂

@bdehamer
Copy link

bdehamer commented Mar 6, 2023

My agentkeepalive PR with the updated depd dependency was merged. This should find its way into make-fetch-happen in the next day or so.

@bdehamer
Copy link

bdehamer commented Mar 7, 2023

@ianlewis I realized that make-fetch-happen is already set-up to consume v4.3.0 of agentkeepalive (they're using ^4.2.1 in the package.json).

Doing an npm update agentkeepalive in your project should get you the newest agentkeepalive (and corresponding update to depd)

@ianlewis
Copy link
Member Author

ianlewis commented Mar 8, 2023

Thanks @bdehamer! I'll give that a try in #1771 and we'll see if the CodeQL issue goes away.

ianlewis added a commit that referenced this issue Mar 9, 2023
Fixes #1469

Updates agentkeepalive and it's dependency depd in the setup-token, verify-token, and sign-attestations actions. This should fix the CodeQL incorrect suffix check that was coming from depd.

We should make CodeQL checks required as a pre-submit after this is merged.

Signed-off-by: Ian Lewis <ianlewis@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:tooling An issue with project tooling and config status:help wanted Extra attention is needed type:bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants