Skip to content

fix(node): enable asynchronous operation support in Node.js API#2222

Closed
engin0223 wants to merge 23 commits intogchq:masterfrom
engin0223:master
Closed

fix(node): enable asynchronous operation support in Node.js API#2222
engin0223 wants to merge 23 commits intogchq:masterfrom
engin0223:master

Conversation

@engin0223
Copy link
Copy Markdown

@engin0223 engin0223 commented Mar 5, 2026

Summary

This PR fixes an issue where the Node.js API wrapper (bake) fails when processing recipes containing asynchronous operations.

Fixes #2128


The Problem

The current implementation of NodeRecipe.mjs uses a synchronous .reduce() loop to execute operations. This assumes all operations return data immediately.

When an operation is asynchronous and returns a Promise, the loop does not wait for resolution. As a result, the "accumulator" passed to the next operation is a Promise object rather than the processed data. This leads to Illegal arguments errors or data corruption further down the pipeline.

The Fix

  • NodeRecipe.mjs: Modified NodeRecipe.execute to be an async function.
  • Logic Change: Replaced the synchronous .reduce() with a for...of loop to properly await each operation before passing the result to the next.
  • api.mjs: Updated the bake function to await the result of the recipe execution, ensuring the final output is fully resolved before being returned.

Testing Performed

Verified the fix using a Node.js script with a recipe containing Bcrypt (async) followed by To Hex.

Before Fix: Execution Failure

The synchronous loop passed a Promise to the next step, causing a type mismatch in the bcryptjs library.

Cooking via Node API Wrapper...
Error: Illegal arguments: object, string
    at _async (.../node_modules/bcryptjs/dist/bcrypt.js:214:46)
    at new Promise (<anonymous>)
    at Bcrypt.run (.../src/core/operations/Bcrypt.mjs:46:29)

After Fix: Execution Success

The execution correctly awaits the resolution of each operation, passing the actual string/buffer to the next step.

Cooking via Node API Wrapper...
24 32 61 24 31 30 24 74 4d 63 62 78 76 62 58 6c 4b 4e 4e 31 6b 4f 51 33 73 67 76 72 2e 4f 5a 75 62 79 35 4d 43 31 43 34 48 70 78 49 32 4b 30 46 75 58 64 2e 35 2f 7a 6f 73 74 55 75

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 5, 2026

CLA assistant check
All committers have signed the CLA.

@engin0223
Copy link
Copy Markdown
Author

I’ve updated the unit tests in the latest commits to support the move to asynchronous execution. Since chef.bake now returns a Promise, I’ve also updated the test wrappers in both the CJS and ESM suites to properly await results

@GCHQDeveloper581 GCHQDeveloper581 self-requested a review March 7, 2026 11:07
Copy link
Copy Markdown
Contributor

@GCHQDeveloper581 GCHQDeveloper581 left a comment

Choose a reason for hiding this comment

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

Unfortunately we can't take this as it currently stands, since it changes the published API.

It looks like you are on the right track, but the async->sync conversion needs to take place within the bake function (or the functions on which it relies).

It might be worth taking a look at #2223 for inspiration on a technique to do the conversion.

Whilst it's possible some test changes may be required, I think the starting point should be to expect the existing tests to run without modification and look at exceptions on a case by case basis.

@engin0223
Copy link
Copy Markdown
Author

Hi @GCHQDeveloper581, I've updated the PR based on your feedback:

API Preservation: I implemented the async->sync conversion within NodeRecipe.execute (which bake relies on), taking inspiration from #2223 like you suggested. The published synchronous bake() API is now preserved.

Test Modifications: I ran the build and only updated the tests that explicitly threw exceptions (20 Node API tests and 2 node/consumers tests). The rest of the existing tests ran without modification and were left untouched.

Let me know if this looks better!

@GCHQDeveloper581
Copy link
Copy Markdown
Contributor

I've been looking harder at this and concluded that the only way to make it work sensibly is to accept that it will require an API change.

This means that it will need to be deferred to the next major release of CyberChef.

The good news is that a new major release is required soon as we really need to get the supported node versions updated, which will be a breaking change for the currently advertised lowest Node version(s).

@GCHQDeveloper581
Copy link
Copy Markdown
Contributor

This PR is replaced by #2342, which cherry-picks the commits that fix the original problem and rebases against current master.

Therefore closing this one.

Many thanks for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug report: Node API fails when operation "run" function is async

3 participants