Fix ReactServerAgent CORS preflight #838
Conversation
|
|
||
| req.send(postParams); | ||
| break; |
There was a problem hiding this comment.
What about the other http methods (CONNECT, OPTIONS, TRACE)? I feel like we should at least have a default case to emit a warning or error or something.
There was a problem hiding this comment.
Fair point. We don't have functions for those methods, so I didn't implement a check for those. I can make this more generic.
…t with POST parameters and an info message when sending a POST/PATCH/PUT request with query parameters. Only call SuperAgent's .send() function if there is data, otherwise this will trick SuperAgent into thinking it should be a POST request.
…scenarios (posting with and without data).
|
I'd like to take a closer look at this one. I'll try to get to it this morning. |
| @@ -233,7 +228,35 @@ function buildSuperagentRequest() { | |||
| } | |||
| } | |||
|
|
|||
| req.send(postParams); | |||
| let hasQueryParams = (this._queryParams.length > 0), | |||
| hasPostParams = (Object.keys(this._postParams).length > 0); | |||
| } | ||
|
|
||
| req.send(postParams); | ||
| } |
There was a problem hiding this comment.
I think there might be an edge case here. It seems like one should be able to POST an empty JSON object to an endpoint, and I think we can't anymore. (Though to be fair, I think that may have been the default before, which probably also doesn't make sense?)
I suspect that there have always been lots of edge cases here that we just haven't run into yet. (I just realized that superagent accepts strings and objects for send, and we only accept objects.
There was a problem hiding this comment.
Correct...any thoughts on the right way to handle this? Here's the basic situation: on GET/HEAD requests, if we set type and/or send to any value (even empty values), the GET/HEAD requests will send a CORS preflight.
There was a problem hiding this comment.
@roblg I handled the edge case were someone wants to send an empty JSON object now. And there's a test for it!
|
Gah. I clicked 'approve' went I meant to just leave the review comments. |
roblg
dismissed
their stale review
Accidentally clicked approve. Meant to comment...
| if (hasQueryParams) { | ||
| logger.info(`Attempting a ${this._method} request with query data (using SuperAgent's .query() function). This might not be what you want.`); | ||
| } | ||
| break; |
There was a problem hiding this comment.
should we have a warning or error or something for the default case?
There was a problem hiding this comment.
I don't think so. If someone is using a method other than GET, HEAD, POST, PUT, or PATCH and they're setting query/post parameters, then they probably know what they are doing and aren't worried about a CORS preflight request being emitted.
| return merge({}, this._postParams); | ||
| return merge({}, this._postParams || {}); | ||
| } | ||
| if (postParams !== null) { |
There was a problem hiding this comment.
I always get a little skittish when undefined and null are treated differently; is there some way we can consolidate them?
There was a problem hiding this comment.
I 100% agree. However, it seems that the current implementation of all of these functions does something different if you call it like .send() (returns the currently set post parameters) vs .send({}) (which saves the post parameters and returns this). In my opinion, that is a bad design for all of those functions...trying to combine setters + getters. That's outside the scope of this PR though.
There was a problem hiding this comment.
FWIW, looking at the corresponding SuperAgent functions, they always return this. They are not trying to combine a setter with a getter function.
There was a problem hiding this comment.
FWIW, I have no recollection of why I thought it was a good idea to combine set/get. I think I was probably thinking of properties, or looking at something else. I doubt they need to be getters as well, but I haven't poked around to see what would need to be updated.
Fixes #619. ReactServerAgent shouldn't create a CORS preflight request when executing simple GET queries of an external API. This patch fixes that problem as well as an unidentified problem where a HEAD request generate a CORS preflight request as well, because it was attempting to pass data to the request using SuperAgent's
.send()method instead of.query().