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: Connection.resolveExternalWSUri should return ws or wss schema #639

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

scbrubaker02
Copy link

@scbrubaker02 scbrubaker02 commented May 18, 2024

Modifies the behavior Connection.resolveExternalWSUri so that it returns as URI with a "ws" or "wss" scheme instead of a "http" or "https" scheme as it does currently.

This avoids the need for callers of this function to correct the scheme in the results. Most importantly in the WebviewComm the scheme was always replaced with a "ws" scheme, resulting in errors like

Mixed Content: The page at 'https://example.com' was loaded over HTTPS, but attempted to connect to the insecure WebSocket endpoint 'ws://example.com'. This request has been blocked; this endpoint must be available over WSS.

in secure contexts.

No other clients of the Connection class seem to rely on the behavior of returning an "http" or "https" schema.

@scbrubaker02
Copy link
Author

scbrubaker02 commented May 29, 2024

@microsoft-github-policy-service agree company="Udacity"

Copy link
Contributor

@scbrubaker02 the command you issued was incorrect. Please try again.

Examples are:

@microsoft-github-policy-service agree

and

@microsoft-github-policy-service agree company="your company"

@andreamah
Copy link
Contributor

Can you create an issue associated with this?

@scbrubaker02
Copy link
Author

@andreamah , this addresses #645

@andreamah
Copy link
Contributor

This seems right, as it's making the behavior in

const wsURL = `${httpUri.scheme === 'https' ? 'wss' : 'ws'}://${wsUri.authority
}${wsUri.path}`;

more universal across the repo. Currently, once I manually navigate to the websocket URL (as discussed in the FAQ, it seems to update just fine for me.
image

In your case, do you do this and it still doesn't update properly?

@scbrubaker02
Copy link
Author

Actually, this change does not fix #645, which is the problem addressed in the FAQ, as pointed out by @andreamah .

The only consequence that been able to observe while running the branch in codespaces is that a "Content-Security-Policy: Upgrading insecure request" warning disappears. The websocket where behavior is affected is not the one responsible for refresh but the one that handles the page history.

Nevertheless, I do think that this is an improvement to code. It can be seen as an alternative fix for #175.

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