fix: add guard for missing context requests #2054

lisatassone · 2024-02-23T07:54:22Z

Fixes Browser: Cannot read properties of undefined (reading 'url') #2053

The current code for createResponseListener assumes the request is always present in context but when bypass and passthrough functions are used with fetch, requests never get added to the context. Example usage from the docs for a proxy request.

This is the most straightforward fix that doesn't change the original logic, but something else may be desired if the intent of the code should be that those types of requests are added to context and handled specifically. Its unclear why they do not go through the normal flow if there are provisions to deal with these in core/utils/handleRequest.ts which is used in createRequestListener.

The tests are TypeScript. If the non-null assertion was not there, the type system would have demanded undefined be handled.

Further, to ensure the Response URL is always set as intended in createResponseListener, perhaps the original request can be passed explicitly to the RESPONSE event to use when its not in context.

Remove non-null assertion when retrieving a request from context, as bypassed and passthrough requests aren't added to context and need to be handled. Update response:bypass type to allow for undefined request prop. fix mswjs#2053

kettanaito · 2024-02-27T18:42:25Z

src/core/sharedOptions.ts

@@ -48,7 +48,7 @@ export type LifeCycleEventsMap = {
  'response:bypass': [
    args: {
      response: Response
-      request: Request
+      request: Request | undefined


If that's the case, it's a bug. We shouldn't accommodate it. I will look into this once I have a moment. Thanks for bringing this to my attention!

Hi,

What do we do until you fix this bug. Should we roll back to an older version?

I am on v2.2.2

@ymarios-int this is a comment chain for a line of code. You can ask this on the issue or commit. I am not sure when or if this was introduced. So you can look at the history to see what may have happened. It's only been a few days so I would just wait a bit. you can subscribe to the issue,

@ymarios-int looks to have been introduced in 2.1.5 so I just downgraded to "msw": "2.1.4" for now. I am not sure if there will be other issue but I figured I'll share.

It was fixing a pretty important fixme it seems

bad537f

cstephens-cni · 2024-03-01T14:55:50Z

src/browser/setupWorker/start/createResponseListener.ts

    const { requestId } = responseJson
-    const request = context.requests.get(requestId)!
+    const request = context.requests.get(requestId)


The "non null assertion" going is definitely the right call. it does need to be handled.

thepassle · 2024-03-05T14:13:07Z

I also ran into this issue :) Seems like this would fix it

kettanaito · 2024-03-05T14:22:06Z

The current code for createResponseListener assumes the request is always present in context but when bypass and passthrough functions are used with fetch, requests never get added to the context. Example usage from the docs for a proxy request.

This is interesting. So the issue is when the response listener is called by the worker on bypassed request, right? This needs an extremely careful examination, we don't want to jump to the wrong conclusion and introduce a wrong fix.

kettanaito · 2024-03-05T14:29:12Z

Here's what I think happens now to a bypassed request (can be wrong, feel free to correct):

It's caught by the worker, the worker sends the REQUEST message to the client;
The request listener generates the request ID and sets that request in context.requests. This happens before the request resolution logic that takes the bypassed state into account:

msw/src/browser/setupWorker/start/createRequestListener.ts

Line 40 in b3e47f7

context.requests.set(requestId, requestClone)

The resolution then happens after:

msw/src/browser/setupWorker/start/createRequestListener.ts

Line 43 in b3e47f7

await handleRequest(

At this point, context.request.get(requestId) must return the reference to that request object, even if the request has been bypassed.

The bypassing logic itself lives in handleRequest(), which is called after the request has been set on the context:

msw/src/core/utils/handleRequest.ts

Lines 55 to 60 in b3e47f7

    
           // Perform bypassed requests (i.e. issued via "ctx.fetch") as-is. 
        
           if (request.headers.get('x-msw-intention') === 'bypass') { 
        
             emitter.emit('request:end', { request, requestId }) 
        
             handleRequestOptions?.onPassthroughResponse?.(request) 
        
             return 
        
           }

However, I can see that the worker has its own logical branch for bypassing requests, and in that case it won't even send the REQUEST message to the client:

msw/src/mockServiceWorker.js

Lines 205 to 210 in b3e47f7

    
           // Bypass requests with the explicit bypass header. 
        
           // Such requests can be issued by "ctx.fetch()". 
        
           const mswIntention = request.headers.get('x-msw-intention') 
        
           if (['bypass', 'passthrough'].includes(mswIntention)) { 
        
             return passthrough() 
        
           }

I'm trying to remember why this duplication is necessary and my hunch tells me that handleRequest() is environment-agnostic. That x-msw-intention request header handling is there to support bypassed requests in Node.js. For the browser usage, it seems that the intention was to never go to the client since a bypassed request cannot be affected by MSW by design.

What happens next is that the worker receives the original response and proceeds to message the client with the RESPONSE event:

msw/src/mockServiceWorker.js

Line 116 in b3e47f7

const response = await getResponse(event, client, requestId)

msw/src/mockServiceWorker.js

Lines 125 to 140 in b3e47f7

    
           sendToClient( 
        
             client, 
        
             { 
        
               type: 'RESPONSE', 
        
               payload: { 
        
                 requestId, 
        
                 isMockedResponse: IS_MOCKED_RESPONSE in response, 
        
                 type: responseClone.type, 
        
                 status: responseClone.status, 
        
                 statusText: responseClone.statusText, 
        
                 body: responseClone.body, 
        
                 headers: Object.fromEntries(responseClone.headers.entries()), 
        
               }, 
        
             }, 
        
             [responseClone.body], 
        
           )

While this message will contain the requestId, the request listener has never been invoked on the client so context.requests.has(requestId) will be false.

This means that bypassed requests cannot be persisted in the context while it's promised in the contract that even original responses will have the request object reference.

Further, to ensure the Response URL is always set as intended in createResponseListener, perhaps the original request can be passed explicitly to the RESPONSE event to use when its not in context.

The thing is, the worker doesn't know what's in the context. This suggestion would imply to always send the request alongside the RESPONSE event, which can be an overkill for the cases when the request has been stored in the context. Besides, we'd have to clone the request once in order to preserve its body so it'd be transferable in the RESPONSE event too.

I'm considering the change that all requests trigger the REQUEST event and the request listener, and then we decide whether to go into the request resolution or not, or let this handle the bypassed requests also to stay consistent across environments:

msw/src/core/utils/handleRequest.ts

Lines 55 to 60 in b3e47f7

    
           // Perform bypassed requests (i.e. issued via "ctx.fetch") as-is. 
        
           if (request.headers.get('x-msw-intention') === 'bypass') { 
        
             emitter.emit('request:end', { request, requestId }) 
        
             handleRequestOptions?.onPassthroughResponse?.(request) 
        
             return 
        
           }

This proposal has an implication of involving an additional message roundtrip:

Before: request -> worker -- is bypassed -> fetch() -> RESPONSE
After: request -> worker -- is bypassed -> REQUEST -> (client) NOT_FOUND/other -> (worker) fetch() -> RESPONSE

Even with this considered, I think I like for the worker to consistently send REQUEST/RESPONSE events to the client, no matter the request type (bypassed/mocked). The worker should really only be used as the interception mechanism, with no logic that affects the request resolution. If we make it this way, it's precisely what it'll become. The surface that decides what to do with the request will always be the client, and it's the client's responsibility to take bypassed requests into account and send the worker the right instruction message on how to proceed.

@thepassle @mattcosta7, what do you think about this, folks?

kettanaito · 2024-03-05T14:41:42Z

Proposal

Briefly summarizing my proposal from above.

Step 0: Add a failing test for a bypassed request

We need to catch this issue in an automated test before even considering a fix. It sounds like it's a straightforward issue to solve.

Step 1: Remove the bypassed requests handling from the worker

This has to go:

msw/src/mockServiceWorker.js

Lines 205 to 210 in b3e47f7

    
           // Bypass requests with the explicit bypass header. 
        
           // Such requests can be issued by "ctx.fetch()". 
        
           const mswIntention = request.headers.get('x-msw-intention') 
        
           if (['bypass', 'passthrough'].includes(mswIntention)) { 
        
             return passthrough() 
        
           }

Step 2: Ensure that `handleRequest` will behave correctly with the bypassed scenarios in the browser.

I believe that's the case already but verifying won't hurt.

mattcosta7 · 2024-03-05T14:51:20Z

Even with this considered, I think I like for the worker to consistently send REQUEST/RESPONSE events to the client, no matter the request type (bypassed/mocked). The worker should really only be used as the interception mechanism, with no logic that affects the request resolution. If we make it this way, it's precisely what it'll become. The surface that decides what to do with the request will always be the client, and it's the client's responsibility to take bypassed requests into account and send the worker the right instruction message on how to proceed.

That makes sense to me, I can take a bit of time later to understand this a bit deeper, but on the surface that sounds like a good path, and the consistency between request handling sounds better than having multiple paths to doing that

lisatassone · 2024-03-05T23:54:29Z

The current code for createResponseListener assumes the request is always present in context but when bypass and passthrough functions are used with fetch, requests never get added to the context. Example usage from the docs for a proxy request.

This is interesting. So the issue is when the response listener is called by the worker on bypassed request, right? This needs an extremely careful examination, we don't want to jump to the wrong conclusion and introduce a wrong fix.

This is correct and why I wanted to draw your attention to it. It felt like a larger decision needed to be made on whether the context should always contain bypassed requests - which would require a more extensive solution.

lisatassone · 2024-03-06T00:16:18Z

However, I can see that the worker has its own logical branch for bypassing requests, and in that case it won't even send the REQUEST message to the client:

msw/src/mockServiceWorker.js

Lines 205 to 210 in b3e47f7

// Bypass requests with the explicit bypass header.

// Such requests can be issued by "ctx.fetch()".

const mswIntention = request.headers.get('x-msw-intention')

if (['bypass', 'passthrough'].includes(mswIntention)) {

return passthrough()

}

I'm trying to remember why this duplication is necessary and my hunch tells me that handleRequest() is environment-agnostic. That x-msw-intention request header handling is there to support bypassed requests in Node.js. For the browser usage, it seems that the intention was to never go to the client since a bypassed request cannot be affected by MSW by design.

This is what I understood as well and mentioned it in the original Issue tagged 👍 . Like you, I couldn't reason why that specific branch of logic was there and not in the dedicated request handlers.

While this message will contain the requestId, the request listener has never been invoked on the client so context.requests.has(requestId) will be false.

This means that bypassed requests cannot be persisted in the context while it's promised in the contract that even original responses will have the request object reference.

Yes, this is what is causing the error to occur at the moment.

I'm considering the change that all requests trigger the REQUEST event and the request listener, and then we decide whether to go into the request resolution or not, or let this handle the bypassed requests also to stay consistent across environments.

Much prefer this solution!

kettanaito · 2024-03-16T13:10:39Z

Merged a few preliminary changes to make this one easier:

Now we can move on with updating the worker script as I described above. @lisatassone, does this sound interesting to you?

kettanaito · 2024-03-16T22:46:23Z

I'm tackling this issue in #2094. Also discovered that we are missing #2093. Nice find.

kettanaito · 2024-03-17T12:57:01Z

Released: v2.2.6 🎉

This has been released in v2.2.6!

📄 Release notes
📦 npm package

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

Predictable release automation by @ossjs/release.

lisatassone · 2024-03-18T00:10:39Z

Nice one! You got through these quickly :D Thanks for this.

lisatassone added 2 commits February 23, 2024 16:51

fix: add type guard for missing context requests

2557a0f

Remove non-null assertion when retrieving a request from context, as bypassed and passthrough requests aren't added to context and need to be handled. Update response:bypass type to allow for undefined request prop. fix mswjs#2053

style: use consistent comment formatting

a162a1b

kettanaito reviewed Feb 27, 2024

View reviewed changes

cstephens-cni reviewed Mar 1, 2024

View reviewed changes

kettanaito mentioned this pull request Mar 16, 2024

fix: make worker integrity mismatch a warning #2090

Merged

kettanaito mentioned this pull request Mar 16, 2024

fix(setupWorker): emit the correct life-cycle events for bypassed requests #2094

Merged

kettanaito mentioned this pull request Mar 16, 2024

Browser: Cannot read properties of undefined (reading 'url') #2053

Closed

4 tasks

kettanaito closed this in #2094 Mar 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add guard for missing context requests #2054

fix: add guard for missing context requests #2054

lisatassone commented Feb 23, 2024 •

edited by kettanaito

Loading

kettanaito Feb 27, 2024

ymarios-int Feb 28, 2024 •

edited

Loading

cstephens-cni Mar 1, 2024

cstephens-cni Mar 1, 2024

cstephens-cni Mar 1, 2024

thepassle commented Mar 5, 2024

kettanaito commented Mar 5, 2024

kettanaito commented Mar 5, 2024 •

edited

Loading

kettanaito commented Mar 5, 2024

mattcosta7 commented Mar 5, 2024

lisatassone commented Mar 5, 2024

lisatassone commented Mar 6, 2024

kettanaito commented Mar 16, 2024

kettanaito commented Mar 16, 2024

kettanaito commented Mar 17, 2024

lisatassone commented Mar 18, 2024

fix: add guard for missing context requests #2054

fix: add guard for missing context requests #2054

Conversation

lisatassone commented Feb 23, 2024 • edited by kettanaito Loading

kettanaito Feb 27, 2024

Choose a reason for hiding this comment

ymarios-int Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

cstephens-cni Mar 1, 2024

Choose a reason for hiding this comment

cstephens-cni Mar 1, 2024

Choose a reason for hiding this comment

cstephens-cni Mar 1, 2024

Choose a reason for hiding this comment

thepassle commented Mar 5, 2024

kettanaito commented Mar 5, 2024

kettanaito commented Mar 5, 2024 • edited Loading

kettanaito commented Mar 5, 2024

Proposal

Step 0: Add a failing test for a bypassed request

Step 1: Remove the bypassed requests handling from the worker

Step 2: Ensure that handleRequest will behave correctly with the bypassed scenarios in the browser.

mattcosta7 commented Mar 5, 2024

lisatassone commented Mar 5, 2024

lisatassone commented Mar 6, 2024

kettanaito commented Mar 16, 2024

kettanaito commented Mar 16, 2024

kettanaito commented Mar 17, 2024

Released: v2.2.6 🎉

lisatassone commented Mar 18, 2024

lisatassone commented Feb 23, 2024 •

edited by kettanaito

Loading

ymarios-int Feb 28, 2024 •

edited

Loading

kettanaito commented Mar 5, 2024 •

edited

Loading

Step 2: Ensure that `handleRequest` will behave correctly with the bypassed scenarios in the browser.