drupal_http_request parses the incoming url and properly determines the username/path but then fails to actually support it.

Attached patch fixes this issue.

Steps to repeat:
Use drupal_http_request on a URL that requires authentication
use http://user:pass@example.com/end/point.php as the $url in your drupal_http_request

Expected results:
Works

Actual results:
Fails with various confusing error messages

CommentFileSizeAuthor
#6 http_request_basic_auth-182410-6.patch845 bytesgreggles
#4 http_request_basic_auth-182410-4.patch820 byteschx
#3 http_request_basic_auth_1.patch819 bytesgreggles
#1 http_request_basic_auth_0.patch806 bytesgreggles
http_request_basic_auth.patch726 bytesgreggles
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
FileSize
http_request_basic_auth_0.patch806 bytes

Slightly better version. Adds a comment to explain what this is and also supports the fact that authentication can work with just http://user@example.com/

Thanks to chx.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Needs work

No. This is a header and must go to the headers, I told you on IRC to use += so if someone specified an Auth header then that's honored.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Status: Needs work » Needs review
FileSize
http_request_basic_auth_1.patch819 bytes

Right - that makes sense to me (now) ;)

So, I did a double take while coding this and chx in reviewing it (again) that when setting the $defaults the header prefix is both the key and the beginning of the value. Look at all the other $defaults to see that this is the case.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community
FileSize
http_request_basic_auth-182410-4.patch820 bytes

Some background: greggles was using an xmlrpc endpoint which has basic auth. I told him to patch this function instead of _xmlrpc. So, after suggesting the patch in the first place and pushing it through the rounds, I actually made a change -- added a space where it was missing :)

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Fixed

Look right, thanks, committed.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Version: 6.x-dev » 5.x-dev
Status: Fixed » Patch (to be ported)
FileSize
http_request_basic_auth-182410-6.patch845 bytes

Thanks, chx and Gábor.

Rerolled against 5.

chx’s picture

chx CreditAttribution: chx commented
Status: Patch (to be ported) » Reviewed & tested by the community

You already proted it, so...

DrupalTestbedBot tested greggles's patch (http://drupal.org/files/issues/http_request_basic_auth_0.patch), the patch passed. For more information visit http://testing.drupal.org/node/112

DrupalTestbedBot tested greggles's patch (http://drupal.org/files/issues/http_request_basic_auth_1.patch), the patch passed. For more information visit http://testing.drupal.org/node/116

DrupalTestbedBot tested chx's patch (http://drupal.org/files/issues/http_request_basic_auth-182410-4.patch), the patch failed. For more information visit http://testing.drupal.org/node/121

DrupalTestbedBot tested greggles's patch (http://drupal.org/files/issues/http_request_basic_auth-182410-6.patch), the patch passed. For more information visit http://testing.drupal.org/node/126

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Category: bug » feature
Status: Reviewed & tested by the community » Closed (won't fix)

Looks like a feature to me.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Category: feature » bug
Status: Closed (won't fix) » Needs review

Actually, it looks like a bug to me. Let me detail this stance:

  • Basic Auth has been present since HTTP/1.0 (sect. 11.1), more than 10 years ago
  • drupal_http_request (dhr) is described in the API ref as Perform an HTTP request. This is a flexible and powerful HTTP client implementation. Correctly handles GET, POST, PUT or any other HTTP requests. Handles redirects.
  • dhr discards user/password parameters when they are present in the URL, so it does not perform the HTTP request as passed to it by the calling program in that case, in spite of what the doc specifies
  • when discarding these parameters, dhr does not throw an error to warn the caller that the HTTP request was not performed exactly as it had been requested

So, all in all, I feel I have to conclude it is indeed a bug, which can be fixed in either of two ways: by documenting the fact that no Auth is supported and rejecting the related parameters when present or by accepting them and processing them as they should be. Either is correct, and the latter is vastly more useful.

It so happens that chx' fix implements the latter nicely : the same code is already present in D6, and the fix doesn't impact any code relying on the current behaviour of dhr, so there is no backwards compatibility issue.

For all these reasons, I'm reopening the issue for consideration towards inclusion the next D5.x minor release.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Needs review » Fixed

Committed to 5.x.

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.