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

support for localhost/127.0.0.1 proxy bypass #2355

Merged
merged 1 commit into from Jun 6, 2015

Conversation

Projects
None yet
3 participants
@elerch
Copy link
Contributor

elerch commented May 11, 2015

This PR addresses #1565 and #2130. It is currently using hardcoded values for localhost (localhost and 127.0.0.1) for bypass. I started wiring this into the settings system but was getting seg faults so I'm clearly doing something wrong there. I'll create a new PR for customization of the bypass values through the settings once I get that worked out.

This was originally worked off rev 7f74d89.

@elerch

This comment has been minimized.

Copy link
Contributor Author

elerch commented May 13, 2015

I've added two commits for the settings. The issue I had seems to be build related - performing a clean build removed the segfault.

The first settings commit adds settings, seeding the list with "localhost" and "127.0.0.1". The second reverts that change but instead detects an empty list and uses "localhost" and "127.0.0.1". This allows you to use a command line flag like --proxy-bypass none, which is technically a host but shouldn't match anything and will therefore force all traffic through the proxy. However, if you completely omit --proxy-bypass, you'll get behavior you probably expect (use the proxy except for localhost/127.0.0.1).

I considered the second change slightly controversial so I've not squashed these commits.

@@ -107,6 +107,20 @@ QNetworkReply * MyNetworkAccessManager::createRequest(Operation op, const QNetwo
return QNetworkAccessManager::createRequest(op, r3, outgoingData);
}

MyNetworkProxyFactory::MyNetworkProxyFactory (QNetworkProxy proxy, QList<QString> bph, ResourceObject & res):

This comment has been minimized.

@ashkulz

ashkulz May 17, 2015

Member

Why do you need the ResourceObject here? It isn't used, so should ideally be removed.

This comment has been minimized.

@elerch

elerch Jun 3, 2015

Author Contributor

Just an oversight - I was originally using it, then removed it but missed this line. Thanks!

QList<QNetworkProxy> MyNetworkProxyFactory::queryProxy (const QNetworkProxyQuery & query) {
QString host = query.url().host();
foreach (const QString & bypassHost, bypassHosts) {
if (host == bypassHost) {

This comment has been minimized.

@ashkulz

ashkulz May 17, 2015

Member

Should ideally use host.compare(bypassHost, Qt::CaseInsensitive) as then google.com and GOOGLE.COM will match -- DNS is normally case-insensitive.

@ashkulz

This comment has been minimized.

Copy link
Member

ashkulz commented May 17, 2015

Also, I don't think the approach taken is ok (bypass proxy for localhost by default). If someone wants that, it is better to opt in via --proxy-bypass localhost instead of having to opt-out via --proxy-bypass none (which will bypass the proxy for a hostname of none). Also, I think using command-line option of --bypass-proxy-for would be better than --proxy-bypass.

Thanks for the contribution, though!

@ashkulz

This comment has been minimized.

Copy link
Member

ashkulz commented May 17, 2015

Also, please squash the commits and rebase it against master and add an entry in both AUTHORS and CHANGELOG.md mentioning the issue: put 1565 in the changelog but the commit message should refer to both issues, see e02ff7e as an example.

@ashkulz

This comment has been minimized.

Copy link
Member

ashkulz commented May 28, 2015

@elerch: any update?

@elerch

This comment has been minimized.

Copy link
Contributor Author

elerch commented Jun 3, 2015

Sorry - got pulled into some fires at the office. Things are dying down so I just got back to this and should be able to make the changes tomorrow.

@elerch elerch force-pushed the elerch:master branch 4 times, most recently from 8078b0b to 8fcd774 Jun 3, 2015

@elerch

This comment has been minimized.

Copy link
Contributor Author

elerch commented Jun 3, 2015

I squashed the commit, made the changes you suggested and rebased on the latest commits. The changes were retested after a build but with Qt upgraded it's probably worth a clean build, so I'm doing that now. This is probably ready to go but I'll do final testing once the build is complete. Let me know if there's any other changes you think I need to make. Thanks!

@elerch

This comment has been minimized.

Copy link
Contributor Author

elerch commented Jun 4, 2015

FYI - Finished re-testing on a clean build and all is well, so this is ready as far as I'm concerned. Let me know if you want any other changes.

@ashkulz

This comment has been minimized.

Copy link
Member

ashkulz commented Jun 4, 2015

Looks good, once you make the above changes I'll merge it ASAP.

@ashkulz ashkulz added this to the 0.12.3 milestone Jun 4, 2015

@ashkulz

This comment has been minimized.

Copy link
Member

ashkulz commented Jun 4, 2015

Also, would prefer if you move up your name so that it is in the list of AUTHORS who have an email address.

@elerch elerch force-pushed the elerch:master branch 2 times, most recently from 91f49d5 to 1a326ff Jun 4, 2015

@elerch

This comment has been minimized.

Copy link
Contributor Author

elerch commented Jun 4, 2015

Your comments all made sense. I made the corresponding changes and retested.

@ashkulz

This comment has been minimized.

Copy link
Member

ashkulz commented Jun 5, 2015

Sorry, a few more nitpicks ... thought you'd remove the unused variable. Also, I think that the commit message should be support bypassing the proxy for specific hosts (i.e. no repeated for)

@elerch elerch force-pushed the elerch:master branch from 1a326ff to 5328973 Jun 5, 2015

@elerch

This comment has been minimized.

Copy link
Contributor Author

elerch commented Jun 5, 2015

no worries - removed the unused variable and changed the message

support bypassing the proxy for specific hosts
This adds the command line option --bypass-proxy-for which will
bypass the proxy when specific hosts are referenced. It is most
useful for bypassing localhost/127.0.0.1 references
(e.g. --bypass-proxy-for localhost)

This fixes #1565 and #2130

@elerch elerch force-pushed the elerch:master branch from 5328973 to 7ffec76 Jun 5, 2015

@ashkulz ashkulz merged commit 7ffec76 into wkhtmltopdf:master Jun 6, 2015

@ashkulz ashkulz added the Merged label Jun 6, 2015

@ashkulz

This comment has been minimized.

Copy link
Member

ashkulz commented Jun 6, 2015

Thank you for the contribution, and appreciate your patience during the review process.

@monken

This comment has been minimized.

Copy link

monken commented Jun 30, 2015

It would be great if you could honor the no_proxy environmental variable which exists exactly for this purpose and is widely used.

@ashkulz

This comment has been minimized.

Copy link
Member

ashkulz commented Jun 30, 2015

I'd appreciate a PR which implements this.

@ashkulz

This comment has been minimized.

Copy link
Member

ashkulz commented Jan 21, 2016

0.12.3 has been released, which should contain the fix for this issue. Please report back if it is not solved with the above version.

@ashkulz ashkulz referenced this pull request Nov 26, 2016

Closed

Problem with no proxy #3216

joshpme added a commit to joshpme/snappy that referenced this pull request Feb 27, 2018

NiR- added a commit to NiR-/snappy that referenced this pull request Dec 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.