bug#10478: 24.0.50; url-http-parse-headers can silently drop the response when handling BASIC AUTHENTICATION

[Jerry Asher <jerry.asher@gmail.com>]

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

So here's my analysis of what I did back then. I think it was short and
even straight-forward, anyone is welcome to rewrite or reimplement it.

Enough cleanup has happened in url-http.el that it makes it difficult to
simply give you a diff today. I don't even know where to get the code as it
was in 2012 to compare my patch from back then to.

But there were "two" bugs.

Bug one: in url-http-handle-authentication, there is a call to
url-retrieve-internal, and that function can return a new buffer with the
contents of the url, but url-http-handle-authentication does not return the
new buffer with the new contents. It drops it on the floor.

So all I did, literally, was to capture the return value and then change
url-http-handle-authentication throughout so that the the new url contents
would be passed back.

in url-http-handle-authentication, the end of the function goes from:

    (if (not (url-auth-registered type))
        (progn
          (widen)
          (goto-char (point-max))
          (insert "<hr>Sorry, but I do not know how to handle " (or type
auth url "")
                  " authentication.  If you'd like to write it,"
                  " please use M-x report-emacs-bug RET.<hr>")
          ;; We used to set a `status' var (declared "special") but I can't
          ;; find the corresponding let-binding, so it's probably an error.
          ;; FIXME: Maybe it was supposed to set `success', i.e. to return
t?
          ;; (setq status t)
*          nil) ;; Not success yet.*

      (let* ((args (url-parse-args (subst-char-in-string ?, ?\; auth)))
             (auth (url-get-authentication auth-url
                                           (cdr-safe (assoc "realm" args))
                                           type t args)))
        (if (not auth)
            t                           ;Success.
          (push (cons (if proxy "Proxy-Authorization" "Authorization") auth)
                url-http-extra-headers)
          (let ((url-request-method url-http-method)
                (url-request-data url-http-data)
                (url-request-extra-headers url-http-extra-headers))
*            (url-retrieve-internal url url-callback-function*
*                                   url-callback-arguments))*
*          nil))))) ;; Not success yet.*


to


    (if (not (url-auth-registered type))
        (progn
          (widen)
          (goto-char (point-max))
          (insert "<hr>Sorry, but I do not know how to handle " type
                  " authentication.  If you'd like to write it,"
                  " send it to " url-bug-address ".<hr>")
          (setq status t)
          *(setq retval nil)*)

      (let* ((args (url-parse-args (subst-char-in-string ?, ?\; auth)))
             (auth (url-get-authentication auth-url
                                           (cdr-safe (assoc "realm" args))
                                           type t args)))
        (if (not auth)
            (progn
              (setq success t)
              *(set retval nil)*)
          (push (cons (if proxy "Proxy-Authorization" "Authorization") auth)
                url-http-extra-headers)
          (let ((url-request-method url-http-method)
                (url-request-data url-http-data)
                (url-request-extra-headers url-http-extra-headers)
               * (response-buffer nil)*)
*            (setq response-buffer*
*                  (url-retrieve-internal url url-callback-function*
*                                         url-callback-arguments))*
            (url-http-debug "Handling authentication return buffer is %s"
                            response-buffer)
            *(setq retval response-buffer)*))))
    (url-http-debug "Handling authentication retval is %s 2:" retval)
*    retval))*


At an appropriate place above this, the declaration of retval is stuffed
into an existing let.

The next bug occurs in url-http-parse-headers where the 401 and 407 cases
which deal with authentication do not take into account that the url
contents may have changed as a result of authentication. And there all I
did was to cut and paste the previously self-declared hack from the 3XX
case in.

So the 3XX case had this code already in it

                 (progn
                   ;; Remember that the request was redirected.
                   (setf (car url-callback-arguments)
                         (nconc (list :redirect redirect-uri)
                                (car url-callback-arguments)))
*                   ;; Put in the current buffer a forwarding pointer to
the new*
*                   ;; destination buffer.*
*                   ;; FIXME: This is a hack to fix
url-retrieve-synchronously*
*                   ;; without changing the API.  Instead url-retrieve
should*
*                   ;; either simply not return the "destination" buffer,
or it*
*                   ;; should take an optional `dest-buf' argument.*
*                   (set (make-local-variable 'url-redirect-buffer)*
*                        (url-retrieve-internal*
*                         redirect-uri url-callback-function*
*                         url-callback-arguments*
*                         (url-silent url-current-object)))*
*                   (url-mark-buffer-as-dead buffer))*

I diligently copied that code into the 401 and 407 cases.


               (`unauthorized           ; 401
                ;; The request requires user authentication.  The response
                ;; MUST include a WWW-Authenticate header field containing a
                ;; challenge applicable to the requested resource.  The
                ;; client MAY repeat the request with a suitable
                ;; Authorization header field.
                (url-http-handle-authentication nil))

to


         (unauthorized                  ; 401
          ;; The request requires user authentication.  The response
          ;; MUST include a WWW-Authenticate header field containing a
          ;; challenge applicable to the requested resource.  The
          ;; client MAY repeat the request with a suitable
          ;; Authorization header field.

          ;; bug patch because url-http-handle-authentication
          ;; might return a new buffer

          (let ((retval (url-http-handle-authentication nil)))
            (url-http-debug "Url Http Parse Headers: handling
authentication return buffer TO %s" retval)
            (when retval
              ;; Put in the current buffer a forwarding pointer to the new
              ;; destination buffer.
              ;; FIXME: This is a hack to fix url-retrieve-synchronously
              ;; without changing the API.  Instead url-retrieve should
              ;; either simply not return the "destination" buffer, or it
              ;; should take an optional `dest-buf' argument.
              (set (make-local-variable 'url-redirect-buffer)
                   retval)
              (url-http-debug "Url Http Parse Headers: handling
authentication return buffer TO %s -> %s 2:"
                              retval url-redirect-buffer)
              (url-mark-buffer-as-dead buffer))))


And I did the same thing for the 407 case.

I didn't test it much back in 2012. It worked for my needs against
posterous, which then went belly up. You should probably test this now more
than I did then, when I wasn't writing so much of a patch, as indicating
where the bugs were and the proper fixes needed to apply.

I hope that helps,

Jerry

On Sat, Jun 3, 2017 at 4:05 AM, Eli Zaretskii <eliz@gnu.org> wrote:

> > From: David Engster <deng@randomsample.de>
> > Date: Sat, 03 Jun 2017 12:41:47 +0200
> > Cc: Jerry Asher <jerry.asher@gmail.com>, 10478@debbugs.gnu.org
> >
> > I'm not sure if the resulting patch will fit in the 'trivial patch'
> > category.
>
> It's hard to tell, as no diffs were posted.
>
> Would it be possible to extract the error-handling part into a
> separate function, so that the actual changes would be as short as
> possible?  Then we could see if they are short enough to accept
> without papers.
>
> Thanks.
>

[-- Attachment #2: Type: text/html, Size: 17730 bytes --]