bug-gnu-emacs
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

bug#10478: 24.0.50; url-http-parse-headers can silently drop the respons


From: Jerry Asher
Subject: bug#10478: 24.0.50; url-http-parse-headers can silently drop the response when handling BASIC AUTHENTICATION
Date: Sat, 3 Jun 2017 05:56:03 -0700

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 <address@hidden> wrote:
> From: David Engster <address@hidden>
> Date: Sat, 03 Jun 2017 12:41:47 +0200
> Cc: Jerry Asher <address@hidden>, address@hidden
>
> 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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]