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

Prefix websocket path with server path #1058

Closed
wants to merge 4 commits into from

Conversation

ryanlovett
Copy link

If noVNC is not served at the root path, prefix the websocket path with the server path. This makes the path to the websocket consistent with the path to all the other web assets. This is convenient so that users do not need to manually change the path setting, though they still can override it.

For example if the websockify server is at /some/sub/dir, with this patch noVNC will connect to /some/sub/dir/websockify rather than /websockify.

If noVNC is not served at the root path, prefix the websocket path with the server path. This makes the path to the websocket consistent with the path to all the other web assets. This is convenient so that users do not need to manually change the path setting, though they still can override it.

For example if the websockify server is at /some/sub/dir, with this patch noVNC will connect to /some/sub/dir/websockify rather than /websockify.
@CendioOssman
Copy link
Member

LGTM. Any problems you guys see, @DirectXMan12 @samhed?

Copy link
Member

@samhed samhed left a comment

Choose a reason for hiding this comment

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

Looks good, thank you @ryanlovett

Copy link
Member

@samhed samhed left a comment

Choose a reason for hiding this comment

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

Hm, I'd like to retract that approval.

If I use the launch.sh script bundled with noVNC (uses websockify) I get a window.location.pathname that is "/vnc.html". This will, with your patch give a default path of "vnc.html/websockify" which is wrong.

Also, we should change the default from 'websockify' to 'websocket' tbh.. We don't want noVNC to be tightly coupled with Websockify.

@ryanlovett
Copy link
Author

Thanks @samhed. I'll update the PR to take the landing html page into account.

I can make a separate PR to change the websocket filename.

@DirectXMan12
Copy link
Member

If we really wanted to do this, we'd have to implement the equivalent of dirname for URLs. I'm not sure how I feel about that.

@CendioOssman
Copy link
Member

Is that so scary though? A simple reverse search for / should be sufficient?

@ryanlovett
Copy link
Author

@samhed I've updated the patch so that it properly handles the landing page. From a code comment:

        /*
         * If novnc is hosted at PATHNAME, the default ws path should be at WS:
         *  PATHNAME                  WS
         *  /                         /websockify
         *  /foo/myvnc                /foo/myvnc/websockify
         *  /foo/myvnc/
         *  /foo/myvnc/vnc.html
         */

This doesn't attempt to rename the socket itself (as you suggested) which I'll do in a separate PR.

* If novnc is hosted at PATHNAME, the default ws path should be at WS:
* PATHNAME WS
* / /websockify
* /foo/myvnc /foo/myvnc/websockify
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want this behaviour? I'd rather avoid getting to deep in to the mess of determining if this is a directory or a file.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not getting much activity from the other maintainers, so I'll make the call then. :)

@ryanlovett, please remove this part and it should be okay to merge.

Copy link
Author

Choose a reason for hiding this comment

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

Oops, thanks for the ping! Can you clarify which part should be removed?

Copy link
Member

Choose a reason for hiding this comment

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

Anything that tries to guess if this is a folder or a file. Just assume everything after the last / should be removed. So /foo/myvnc => /foo/websockify and /bar/vnc.html => /bar/websockify.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, thanks. Should I treat /foo/myvnc/ the same as /foo/myvnc then? (Pedantically) that would go against "assume everything after the last / should be removed" since in the trailing slash case it would remove nothing. But I think it is more in-line with "avoid ... determining if this is a directory or a file".

@CendioOssman
Copy link
Member

Ping @ryanlovett

@CendioOssman
Copy link
Member

Crap... maybe this won't work after all.

If we have the page https://www.example.com/folder/index.html, then it can be reached via:

  • https://www.example.com/folder/index.html
  • https://www.example.com/folder/
  • https://www.example.com/folder

All three should result in the same WebSocket URL or we'll have lots of confused users. :/

Hard coding the page name only works as long as people don't change it (which they do). So that's not really a viable solution.

Ideas? Script relative perhaps? If that is possible to determine.

@DirectXMan12
Copy link
Member

I think maybe we should just consider making it easily configurable (i.e. people can include an extra script in the page, or change the base page, or what have you) instead of trying to autodetect. I can see lots of scenarios where the autodetect doesn't get it right due to differing URL layouts (e.g. putting novnc static content under a /static subpath, for instance).

If it's just easy to customize (i.e. setting a script block in the main page, or something), I think that'd probably solve both problems.

@CendioOssman
Copy link
Member

Are we able to progress on this? Or should we close the PR?

@samhed
Copy link
Member

samhed commented Sep 28, 2018

I'm leaning towards a close.

@ryanlovett
Copy link
Author

Yeah, I'll close. I think it'd be easier to create a separate frontend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants