unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#11981: 24.1.50; url-http-parse-headers should not disable file name handlers since it breaks auth-source
@ 2012-07-18 18:38 David Engster
  2012-07-19  6:35 ` Michael Albinus
  0 siblings, 1 reply; 11+ messages in thread
From: David Engster @ 2012-07-18 18:38 UTC (permalink / raw)
  To: 11981

The function `url-http-parse-headers' temporarily disables all file name
handlers to circumvent a bug where Tramp could kick in because the URL
might look like a remote file (#6717). This is the offending snippet:

	;; The filename part of a URL could be in remote file syntax,
	;; see Bug#6717 for an example.  We disable file name
	;; handlers, therefore.
	(file-name-handler-alist nil))

However, this workaraound assumes that everything that gets afterwards
in the `let'-section does not depend on file name handlers. However,
this is not the case: `url-http-handle-authentication' is called which
in turn calls the auth-source package. This package however supports
encrypted authinfo files which have to be automatically decrypted
through `auto-encryption-mode'. This, of course, works through file name
handlers.

Here's a backtrace from `auth-source-search' resulting from a call to
`url-retrieve-synchronously' on a password-protected URL:

  auth-source-search(:max 1 :host "www.google.com:443" :port "https")
  url-do-auth-source-search("www.google.com:443" "https" :user)
  url-basic-auth([cl-struct-url "https" nil nil "www.google.com" 443  "/calendar/dav/MY-GOOGLE-ACCOUNT/events/" nil nil t nil t] t nil "Google CalDAV" (("realm" . "Google CalDAV") ("basic")))
  url-get-authentication("https://www.google.com/calendar/dav/MY-GOOGLE-ACCOUNT/events/" "Google CalDAV" "basic" t (("realm" . "Google CalDAV") ("basic")))
  url-http-handle-authentication(nil)
  [ ... ]
  url-http-parse-headers()
  url-http-chunked-encoding-after-change-function(500 507 7)
  url-http-generic-filter(#<process www.google.com<1>> "\n0\n\n")
  [ ... ]
  url-retrieve-synchronously("https://www.google.com/calendar/dav/MY-GOOGLE-ACCOUNT/events/")


Note that after `url-http-parse-headers' the variable
`file-name-handler-alist' is set to 'nil', thus when
`auth-source-search' loads an encrypted authinfo.gpg file, it won't
automatically get decrypted.

-David





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#11981: 24.1.50; url-http-parse-headers should not disable file name handlers since it breaks auth-source
  2012-07-18 18:38 bug#11981: 24.1.50; url-http-parse-headers should not disable file name handlers since it breaks auth-source David Engster
@ 2012-07-19  6:35 ` Michael Albinus
  2012-07-19  8:55   ` Stefan Monnier
  0 siblings, 1 reply; 11+ messages in thread
From: Michael Albinus @ 2012-07-19  6:35 UTC (permalink / raw)
  To: David Engster; +Cc: 11981

David Engster <deng@randomsample.de> writes:

> However, this workaraound assumes that everything that gets afterwards
> in the `let'-section does not depend on file name handlers. However,
> this is not the case: `url-http-handle-authentication' is called which
> in turn calls the auth-source package. This package however supports
> encrypted authinfo files which have to be automatically decrypted
> through `auto-encryption-mode'. This, of course, works through file name
> handlers.

Does it work, if Tramp is disabled instead?

--8<---------------cut here---------------start------------->8---
*** /usr/local/src/emacs/lisp/url/url-http.el.~109155~  2012-07-19
    08:29:53.339768264 +0200
--- /usr/local/src/emacs/lisp/url/url-http.el   2012-07-19
    08:28:26.428326961 +0200
***************
*** 510,518 ****
    ;; other status symbols: jewelry and luxury cars
    (status-symbol (cadr (assq url-http-response-status url-http-codes)))
    ;; The filename part of a URL could be in remote file syntax,
!   ;; see Bug#6717 for an example.  We disable file name
!   ;; handlers, therefore.
!   (file-name-handler-alist nil))
      (setq class (/ url-http-response-status 100))
      (url-http-debug "Parsed HTTP headers: class=%d status=%d" class url-http-response-status)
      (when (url-use-cookies url-http-target-url)
--- 510,517 ----
    ;; other status symbols: jewelry and luxury cars
    (status-symbol (cadr (assq url-http-response-status url-http-codes)))
    ;; The filename part of a URL could be in remote file syntax,
!   ;; see Bug#6717 for an example.  We disable Tramp, therefore.
!   (tramp-mode nil))
      (setq class (/ url-http-response-status 100))
      (url-http-debug "Parsed HTTP headers: class=%d status=%d" class url-http-response-status)
      (when (url-use-cookies url-http-target-url)
--8<---------------cut here---------------end--------------->8---

> -David

Best regards, Michael.





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#11981: 24.1.50; url-http-parse-headers should not disable file name handlers since it breaks auth-source
  2012-07-19  6:35 ` Michael Albinus
@ 2012-07-19  8:55   ` Stefan Monnier
  2012-07-19 12:51     ` David Engster
  0 siblings, 1 reply; 11+ messages in thread
From: Stefan Monnier @ 2012-07-19  8:55 UTC (permalink / raw)
  To: Michael Albinus; +Cc: 11981, David Engster

>> However, this workaraound assumes that everything that gets afterwards
>> in the `let'-section does not depend on file name handlers. However,
>> this is not the case: `url-http-handle-authentication' is called which
>> in turn calls the auth-source package. This package however supports
>> encrypted authinfo files which have to be automatically decrypted
>> through `auto-encryption-mode'. This, of course, works through file name
>> handlers.
> Does it work, if Tramp is disabled instead?

I think I'd first like to better understand bug#6717: why do we take
a local part of a URL, let it start with / and then pass it to
file-name-directory?  That sounds like a problem in itself.


        Stefan





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#11981: 24.1.50; url-http-parse-headers should not disable file name handlers since it breaks auth-source
  2012-07-19  8:55   ` Stefan Monnier
@ 2012-07-19 12:51     ` David Engster
  2012-07-21 14:31       ` David Engster
  0 siblings, 1 reply; 11+ messages in thread
From: David Engster @ 2012-07-19 12:51 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: 11981, Michael Albinus

Stefan Monnier writes:
>>> However, this workaraound assumes that everything that gets afterwards
>>> in the `let'-section does not depend on file name handlers. However,
>>> this is not the case: `url-http-handle-authentication' is called which
>>> in turn calls the auth-source package. This package however supports
>>> encrypted authinfo files which have to be automatically decrypted
>>> through `auto-encryption-mode'. This, of course, works through file name
>>> handlers.
>> Does it work, if Tramp is disabled instead?
>
> I think I'd first like to better understand bug#6717: why do we take
> a local part of a URL, let it start with / and then pass it to
> file-name-directory?  That sounds like a problem in itself.

I agree. I wonder which call to `file-name-directory' threw the error in
the original report - the most likely candidate is `url-file-directory',
which should simply be rewritten to not use `file-name-directory'.

-David





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#11981: 24.1.50; url-http-parse-headers should not disable file name handlers since it breaks auth-source
  2012-07-19 12:51     ` David Engster
@ 2012-07-21 14:31       ` David Engster
  2012-08-06 16:01         ` Stefan Monnier
  0 siblings, 1 reply; 11+ messages in thread
From: David Engster @ 2012-07-21 14:31 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: 11981, Michael Albinus

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

David Engster writes:
> Stefan Monnier writes:
>>>> However, this workaraound assumes that everything that gets afterwards
>>>> in the `let'-section does not depend on file name handlers. However,
>>>> this is not the case: `url-http-handle-authentication' is called which
>
>>>> in turn calls the auth-source package. This package however supports
>>>> encrypted authinfo files which have to be automatically decrypted
>>>> through `auto-encryption-mode'. This, of course, works through file name
>>>> handlers.
>>> Does it work, if Tramp is disabled instead?
>>
>> I think I'd first like to better understand bug#6717: why do we take
>> a local part of a URL, let it start with / and then pass it to
>> file-name-directory?  That sounds like a problem in itself.
>
> I agree. I wonder which call to `file-name-directory' threw the error in
> the original report - the most likely candidate is `url-file-directory',
> which should simply be rewritten to not use `file-name-directory'.

Patch attached. It even fixes another bug along the way:
`url-file-(non)directory' would not work on hexified URLs.

I'm not 100% sure this also fixes bug #6717; it might be that there's
another file-* call lurking somewhere.

-David


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: url-util-diff.patch --]
[-- Type: text/x-patch, Size: 1180 bytes --]

=== modified file 'lisp/url/url-util.el'
--- lisp/url/url-util.el	2012-07-11 23:13:41 +0000
+++ lisp/url/url-util.el	2012-07-21 14:25:59 +0000
@@ -246,18 +246,24 @@
   "Return the directory part of FILE, for a URL."
   (cond
    ((null file) "")
-   ((string-match "\\?" file)
-    (file-name-directory (substring file 0 (match-beginning 0))))
-   (t (file-name-directory file))))
+   ((string-match "\\(\\?\\|%3[fF]\\)" file)
+    (url-file-directory (substring file 0 (match-beginning 0))))
+   ((string-match "\\(.*\\(/\\|%2[fF]\\)\\)" file)
+    (match-string 1 file))
+   (t
+    "")))
 
 ;;;###autoload
 (defun url-file-nondirectory (file)
   "Return the nondirectory part of FILE, for a URL."
   (cond
    ((null file) "")
-   ((string-match "\\?" file)
-    (file-name-nondirectory (substring file 0 (match-beginning 0))))
-   (t (file-name-nondirectory file))))
+   ((string-match "\\(\\?\\|%3[fF]\\)" file)
+    (url-file-nondirectory (substring file 0 (match-beginning 0))))
+   ((string-match ".*\\(?:/\\|%2[fF]\\)\\(.*\\)" file)
+    (match-string 1 file))
+   (t
+    "")))
 
 ;;;###autoload
 (defun url-parse-query-string (query &optional downcase allow-newlines)


^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#11981: 24.1.50; url-http-parse-headers should not disable file name handlers since it breaks auth-source
  2012-07-21 14:31       ` David Engster
@ 2012-08-06 16:01         ` Stefan Monnier
  2012-08-06 17:18           ` Jan Djärv
  0 siblings, 1 reply; 11+ messages in thread
From: Stefan Monnier @ 2012-08-06 16:01 UTC (permalink / raw)
  To: David Engster; +Cc: 11981, Michael Albinus

> @@ -246,18 +246,24 @@
>    "Return the directory part of FILE, for a URL."
>    (cond
>     ((null file) "")
> -   ((string-match "\\?" file)
> -    (file-name-directory (substring file 0 (match-beginning 0))))
> -   (t (file-name-directory file))))
> +   ((string-match "\\(\\?\\|%3[fF]\\)" file)
> +    (url-file-directory (substring file 0 (match-beginning 0))))

Should %3F really be treated like a ? (i.e. a special char that
delimits two different parts of a URL) rather than like "a normal ?
character encoded so as not to delimit two different parts of a URL"?


        Stefan





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#11981: 24.1.50; url-http-parse-headers should not disable file name handlers since it breaks auth-source
  2012-08-06 16:01         ` Stefan Monnier
@ 2012-08-06 17:18           ` Jan Djärv
  2012-08-12 14:52             ` David Engster
  0 siblings, 1 reply; 11+ messages in thread
From: Jan Djärv @ 2012-08-06 17:18 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: 11981, Michael Albinus, David Engster

Hello.

6 aug 2012 kl. 18:01 skrev Stefan Monnier:

>> @@ -246,18 +246,24 @@
>>   "Return the directory part of FILE, for a URL."
>>   (cond
>>    ((null file) "")
>> -   ((string-match "\\?" file)
>> -    (file-name-directory (substring file 0 (match-beginning 0))))
>> -   (t (file-name-directory file))))
>> +   ((string-match "\\(\\?\\|%3[fF]\\)" file)
>> +    (url-file-directory (substring file 0 (match-beginning 0))))
> 
> Should %3F really be treated like a ? (i.e. a special char that
> delimits two different parts of a URL) rather than like "a normal ?
> character encoded so as not to delimit two different parts of a URL"?
> 

A normal non-delimiting ?.  How else can one get a ? in to an URL?

	Jan D.






