bug#21350: 25.0.50; Do not automatically include authorization header in HTTP redirects

bug#21350: 25.0.50; Do not automatically include authorization header in HTTP redirects

[Thomas Fitzsimmons]

[-- Attachment #1: Type: text/plain, Size: 466 bytes --]

Hi,

This patch is required for url-http-ntlm.el to handle redirects.  I'd
like someone more familiar with url-http.el to review it.  Basically,
this patch leaves it up to the authentication scheme to decide whether
to include an "Authorization" across a redirect or not.

I tested this on normal redirects (independent of url-http-ntlm.el) and
it seems to work fine, with the built-in Basic authorization scheme
re-adding the header where required.

Thanks,
Thomas

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Do-not-include-authorization-header-in-an-HTTP-redir.patch --]
[-- Type: text/x-patch, Size: 1325 bytes --]

From 26b71ed091d23853d1345295004a93c28ef287b9 Mon Sep 17 00:00:00 2001
From: Thomas Fitzsimmons <fitzsim@fitzsim.org>
Date: Tue, 25 Aug 2015 22:27:44 -0400
Subject: [PATCH] Do not include authorization header in an HTTP redirect

* lisp/url/url-http.el (url-http-parse-headers): Do not
automatically include Authorization header in redirect.
---
 lisp/url/url-http.el | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lisp/url/url-http.el b/lisp/url/url-http.el
index 6a7d8e2..4f3213d 100644
--- a/lisp/url/url-http.el
+++ b/lisp/url/url-http.el
@@ -646,6 +646,15 @@ (defun url-http-parse-headers ()
                ;; compute the redirection relative to the URL of the proxy.
 	       (setq redirect-uri
 		     (url-expand-file-name redirect-uri url-http-target-url)))
+	   ;; Don't automatically include authorization header in redirect.
+	   ;; If needed it will be regenerated by the relevant auth scheme
+	   ;; when the new request happens.
+	   (setq url-http-extra-headers
+		 (let (result)
+		   (dolist (header url-http-extra-headers)
+		     (if (not (equal (car header) "Authorization"))
+			 (push header result)))
+		   (nreverse result)))
            (let ((url-request-method url-http-method)
 		 (url-request-data url-http-data)
 		 (url-request-extra-headers url-http-extra-headers))
-- 
2.4.2

bug#21350: 25.0.50; Do not automatically include authorization header in HTTP redirects

[Stefan Monnier]

> This patch is required for url-http-ntlm.el to handle redirects.  I'd
> like someone more familiar with url-http.el to review it.

I'm not sure if there is such a someone, to tell you the truth.  I can
give you comments about Elisp style:

+	   ;; Don't automatically include authorization header in redirect.
+	   ;; If needed it will be regenerated by the relevant auth scheme
+	   ;; when the new request happens.
+	   (setq url-http-extra-headers
+		 (let (result)
+		   (dolist (header url-http-extra-headers)
+		     (if (not (equal (car header) "Authorization"))
+			 (push header result)))
+		   (nreverse result)))

IIUC this is like:

  (let ((a (assoc "Authorization" url-http-extra-headers)))
    (if a (setq url-http-extra-headers (delq a url-http-extra-headers))))

Tho maybe it should be `remq' rather than `delq'.


        Stefan

bug#21350: 25.0.50; Do not automatically include authorization header in HTTP redirects

[Thomas Fitzsimmons]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> This patch is required for url-http-ntlm.el to handle redirects.  I'd
>> like someone more familiar with url-http.el to review it.
>
> I'm not sure if there is such a someone, to tell you the truth.  I can
> give you comments about Elisp style:

OK, thanks.

> +	   ;; Don't automatically include authorization header in redirect.
> +	   ;; If needed it will be regenerated by the relevant auth scheme
> +	   ;; when the new request happens.
> +	   (setq url-http-extra-headers
> +		 (let (result)
> +		   (dolist (header url-http-extra-headers)
> +		     (if (not (equal (car header) "Authorization"))
> +			 (push header result)))
> +		   (nreverse result)))
>
> IIUC this is like:
>
>   (let ((a (assoc "Authorization" url-http-extra-headers)))
>     (if a (setq url-http-extra-headers (delq a url-http-extra-headers))))
>
> Tho maybe it should be `remq' rather than `delq'.

I was trying to remove all occurrences of "Authorization", just in case,
since that's what url-http-ntlm did.  I looked at remq and delq.  delq
looks like it would be faster.  I'm not sure why I would use remq since
I'm overwriting url-http-extra-headers anyway.

url-http-ntlm did this:

(defun url-http-ntlm-rmssoc (key alist)
  (remove* key alist :key 'car :test 'equal))

but should I avoid using cl-lib in this context?  Another consideration
is that I want to be able to backport this change (as an ELPA-installed
patch) all the way back to Emacs 24.1, so maybe that's another reason
not to use cl-lib.

Thomas

bug#21350: 25.0.50; Do not automatically include authorization header in HTTP redirects

[Stefan Monnier]

> looks like it would be faster.  I'm not sure why I would use remq since
> I'm overwriting url-http-extra-headers anyway.

It depends on where that list comes from and where it might have been
stored in the mean time.  If we know that noone else points to that
list, then `delq' is the best option.

> but should I avoid using cl-lib in this context?

No, you can feel free to use cl-lib.

> Another consideration is that I want to be able to backport this
> change (as an ELPA-installed patch) all the way back to Emacs 24.1, so
> maybe that's another reason not to use cl-lib.

cl-lib is in GNU ELPA and works fine for Emacs-24.1 (and AFAICT it also
works on Emacs-22 and XEmacs).


        Stefan

bug#21350: 25.0.50; Do not automatically include authorization header in HTTP redirects

[Thomas Fitzsimmons]

[-- Attachment #1: Type: text/plain, Size: 1088 bytes --]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> looks like it would be faster.  I'm not sure why I would use remq since
>> I'm overwriting url-http-extra-headers anyway.
>
> It depends on where that list comes from and where it might have been
> stored in the mean time.  If we know that noone else points to that
> list, then `delq' is the best option.
>
>> but should I avoid using cl-lib in this context?
>
> No, you can feel free to use cl-lib.
>
>> Another consideration is that I want to be able to backport this
>> change (as an ELPA-installed patch) all the way back to Emacs 24.1, so
>> maybe that's another reason not to use cl-lib.
>
> cl-lib is in GNU ELPA and works fine for Emacs-24.1 (and AFAICT it also
> works on Emacs-22 and XEmacs).

Here's the updated patch that I tested.  Does it look OK stylistically?

I'm going to try to set up some sort of reproducible test for the
various auth schemes across redirects before pushing this, to try to
prove that I'm not breaking some redirect scenarios with this.  I'll see
how far I get with that before pushing.

Thomas

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-Do-not-include-authorization-header-in-an-HTTP-redir.patch --]
[-- Type: text/x-patch, Size: 1245 bytes --]

From 5a3c80ca5323cde23eca4638a28e4f8cc28dd2df Mon Sep 17 00:00:00 2001
From: Thomas Fitzsimmons <fitzsim@cisco.com>
Date: Sun, 6 Sep 2015 15:56:53 -0400
Subject: [PATCH 2/2] Do not include authorization header in an HTTP redirect

* lisp/url/url-http.el (url-http-parse-headers): Do not
automatically include Authorization header in redirect.
---
 lisp/url/url-http.el | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lisp/url/url-http.el b/lisp/url/url-http.el
index 6a7d8e2..b5c1a33 100644
--- a/lisp/url/url-http.el
+++ b/lisp/url/url-http.el
@@ -646,6 +646,12 @@ should be shown to the user."
                ;; compute the redirection relative to the URL of the proxy.
 	       (setq redirect-uri
 		     (url-expand-file-name redirect-uri url-http-target-url)))
+	   ;; Do not automatically include an authorization header in the
+	   ;; redirect.  If needed it will be regenerated by the relevant
+	   ;; auth scheme when the new request happens.
+	   (setq url-http-extra-headers
+		 (cl-remove "Authorization"
+			    url-http-extra-headers :key 'car :test 'equal))
            (let ((url-request-method url-http-method)
 		 (url-request-data url-http-data)
 		 (url-request-extra-headers url-http-extra-headers))
-- 
1.8.3.1

bug#21350: 25.0.50; Do not automatically include authorization header in HTTP redirects

[Stefan Monnier]

> Here's the updated patch that I tested.  Does it look OK stylistically?

Yes, but you need to change the beginning of the file so cl-lib is not
only require when compiling but also at run-time (since cl-remove is
not a macro but a function).


        Stefan

bug#21350: 25.0.50; Do not automatically include authorization header in HTTP redirects

[Thomas Fitzsimmons]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Here's the updated patch that I tested.  Does it look OK stylistically?
>
> Yes, but you need to change the beginning of the file so cl-lib is not
> only require when compiling but also at run-time (since cl-remove is
> not a macro but a function).

OK, I pushed the patch.  Thanks for reviewing.

I had hoped to publish a Docker image that would allow testing the
various authorization schemes across redirects, but configuring a server
to authenticate with NTLM using Free Software proved too difficult.  I
did test against a proprietary NTLM implementation, and against the two
built-in auth schemes as well.  The results were:

   |          Authenticated Redirect          |
   |-------------+---------------+------------|
   | Auth Scheme | Without Patch | With Patch |
   |-------------+---------------+------------|
   | Basic       | Works         | Works      |
   | Digest      | Fails         | Fails      |
   | NTLM        | Fails         | Works      |

I'm not sure what's wrong with the digest scheme (Firefox works), but
this patch doesn't make digest redirects worse.

Thomas