Skip to content

Extend origins option to allow a function #12

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.

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

Merged
merged 8 commits into from
Apr 11, 2025
Merged

Conversation

timdown
Copy link

@timdown timdown commented Jan 24, 2025

This is to support custom origin checking in the Overleaf real-time service.

Changes:

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

This 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 origins option, probably using a regex. I've opted for a a minimal change that simply allows a function to be passed into the origins option, which receives the origin/referrer and the request object. If the origins setting is not a function, it behaves exactly as before, and the default is still to allow any origin.

This will fix https://github.com/overleaf/internal/issues/22644, in combination with some small tweaks to the check the origin in the real-time service.

We'll need to create a tag (probably 0.9.19-overleaf-11) once this is merged so that we can point the real-time dependency to the new version.

@timdown timdown requested a review from Seinzu January 24, 2025 13:09
@timdown timdown force-pushed the td-origins-setting branch 5 times, most recently from 1d6488a to 1c0aa2f Compare January 31, 2025 17:34
@timdown timdown force-pushed the td-origins-setting branch from 1c0aa2f to 0904bb6 Compare January 31, 2025 17:43
@timdown timdown requested a review from Seinzu February 7, 2025 10:00
@timdown timdown changed the title Add support for supplying a function for origins to allow more sensible origin verification Make origins option more flexible and similar to later versions of socket.io Feb 7, 2025
Copy link

@Seinzu Seinzu left a comment

Choose a reason for hiding this comment

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

I think we need to fix the default case before we can go ahead. The rest of it looks good.

@Seinzu Seinzu assigned timdown and unassigned Seinzu Feb 7, 2025
@Seinzu
Copy link

Seinzu commented Feb 7, 2025

I'm not a big fan of the way the default setup works but it doesn't seem to be blocking

@timdown timdown force-pushed the td-origins-setting branch from 8f53204 to fad9007 Compare March 28, 2025 08:56
@timdown timdown force-pushed the td-origins-setting branch from 2e1f680 to 322d867 Compare March 28, 2025 17:51
@timdown timdown requested a review from Seinzu March 29, 2025 10:48
@timdown timdown removed their assignment Apr 2, 2025
@timdown
Copy link
Author

timdown commented Apr 2, 2025

I've updated this by changing the origins option back to how it was (including defaulting to letting anything through) and allowing it to be function, so it's just a simple escape hatch. This seems the simplest possible change. I have kept the refactor that removes duplication of the origin check code in the various websocket implementations, which seems safe to me.

Copy link

@Seinzu Seinzu left a comment

Choose a reason for hiding this comment

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

I haven't tested this version (but have tested similar implementations). I think this is the lowest risk thing we can do for now.

@Seinzu Seinzu assigned timdown and unassigned Seinzu Apr 7, 2025
@timdown timdown changed the title Make origins option more flexible and similar to later versions of socket.io Extend origins option to allow a function Apr 11, 2025
@timdown timdown changed the title Extend origins option to allow a function Extend origins option to allow a function Apr 11, 2025
@timdown timdown merged commit 88255b9 into 0.9.x Apr 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants