Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make origins option more flexible and similar to later versions of socket.io #12

Open
wants to merge 3 commits into
base: 0.9.x
Choose a base branch
from
Open

Make origins option more flexible and similar to later versions of socket.io #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bf1747c
Remove FlashSocket
timdown Jan 31, 2025
d44882d
Remove HTML file transport
timdown Jan 31, 2025
0904bb6
Overhaul origin option to allow regex, function, boolean, string and …
timdown Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions History.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
0.9.19-overleaf-11 / 2025-01-24
===============================

* Overleaf: Overhaul origins option to allow a regex, function, string or boolean, or an array of any mix of these
* Overleaf: Change origins option default to disallow any origin
* Overleaf: Remove htmlfile transport
* Overleaf: Remove flashsocket transport

0.9.19-overleaf-10 / 2023-04-25
===============================

Expand Down
2 changes: 1 addition & 1 deletion Readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ Configuration in socket.io is TJ-style:
var io = require('socket.io').listen(80);

io.configure(function () {
io.set('transports', ['websocket', 'flashsocket', 'xhr-polling']);
io.set('transports', ['websocket', 'xhr-polling']);
});

io.configure('development', function () {
Expand Down
62 changes: 32 additions & 30 deletions lib/manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ exports = module.exports = Manager;

var defaultTransports = exports.defaultTransports = [
'websocket'
, 'htmlfile'
, 'xhr-polling'
, 'jsonp-polling'
];
Expand All @@ -60,7 +59,7 @@ function Manager (server, options) {
this.namespaces = {};
this.sockets = this.of('');
this.settings = {
origins: '*:*'
origins: []
, log: true
, store: new MemoryStore
, logger: new Logger
Expand All @@ -76,8 +75,6 @@ function Manager (server, options) {
, 'heartbeat interval': 25
, 'heartbeat timeout': 60
, 'polling duration': 20
, 'flash policy server': true
, 'flash policy port': 10843
, 'destroy upgrade': true
, 'destroy buffer size': 10E7
, 'browser client': true
Expand Down Expand Up @@ -873,40 +870,45 @@ Manager.prototype.handshakeData = function (data, connection) {
};
};

function isString(s) {
return typeof s === 'string' || s instanceof String;
}

// Adapted from Express's CORS module
// (https://github.com/expressjs/cors/blob/1cfb3709dec33dfa7ae95a3a554f2dd10498c7f9/lib/index.js)
Manager.prototype.isOriginAllowed = function (origin) {
var allowedOrigin = this.get('origins');
if (typeof allowedOrigin === 'function') {
return allowedOrigin(origin);
} else if (Array.isArray(allowedOrigin)) {
for (var i = 0; i < allowedOrigin.length; ++i) {
if (this.isOriginAllowed(origin, allowedOrigin[i])) {
return true;
}
}
return false;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this mean that the default empty array will block access?

Copy link
Author

@timdown timdown Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else if (isString(allowedOrigin)) {
return allowedOrigin === '*' || origin === allowedOrigin;
} else if (allowedOrigin instanceof RegExp) {
return allowedOrigin.test(origin);
} else {
return !!allowedOrigin;
}
};

/**
* Verifies the origin of a request.
*
* @api private
*/

Manager.prototype.verifyOrigin = function (request) {
var origin = request.headers.origin || request.headers.referer
, origins = this.get('origins');

if (origin === 'null') origin = '*';

if (origins.indexOf('*:*') !== -1) {
return true;
var origin = request.headers.origin || request.headers.referer;
Copy link

@Seinzu Seinzu Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. I alluded to that in the description, although maybe slightly obliquely.

var allowed = this.isOriginAllowed(origin);
if (!origin && !allowed) {
this.log.warn('origin missing from handshake, yet required by config', { headers: request.headers });
}

if (origin) {
try {
var parts = url.parse(origin);
parts.port = parts.port || 80;
var ok =
~origins.indexOf(parts.hostname + ':' + parts.port) ||
~origins.indexOf(parts.hostname + ':*') ||
~origins.indexOf('*:' + parts.port);
if (!ok) this.log.warn('illegal origin: ' + origin);
return ok;
} catch (ex) {
this.log.warn('error parsing origin');
}
}
else {
this.log.warn('origin missing from handshake, yet required by config');
}
return false;
return allowed;
};

/**
Expand Down
149 changes: 0 additions & 149 deletions lib/transports/flashsocket.js
View file
Open in desktop

This file was deleted.

83 changes: 0 additions & 83 deletions lib/transports/htmlfile.js
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions lib/transports/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@

module.exports = {
websocket: require('./websocket')
, flashsocket: require('./flashsocket')
, htmlfile: require('./htmlfile')
, 'xhr-polling': require('./xhr-polling')
, 'jsonp-polling': require('./jsonp-polling')
};
29 changes: 4 additions & 25 deletions lib/transports/websocket/hybi-07-12.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,32 +155,11 @@ WebSocket.prototype.onSocketConnect = function () {
*/

WebSocket.prototype.verifyOrigin = function (origin) {
var origins = this.manager.get('origins');

if (origin === 'null') origin = '*';

if (origins.indexOf('*:*') !== -1) {
return true;
}

if (origin) {
try {
var parts = url.parse(origin);
parts.port = parts.port || 80;
var ok =
~origins.indexOf(parts.hostname + ':' + parts.port) ||
~origins.indexOf(parts.hostname + ':*') ||
~origins.indexOf('*:' + parts.port);
if (!ok) this.log.warn('illegal origin: ' + origin);
return ok;
} catch (ex) {
this.log.warn('error parsing origin');
}
}
else {
this.log.warn('origin missing from websocket call, yet required by config');
var allowed = this.manager.isOriginAllowed(origin);
if (!origin && !allowed) {
this.log.warn('origin missing from websocket call, yet required by config', { headers: this.req.headers });
}
return false;
return allowed;
};

/**
Expand Down
Loading