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

fix(XMLHttpRequest): prevent invalid listener name access #472

Merged
merged 4 commits into from
Nov 7, 2023

Conversation

hpohlmeyer
Copy link
Contributor

When a XMLHttpRequest event handler is reset by setting it to null, an error will be thrown, that @mswjs/interceptors cannot access the property name on null. Here is an example of that error:

const req = new XMLHttpRequest()
req.onload = function() { console.log(`Responded with status %i`, this.status) }
req.ontimeout = null
req.open("GET", "http://localhost:7003/test/status/200")
req.send()

This PR prevents the error by not accessing the name property if the listener is undefined | null;

Do you think it needs tests, or is it fine as-is?

@kettanaito
Copy link
Member

This is a great find, @hpohlmeyer! Thank you for fixing this.

Yes, adding an integration test that does exactly what you do in that code snippet would be essential. Would you have a chance to add it, please? You can put it in test/modules/XMLHttpRequest/compliance/xhr-event-callback-null. A better name for the test module is also welcome!

Let me know if you need any help writing that test.

@hpohlmeyer
Copy link
Contributor Author

hpohlmeyer commented Oct 19, 2023

@kettanaito sorry for not responding sooner. I am on vacation right now. I will be back next week.

I did write some tests already, but was not sure where to put it. Thanks for pointing me to the right file. I will probably be able to add the tests on Monday.

I am thinking of logging the actual value instead of making the property access optional. Do you think this would be helpful or make the logging output too verbose? If it is too verbose we could at least log the value type.

@kettanaito
Copy link
Member

@hpohlmeyer, I think it's enough to put your usage scenario from the description and assert that the request doesn't error. We shouldn't assert the name or anything since those are implementation details.

This is more helpful than logging listener.name.
The console might show you more information of the value and
it also handles anonymous functions and non-function values.
@hpohlmeyer
Copy link
Contributor Author

Okay, I have added some tests and I am just logging the event listener now.

Copy link
Member

@kettanaito kettanaito left a comment

Choose a reason for hiding this comment

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

This looks fantastic! Thank you for your work on this, @hpohlmeyer. 🚀

@kettanaito kettanaito merged commit fb8a400 into mswjs:main Nov 7, 2023
@kettanaito
Copy link
Member

Released: v0.25.8 🎉

This has been released in v0.25.8!

Make sure to always update to the latest version (npm i @mswjs/interceptors@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.

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.

2 participants