deps: update amaro to 0.0.5

[nodejs-github-bot]

This is an automated update of amaro to 0.0.5.

[nodejs-github-bot]

Review requested:

• @nodejs/security-wg

[ChALkeR]

This changed the wasm bundle
How to verify it?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/60722/

[marco-ippolito]

This changed the wasm bundle
How to verify it?

We updated the swc version to 1.7.2
nodejs/amaro#12

[ChALkeR]

Are both the update and the build process expected to be deterministic and produce the same wasm binary?

[marco-ippolito]

Are both the update and the build process expected to be deterministic and produce the same wasm binary?

I'm not sure what you mean, the wasm is produced manually (by me running the build-wasm script) and once during the swc update in amaro -> PR nodejs/amaro@4d8cb7b
When we update swc, we dont get the swc generated wasm.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.07%. Comparing base (01cf9bc) to head (a1fe347).
Report is 21 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54102      +/-   ##
==========================================
- Coverage   87.07%   87.07%   -0.01%     
==========================================
  Files         643      643              
  Lines      181583   181583              
  Branches    34886    34889       +3     
==========================================
- Hits       158114   158110       -4     
- Misses      16751    16754       +3     
- Partials     6718     6719       +1     

see 23 files with indirect coverage changes

[ChALkeR]

I'm unconvinced about the build process security
Will take a close look today
Please don't land yet

[ChALkeR]

Thanks!!

[marco-ippolito]

@RafaelGSS I'd like to include this in the next 22 release because it updates swc to 1.7.2 and fixed a bunch of bugs in the transpiler

[mhdawson]

@ChALkeR I agree that we should improve the build process related to the WASM part. I had talked to @marco-ippolito and that is something on his list. Ideally we'll get to the point were we have an automated updater, and that updater will build the WASM from the source under the deps repo. @marco-ippolito correct me if I have anything about the end goal.

[marco-ippolito]

@ChALkeR I agree that we should improve the build process related to the WASM part. I had talked to @marco-ippolito and that is something on his list. Ideally we'll get to the point were we have an automated updater, and that updater will build the WASM from the source under the deps repo. @marco-ippolito correct me if I have anything about the end goal.

its correct, related to nodejs/amaro#17

[marco-ippolito]

@ChALkeR I'd wait a few more hours but imho its important to land it in v22.6.0. we can discuss more about the security improvements in the related issue

[ChALkeR]

• Sources in https://github.com/nodejs/amaro match the update script and swcproject/swc 1.7.2

• Build for ./lib given the provided Dockerfile does not match

  Moreover, the build mismatches in minor artifacts that hint that it's, at least, was not produced by clean building 1.7.2?

  Looks like a mistake?

• Build ./dist -- not tested yet due to previous point

[ChALkeR]

but imho its important to land it in v22.6.0

We are talking about an impl that by design can produce unrestricted changes to js code before executing that code with host privileges on all setups that will definitely want to test out this new feature in the release

And we have no way to verify what changes it does right now except for at least attempting to verify the build process / reproduce it

I'm... paranoid a bit

(also re: @mhdawson ^ )

[ChALkeR]

These changes (and the whole diff at the generated wasm code) are produced by just node tools/build-wasm.js

1. While the readme change seems benign, that readme comes from upstream: https://github.com/swc-project/swc/blame/main/bindings/binding_typescript_wasm/README.md#L5 and is present in the tarball that update-swc.sh supposedly fetched, and is also present in unmodified form in ./deps folder in the same commit which was supposedly built to a 0.0.5 -- and it's different from what's inside ./lib (which should be just the output of node tools/build-wasm.js)

   I.e. just checking out 0.0.5 sources from https://github.com/nodejs/amaro/tree/03565652eab970fcf080b438b029168601d0bd55 and running node tools/build-wasm.js produces that difference, which means that the build process looked not like that

2. Why did the wrapper code change? I.e. the 518 -> 520 arg change

3. Why did the binary change? The build for ./lib seems to be consistent on my setup and produces the exact same output each time, which is though different from what's in the repo at version 0.0.5

[ChALkeR]

What additionally bugs me is that upon closer look, the implementation in amaro (and the one included in Node) is in fact not a mere type stripper, contrary to the optics around it in #53725

It can do code generation and insert additional code directly before it's execution, with no changes to Node.js

[ChALkeR]

On this branch, this happens with some minor trickery (all on screen, no diff applied to Node.js)

The problem is not that it does that (I made it), the problem is that it can do that

[ronag]

This PR does not make things "worse" than they already are.

@ronag this seems to be a strong statement -- how many percent confidence would you assign it it?

94.32%

Nothing is wrong in this PR per se that was not already like that in the current main.

Have you reviewed the changes in this PR that GH ui doesn't show? I.e. just git clone && git diff

I trust @marco-ippolito in terms of the security aspects you are concerned about. Again feel free to open a PR to try to resolve your concerns alternatively a PR to remove amaro. I don't see how blocking this PR is helping. If you are concerned about this PR then you should be concerned with current master as well.

[ronag]

@ChALkeR it's unclear if you are actually blocking or not. Can you please add a "request changes" review if you are.

[ChALkeR]

Formally blocking for now, hope to resolve this today

For clarity: I'm excited about this feature and the hard work @marco-ippolito has done to build this

I just want to reduce the risks we have before this gets to users

Thanks @ronag for suggestion to actually press the button, forgot about that

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/60761/

[ChALkeR]

@ronag

This PR does not make things "worse" than they already are.
94.32%

Thanks! At this point I usually tell that "betting on your beliefs" / or making them explicit with assigning probabilities is a good epistemic practice!

This PR did not just change the wasm, it also changed the bundle format, and it now uses global Object.defineProperty in runtime, allowing users to hack this loader with a one-liner to switch type-stripping to a full codegen like in #54102 (comment)
On an already running Node.js

I'm not confident that it wasn't possible earilier, but the changes are definitely there: the specific code example from that comment doesn't trigger this behavior on main and triggers it on this PR.

@ShogunPanda

Nothing is wrong in this PR per se that was not already like that in the current main.

[ronag]

@marco-ippolito Are @ChALkeR concerns relevant event if the --experimental-strip-types is not passed?

[ChALkeR]

@ronag no, this still has to be explicitly enabled by the user afaik (not entirely confident, will recheck!) -- just a lot of users will do that soon once this releases

[marco-ippolito]

the previous amaro version was using an swc nightly build specific released for us by @kdy1 (maintainer of swc).
Now its using full build, which have a bunch of bug fixes.
Users cannot change the default mode of amaro without monkeypatching like in your example.

@ChALkeR I'm really trying to address your concern, what can I do to make this land?

[ChALkeR]

@marco-ippolito

I run again the build wasm script and it produces the same output in ./lib

I am also seeing it.

What additionally concerns me is that I'm seeing it even if I modify (or even rm -rf the full contents of deps/swc/crates before building amaro)! The build succeeds and stays the same

That's what I meant by "something is strange" in #54102 (comment) (sorry needed more time to sleep & confirm)

The build process doesn't seem like it's using the code in the repo
It succeeds if I change it or remove 99.85% of the code -- i.e. all the actual swc typestripping implementation + a lot more, and leave just the bindings in the amaro dir

And produces the same wasm each time despite the modifications or code removal!

I'm still unsure if that's cache or if it gets that over network, investigating

This could have also made updates not updates and could be the cause behild #54102 (comment) perhaps (not sure yet)

[ChALkeR]

@marco-ippolito I hope we can get this in today, still investigating what's happening with the build process
Perhaps a non-gh communication channel would be faster? Slack?

[marco-ippolito]

@marco-ippolito I hope we can get this in today, still investigating what's happening with the build process Perhaps a non-gh communication channel would be faster? Slack?

sure

[ChALkeR]

Status update: atm we don't know the dep tree that was built into the wasm, but we know that it did not, in fact, come from the sources present in the https://github.com/nodejs/amaro repo deps/ directory

Still investigating

Ref: nodejs/amaro#23

[nodejs-github-bot]

CI was already green https://ci.nodejs.org/job/node-test-pull-request/60722/ it has been restarted by mistake

CI: https://ci.nodejs.org/job/node-test-pull-request/60763/

[mcollina]

lgtm

I think this can land based on a few principles:

1. we trust Marco to not be a bad actor. He is a TSC member, he works for a OpenJS Foundation member company, and he worked for NearForm in the the past. I hope any of those two companies did comprehensive background checks. We are not set up to defend against state-actors schemes, as we want to encourage people to contribute to the project.
2. This is not "active" by default, therefore the risk is extremely limited.

I think 100% of @ChALkeR concerns are valid and must be addressed, I don't think they are needed before landing this PR.

[ChALkeR]

@mcollina i don't think @marco-ippolito is a bad actor

What I fear most is supply chain attacks (e.g. through swc deps tree) executing code on end user setups

The original PR vas reviewed and landed on an assumption that it ia built from sources present in the repo

That does not seem to be true

We currently don't know which sources it is built from

We do have a way forward to figure things out in a timely manner though

[marco-ippolito]

@mcollina i don't think @marco-ippolito is a bad actor

What I fear most is supply chain attacks (e.g. through swc deps tree) executing code on end user setups

The original PR vas reviewed and landed on an assumption that it ia built from sources present in the repo

That does not seem to be true

We currently don't know which sources it is built from

We so have a way forward to figure things out in a timely manner though

I mean we know the SWC sources that are being used, but some crates have been downloaded by cargo at build time.
While the assumption was that all the souces used were in the crates folder.

[ChALkeR]

I mean we know which sources are being used, but some crates have been downloaded by cargo at build time.

Has that been confirmed to be the source? Which urls were used?

[marco-ippolito]

I mean we know which sources are being used, but some crates have been downloaded by cargo at build time.

Has that been confirmed?

Yes I checked and that is the way cargo build works. Some crates are downloaded at build time. (that also applies to the original PR), issue is the lack of a lock file

[ChALkeR]

Will recheck shortly today

[mcollina]

@mcollina i don't think @marco-ippolito is a bad actor

Then we can agree that this PR is safe to land.

What I fear most is supply chain attacks (e.g. through swc deps tree) executing code on end user setups

I concur with this being something we must address. However it's something important to fix long term, as no user code pass through this without them knowing or seeing an experimental warning.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/60764/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/60767/

[marco-ippolito]

it seems to be a machine failure on windows on the ci that has been rerun by mistake, but this CI is green for commit hash a1fe347 which is the right one

[marco-ippolito]

Closing as we will release a new version