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

URLs are not passed to shExpMatch #58

Closed
camh- opened this issue Oct 5, 2020 · 2 comments · Fixed by #72
Closed

URLs are not passed to shExpMatch #58

camh- opened this issue Oct 5, 2020 · 2 comments · Fixed by #72

Comments

@camh-
Copy link
Collaborator

camh- commented Oct 5, 2020

A CONNECT verb used when proxying usually contains just a hostname:port, not a full URL - i.e. it does not contain a scheme.

However, certain code in alpaca expects this to be a URL - in particular, it is passed to shExpMatch as a URL but without the scheme, so any matches against a pattern with a scheme will fail. It seems that chrome does pass a URL here, from anecdotal evidence.

I think we need to add the https scheme if it is missing. I'm not sure it makes sense for any scheme other than https to be used for the tunnel.

@samuong
Copy link
Owner

samuong commented Oct 5, 2020

Interesting, this would definitely break PAC files which call shExpMatch with a glob starting with http*://, for example.

I agree that Alpaca should add a scheme if the URL does not already contain one - I guess this would happen in either findProxyForRequest() or FindProxyForURL().

Makes sense to go with https as the scheme in all cases, at least initially. But I do wonder whether there are any cases where CONNECT is used for non-TLS tunnels.

@samuong
Copy link
Owner

samuong commented Oct 14, 2020

Btw, in addition to adding a scheme, does Alpaca need to do anything with the port? E.g. if we get a request like CONNECT example.com:443 HTTP/1.1 - should the PAC function get a url of https://example.com:443 or just https://example.com?

I'm not sure what PAC scripts out in the wild expect, or what browsers like Chrome do...

samuong added a commit that referenced this issue Nov 14, 2021
Also change FindProxyForURL() to pass by value rather than by reference.
This function modifies its copy of the URL, so we'll make sure to do
that on a copy rather than on the original.

Fixes #58
samuong added a commit that referenced this issue Nov 14, 2021
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
samuong added a commit that referenced this issue Nov 14, 2021
)

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
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 a pull request may close this issue.

2 participants