Avoid calling allowCrossDomain twice per request
#5682
Conversation
`api.use('/', middleware, ...)` will end up calling `middleware` for _every_ request, even if no routers in the `...` part matches.
This is because passing a router to express is just like passing any other route handler. The only thing that happens when it doesn't match a route is that it calls `next`, but by that point, the middleware has already run.
The changes in the PR avoids adding the middleware twice for every route except file upload routes. Which will make express not call `allowCrossDomain` twice for every incoming request.
Codecov Report
@@ Coverage Diff @@
## master #5682 +/- ##
=========================================
+ Coverage 94.19% 94.2% +0.01%
=========================================
Files 129 129
Lines 9248 9248
=========================================
+ Hits 8711 8712 +1
+ Misses 537 536 -1
Continue to review full report at Codecov.
|
|
Makes sense for me. I am only worried about the public api (https://github.com/parse-community/parse-server/blob/master/src/ParseServer.js#L169) that is currently also accepting cross domain headers. Right? |
I'm not sure I understand exactly what you mean here 🤔 Both before and after this change the The new code should be functionally equivalent to the old one. A good way to think about it is in steps: app.use('/', middleware, router)
// is the same as
app.use('/', middleware)
app.use('/', router)
// is the same as
app.use(middleware)
app.use('/', router)Thus the very first line using parse-server/src/ParseServer.js Lines 155 to 161 in 466a049 as this is the same as: api.use(middlewares.allowCrossDomain)
api.use('/', new FilesRouter().expressRouter({ maxUploadSize: maxUploadSize })) |
|
Also, just saw that the parse-server/src/Routers/FilesRouter.js Lines 20 to 39 in 6ffc413 So I tracked down the introduction of the extra call to this commit: At that point, the aforementioned cors thing were in place as well, but they are only covering To be honest, I'm not really sure what the goal here is 😄 If we want CORS headers on every single call, we should add a single If there are only some routes that should have CORS, this call should be moved into the specific file routes instead. As the code works right now, CORS is applied to every single request. I'd be happy to submit a patch for either way... |
|
|
I think req.xhr will work without cors if the script is running on a page in the same domain. But anyway the public api is running with cors for a long while and changing this will probably break some apps. So let's keep the PR how it is and merge. |
|
Nice, I'll submit a follow up PR to remove some more redundant invocations as well 👍 |
) `api.use('/', middleware, ...)` will end up calling `middleware` for _every_ request, even if no routers in the `...` part matches. This is because passing a router to express is just like passing any other route handler. The only thing that happens when it doesn't match a route is that it calls `next`, but by that point, the middleware has already run. The changes in the PR avoids adding the middleware twice for every route except file upload routes. Which will make express not call `allowCrossDomain` twice for every incoming request.
api.use('/', middleware, ...)will end up callingmiddlewarefor every request, even if no routers in the...part matches.This is because passing a router to express is just like passing any other route handler. The only thing that happens when it doesn't match a route is that it calls
next, but by that point, the middleware has already run.The changes in the PR avoids adding the middleware twice for every route except file upload routes. Which will make express not call
allowCrossDomaintwice for every incoming request.Happy to explain more if there's is something unclear, just happened to stumble upon this when debugging a CORS issue...