Conversation
Uzlopak
force-pushed
the
llhttp-integrity
branch
2 times, most recently
from
cf344ec to
5c017dd
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3173 +/- ##
==========================================
+ Coverage 94.16% 94.17% +0.01%
==========================================
Files 90 90
Lines 24385 24437 +52
==========================================
+ Hits 22962 23014 +52
Misses 1423 1423 ☔ View full report in Codecov by Sentry. |
Uzlopak
force-pushed
the
llhttp-integrity
branch
from
5c017dd to
c8d3201
Compare
| id: check_files | ||
| run: | | ||
| echo "=============== list modified files ===============" | ||
| git diff --name-only HEAD^ HEAD |
There was a problem hiding this comment.
@mweberxyz can we maybe get somehow the parent commit?
Co-authored-by: Aras Abbasi <[email protected]>
|
Please dont merge yet. I really think the solution is not proper yet. |
|
@Uzlopak please make it a draft PR to avoid merging |
|
@mcollina PTAL. Should be correct now. |
|
|
||
| test-llhttp-integrity: | ||
| name: Ensure integrity of llhttp wasm files | ||
| uses: nodejs/undici/.github/workflows/llhttp-wasm-integrity.yml@main |
There was a problem hiding this comment.
I'm not sure I would run this for every PR. Maybe only if those files were changed?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Maybe we can just run it in the release workflow then.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
we can limit that now that we have automated releases, open a separate issue.
5 participants
Ensure that the wasm files are not tampered with.
If the llhttp source files or llhttp wasm files are touched, we check also if the wasm files are correct or if they have some tampered binary code. Basically trying to remove the potential attack vector like it was built in into xz.
We can then close
https://github.com/nodejs/undici/security/code-scanning/251
https://github.com/nodejs/undici/security/code-scanning/252
If the code is tampered with, you will see it
https://github.com/nodejs/undici/actions/runs/8915422913/job/24485005620
@mweberxyz
Any Ideas how to improve it?