GNU bug report logs - #21350
25.0.50; Do not automatically include authorization header in HTTP redirects

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 21350 in the body.
You can then email your comments to 21350 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
To: bug-gnu-emacs <at> gnu.org
Subject: 25.0.50;
 Do not automatically include authorization header in HTTP redirects
Date: Tue, 25 Aug 2015 22:37:46 -0400

[Message part 1 (text/plain, inline)]

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

[0001-Do-not-include-authorization-header-in-an-HTTP-redir.patch (text/x-patch, attachment)]

Message #10 received at 21350 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
Cc: 21350 <at> debbugs.gnu.org
Subject: Re: bug#21350: 25.0.50;
 Do not automatically include authorization header in HTTP redirects
Date: Sat, 29 Aug 2015 11:21:35 -0400

> 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

Message #13 received at 21350 <at> debbugs.gnu.org (full text, mbox):

From: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 21350 <at> debbugs.gnu.org
Subject: Re: bug#21350: 25.0.50;
 Do not automatically include authorization header in HTTP redirects
Date: Mon, 31 Aug 2015 22:33:05 -0400

Stefan Monnier <monnier <at> 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

Message #16 received at 21350 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
Cc: 21350 <at> debbugs.gnu.org
Subject: Re: bug#21350: 25.0.50;
 Do not automatically include authorization header in HTTP redirects
Date: Mon, 31 Aug 2015 23:58:17 -0400

> 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

Message #19 received at 21350 <at> debbugs.gnu.org (full text, mbox):

From: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 21350 <at> debbugs.gnu.org
Subject: Re: bug#21350: 25.0.50;
 Do not automatically include authorization header in HTTP redirects
Date: Sun, 06 Sep 2015 20:10:27 -0400

[Message part 1 (text/plain, inline)]

Stefan Monnier <monnier <at> 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

[0002-Do-not-include-authorization-header-in-an-HTTP-redir.patch (text/x-patch, attachment)]

Message #22 received at 21350 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
Cc: 21350 <at> debbugs.gnu.org
Subject: Re: bug#21350: 25.0.50;
 Do not automatically include authorization header in HTTP redirects
Date: Mon, 07 Sep 2015 11:23:08 -0400

> 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

Message #27 received at 21350-done <at> debbugs.gnu.org (full text, mbox):

From: Thomas Fitzsimmons <fitzsim <at> fitzsim.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 21350-done <at> debbugs.gnu.org
Subject: Re: bug#21350: 25.0.50;
 Do not automatically include authorization header in HTTP redirects
Date: Wed, 23 Sep 2015 02:09:32 -0400

Stefan Monnier <monnier <at> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.