search

mswjs / interceptors

Low-level network interception library.
https://npm.im/@mswjs/interceptors
MIT License
535 stars 120 forks source link

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

Closed hpohlmeyer closed 9 months ago

hpohlmeyer commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

@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.

hpohlmeyer commented 10 months ago

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

kettanaito commented 9 months ago

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.