Avoid a redirect loop in url-http

Avoid a redirect loop in url-http

[Diane Murray]

On occasion url-http gets stuck in an endless redirect loop.  This can
be fixed by simply checking if the number of redirected URLs in
`url-callback-arguments' hasn't gone over a certain number (which
could be a customizable variable).  If it has been redirected too many
times, rather than call `url-retrieve-internal' again,
`url-http-parse-headers' should just let the callback function get
activated - after adding a redirect error to `url-callback-arguments',
so that the callback knows that the maximum number of redirects was
reached.

I've fixed this on my system, but I'm not including the changes I
made.  I'm currently waiting to get the Emacs copyright release form
(the FSF copyright-clerk has confirmed that the papers will be sent),
and by the time I receive the papers, sign them, and return them, they
probably wouldn't arrive back before the tentative release date for
Emacs 22.  If you'd like me to send my patch, please let me know.

--
Diane Murray

Re: Avoid a redirect loop in url-http

[Chong Yidong]

Diane Murray <disumu@x3y2z1.net> writes:

> On occasion url-http gets stuck in an endless redirect loop.  This can
> be fixed by simply checking if the number of redirected URLs in
> `url-callback-arguments' hasn't gone over a certain number (which
> could be a customizable variable).  If it has been redirected too many
> times, rather than call `url-retrieve-internal' again,
> `url-http-parse-headers' should just let the callback function get
> activated - after adding a redirect error to `url-callback-arguments',
> so that the callback knows that the maximum number of redirects was
> reached.

Thanks.

How about this patch?  It doesn't implement a defcustom as you
suggested; it simply searches for a redirect loop.  It also simply
signals an error if a loop is found.

(The indentation of existing code is left unchanged to make the patch
easier to read.)

*** emacs/lisp/url/url-http.el.~1.52.~	2007-04-01 11:32:35.000000000 -0400
--- emacs/lisp/url/url-http.el	2007-04-12 00:17:40.000000000 -0400
***************
*** 555,561 ****
  		     (url-expand-file-name redirect-uri url-http-target-url)))
             (let ((url-request-method url-http-method)
  		 (url-request-data url-http-data)
! 		 (url-request-extra-headers url-http-extra-headers))
  	     ;; Remember that the request was redirected.
  	     (setf (car url-callback-arguments)
  		   (nconc (list :redirect redirect-uri)
--- 555,571 ----
  		     (url-expand-file-name redirect-uri url-http-target-url)))
             (let ((url-request-method url-http-method)
  		 (url-request-data url-http-data)
! 		 (url-request-extra-headers url-http-extra-headers)
! 		 (events (car url-callback-arguments)))
! 	     ;; Check for redirect loops
! 	     (while events
! 	       (if (eq (car events) :redirect)
! 		   (and (setq events (cdr events))
! 			(if (eq (car events) redirect-uri)
! 			    (error "HTTP redirect loop detected")
! 			  (setq events (cdr events))))
! 		 (and (setq events (cdr events))
! 		      (setq events (cdr events)))))
  	     ;; Remember that the request was redirected.
  	     (setf (car url-callback-arguments)
  		   (nconc (list :redirect redirect-uri)
***************
*** 570,576 ****
  		   (url-retrieve-internal
  		    redirect-uri url-callback-function
  		    url-callback-arguments))
! 	      (url-mark-buffer-as-dead (current-buffer))))))
        (4				; Client error
         ;; 400 Bad Request
         ;; 401 Unauthorized
--- 580,586 ----
  		   (url-retrieve-internal
  		    redirect-uri url-callback-function
  		    url-callback-arguments))
! 	      (url-mark-buffer-as-dead (current-buffer)))))))
        (4				; Client error
         ;; 400 Bad Request
         ;; 401 Unauthorized

Re: Avoid a redirect loop in url-http

[Kim F. Storm]

Chong Yidong <cyd@stupidchicken.com> writes:

> How about this patch?  It doesn't implement a defcustom as you
> suggested; it simply searches for a redirect loop.  It also simply
> signals an error if a loop is found.

Clever!

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Avoid a redirect loop in url-http

[Diane Murray]

On Thu, 12 Apr 2007 Chong Yidong <cyd@stupidchicken.com> wrote:

> Diane Murray <disumu@x3y2z1.net> writes:

>> On occasion url-http gets stuck in an endless redirect loop.  This
>> can be fixed by simply checking if the number of redirected URLs in
>> `url-callback-arguments' hasn't gone over a certain number (which
>> could be a customizable variable).  If it has been redirected too
>> many times, rather than call `url-retrieve-internal' again,
>> `url-http-parse-headers' should just let the callback function get
>> activated - after adding a redirect error to
>> `url-callback-arguments', so that the callback knows that the
>> maximum number of redirects was reached.

> How about this patch?  It doesn't implement a defcustom as you
> suggested; it simply searches for a redirect loop.  It also simply
> signals an error if a loop is found.

I apologize for providing an explanation of the problem that was much
too brief.  Here's an example: There are sites that for some reason or
another keep returning redirect responses.  Some redirect to the same
host and file path but with slightly different query arguments,
possibly because the page expects a cookie to be set and url-cookie
doesn't allow it.  Other mischievous sites might do something similar
just to try and cause problems.  Since these are all distinct URLs,
they won't be equal, which is why I suggested checking how many times
the request has been redirected.

Regarding the error in your patch - if a real error is returned the
callback won't get called.  I think it would be better to add the
error to `url-callback-arguments' like the connection-failed error in
`url-http-async-sentinel' and to force a callback, so that the
function that calls url-retrieve/url-http can deal with things
appropriately.

--
Diane Murray

Re: Avoid a redirect loop in url-http

[Chong Yidong]

Diane Murray <disumu@x3y2z1.net> writes:

> There are sites that for some reason or another keep returning
> redirect responses.  Some redirect to the same host and file path
> but with slightly different query arguments, possibly because the
> page expects a cookie to be set and url-cookie doesn't allow it.
> Other mischievous sites might do something similar just to try and
> cause problems.  Since these are all distinct URLs, they won't be
> equal, which is why I suggested checking how many times the request
> has been redirected.

I see.

How about this patch?

*** emacs/lisp/url/url-vars.el.~1.16.~	2007-01-20 22:24:41.000000000 -0500
--- emacs/lisp/url/url-vars.el	2007-04-12 11:32:34.000000000 -0400
***************
*** 320,325 ****
--- 320,331 ----
    "\\`\\([-a-zA-Z0-9+.]+:\\)"
    "A regular expression that will match an absolute URL.")
  
