If an http request returns a redirect and specifies "." as the location, drupal_http_request will call itself with "." and report "missing schema". Most notably, Buildbot does this when you try and force a build. The solution is fairly simple but I haven't really tested it for any extensive period of time.

--- common.inc.old	2009-09-26 17:42:15.000000000 -0400
+++ common.inc	2009-09-26 17:43:36.000000000 -0400
@@ -583,10 +583,14 @@
     case 301: // Moved permanently
     case 302: // Moved temporarily
     case 307: // Moved temporarily
-      $location = $result->headers['Location'];
+      if($result->headers['Location'] == '.') {
+		  $location = $url;
+	  } else {
+		  $location = $result->headers['Location'];
+	  }
 
       if ($retry) {
-        $result = drupal_http_request($result->headers['Location'], $headers, $method, $data, --$retry);
+        $result = drupal_http_request($location, $headers, $method, $data, --$retry);
         $result->redirect_code = $result->code;
       }
       $result->redirect_url = $location;
CommentFileSizeAuthor
#16 588928-16-drupal_http_request-partial_location_redirect-8.x.patch5.64 KBDavid Stosik
#15 588928-15-drupal_http_request-partial_location_redirect-7.x.patch5.4 KBDavid Stosik
#11 588928-11-drupal_http_request-partial_location_redirect-7.x.patch5.4 KBDavid Stosik
#10 588928-10-drupal_http_request-partial_location_redirect-8.x.patch5.64 KBDavid Stosik
#8 588928-drupal_http_request-partial_location_redirect-7.x.patch1.41 KBDavid Stosik
#8 588928-drupal_http_request-partial_location_redirect-8.x.patch1.43 KBDavid Stosik
#2 common.inc_.patch890 bytesikogan
#1 common.inc_.patch1.12 KBikogan

Comments

ikogan’s picture

ikogan commented
StatusFileSize
new common.inc_.patch1.12 KB

Apparently this also fails for any relative URL (.., for example). Here's a fix that solves that as well:

--- common.inc.old  2009-10-05 00:51:46.000000000 -0400
+++ common.inc  2009-10-13 01:27:18.000000000 -0400
@@ -583,10 +583,18 @@
     case 301: // Moved permanently
     case 302: // Moved temporarily
     case 307: // Moved temporarily
-      $location = $result->headers['Location'];
+      if($result->headers['Location'][0] == '.') {
+          if($url[strlen($url)-1] != '/') {
+              $url .= "/";
+          }
+          
+          $location = $url .  $result->headers['Location'];
+      } else {
+          $location = $result->headers['Location'];
+      }

       if ($retry) {
-        $result = drupal_http_request($result->headers['Location'], $headers, $method, $data, --$retry);
+        $result = drupal_http_request($location, $headers, $method, $data, --$retry);
         $result->redirect_code = $result->code;
       }
       $result->redirect_url = $location;
ikogan’s picture

ikogan commented
StatusFileSize
new common.inc_.patch890 bytes

Apparently I failed to attach the correct patch file, here's one that isn't wrong.

nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented
Status: Active » Needs review

Have to tell the test bot there's a patch here

nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented
Version: 6.13 » 6.15

Also does this on the latest

Status: Needs review » Needs work

The last submitted patch, common.inc_.patch, failed testing.

David Stosik’s picture

David Stosik commented
Version: 6.15 » 7.x-dev

Broken on Drupal 7 too.

Example: try drupal_http_request('http://toulouse.fr');

damien tournoud’s picture

damien tournoud commented
Version: 7.x-dev » 8.x-dev
Category: bug » feature
Status: Needs work » Active

This is not actually a bug. HTTP/1.1 clearly mandates the Location header to be absolute. We should support that anyway, because many servers are broken, but this is not technically a bug.

We should check for :// in the header and if not present build a new URL based on the old URL (if starts with a "/", replace the whole path portion of the old URL, if not, append to the old URL).

David Stosik’s picture

David Stosik commented
Title: drupal_http_request fails for redirects to "." » drupal_http_request broken for a lot of websites using partial URL Location on redirect
Status: Active » Needs review
StatusFileSize
new 588928-drupal_http_request-partial_location_redirect-8.x.patch1.43 KB
new 588928-drupal_http_request-partial_location_redirect-7.x.patch1.41 KB

Well, actually, as most browsers (if not all), even wget or curl support this "non standard" redirect, and as some important websites (such as www.microsoft.com, www.lenovo.com, www.airfrance.fr, www.britishairways.com), I thought that this is not a uncommon case, thus not acceptable.

I mean, if I call drupal_http_request('http://www.microsoft.com'); I expect to get Microsoft's homepage as a result, not an error telling me that schema is missing although I clearly set one myself.

On a similar case, if a theme is intended to work with IE9, but doesn't display well because of CSS issues, then it's usually called a bug, not a "feature request because IE9 doesn't follow standards".

I guess, and I am hoping that drupal_http_request is intended to work with as many existing sites as possible, so when I find a site that shows up in all common browsers, but throws me weird error when called through drupal_http_request(), I blame drupal_http_request(), and call it a bug...

Anyway, I provided a patch for Drupal 7 and 8. I'm sure this needs work, but I would like to have others' opinion on this before giving it more time.

Regards,

David

Status: Needs review » Needs work

The last submitted patch, 588928-drupal_http_request-partial_location_redirect-8.x.patch, failed testing.

David Stosik’s picture

David Stosik commented
Status: Needs work » Needs review
StatusFileSize
new 588928-10-drupal_http_request-partial_location_redirect-8.x.patch5.64 KB

Here is a new patch on Drupal 8, with associated tests.

David Stosik’s picture

David Stosik commented
Version: 8.x-dev » 7.x-dev
Assigned: Unassigned » David Stosik
StatusFileSize
new 588928-11-drupal_http_request-partial_location_redirect-7.x.patch5.4 KB

And Drupal 7 one.
(Is there a way to launch SimpleTest on Drupal 7 on this patch? Trying to set version to Drupal 7 temporarily.)

David Stosik’s picture

David Stosik commented
Version: 7.x-dev » 8.x-dev

Let's switch the issue back to Drupal 8, now the test request has been sent.

Status: Needs review » Needs work

The last submitted patch, 588928-11-drupal_http_request-partial_location_redirect-7.x.patch, failed testing.

David Stosik’s picture

David Stosik commented

I have no idea why this doesn't pass.
Locally, the test passes on both www.example.com and www.example.com/drupal/ cases.

David Stosik’s picture

David Stosik commented
Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review
StatusFileSize
new 588928-15-drupal_http_request-partial_location_redirect-7.x.patch5.4 KB

By the way, I spotted a copy-paste error, so here is a new version. Comments are welcome.

David Stosik’s picture

David Stosik commented
Version: 7.x-dev » 8.x-dev
StatusFileSize
new 588928-16-drupal_http_request-partial_location_redirect-8.x.patch5.64 KB

Aaaaand, Drupal 8.

mikeytown2’s picture

mikeytown2 commented
Version: 8.x-dev » 7.x-dev
Status: Needs review » Closed (duplicate)

D8 has guzzle #1862398: [meta] Replace drupal_http_request() with Guzzle
Issue for D7 and below: #164365: drupal_http_request() does handle (invalid) non-absolute redirects (RFC 7231)