^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#11981: 24.1.50; url-http-parse-headers should not disable file name handlers since it breaks auth-source
  2012-08-06 17:18           ` Jan Djärv
@ 2012-08-12 14:52             ` David Engster
  2012-08-12 17:36               ` Stefan Monnier
  0 siblings, 1 reply; 11+ messages in thread
From: David Engster @ 2012-08-12 14:52 UTC (permalink / raw)
  To: Jan Djärv; +Cc: 11981, Michael Albinus

Jan Djärv writes:
> Hello.
>
> 6 aug 2012 kl. 18:01 skrev Stefan Monnier:
>
>>> @@ -246,18 +246,24 @@
>>>   "Return the directory part of FILE, for a URL."
>>>   (cond
>>>    ((null file) "")
>>> -   ((string-match "\\?" file)
>>> -    (file-name-directory (substring file 0 (match-beginning 0))))
>>> -   (t (file-name-directory file))))
>>> +   ((string-match "\\(\\?\\|%3[fF]\\)" file)
>>> +    (url-file-directory (substring file 0 (match-beginning 0))))
>> 
>> Should %3F really be treated like a ? (i.e. a special char that
>> delimits two different parts of a URL) rather than like "a normal ?
>> character encoded so as not to delimit two different parts of a URL"?
>> 
>
> A normal non-delimiting ?.  How else can one get a ? in to an URL?

You are both right, of course; I was overcompensating. The match for
%3[fF] must be removed.

-David





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#11981: 24.1.50; url-http-parse-headers should not disable file name handlers since it breaks auth-source
  2012-08-12 14:52             ` David Engster
@ 2012-08-12 17:36               ` Stefan Monnier
  2012-08-13 19:23                 ` David Engster
  0 siblings, 1 reply; 11+ messages in thread
From: Stefan Monnier @ 2012-08-12 17:36 UTC (permalink / raw)
  To: David Engster; +Cc: 11981, Michael Albinus

> You are both right, of course; I was overcompensating. The match for
> %3[fF] must be removed.

Thanks, I installed your patch with this part adjusted (as well as the
default case changed to better match file-name-(non)directory's
behavior).
Can we now re-enable file-name-handlers in url-http-parse-headers?


        Stefan





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#11981: 24.1.50; url-http-parse-headers should not disable file name handlers since it breaks auth-source
  2012-08-12 17:36               ` Stefan Monnier
@ 2012-08-13 19:23                 ` David Engster
  2012-08-14 14:55                   ` Stefan Monnier
  0 siblings, 1 reply; 11+ messages in thread
From: David Engster @ 2012-08-13 19:23 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: 11981, Michael Albinus

Stefan Monnier writes:
>> You are both right, of course; I was overcompensating. The match for
>> %3[fF] must be removed.
>
> Thanks, I installed your patch with this part adjusted (as well as the
> default case changed to better match file-name-(non)directory's
> behavior).
> Can we now re-enable file-name-handlers in url-http-parse-headers?

I think so. I re-enabled the file-name-handlers, created a test feed
with a colon in it

http://www.randomsample.de/foo:bar.xml

and I could subscribe to it with Gnus (the original URLs in Bug #6717
gave me http 405 errors).

-David





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#11981: 24.1.50; url-http-parse-headers should not disable file name handlers since it breaks auth-source
  2012-08-13 19:23                 ` David Engster
@ 2012-08-14 14:55                   ` Stefan Monnier
  0 siblings, 0 replies; 11+ messages in thread
From: Stefan Monnier @ 2012-08-14 14:55 UTC (permalink / raw)
  To: David Engster; +Cc: 11981-done, Michael Albinus

>> Can we now re-enable file-name-handlers in url-http-parse-headers?

> I think so. I re-enabled the file-name-handlers, created a test feed
> with a colon in it
> http://www.randomsample.de/foo:bar.xml
> and I could subscribe to it with Gnus (the original URLs in Bug #6717
> gave me http 405 errors).

OK, re-enabled, then, thank you,


        Stefan





^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2012-08-14 14:55 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-07-18 18:38 bug#11981: 24.1.50; url-http-parse-headers should not disable file name handlers since it breaks auth-source David Engster
2012-07-19  6:35 ` Michael Albinus
2012-07-19  8:55   ` Stefan Monnier
2012-07-19 12:51     ` David Engster
2012-07-21 14:31       ` David Engster
2012-08-06 16:01         ` Stefan Monnier
2012-08-06 17:18           ` Jan Djärv
2012-08-12 14:52             ` David Engster
2012-08-12 17:36               ` Stefan Monnier
2012-08-13 19:23                 ` David Engster
2012-08-14 14:55                   ` Stefan Monnier

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).