[PATCH] web: authorization header scheme should be capitalized

[PATCH] web: authorization header scheme should be capitalized

[Aleix Conchillo Flaqué]

* module/web/http.scm (write-credentials): capitalize authorization
header scheme. See, for example,
https://datatracker.ietf.org/doc/html/rfc7617#section-2
---
 module/web/http.scm            | 4 ++--
 test-suite/tests/web-http.test | 7 ++++---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/module/web/http.scm b/module/web/http.scm
index 4276e1744..312c28934 100644
--- a/module/web/http.scm
+++ b/module/web/http.scm
@@ -965,10 +965,10 @@ as an ordered alist."
 (define (write-credentials val port)
   (match val
     (('basic . cred)
-     (put-string port "basic ")
+     (put-string port "Basic ")
      (put-string port cred))
     ((scheme . params)
-     (put-symbol port scheme)
+     (put-string port (string-titlecase (symbol->string scheme)))
      (put-char port #\space)
      (write-key-value-list params port))))
 
diff --git a/test-suite/tests/web-http.test b/test-suite/tests/web-http.test
index 63377349c..df25030de 100644
--- a/test-suite/tests/web-http.test
+++ b/test-suite/tests/web-http.test
@@ -336,9 +336,10 @@
   (pass-if-parse authorization "Digest foooo" '(digest foooo))
   (pass-if-parse authorization "Digest foo=bar,baz=qux"
                  '(digest (foo . "bar") (baz . "qux")))
-  (pass-if-round-trip "Authorization: basic foooo\r\n")
-  (pass-if-round-trip "Authorization: digest foooo\r\n")
-  (pass-if-round-trip "Authorization: digest foo=bar, baz=qux\r\n")
+  (pass-if-round-trip "Authorization: Basic foooo\r\n")
+  (pass-if-round-trip "Authorization: Bearer token\r\n")
+  (pass-if-round-trip "Authorization: Digest foooo\r\n")
+  (pass-if-round-trip "Authorization: Digest foo=bar, baz=qux\r\n")
   (pass-if-parse expect "100-continue, foo" '((100-continue) (foo)))
   (pass-if-parse from "foo@bar" "foo@bar")
   (pass-if-parse host "qux" '("qux" . #f))
-- 
2.34.1

Re: [PATCH] web: authorization header scheme should be capitalized

[Aleix Conchillo Flaqué]

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

Sorry, forgot to fix tests in my original email.

This is actually an important bug fix since some servers won't accept
lowercase Authorization header schemes and there's no way around this in
Guile, AFAIK.

Here are a few RFC where they explicitly mention capitalized strings:

"Basic" scheme: https://datatracker.ietf.org/doc/html/rfc7617#section-2
"Bearer" scheme: https://datatracker.ietf.org/doc/html/rfc6750#section-2.1

Aleix

On Thu, Jun 23, 2022 at 1:28 PM Aleix Conchillo Flaqué <aconchillo@gmail.com>
wrote:

> * module/web/http.scm (write-credentials): capitalize authorization
> header scheme. See, for example,
> https://datatracker.ietf.org/doc/html/rfc7617#section-2
> ---
>  module/web/http.scm            | 4 ++--
>  test-suite/tests/web-http.test | 7 ++++---
>  2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/module/web/http.scm b/module/web/http.scm
> index 4276e1744..312c28934 100644
> --- a/module/web/http.scm
> +++ b/module/web/http.scm
> @@ -965,10 +965,10 @@ as an ordered alist."
>  (define (write-credentials val port)
>    (match val
>      (('basic . cred)
> -     (put-string port "basic ")
> +     (put-string port "Basic ")
>       (put-string port cred))
>      ((scheme . params)
> -     (put-symbol port scheme)
> +     (put-string port (string-titlecase (symbol->string scheme)))
>       (put-char port #\space)
>       (write-key-value-list params port))))
>
> diff --git a/test-suite/tests/web-http.test
> b/test-suite/tests/web-http.test
> index 63377349c..df25030de 100644
> --- a/test-suite/tests/web-http.test
> +++ b/test-suite/tests/web-http.test
> @@ -336,9 +336,10 @@
>    (pass-if-parse authorization "Digest foooo" '(digest foooo))
>    (pass-if-parse authorization "Digest foo=bar,baz=qux"
>                   '(digest (foo . "bar") (baz . "qux")))
> -  (pass-if-round-trip "Authorization: basic foooo\r\n")
> -  (pass-if-round-trip "Authorization: digest foooo\r\n")
> -  (pass-if-round-trip "Authorization: digest foo=bar, baz=qux\r\n")
> +  (pass-if-round-trip "Authorization: Basic foooo\r\n")
> +  (pass-if-round-trip "Authorization: Bearer token\r\n")
> +  (pass-if-round-trip "Authorization: Digest foooo\r\n")
> +  (pass-if-round-trip "Authorization: Digest foo=bar, baz=qux\r\n")
>    (pass-if-parse expect "100-continue, foo" '((100-continue) (foo)))
>    (pass-if-parse from "foo@bar" "foo@bar")
>    (pass-if-parse host "qux" '("qux" . #f))
> --
> 2.34.1
>
>

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

Re: [PATCH] web: authorization header scheme should be capitalized

[Maxime Devos]

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

Aleix Conchillo Flaqué schreef op do 23-06-2022 om 13:27 [-0700]:
> +     (put-string port (string-titlecase (symbol->string scheme)))

I'd add a little explanation in a comment (e.g.:

   ;; While according to RFC 7617 Schemes are case-insensitive:
   ;;
   ;; ‘Note that both scheme and parameter names are matched
   ;; case-insensitive’
   ;;
   ;; some software (*) incorrectly assumes title case for scheme
   ;; names, so use the more titlecase.
   ;;
   ;; (*): See, e.g.,
   ;; <https://[bug report 1]/>
   ;; <https://[bug report 2]/>
   
I think it's reasonable to do some changes in Guile to work-around
potential bugs in other software that Guile has no control over or even
knows about, at the same time Guile seems to be just following the
spec, the compatibility bug seems to be in the other software, so to
help the other software a bit, I think it would be best to report
things in the buggy software too.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

Re: [PATCH] web: authorization header scheme should be capitalized

[Aleix Conchillo Flaqué]

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

On Thu, Jun 23, 2022 at 1:40 PM Maxime Devos <maximedevos@telenet.be> wrote:

> Aleix Conchillo Flaqué schreef op do 23-06-2022 om 13:27 [-0700]:
> > +     (put-string port (string-titlecase (symbol->string scheme)))
>
> I'd add a little explanation in a comment (e.g.:
>
>    ;; While according to RFC 7617 Schemes are case-insensitive:
>    ;;
>    ;; ‘Note that both scheme and parameter names are matched
>    ;; case-insensitive’
>    ;;
>    ;; some software (*) incorrectly assumes title case for scheme
>    ;; names, so use the more titlecase.
>    ;;
>    ;; (*): See, e.g.,
>    ;; <https://[bug report 1]/>
>    ;; <https://[bug report 2]/>
>
> I think it's reasonable to do some changes in Guile to work-around
> potential bugs in other software that Guile has no control over or even
> knows about, at the same time Guile seems to be just following the
> spec, the compatibility bug seems to be in the other software, so to
> help the other software a bit, I think it would be best to report
> things in the buggy software too.
>
>
>
I think you are right. The spec says clearly that it should be case
insensitive.

So, disregard this patch.

Aleix

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

Re: [PATCH] web: authorization header scheme should be capitalized

[Maxime Devos]

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

Aleix Conchillo Flaqué schreef op do 23-06-2022 om 13:42 [-0700]:
> 
> [...]
> I think you are right. The spec says clearly that it should be case
> insensitive.
> 
> So, disregard this patch.

TBC, did you encounter problems in the wild that made you write the
patch?

Also, there's still a potential patch to be had, e.g. you could add a
test checking that Guile properly supports schemes in other cases (if
not done already).

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

Re: [PATCH] web: authorization header scheme should be capitalized

[Aleix Conchillo Flaqué]

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

On Thu, Jun 23, 2022 at 1:45 PM Maxime Devos <maximedevos@telenet.be> wrote:

> Aleix Conchillo Flaqué schreef op do 23-06-2022 om 13:42 [-0700]:
> >
> > [...]
> > I think you are right. The spec says clearly that it should be case
> > insensitive.
> >
> > So, disregard this patch.
>
> TBC, did you encounter problems in the wild that made you write the
> patch?
>
>
Yes, and I just posted it here:

https://community.spotify.com/t5/Spotify-for-Developers/API-Authorization-header-doesn-t-follow-HTTP-spec/m-p/5397381#M4917

> Also, there's still a potential patch to be had, e.g. you could add a
> test checking that Guile properly supports schemes in other cases (if
> not done already).
>
>
What do you mean?

Aleix

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

Re: [PATCH] web: authorization header scheme should be capitalized

[Maxime Devos]

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

Aleix Conchillo Flaqué schreef op do 23-06-2022 om 14:13 [-0700]:
> https://community.spotify.com/t5/Spotify-for-Developers/API-Authorization-header-doesn-t-follow-HTTP-spec/m-p/5397381#M4917
> > Also, there's still a potential patch to be had, e.g. you could add
> > a test checking that Guile properly supports schemes in other cases
> > (if not done already).
> 
> What do you mean?

Even if there is nothing that _has_ to be done in Guile, there's still
thing that _can_ be done in Guile to improve Guile's test suite -- in
this case, a test in the test suite that the Guile's web code
understands both lowercase and uppercase and titlecase authorisation
schemes.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

Re: [PATCH] web: authorization header scheme should be capitalized

[Aleix Conchillo Flaqué]

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

On Thu, Jun 23, 2022 at 3:20 PM Maxime Devos <maximedevos@telenet.be> wrote:

> Aleix Conchillo Flaqué schreef op do 23-06-2022 om 14:13 [-0700]:
> >
> https://community.spotify.com/t5/Spotify-for-Developers/API-Authorization-header-doesn-t-follow-HTTP-spec/m-p/5397381#M4917
> > > Also, there's still a potential patch to be had, e.g. you could add
> > > a test checking that Guile properly supports schemes in other cases
> > > (if not done already).
> >
> > What do you mean?
>
> Even if there is nothing that _has_ to be done in Guile, there's still
> thing that _can_ be done in Guile to improve Guile's test suite -- in
> this case, a test in the test suite that the Guile's web code
> understands both lowercase and uppercase and titlecase authorisation
> schemes.
>
>
Ah, got it. Yes, that would make sense.

I was thinking about it again. I know that Guile complies with the standard
but since, I would say, capitalized schemes is what most libraries use,
would it make sense to switch to that? I don't really expect big companies
to fix this kind of stuff fast and in the meantime we can't use Guile for
certain things. I have to say I've never seen lowercase Authorization
header schemes.

Best,

Aleix

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

Re: [PATCH] web: authorization header scheme should be capitalized

[Maxime Devos]

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

Aleix Conchillo Flaqué schreef op do 23-06-2022 om 18:46 [-0700]:
> Ah, got it. Yes, that would make sense.
> 
> I was thinking about it again. I know that Guile complies with the
> standard but since, I would say, capitalized schemes is what most
> libraries use, would it make sense to switch to that? I don't really
> expect big companies to fix this kind of stuff fast and in the
> meantime we can't use Guile for certain things. I have to say I've
> never seen lowercase Authorization header schemes.

As long as we remember to _also_ report the bug in the buggy
‘consumers’ of the header (in this case, those big companies) and
remember that lower-case isn't buggy, switching to the more
conventional title-case seems sensible to me.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

Re: [PATCH] web: authorization header scheme should be capitalized

[Aleix Conchillo Flaqué]

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

On Fri, Jun 24, 2022 at 1:35 AM Maxime Devos <maximedevos@telenet.be> wrote:

> Aleix Conchillo Flaqué schreef op do 23-06-2022 om 18:46 [-0700]:
> > Ah, got it. Yes, that would make sense.
> >
> > I was thinking about it again. I know that Guile complies with the
> > standard but since, I would say, capitalized schemes is what most
> > libraries use, would it make sense to switch to that? I don't really
> > expect big companies to fix this kind of stuff fast and in the
> > meantime we can't use Guile for certain things. I have to say I've
> > never seen lowercase Authorization header schemes.
>
> As long as we remember to _also_ report the bug in the buggy
> ‘consumers’ of the header (in this case, those big companies) and
> remember that lower-case isn't buggy, switching to the more
> conventional title-case seems sensible to me.
>
>
Great. I'll re-word the patch then and add your suggested additional tests.

Thank you!

Aleix

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

Re: [PATCH] web: authorization header scheme should be capitalized

[Dr. Arne Babenhauserheide]

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


Aleix Conchillo Flaqué <aconchillo@gmail.com> writes:

> On Thu, Jun 23, 2022 at 3:20 PM Maxime Devos <maximedevos@telenet.be> wrote:
>
>  Aleix Conchillo Flaqué schreef op do 23-06-2022 om 14:13 [-0700]:
>  > https://community.spotify.com/t5/Spotify-for-Developers/API-Authorization-header-doesn-t-follow-HTTP-spec/m-p/5397381#M4917
>  > > Also, there's still a potential patch to be had, e.g. you could add
>  > > a test checking that Guile properly supports schemes in other cases
>  > > (if not done already).
>  > 
>  > What do you mean?
>
>  Even if there is nothing that _has_ to be done in Guile, there's still
>  thing that _can_ be done in Guile to improve Guile's test suite -- in
>  this case, a test in the test suite that the Guile's web code
>  understands both lowercase and uppercase and titlecase authorisation
>  schemes.
>
> Ah, got it. Yes, that would make sense.
>
> I was thinking about it again. I know that Guile complies with the standard but since, I would say, capitalized schemes is what most libraries use, would
> it make sense to switch to that? I don't really expect big companies to fix this kind of stuff fast and in the meantime we can't use Guile for certain
> things. I have to say I've never seen lowercase Authorization header schemes.

I think that it makes sense to ensure that Guile works with other
libraries. It’s this kind of compatibility code that makes the
difference between a tool that’s good in theory and one that works in
practice.

The robustness principle applies here:¹ Be lenient in what you accept
and strict in what you send — sending the header in lowercase requires
others to be lenient which cannot work.

Best wishes,
Arne

¹: While the robustness principle can be harmful when you’re the one
   mantaining the spec, because it can prevent required fixes in the
   spec (<https://www.ietf.org/archive/id/draft-iab-protocol-maintenance-05.html>),
   it applies here, because we cannot change the spec or what others
   accept.
- 
Unpolitisch sein
heißt politisch sein,
ohne es zu merken.
draketo.de

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 1125 bytes --]

[PATCH] web: authorization header scheme should be capitalized

[Aleix Conchillo Flaqué]

* module/web/http.scm (write-credentials): capitalize authorization
header scheme. See, for example,
https://datatracker.ietf.org/doc/html/rfc7617#section-2
---
 module/web/http.scm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/module/web/http.scm b/module/web/http.scm
index 4276e1744..312c28934 100644
--- a/module/web/http.scm
+++ b/module/web/http.scm
@@ -965,10 +965,10 @@ as an ordered alist."
 (define (write-credentials val port)
   (match val
     (('basic . cred)
-     (put-string port "basic ")
+     (put-string port "Basic ")
      (put-string port cred))
     ((scheme . params)
-     (put-symbol port scheme)
+     (put-string port (string-titlecase (symbol->string scheme)))
      (put-char port #\space)
      (write-key-value-list params port))))
 
-- 
2.34.1