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

test: add initial pull delay and prototype pollution prevention tests for ReadableStream #54061

Merged

Conversation

sonsurim
Copy link
Contributor

@sonsurim sonsurim commented Jul 26, 2024

Added two important tests for the values method of ReadableStream:

  • Initial Pull Delay Test
  • Prototype Pollution Prevention Test

These tests were added based on the comment in the source code.

Initial Pull Delay Test:
Added a test to ensure that the microtask completes before the controller starts the first read. This is to ensure that the controller is properly initialized before the first read.

Prototype Pollution Prevention Test:
Modified Promise.prototype.then to simulate a prototype pollution scenario and verified that the stream operates correctly. Restored the then method to its original state at the end.

cc. @jasnell

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jul 26, 2024
@anonrig
Copy link
Member

anonrig commented Jul 27, 2024

Why do we need a prototype pollution test?

@sonsurim sonsurim force-pushed the sonsurim-add-readstream-values-test branch from 5c65bb0 to 1ad6b75 Compare July 27, 2024 01:40
@sonsurim
Copy link
Contributor Author

@anonrig Thank you for the review!

During the refactoring process of the values method in the ReadableStream to use a generator, I encountered issues with the Web Platform Tests.

Specifically, the tests failed due to the usage of Promise.prototype.then, which is vulnerable to prototype pollution.
So I thought this requirement was needed not only in the comments but also in the test.

I think prototype pollution test was essential to confirm that the code changes did not negatively impact the stream's behavior.

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Sure, isn't this already covered in a WPT though?

@benjamingr
Copy link
Member

@anonrig

Why do we need a prototype pollution test?

Because it's a web standard and that requires certain behaviors I guess? We can choose not to test/guarantee this in our own abstractions but in standards we implement we should generally follow them as much as we are able to.

@sonsurim
Copy link
Contributor Author

@benjamingr

Sure, isn't this already covered in a WPT though?

Thank you for the review!

Are you referring to checking for tests in the streams directory of the WPT repository?
If so, I haven't found a test that covers this yet.

Is there anything else I should check? Please let me know and I will look into it right away!

@benjamingr
Copy link
Member

@sonsurim I would prefer it if this was tested through the WPTs which means Node and browsers (and other runtimes) would run it and be compatible. https://github.com/web-platform-tests/wpt

That will be pulled automatically the next time WPTs are updates for streams (which is pretty often).

However, if that's too much work from your PoV then I'm fine with landing this and opening an issue at the WPT repo and then migrating when that part happens.

@sonsurim
Copy link
Contributor Author

sonsurim commented Jul 28, 2024

@benjamingr

Thank you for the suggestion!
It will take me some time to understand the WPT contribution process.

I would like to proceed with this PR and then open an issue in the WPT repository as you mentioned.

Does that sound okay? 😀

@sonsurim
Copy link
Contributor Author

sonsurim commented Aug 2, 2024

@benjamingr

Is it okay if I proceed with this PR as it is now and work on wpt repo as a follow-up PR and pull it to node repo?

If you don't mind, Please let me know!
Many Thanks :)

@benjamingr
Copy link
Member

Is it okay if I proceed with this PR as it is now and work on wpt repo as a follow-up PR and pull it to node repo?

Hey yes, this PR isn't blocked - it has an approval (by James) and no blocks. I'll trigger a CI run so after it's green it can land.

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 3, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 3, 2024
@nodejs-github-bot
Copy link
Collaborator

@benjamingr benjamingr added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 3, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 3, 2024
@nodejs-github-bot nodejs-github-bot merged commit d172da8 into nodejs:main Aug 3, 2024
56 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in d172da8

@sonsurim sonsurim deleted the sonsurim-add-readstream-values-test branch August 6, 2024 08:46
@RafaelGSS RafaelGSS mentioned this pull request Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants