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

Pass complete URLs to PAC script, assuming https if scheme is empty #72

Merged
merged 1 commit into from
Nov 14, 2021

Conversation

samuong
Copy link
Owner

@samuong samuong commented Nov 14, 2021

Also change FindProxyForURL() to pass by value rather than by reference. This
function modifies the URL, so make sure to do that on a copy rather than on the
original.

Fixes #58

Also change FindProxyForURL() to pass by value rather than by reference.
This function modifies the URL, so do that on a copy rather than the
original.

Fixes #58
Copy link
Collaborator

@camh- camh- left a comment

Choose a reason for hiding this comment

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

One suggestion, but otherwise LGTM

if u.Scheme == "" {
// When a net/http Server parses a CONNECT request, the URL will
// have no Scheme. In that case, assume the scheme is "https".
u.Scheme = "https"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be a little smarter by looking at the port if specified. All the CONNECT examples specify it, but a quick reading of the spec suggests that maybe port is not mandatory. But if port is 80, you could probably safely assume http and not https.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I was considering that, but decided against it because I didn't want to have to maintain a mapping. It would be nice if we could use something like this services map, which is exposed in net.LookupPort(), although that mapping is going in the opposite direction to what we'd need here.

Also iiuc port 80 could mean ws:// too, and maybe that's more likely to be the case when someone does a CONNECT request to port 80?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@camh- camh- Nov 14, 2021

Choose a reason for hiding this comment

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

There is /etc/services too - presumably any of them could have a scheme. I guess there is a registry - I thought it was a free-for-all.

@samuong samuong merged commit e522210 into master Nov 14, 2021
@samuong samuong deleted the noscheme branch November 14, 2021 06:29
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.

URLs are not passed to shExpMatch
2 participants