bug#44906: Substitute requests fail if URL has trailing slash

bug#44906: Substitute requests fail if URL has trailing slash

[Hartmut Goebel]

If the substitute-URL ends with a slash, api requests fail.

Expected behavior:

Substitute-URLs with and without trailing slash should behave the same.
This is especially true for substitute-URLs with empty path
("http://server") and path "/" (http://server/") - for which the RFCs
explicitly state to be equivalent.

According to RFC 7230, sec 2.7.3 "http and https URI Normalization and
Comparison" [1]:

   […] an empty
   path component is equivalent to an absolute path of "/", so the
   normal form is to provide a path of "/" instead.

[1] https://tools.ietf.org/html/rfc7230#section-2.7.3


How to reproduce:

no trailing slash:

$ guix weather --substitute-urls="https://ci.guix.gnu.org" gcc-toolchain
…
https://ci.guix.gnu.org
  100.0% substitutes available (3 out of 3)
…

Trailing slash:

$ guix weather --substitute-urls="https://ci.guix.gnu.org/" gcc-toolchain
…
https://ci.guix.gnu.org/
  0.0% substitutes available (0 out of 3)
…
  'https://ci.guix.gnu.org//api/queue?nr=1000' returned 400 ("Bad Request")

-- 
Regards
Hartmut Goebel

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

bug#44906: Substitute requests fail if URL has trailing slash

[zimoun]

Dear,

Thank you for the report.


Tweaking the function such as:

--8<---------------cut here---------------start------------->8---
(define (narinfo-request cache-url path)
  "Return an HTTP request for the narinfo of PATH at CACHE-URL."
  (let ((url (string-append cache-url "/" (store-path-hash-part path)
                            ".narinfo"))
        (headers '((User-Agent . "GNU Guile"))))
    (format #t "~%Narinfo request: ~a~%~%" url)
    (build-request (string->uri url) #:method 'GET #:headers headers)))
--8<---------------cut here---------------end--------------->8---

and removing the cache adequately, then running:

--8<---------------cut here---------------start------------->8---
./pre-inst-env guix weather \
     --substitute-urls="https://ci.guix.gnu.org/ https://ci.guix.gnu.org" \
     hello
computing 1 package derivations for x86_64-linux...

looking for 1 store items on https://ci.guix.gnu.org/...

Narinfo request: https://ci.guix.gnu.org//a462kby1q51ndvxdv3b6p0rsixxrgx1h.narinfo

updating substitutes from 'https://ci.guix.gnu.org/'... 100.0%
https://ci.guix.gnu.org/
  0.0% substitutes available (0 out of 1)

[...]

  'https://ci.guix.gnu.org//api/queue?nr=1000' returned 400 ("Bad Request")

looking for 1 store items on https://ci.guix.gnu.org...

Narinfo request: https://ci.guix.gnu.org/a462kby1q51ndvxdv3b6p0rsixxrgx1h.narinfo

updating substitutes from 'https://ci.guix.gnu.org'... 100.0%
https://ci.guix.gnu.org
  100.0% substitutes available (1 out of 1)

[...]

  at least 1,000 queued builds

[...]

  build rate: 36.89 builds per hour

[...]
--8<---------------cut here---------------end--------------->8---


On Fri, 27 Nov 2020 at 22:19, Hartmut Goebel <h.goebel@crazy-compilers.com> wrote:

> According to RFC 7230, sec 2.7.3 "http and https URI Normalization and
> Comparison" [1]:
>
>    […] an empty
>    path component is equivalent to an absolute path of "/", so the
>    normal form is to provide a path of "/" instead.
>
> [1] https://tools.ietf.org/html/rfc7230#section-2.7.3

Now, the question is where should the fix go?  “guix publish” exposing
the narinfos or “guix weather“?  Or both?


From my understanding, one fix should go to ‘guix publish’ exposing the
narinfos since:

   https://ci.guix.gnu.org//a462kby1q51ndvxdv3b6p0rsixxrgx1h.narinfo

should be a valid URL and return the narinfo file.  However, taking this
road, it means that the cache folder will not be the same:

~/.cache/guix/substitute/x2wcz6gz3evwlqcrz3fqstmezkfcfnpfb5kfyxbz7kjikc7upkiq/
~/.cache/guix/substitute/4refhwxbjmeua2kwg2nmzhv4dg4d3dorpjefq7kiciw2pfhaf26a/

https://ci.guix.gnu.org/ resp. https://ci.guix.gnu.org  Therefore, ‘guix
weather’ should be fixed too.


WDYT?

All the best,
simon

bug#44906: Substitute requests fail if URL has trailing slash

[Hartmut Goebel]

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

Am 28.11.20 um 00:37 schrieb zimoun:
> Now, the question is where should the fix go?  “guix publish” exposing
> the narinfos or “guix weather“?  Or both?

I propose fixing all places where string-append is used to join URLs, 
since joining URLs is not the same as string concatenation. We might 
restrict our algorithm to only joining a path. 
<https://tools.ietf.org/html/rfc3986#section-5.2.2> shows the complete 
algorithm, where this is the relevant part for only joining a path 
(R.path) to a base URL's path (T.path).

                if (R.path starts-with "/") then
                   T.path = remove_dot_segments(R.path);
                else
                   T.path = merge(Base.path, R.path);
                   T.path = remove_dot_segments(T.path);

(Side-node: guile module (web uri) 
<https://www.gnu.org/software/guile/manual/html_node/URIs.html> seems to 
lack respective, easy to use functions.)

-- 
Regards
Hartmut Goebel

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


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

bug#44906: Substitute requests fail if URL has trailing slash

[Ludovic Courtès]

Hi,

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

> I propose fixing all places where string-append is used to join URLs,
> since joining URLs is not the same as string concatenation. We might 
> restrict our algorithm to only joining a
> path. <https://tools.ietf.org/html/rfc3986#section-5.2.2> shows the
> complete algorithm, where this is the relevant part for only joining a
> path (R.path) to a base URL's path (T.path).
>
>                if (R.path starts-with "/") then
>                   T.path = remove_dot_segments(R.path);
>                else
>                   T.path = merge(Base.path, R.path);
>                   T.path = remove_dot_segments(T.path);

To begin with, we could define ‘url-append’ in (guix http-client), say,
and use it in (guix scripts substitute).

Eventually it would be nice to have that in (web uri).

Thoughts?

Ludo’.

bug#44906: Substitute requests fail if URL has trailing slash

[Mark H Weaver]

Hi,

Ludovic Courtès <ludo@gnu.org> writes:

> Hartmut Goebel <h.goebel@crazy-compilers.com> skribis:
>
>> I propose fixing all places where string-append is used to join URLs,
>> since joining URLs is not the same as string concatenation. We might 
>> restrict our algorithm to only joining a
>> path. <https://tools.ietf.org/html/rfc3986#section-5.2.2> shows the
>> complete algorithm, where this is the relevant part for only joining a
>> path (R.path) to a base URL's path (T.path).
>>
>>                if (R.path starts-with "/") then
>>                   T.path = remove_dot_segments(R.path);
>>                else
>>                   T.path = merge(Base.path, R.path);
>>                   T.path = remove_dot_segments(T.path);
>
> To begin with, we could define ‘url-append’ in (guix http-client), say,
> and use it in (guix scripts substitute).
>
> Eventually it would be nice to have that in (web uri).

Note that 'resolve-uri-reference' in (guix build download) implements
the algorithm specified in RFC 3986 section 5.2.2, for purposes of
supporting HTTP redirects.  Perhaps some of that code will be useful.

     Regards,
       Mark

bug#44906: [PATCH 0/3] Properly construct URLs if base-url has trailing slash

[Hartmut Goebel]

Here is now an attempt to solve the issue. It had to be fixed in
guix/substitutes.scm and guix/ci.scm only. In guix/scripts/publish.scm I did
not spot any place where wrong URLs are constructed.

Many thanks to Mark for pointing to 'resolve-uri-reference'.

Regarding CI: I did some tests, so these should work. Anyhow, I did not find a
tests-suite for fully testing this part.

Hartmut Goebel (3):
  substitute: Fix handling of short option "-h".
  substitutes: Properly construct URLs.
  ci: Properly construct URLs.

 guix/ci.scm                 | 79 +++++++++++++++++++++----------------
 guix/scripts/substitute.scm |  2 +-
 guix/substitutes.scm        | 13 +++---
 3 files changed, 55 insertions(+), 39 deletions(-)

-- 
2.30.2