1419658 - Change Basic authentication request header username and password character encoding to UTF-8 (used to be ISO-8859-1)

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement, P3)

Description

[Olivier]

Access a website with basic auth, using login "é" and password "é"

Firefox sends this: `Authorization Basic 6Trp`

Curl it:

`curl -k -u é:é -iv http://fubar`

Gives: `Authorization: Basic w6k6w6k=`

Trying with anything outside of latin1 (eg: ∞) will of course give similar results.

Understood there is some (quite old) background discussion over here: https://bugzilla.mozilla.org/show_bug.cgi?id=436033

At this point in time though, the rest of the ecosystem (Chrome, curl, nginx, and likely others) have settled on utf8.

Comment 1

[Olivier]

For the record, embedding the login:password into the url (`é:é@fubar`) gives the expected result.

Sounds like the prompt encoding is messed-up.

Comment 2

[Honza Bambas (:mayhemer)]

I think this is duplicate of bug 41489.

Comment 3

[Olivier]

Mmmm... not sure...

There seem to be two bugs here:
 * the fact that firefox treats url-embedded-credentials and prompt-credentials differently is one issue
 * how firefox should treat the input is another (<- that's probably the "dupe" part?)

Comment 4

[Olivier]

About the component (Networking:HTTP), not sure either.
Since url-embedded credentials are working fine, it feels more like a firefox issue with the way the prompt is handled?

Oh well, you know best, I'm just guessing :-)

Comment 5

[Junior [inactive]]

Hello njn,
Do we have base64 encoding which doesn't drop high bits?
I find [1] only :(


[1] https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/xpcom/io/Base64.h#32-34

Comment 6

[Junior [inactive]]

Attachment: BaseAuth16bits, WIPv1

Comment 7

[Junior [inactive]]

Digest should be with issue, too.
https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/netwerk/protocol/http/nsHttpDigestAuth.cpp#317

Comment 8

[Nicholas Nethercote [inactive]]

> Do we have base64 encoding which doesn't drop high bits?
> I find [1] only :(
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 7a8c667bdd2a4a32746c9862356e199627c0896d/xpcom/io/Base64.h#32-34

That's the only one I know of. erahm, do you know any more about this?

Comment 9

[Mark Nottingham]

See also the "charset" param:
  http://httpwg.org/specs/rfc7617.html#rfc.section.2.1

Comment 10

[Junior [inactive]]

(In reply to mnot from comment #9)
> See also the "charset" param:
>   http://httpwg.org/specs/rfc7617.html#rfc.section.2.1

Thanks for your input.
Obviously we're not supporting charset now.

This reminds me to check the spec for default charset now.
It's 8-bits char set with the common sense ISO-8859-1, including é

Comment 11

[Eric Rahm [:erahm]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> > Do we have base64 encoding which doesn't drop high bits?
> > I find [1] only :(
> > 
> > [1]
> > https://searchfox.org/mozilla-central/rev/
> > 7a8c667bdd2a4a32746c9862356e199627c0896d/xpcom/io/Base64.h#32-34
> 
> That's the only one I know of. erahm, do you know any more about this?

The "binary versions" (input stream and char*) [1] don't mess with high bits. The one you referenced *does* drop the high bits.

[1] https://searchfox.org/mozilla-central/rev/ba2b0cf4d16711d37d4bf4d267b187c9a27f6638/xpcom/io/Base64.h#16-30

Comment 12

[Junior [inactive]]

(In reply to Junior[:junior] from comment #7)
> Digest should be with issue, too.
> https://searchfox.org/mozilla-central/rev/
> 7a8c667bdd2a4a32746c9862356e199627c0896d/netwerk/protocol/http/
> nsHttpDigestAuth.cpp#317

Spec only specifies that we need to support 8 bits. I guess Digest auth is good here.

Comment 13

[Junior [inactive]]

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #11)
> (In reply to Nicholas Nethercote [:njn] from comment #8)
> > > Do we have base64 encoding which doesn't drop high bits?
> > > I find [1] only :(
> > > 
> > > [1]
> > > https://searchfox.org/mozilla-central/rev/
> > > 7a8c667bdd2a4a32746c9862356e199627c0896d/xpcom/io/Base64.h#32-34
> > 
> > That's the only one I know of. erahm, do you know any more about this?
> 
> The "binary versions" (input stream and char*) [1] don't mess with high
> bits. The one you referenced *does* drop the high bits.
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> ba2b0cf4d16711d37d4bf4d267b187c9a27f6638/xpcom/io/Base64.h#16-30

It works well, thanks!

Comment 14

[Junior [inactive]]

Attachment: basicAuth8bits, v2

not sure if we need to bump the kAllKnownFlags

Comment 15

[Honza Bambas (:mayhemer)]

Please rather consult patches in bug 41489.  Those would be nice to get finished.  It's IMO not that simple as to just convert to UTF-8 all the time.  An extension to the request has been specified for this.

Comment 16

[Junior [inactive]]

Did you mean the "charset" mentioned by comment 9?
Now we're not support this extension now.
I could also take bug 41489.
Good to know patches are already there.

On the other hand, we should support ISO-8859-1 (i.e., printable character in 128-255)
no matter the charset is specified or not.

The patch in bug 41489 doesn't cover this.
My idea is to land this first and to rebase after.

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to Junior[:junior] from comment #16)
> Did you mean the "charset" mentioned by comment 9?
> Now we're not support this extension now.

Bug 41489 is trying to impl that.

> I could also take bug 41489.

it would be great!  unfortunately the contributor stopped responding completely (there were always things to fix and me and him both got frustrated from the everlasting pingpong)

> Good to know patches are already there.
> 
> On the other hand, we should support ISO-8859-1 (i.e., printable character
> in 128-255)
> no matter the charset is specified or not.

I think it's backward compat.  OTOH a nice prop of utf8 is that it is 7bit ascii when you have 7bit chars in the source you convert from.

> 
> The patch in bug 41489 doesn't cover this.

why exactly?  I though it was more like a universal thing.

> My idea is to land this first and to rebase after.

maybe this patch will fix most of the cases for basic.  would be good to update digest as well, but it way less used, so lesser impact.

thanks!

Comment 18

[Dragana Damjanovic [:dragana]]

Comment on attachment 8934882 [details] [diff] [review]
basicAuth8bits, v2

I will let Honza review this.

Comment 19

[Junior [inactive]]

> > Good to know patches are already there.
> > 
> > On the other hand, we should support ISO-8859-1 (i.e., printable character
> > in 128-255)
> > no matter the charset is specified or not.
> 
> I think it's backward compat.  OTOH a nice prop of utf8 is that it is 7bit
> ascii when you have 7bit chars in the source you convert from.
> 
> > 
> > The patch in bug 41489 doesn't cover this.
> 
> why exactly?  I though it was more like a universal thing.
> 
for example:
(1) |WWW-Authenticate: "Basic realm=foo"| indicates ISO-8859-1 charset.
We need to support ascii and those non-ascii in 128-255 like 'é'.

(2) And |WWW-Authenticate: "Basic realm=foo charset=UTF-8" indicates UTF-8 set.
UTF-8 supports >2000 chars like '∞'

This bug implements (1) and bug 41489 implements (2)
i.e., with patches in bug 41489, we still don't support 'é' with |WWW-Authenticate: "Basic realm=foo"| 

> > My idea is to land this first and to rebase after.
> 
> maybe this patch will fix most of the cases for basic.  would be good to
> update digest as well, but it way less used, so lesser impact.
> 
> thanks!

I didn't test, but there's no downgrade from UTF-8 to ascii in [1]
I believe Digest is fine with charset is not specified.

[1] https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/netwerk/protocol/http/nsHttpDigestAuth.cpp#182-393

Comment 20

[Honza Bambas (:mayhemer)]

(In reply to Junior[:junior] from comment #19)
> for example:
> (1) |WWW-Authenticate: "Basic realm=foo"| indicates ISO-8859-1 charset.
> We need to support ascii and those non-ascii in 128-255 like 'é'.
> 
> (2) And |WWW-Authenticate: "Basic realm=foo charset=UTF-8" indicates UTF-8
> set.
> UTF-8 supports >2000 chars like '∞'
> 
> This bug implements (1) 

No.  This presumes "Basic realm=foo charset=UTF-8" has been sent by the server.

> and bug 41489 implements (2)
> i.e., with patches in bug 41489, we still don't support 'é' with
> |WWW-Authenticate: "Basic realm=foo"| 

Yeah.  And that is IMO correct.  That is exactly the reason why servers should send the extension and explicitly say which charset they expect the username and password be at.  The default is, unless I'm mistaken, still ISO-8859-1.

If other major browsers are these days defaulting to UTF-8 without the "charset=UTF-8" response header extension, then we can take this patch and probably close 41489 as invalid (after making the same default change for digest)

(In reply to Olivier from comment #3)
> Mmmm... not sure...
> 
> There seem to be two bugs here:
>  * the fact that firefox treats url-embedded-credentials and
> prompt-credentials differently is one issue
>  * how firefox should treat the input is another (<- that's probably the
> "dupe" part?)

This is a good point.  We accept url specified creds as UTF-8 probably because we parse URLs as UTF-8 by default.  Question is how we will behave when the charset for a URL is different (specified in markup, and use clicking a link with credentials)  Asking Valentin on these bits (we may have removed charset specification from our url APIs and assume utf8).  That would be a good argument to accept patch for this bug.

Comment 21

[Junior [inactive]]

(In reply to Honza Bambas (:mayhemer) from comment #20)
> (In reply to Junior[:junior] from comment #19)
> > for example:
> > (1) |WWW-Authenticate: "Basic realm=foo"| indicates ISO-8859-1 charset.
> > We need to support ascii and those non-ascii in 128-255 like 'é'.
> > 
> > (2) And |WWW-Authenticate: "Basic realm=foo charset=UTF-8" indicates UTF-8
> > set.
> > UTF-8 supports >2000 chars like '∞'
> > 
> > This bug implements (1) 
> 
> No.  This presumes "Basic realm=foo charset=UTF-8" has been sent by the
> server.
> 
Mm... You remind me that this patch supports more than ISO-8859-1 
> > and bug 41489 implements (2)
> > i.e., with patches in bug 41489, we still don't support 'é' with
> > |WWW-Authenticate: "Basic realm=foo"| 
> 
> Yeah.  And that is IMO correct.  That is exactly the reason why servers
> should send the extension and explicitly say which charset they expect the
> username and password be at.  The default is, unless I'm mistaken, still
> ISO-8859-1.
> 
> If other major browsers are these days defaulting to UTF-8 without the
> "charset=UTF-8" response header extension, then we can take this patch and
> probably close 41489 as invalid (after making the same default change for
> digest)

charset only allowed UTF-8 now [1] (we can't specify the charset is default or ISO-8859-1)
If charset is not set, it is ISO-8859-1 (spec specifies 8-bits char, and ISO-8859-1 is kinda consensus)

I'm not an auth master, but I never see the charset :p
Chrome seems use UTF-8 as the default [2]

At least we can exploit the tests in bug 41489 

[1] http://httpwg.org/specs/rfc7617.html#rfc.section.2.1
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=436033#c6

Comment 22

[Honza Bambas (:mayhemer)]

Thanks for the summary.  Since this bug is going around on and on and we don't do anything about it because we are scared to break the web, let's do it and change the default to UTF-8 and see what happens.  This should be made public on the right forums, tho.

Comment 23

[Honza Bambas (:mayhemer)]

Comment on attachment 8934882 [details] [diff] [review]
basicAuth8bits, v2

Review of attachment 8934882 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpBasicAuth.cpp
@@ +86,5 @@
>      // we only know how to deal with Basic auth for http.
>      bool isBasicAuth = !PL_strncasecmp(challenge, "basic", 5);
>      NS_ENSURE_TRUE(isBasicAuth, NS_ERROR_UNEXPECTED);
>  
> +    // we work with UTF8 around here

UTF-8

@@ +93,3 @@
>      userpass.Append(':'); // always send a ':' (see bug 129565)
>      if (password)
> +        AppendUTF16toUTF8(password, userpass);

{ } please

@@ +96,3 @@
>  
> +    nsAutoCString authString;
> +    NS_ENSURE_SUCCESS(Base64Encode(userpass, authString), NS_ERROR_FAILURE);

go via rv please or do if (NS_FAILED(..) ..

Comment 24

[Valentin Gosu [:valentin] (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #20)
> This is a good point.  We accept url specified creds as UTF-8 probably
> because we parse URLs as UTF-8 by default.  Question is how we will behave
> when the charset for a URL is different (specified in markup, and use
> clicking a link with credentials)  Asking Valentin on these bits (we may
> have removed charset specification from our url APIs and assume utf8).  That
> would be a good argument to accept patch for this bug.

We use UTF-8 for all parts of the URL, except the query - where we use the charset specified by the page.

Comment 25

[Junior [inactive]]

Attachment: basicAuth8bits, v3

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58ff1cb841b6571a7e8d61bd55dbc9a701505da5

Comment 26

[Julian Reschke]

Changing the default is very likely to break things as there are existing usages which rely on the ISO-8859-1 default. See discussion in <https://www.greenbytes.de/tech/webdav/rfc7617.html#rfc.section.B.3>.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6f90940e417
let Basic http auth support ISO-8859-1 user/password, r=honzab

Comment 28

[Honza Bambas (:mayhemer)]

Adding Kohei as owner of fxsitecompat.com!

Comment 29

[Kohei Yoshino]

Thanks :) Adding the dev-doc-needed keyword for MDN as well.

Comment 30

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/f6f90940e417

Comment 31

[Kohei Yoshino]

So I was about to write a site compatibility doc for this, but given that Chrome already uses UTF-8 and IIUC UTF-8 is compatible with ISO-8859-1 within the range of alphanumeric and common symbols traditionally allowed for username/password, I _think_ this change won’t break anything in the wild. If there is a report of any affected site, I’m happy to post a note at that time :)

Comment 32

[Chris Mills [:cmills]]

I've mentioned this in the docs, as I think it is still worth highlighting, just in case:

https://developer.mozilla.org/en-US/docs/Web/HTTP/Authentication#Character_encoding_of_HTTP_authentication
https://developer.mozilla.org/en-US/Firefox/Releases/59#Security

Let me know if this sounds OK. Thanks!

Comment 33

[Masatoshi Kimura [:emk]]

Re-requesting site-compat because this change actually broke some sites. RFC even warned about the user-agent sniffing:
https://tools.ietf.org/html/rfc7617#appendix-B.3

Comment 34

[Daniel]

Could you please check how this change affected the auth per XMLHttpRequest.open()?
A password with special characters is still accepted per Firefox UI, but no longer per XMLHttpRequest (given as UTF-8).
Chrome does in either way.
It was Chrome compatible before V59 (at least in the range of compatibilty between ISO-8859-1 and UTF-8).

Comment 35

[Kohei Yoshino]

Thanks for the heads-up. Posted the site compatibility note. Will tweet on Monday. https://www.fxsitecompat.com/en-CA/docs/2018/basic-auth-credentials-are-now-encoded-in-utf-8-instead-of-iso-8859-1/