[bug#36976] [PATCH 1/1] download: Map file-name characters not allowed in store.

[bug#36976] [PATCH 1/1] download: Map file-name characters not allowed in store.

[Hartmut Goebel]

In the file-name to be used for storing into the store, replace any
character not allowed in the store-file-name by an underscore.
This is only done when NAME is not given and thus defaults to
toe URL's basename. If NAME is given, this is used unchanged,
allowing for more control by other functions.

Fixes <http://issues.guix.gnu.org/issue/26175>

* guix/download.scm (safe-name): New function.
  (download-to-store): NAME defaults to the "safe" basename of URL.
---
 guix/download.scm | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/guix/download.scm b/guix/download.scm
index b24aaa0a86..249f612237 100644
--- a/guix/download.scm
+++ b/guix/download.scm
@@ -6,6 +6,7 @@
 ;;; Copyright © 2016 David Craven <david@craven.ch>
 ;;; Copyright © 2017 Tobias Geerinckx-Rice <me@tobias.gr>
 ;;; Copyright © 2019 Guy Fleury Iteriteka <hoonandon@gmail.com>
+;;; Copyright © 2019 Hartmut Goeel <h.goebel@crazy-compilers.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -563,13 +564,27 @@ own.  This helper makes it easier to deal with \"zip bombs\"."
                       #:graft? #f
                       #:local-build? #t)))
 
-(define* (download-to-store store url #:optional (name (basename url))
+(define (safe-name name)
+  "Replace any character not allowed in a stroe name by an underscore."
+
+  (define valid-characters
+    ;; according to nix/libstore/store-api.cc
+    (string->list (string-append "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+                                 "abcdefghijklmnopqrstuvwxyz"
+                                 "0123456789" "+-._?=")))
+  (string-map (lambda (c)
+                (if (member c valid-characters) c #\_))
+              name))
+
+(define* (download-to-store store url
+                            #:optional (name (safe-name (basename url)))
                             #:key (log (current-error-port)) recursive?
                             (verify-certificate? #t))
-  "Download from URL to STORE, either under NAME or URL's basename if
-omitted.  Write progress reports to LOG.  RECURSIVE? has the same effect as
-the same-named parameter of 'add-to-store'.  VERIFY-CERTIFICATE? determines
-whether or not to validate HTTPS server certificates."
+  "Download from URL to STORE, either under NAME. If NAME is omitted, URL's
+basename with invalid characters replaced is used.  Write progress reports to
+LOG.  RECURSIVE? has the same effect as the same-named parameter of
+'add-to-store'.  VERIFY-CERTIFICATE? determines whether or not to validate
+HTTPS server certificates."
   (define uri
     (string->uri url))
 
-- 
2.21.0

[bug#36976] [PATCH 1/1] download: Map file-name characters not allowed in store.

[Ludovic Courtès]

Hello,

Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:

> In the file-name to be used for storing into the store, replace any
> character not allowed in the store-file-name by an underscore.
> This is only done when NAME is not given and thus defaults to
> toe URL's basename. If NAME is given, this is used unchanged,
> allowing for more control by other functions.
>
> Fixes <http://issues.guix.gnu.org/issue/26175>
>
> * guix/download.scm (safe-name): New function.
>   (download-to-store): NAME defaults to the "safe" basename of URL.

What about moving this automatic renaming feature to (guix scripts
download)?  I’d rather not do it in the core APIs.

> +(define (safe-name name)
> +  "Replace any character not allowed in a stroe name by an underscore."
                                               ^^
Typo.

I’d call it ‘ensure-valid-store-file-name’ or similar, WDYT?

> +  (define valid-characters
> +    ;; according to nix/libstore/store-api.cc
> +    (string->list (string-append "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
> +                                 "abcdefghijklmnopqrstuvwxyz"
> +                                 "0123456789" "+-._?=")))
> +  (string-map (lambda (c)
> +                (if (member c valid-characters) c #\_))
> +              name))

Instead of a list, please use a SRFI-14 “character set”, like this:

  (define valid
    (string->char-set …))

  (string-map (lambda (c)
                (if (char-set-contains? valid c) …))
              name)

Thanks,
Ludo’.

[bug#36976] [PATCH 1/1] download: Map file-name characters not allowed in store.

[Hartmut Goebel]

Hi,

Thanks for the review and the coding suggestions..

Am 23.08.19 um 23:08 schrieb Ludovic Courtès:
>> * guix/download.scm (safe-name): New function.
>>   (download-to-store): NAME defaults to the "safe" basename of URL.
> What about moving this automatic renaming feature to (guix scripts
> download)?  I’d rather not do it in the core APIs.
`download-to-store store` was defined as:

   (define* (download-to-store store url #:optional (name (basename url)) …

When developing this patch, I decided to put it into the core since
users of this function would expect to be allowed to just pass any url
and don't need to take care about valid characters. If not doing the
automatic renaming here, users would need to perform the conversion
prior to calling this function in any case (except when 100% sure only
valid characters are used).

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

[bug#36976] [PATCH 1/1] download: Map file-name characters not allowed in store.

[Ludovic Courtès]

Hello,

Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:

> Thanks for the review and the coding suggestions..
>
> Am 23.08.19 um 23:08 schrieb Ludovic Courtès:
>>> * guix/download.scm (safe-name): New function.
>>>   (download-to-store): NAME defaults to the "safe" basename of URL.
>> What about moving this automatic renaming feature to (guix scripts
>> download)?  I’d rather not do it in the core APIs.
> `download-to-store store` was defined as:
>
>    (define* (download-to-store store url #:optional (name (basename url)) …
>
> When developing this patch, I decided to put it into the core since
> users of this function would expect to be allowed to just pass any url
> and don't need to take care about valid characters. If not doing the
> automatic renaming here, users would need to perform the conversion
> prior to calling this function in any case (except when 100% sure only
> valid characters are used).

Yes, but that’s OK to me: IMO, procedures have to focused on one thing;
users can perform additional processing beforehand if they need it.

Conversely, commands have to do the right thing by default, which is why
I agree that ‘guix download’ should rename automatically when needed.

How does that sound?

Ludo’.

bug#26175: [bug#36976] [PATCH 1/1] download: Map file-name characters not allowed in store.

[Hartmut Goebel]

Hi Ludo,

while http://issues.guix.gnu.org/issue/36976 is going to solve the issue
for "guix download", I found that there are other cases where invalid
characters in store names appear. Thus we need a more elaborate solution
- or several solutions.

Any suggestions which cases to check and how to fix them?


E.g. "@" and "%" are not allowed in package source base names: When
building the package below, which used the "offending URL, yields an error:

guix build: error: invalid character `@' in name
`kde-l10n-ca@valencia-14.11.80.tar.xz.drv'

Same when trying to work around this be using "…%40…".

(use-modules (guix packages) (guix download) (guix build-system gnu))

(package
  (name "kde-l10n-ca-valencia")
  (version "14.11.80")
  (source
   (origin
     (method url-fetch)
     (uri (string-append "mirror://kde//Attic/applications/"
                         version "/src/kde-l10n/"
                         "kde-l10n-ca@valencia-" version ".tar.xz"))
     (sha256 (base32
"1mqadassxcm0m9r1l02m5vr4bbandn48xz8gifvxmb4wiz8i8d0w"))))
  (build-system gnu-build-system)
  (synopsis "") (description "") (license "") (home-page ""))

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

bug#26175: [bug#36976] [PATCH 1/1] download: Map file-name characters not allowed in store.

[Ludovic Courtès]

Hi,

Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:

>    (origin
>      (method url-fetch)
>      (uri (string-append "mirror://kde//Attic/applications/"
>                          version "/src/kde-l10n/"
>                          "kde-l10n-ca@valencia-" version ".tar.xz"))

In this case just add a ‘file-name’ field.  I think that’s a reasonable
expectation.

Thanks,
Ludo’.

bug#26175: [bug#36976] [PATCH 1/1] download: Map file-name characters not allowed in store.

[Hartmut Goebel]

Am 04.09.19 um 12:32 schrieb Ludovic Courtès:
> Hi,
>
> Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:
>
>>    (origin
>>      (method url-fetch)
>>      (uri (string-append "mirror://kde//Attic/applications/"
>>                          version "/src/kde-l10n/"
>>                          "kde-l10n-ca@valencia-" version ".tar.xz"))
> In this case just add a ‘file-name’ field.  I think that’s a reasonable
> expectation.
>
> Thanks,
> Ludo’.

Agreed. WDYT about adding this as a hint when the error shows up?

How can I catch the "error: invalid character `@' in name" in guix build?

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

bug#26175: [bug#36976] [PATCH 1/1] download: Map file-name characters not allowed in store.

[Ludovic Courtès]

Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:

> Am 04.09.19 um 12:32 schrieb Ludovic Courtès:
>> Hi,
>>
>> Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:
>>
>>>    (origin
>>>      (method url-fetch)
>>>      (uri (string-append "mirror://kde//Attic/applications/"
>>>                          version "/src/kde-l10n/"
>>>                          "kde-l10n-ca@valencia-" version ".tar.xz"))
>> In this case just add a ‘file-name’ field.  I think that’s a reasonable
>> expectation.
>>
>> Thanks,
>> Ludo’.
>
> Agreed. WDYT about adding this as a hint when the error shows up?
>
> How can I catch the "error: invalid character `@' in name" in guix build?

Unfortunately it cannot really be caught.  I mean, you could catch
‘&store-protocol-error’ error conditions, but then the error message is
just a string, there’s no error code you can compare against.

Ludo’.

bug#26175: [bug#36976] [PATCH 1/1] download: Map file-name characters not allowed in store.

[Hartmut Goebel]

Done, see dec845606d2d184da31065fa26cd951b84b3ce2d and
<http://issues.guix.gnu.org/issue/36976#5>

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

bug#36976: [PATCH 1/1] download: Map file-name characters not allowed in store.

[Hartmut Goebel]

Committed as dec845606d2d184da31065fa26cd951b84b3ce2d

Thank for the review.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

[bug#36976] [Patch v2] guix download: Ensure destination file-name is valid in the store.

[Hartmut Goebel]

Avoid invalid store-file-name by explicitly passing the destination
name, replacing any character not allowed in the store-file-name by an
underscore.

Fixes <http://issues.guix.gnu.org/issue/26175>

* guix/scripts/download.scm (safe-naensure-valid-store-file-nameme):
  New function. (download-to-store*): Use it to generate a "safe"
  basename of URL.
---
 guix/scripts/download.scm | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/guix/scripts/download.scm b/guix/scripts/download.scm
index d8fe71ce12..22cd75ea0b 100644
--- a/guix/scripts/download.scm
+++ b/guix/scripts/download.scm
@@ -33,6 +33,7 @@
   #:use-module (web uri)
   #:use-module (ice-9 match)
   #:use-module (srfi srfi-1)
+  #:use-module (srfi srfi-14)
   #:use-module (srfi srfi-26)
   #:use-module (srfi srfi-37)
   #:use-module (rnrs bytevectors)
@@ -54,9 +55,23 @@
        (url-fetch url file #:mirrors %mirrors)))
     file))
 
+(define (ensure-valid-store-file-name name)
+  "Replace any character not allowed in a stror name by an underscore."
+
+  (define valid
+    ;; according to nix/libstore/store-api.cc
+    (string->char-set (string-append "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+                                     "abcdefghijklmnopqrstuvwxyz"
+                                     "0123456789" "+-._?=")))
+  (string-map (lambda (c)
+                (if (char-set-contains? valid c) c #\_))
+              name))
+
+
 (define* (download-to-store* url #:key (verify-certificate? #t))
   (with-store store
     (download-to-store store url
+                       (ensure-valid-store-file-name (basename url))
                        #:verify-certificate? verify-certificate?)))
 
 (define %default-options
-- 
2.21.0