Allow origin function to consider empty origin #13
Conversation
There was a problem hiding this comment.
This looks good and works in my testing. We'll also need a change in the real-time origin check because otherwise we'll get an error in URL.parse(origin).origin when origin is undefined. The simplest thing might be an explicit check and an early return, right at the start:
if (!origin) {
// There is no Origin or Referer header. We're going to pass this anyway
// for now but log it
logger.warn({ req }, 'No Origin or Referer header')
return true
}
lib/manager.js
Outdated
| ~origins.indexOf('*:' + parts.port); | ||
| } | ||
| else { | ||
| this.log.warn('origin missing from handshake, yet required by config'); |
There was a problem hiding this comment.
"handshake" is not always accurate because the websocket implementations use "websocket call" instead, but that's a minor issue.
There was a problem hiding this comment.
I didn't change the original message (was line 904) which I had assumed was in the library already but maybe you added it?
There was a problem hiding this comment.
It's the websocket ones that I was thinking of. They had the same origin check with a slightly different message, and I changed their origin check to use the one in Manager but kept the message (example).
There was a problem hiding this comment.
I've switched which message I considered redundant so that the one in the manager function is now removed and the others have been reinstated. Is that what you wanted?
There was a problem hiding this comment.
I just wanted to point out that the websocket message had changed, but I was happy to go with it. I do prefer the new version though 😄
package.json
Outdated
| { | ||
| "name": "socket.io" | ||
| , "version": "0.9.19-overleaf-11" | ||
| , "version": "0.9.20-overleaf-12" |
There was a problem hiding this comment.
I think the naming convention for our releases is 0.9.19-overleaf-x (i.e. the 19 doesn't get incremented).
|
Here's my version of the real-time code :) 
Pretty similar (the debug log was just for local testing and I don't think the header check is really necessary - it can be assumed). |
Seinzu
force-pushed
the
ar-allow-origin-function-to-consider-empty-origin
branch
from
6c0e041 to
0a9962f
Compare
Great. Sorry to suggest code, which I know can be annoying. I just had some code that I added after getting an error while testing. |
Seinzu
force-pushed
the
ar-allow-origin-function-to-consider-empty-origin
branch
from
0a9962f to
b059656
Compare
No worries! I just thought it was funny that they were pretty similar. |
The kind of change this PR does introduce
Current behaviour
Currently when an empty
originis passed and norefererheader is provided then the request is automatically declined.New behaviour
In the situation described above and a function has been provided in the configuration (as
origins) then the normalised origin will be passed through to the provided function and a decision can be made in the function. Behaviour remains the same in the case that a string is provided.Other information (e.g. related issues)
Browsers can choose to not send a
refererheader so it is possible that in a same-origin request we will get neither.While websockets generally do not follow the same-origin rules, socket.io has an HTTP request to get an upgrade to wss that will fall under the same-origin rules.