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.

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

Fix a number of server compatibility issues #808

Closed
wants to merge 4 commits into from
Closed

Conversation

jgosmann
Copy link
Collaborator

  • This allows IP addresses like 127.0.0.1 as websocket origin for servers listening on localhost. Actually the whole 127.0.0.0/8 is allowed because all of these addresses are reserved for loopback. This is meant to allow direct use of the IP if localhost does not resolve correctly.
  • Adds IPv6 support and allows [::1] (and equivalent addresses) accordingly as websocket origin. This is meant to allow to address the server with localhost on computers where this resolves to the IPv6 address (though with a sane configuration the IPv4 address should be tried when the IPv6 address fails).
  • Safari complains when you send LF instead of CRLF, so we're sending CRLF now.

This is all meant to address issues reported in #807. However, one issue remains: Firefox does not support using IPv4 addresses instead of localhost because it complains that accessing localStorage is not a secure operation. I won't fix this for now because it is not related to the server refactoring and I don't understand why exactly Firefox complains (Chrome and Safari are fine with it.)

@mention-bot
Copy link

@jgosmann, thanks for your PR! By analyzing the annotation information on this pull request, we identified @tcstewar, @tbekolay and @pabogdan to be potential reviewers

@Seanny123
Copy link
Collaborator

This fixes the problem in Chrome, but not Safari, where everything is only half-loaded:
screen shot 2016-07-14 at 5 55 14 pm

@jgosmann
Copy link
Collaborator Author

What URL are you using?

On July 14, 2016 8:56:00 PM EDT, Sean Aubin notifications@github.com wrote:

This fixes the problem in Chrome, but not Safari, where everything is
only half-loaded:
screen shot 2016-07-14 at 5 55 14 pm


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#808 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

@Seanny123
Copy link
Collaborator

The localhost one

@jgosmann
Copy link
Collaborator Author

Safari gives me some CSS errors, but seems to work for me.
Are there any suspicious messages in the developer console?

@Seanny123
Copy link
Collaborator

I lost access to the computer that was causing the problem, so I can't
follow up on this. Let's ignore that problem for now.

On Thu, Jul 14, 2016 at 6:10 PM, Jan Gosmann notifications@github.com
wrote:

Safari gives me some CSS errors, but seems to work for me.
Are there any suspicious messages in the developer console?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#808 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AB1HaR0phZQ2eC4GGvz5WwK1ivFgLd7Nks5qVt4XgaJpZM4JM-rt
.

@@ -102,17 +103,47 @@ class GuiRequestHandler(server.HttpWsRequestHandler):
'/favicon.ico': 'serve_favicon',
}

def get_expected_origins(self):
session = self.get_session()
def _is_loopback(self, host):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a quick docstring about what this function is doing? From what I can parse this is used to verify if the expected host was reached? This function only seems to get called in assert statements.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would say something like "Checks whether host refers to the loopback interface." Not sure if that really adds any value because to me it is what the function name already says in more words.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right. I wrote that comment when I was sleepy. I shouldn't review code when I'm sleepy.

@Seanny123
Copy link
Collaborator

LGTM

@jgosmann
Copy link
Collaborator Author

jgosmann commented Nov 3, 2017

This PR has become obsolete.

  • The problem of IP addresses as websocket origin has been solved in Token based authentication #915 as it requires a login for any sort of connection even for localhost. Thus, no special case code for that is needed.
  • IPv6 support has been added in a better way in Add dual stack IPv6 support. #864.
  • A fix for the Safari problem is also already merged into master and released as 0.3.1.

@jgosmann jgosmann closed this Nov 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants