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.

Sign up for GitHub

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

Jump to bottom

SOCKS5 support #109

Closed
wants to merge 1 commit into from
Closed

SOCKS5 support #109

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Mar 30, 2014

This was pretty simple and it works well (for cURL)

staabm
staabm reviewed Mar 30, 2014
View reviewed changes
library/Requests/Proxy/SOCKS5.php
*
* @since 1.6
* @throws Requests_Exception On incorrect number of arguments (`authbasicbadargs`)
* @param array|null $args Array of user and password. Must have exactly two elements
Copy link
Contributor

Choose a reason for hiding this comment

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

"MUst have exactly 2 elements" is not correct

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.09%) when pulling e316355 on pwnsdx:master into 81a917f on rmccue:master.

staabm
staabm reviewed Mar 30, 2014
View reviewed changes
library/Requests/Proxy/SOCKS5.php
* @param Requests_Hooks $hooks Hook system
*/
public function register(Requests_Hooks &$hooks) {
$hooks->register('curl.before_send', array(&$this, 'curl_before_send'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why oass by ref, this should trigger a warning?

Copy link
Author

Choose a reason for hiding this comment

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

Don't know, I just copy/past the original HTTP proxy library (in Requests).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see

Copy link
Author

Choose a reason for hiding this comment

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

Maybe merging HTTP, SOCKS4 and SOCKS5 support in one file may be better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@staabm Won't cause a warning, but you're correct that it's not needed, just a bad habit of mine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, separate files for different proxies is better. Should be one-class-per-file, and HTTP/SOCKS5 should definitely be separate classes.

@ghost ghost mentioned this pull request Mar 30, 2014
Open
rmccue
rmccue reviewed Mar 31, 2014
View reviewed changes
library/Requests.php
@@ -500,8 +500,18 @@ protected static function set_defaults(&$url, &$headers, &$data, &$type, &$optio
$options['auth']->register($options['hooks']);
}

if (!empty($options['proxy'])) {
$options['proxy'] = new Requests_Proxy_HTTP($options['proxy']);
if (!empty($options['proxy']['authentication']) AND
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be && instead of AND. The latter has a different precedence that can be confusing to work with.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, habit... ;)

@rmccue
Copy link
Collaborator

rmccue commented Mar 31, 2014

Thanks for this! :)

Two concerns I have:

  1. I personally have no way of testing this, as I don't have a SOCKS proxy. I can't confirm that it works without testing it myself.
  2. This introduces more untested code into Requests, which I'd really like to avoid. I'm not sure if there's any simple, standalone proxies though: see Add proxy testing #75 for work on HTTP proxy testing, we might be able to reuse it?

If we add a test SOCKS proxy to the tests, that solves both issues for me, but I'm not sure the easiest way to do that.

@ghost
Copy link
Author

ghost commented Mar 31, 2014

I tested the HTTP proxy support days ago, it don't works. Got an error on CONNECT request or something like that. But I can confirm that the cURL SOCKS Support I made quickly works well.

@rmccue
Copy link
Collaborator

rmccue commented Mar 31, 2014

I tested the HTTP proxy support days ago, it don't works. Got an error on CONNECT request or something like that.

Hmm, interesting. What were you using to test?

@ozh Do you have a test HTTP proxy setup I can use to check this?

But I can confirm that the cURL SOCKS Support I made quickly works well.

I'd like to be able to independently confirm that. :)

I'll check out if mitmproxy can do it; we might be able to work this into #75.

@ghost
Copy link
Author

ghost commented Mar 31, 2014

Comment removed by mistake. S***.

So, in short, download TorBrowser Bundle, run it and then add this in the Requests options:

'proxy' =>
            'type' => 'SOCKS5',
            'authentication' => [
                '127.0.0.1:9150'
            ],
],

You should be able to try.

@ghost ghost closed this Apr 1, 2019
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@coveralls @rmccue @staabm