Make origins option more flexible and similar to later versions of socket.io
#12
Conversation
timdown
force-pushed
the
td-origins-setting
branch
5 times, most recently
from
1d6488a to
1c0aa2f
Compare
…array and remove original implementation
timdown
force-pushed
the
td-origins-setting
branch
from
1c0aa2f to
0904bb6
Compare
timdown
changed the title
origins to allow more sensible origin verificationorigins option more flexible and similar to later versions of socket.io
| return true; | ||
| } | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Won't this mean that the default empty array will block access?
There was a problem hiding this comment.
Yes, this is similar to what recent versions of socket.io do: https://socket.io/docs/v4/handling-cors/#configuration. Do you think it's a problem?
There was a problem hiding this comment.
I think that by default CORS is disabled in the current version of socket.io so a request from overleaf.com -> overleaf.com would succeed with a default configuration (see this example of same origin with no CORS). This code will fail regardless of origin (even same origin requests will fail because it is impossible for anything to pass the default setup).
I think the original report would be addressed by just not sending the Access-Control-Allow-Origin header and disallowing requests with an explicit Origin but I think that websockets don't have to obey Same Site rules so we'd be potentially vulnerable and might well get another report for the upgraded endpoint.
It's possible to work around this API design in real-time by passing a wildcard rule if we don't configure anything (otherwise we will have a breaking change in CE and Server Pro) but the API feels weird to me to be unable to serve requests by default.
There was a problem hiding this comment.
I think that by default CORS is disabled in the current version of socket.io so a request from overleaf.com -> overleaf.com would succeed with a default configuration (see this example of same origin with no CORS). This code will fail regardless of origin (even same origin requests will fail because it is impossible for anything to pass the default setup).
Oh yes, that's a good point. I already feel as though I've changed too much code here. I'll have a little think.
|
|
||
| if (origins.indexOf('*:*') !== -1) { | ||
| return true; | ||
| var origin = request.headers.origin || request.headers.referer; |
There was a problem hiding this comment.
The original code did a bit of normalisation on the origin to fix differences between an origin and a referer. We can account for this by using a function in real-time so it doesn't need to change - just noting it as a difference.
There was a problem hiding this comment.
Indeed. I alluded to that in the description, although maybe slightly obliquely.
|
I'm not a big fan of the way the default setup works but it doesn't seem to be blocking |
This is to support custom origin checking in the Overleaf real-time service.
Changes:
originsoption to allow a regex, function, string or boolean, or an array of any mix of theseoriginsoption default to disallow any originhtmlfiletransportflashsockettransportThis still falls back to checking the referrer header when origin is missing. This is a questionable practice and modern versions of socket.io don't do this. The referrer header also has the path, unlike the origin, which means we have to handle that in the
originsoption, probably using a regex. I couldn't see an easy way round it though because our pre-flight requests have no origin header, and I'm guessing we'd need to update our fork ofsocket.io-clientto fix that, which seemed too risky to me.