+ (defcustom url-max-redirections 30
+   "*The maximum number of redirection requests to honor in a HTTP connection.
+ A negative number means to honor an unlimited number of redirection requests."
+   :type 'integer
+   :group 'url)
+ 
  (defcustom url-confirmation-func 'y-or-n-p
    "*What function to use for asking yes or no functions.
  Possible values are `yes-or-no-p' or `y-or-n-p', or any function that
*** emacs/lisp/url/url-http.el.~1.52.~	2007-04-01 11:32:35.000000000 -0400
--- emacs/lisp/url/url-http.el	2007-04-12 11:44:48.000000000 -0400
***************
*** 556,561 ****
--- 556,575 ----
             (let ((url-request-method url-http-method)
  		 (url-request-data url-http-data)
  		 (url-request-extra-headers url-http-extra-headers))
+ 	     ;; Check existing number of redirects
+ 	     (if (or (< url-max-redirections 0)
+ 		     (and (> url-max-redirections 0)
+ 			  (let ((events (car url-callback-arguments))
+ 				(old-redirects 0))
+ 			    (while events
+ 			      (if (eq (car events) :redirect)
+ 				  (setq old-redirects (1+ old-redirects)))
+ 			      (and (setq events (cdr events))
+ 				   (setq events (cdr events))))
+ 			    (< old-redirects url-max-redirections))))
+ 		 ;; url-max-redirections hasn't been reached, so go
+ 		 ;; ahead and redirect.
+ 		 (progn
  	     ;; Remember that the request was redirected.
  	     (setf (car url-callback-arguments)
  		   (nconc (list :redirect redirect-uri)
***************
*** 570,576 ****
  		   (url-retrieve-internal
  		    redirect-uri url-callback-function
  		    url-callback-arguments))
! 	      (url-mark-buffer-as-dead (current-buffer))))))
        (4				; Client error
         ;; 400 Bad Request
         ;; 401 Unauthorized
--- 584,598 ----
  		   (url-retrieve-internal
  		    redirect-uri url-callback-function
  		    url-callback-arguments))
! 	      (url-mark-buffer-as-dead (current-buffer)))
! 	       ;; We hit url-max-redirections, so issue an error and
! 	       ;; stop redirecting.
! 	       (setf (car url-callback-arguments)
! 		     (nconc (list :error (list 'error
! 					       'http
! 					       url-http-response-status))
! 			    (car url-callback-arguments)))
! 	       (setq success t))))))
        (4				; Client error
         ;; 400 Bad Request
         ;; 401 Unauthorized

Re: Avoid a redirect loop in url-http

[Diane Murray]

On Thu, 12 Apr 2007 Chong Yidong <cyd@stupidchicken.com> wrote:

> How about this patch?

Yes, it works.  Thank you.

I recommend sending a `url-http-debug' message when the maximum number
of redirections has been reached.  I'd also mention in the doc string
of `url-max-redirections' that it's probably not the best idea to
allow unlimited redirections.

A couple questions:
Should the test be made after adding the new redirect-uri to
`url-callback-arguments'?  Could the error that's set be a little more
specific - perhaps something like 'redirect-limit instead of 'http?

--
Diane Murray

Re: Avoid a redirect loop in url-http

[Chong Yidong]

Diane Murray <disumu@x3y2z1.net> writes:

> I recommend sending a `url-http-debug' message when the maximum number
> of redirections has been reached.

OK.

> Should the test be made after adding the new redirect-uri to
> `url-callback-arguments'?

There's no real difference, I think.

> Could the error that's set be a little more specific - perhaps
> something like 'redirect-limit instead of 'http?

OK, it now signals a http-redirect-limit.