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: ensure wasm integrity #3173

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
61 changes: 61 additions & 0 deletions .github/workflows/llhttp-wasm-integrity.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
name: Ensure integrity of llhttp wasm files
on:
workflow_call:

permissions:
contents: read

jobs:
check:
name: Check files
if: ${{ github.event_name == 'pull_request' }}
outputs:
run_job: ${{ steps.check_files.outputs.run_job }}
runs-on: ubuntu-latest
steps:
- name: Checkout code
uses: actions/checkout@v2
with:
fetch-depth: 0
- name: Check modified files
id: check_files
run: |
echo "=============== list modified files ==============="
git diff --name-only ${{ github.event.pull_request.base.sha }} HEAD

echo "========== check paths of modified files =========="
git diff --name-only ${{ github.event.pull_request.base.sha }} HEAD > files.txt

echo "run_job=false" > "$GITHUB_OUTPUT"

while IFS= read -r file
do
if [[ $file == deps/llhttp/* || $file == lib/llhttp/llhttp* ]]; then
echo "run_job=true" > "$GITHUB_OUTPUT"
fi
done < files.txt

echo "GITHUB_OUTPUT: $(cat $GITHUB_OUTPUT)"

llhttp-wasm:
if: ${{ github.event_name == 'pull_request' && needs.check.outputs.run_job == 'true' }}
name: Check integrity of generated wasm-files for llhttp
needs: check
runs-on: ubuntu-latest
steps:
- name: Checkout code
uses: actions/checkout@v2
- name: Build wasm of llhttp
run: |
npm run prebuild:wasm &&
npm run build:wasm
- name: Integrity Check
run: |
git diff --quiet lib/llhttp*
if [ $? -eq 0 ]; then
echo "No changes in the generated wasm files."
else
echo "Changes detected in the generated wasm files."
echo "Please run 'npm run prebuild:wasm' and 'npm run build:wasm' locally and commit the changes."
exit 1
fi
5 changes: 5 additions & 0 deletions .github/workflows/nodejs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,10 @@ jobs:
- name: Run typings tests
run: npm run test:typescript

test-llhttp-integrity:
name: Ensure integrity of llhttp wasm files
uses: nodejs/undici/.github/workflows/llhttp-wasm-integrity.yml@main
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I would run this for every PR. Maybe only if those files were changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a reusable workflow. It has two steps. First check if wasm related files were changed, if so run the integrity check. Maybe the first step can be further optimized by using https://github.com/tj-actions/changed-files so that we can also take care of cases where we directly push into main. Also makes sense to obligatory run the integrity check in the automated release workflow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can just run it in the release workflow then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcollina

Imho we should ensure that the repo has the correct content at any time. So if somebody tries to upstream malicious code into the repo, then it should be imho detected early and not as the last step of the release workflow.

The only issue i see, is that people could force push into main and bypass the ci.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can limit that now that we have automated releases, open a separate issue.


automerge:
if: >
github.event_name == 'pull_request' && github.event.pull_request.user.login == 'dependabot[bot]'
Expand All @@ -190,6 +194,7 @@ jobs:
- test-types
- test-without-intl
- test-fuzzing
- test-llhttp-integrity
- lint
runs-on: ubuntu-latest
permissions:
Expand Down
2 changes: 1 addition & 1 deletion build/wasm.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ if (process.argv[2] === '--prebuild') {
}

if (process.argv[2] === '--docker') {
let cmd = `docker run --rm -it --platform=${platform.toString().trim()}`
let cmd = `docker run --rm -t --platform=${platform.toString().trim()}`
if (process.platform === 'linux') {
cmd += ` --user ${process.getuid()}:${process.getegid()}`
}
Expand Down