Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Content bug: Inaccurate explanation on request credentials #13063

Closed
zellwk opened this issue Feb 17, 2022 · 13 comments · Fixed by #34437
Closed

Content bug: Inaccurate explanation on request credentials #13063

zellwk opened this issue Feb 17, 2022 · 13 comments · Fixed by #34437

Comments

@zellwk
Copy link

zellwk commented Feb 17, 2022

What page(s) did you find the problem on?

Specific page section or heading?

  • Under Syntax > Value for the request credentials page
  • The intro text for the Access-Control-Allow-Credentials page

What is the problem?

There are a few problems:

  1. Explanation on credentials weren't accurate because Authorisation headers were sent regardless whether 'credentials' were set to omit, same-origin, or include.

FLsiY4PVQAIMy9r

  1. The Access-Control-Allow-Credentials page mentioned that credentials are cookies, authorization headers, or TLS client certificates.

CleanShot 2022-02-16 at 22 34 26

A few things here:

  1. Access-Control-Allow-Credentials are not required for Authorization headers. They can be sent regardless whether Access-Control-Allow-Credentials is present.
  2. Authorization headers only need Authorization to be included Access-Control-Allowed-Headers.
  3. Access-Control-Allow-Credentials are not required to send cookies between domains and subdomains. This is not listed anywhere but I thought it'll be good to make things clear.

What did you expect to see?

Three things:

  1. I recommend removing "basic http auth" from the request credentials page since it doesn't look correct.
  2. I recommend updating the pages to reflect a more accurate understanding for Access-Control-Allow-Credentials. Not sure what to update at this point, but I can take a stab at it.
  3. Maybe we can point the "etc" part in Request Credentials page to the Access-Control-Allow-Credentials page because they seem linked. I'm not sure whether this is accurate though.

Did you test this? If so, how?

I tested credentials and wrote an article about my tests. I focused on cookies in this article and didn't include the Authorization header parts. There's a test repository included in the article. I can create a test repo for the authorization part if necessary. Just let me know.

@zellwk zellwk added the needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. label Feb 17, 2022
@zellwk zellwk changed the title Content bug: <TITLE OF PROBLEM> Feb 17, 2022
@sideshowbarker sideshowbarker added Content:HTTP HTTP docs Content:WebAPI Web API docs needs info Needs more information to review or act on. and removed needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. labels Feb 17, 2022
@sideshowbarker
Copy link
Collaborator

sideshowbarker commented Feb 17, 2022

Authorisation headers were sent regardless whether 'credentials' were set to omit, same-origin, or include.

Do you have a minimal reproducible test case or demo you can post as a comment here to demonstrate that behavior?

I’m aware of https://zellwk.com/blog/how-fetch-credentials-handles-cookies/ and https://github.com/zellwk/demos/tree/main/fetch-credentials but I think it’s unlikely that other contributors will go through all that in order to extract a minimal reproducible test case.

I can say with confidence that every test I have personally ever run with the Fetch API confirms the opposite of what’s asserted in the issue description here; that is, the Fetch API in fact does not send credentials in requests unless sending of credentials is explicitly requested in the call to the API.

@zellwk
Copy link
Author

zellwk commented Feb 17, 2022

@sideshowbarker Gladly.

Here's a reduced test case. https://github.com/zellwk/demos/tree/main/fetch-credentials-mdn/fetch-omit-credentials

There are two folders — frontend and server. Please run both servers at the same time. I've detailed what to watch out for in frontend's index.html file.

@zellwk
Copy link
Author

zellwk commented Feb 17, 2022

Regarding the second point: Access-Control-Allow-Credentials is not needed to send credentials between subdomains and domains, but the credentials option is required on Fetch in the frontend.

Here's a reduced test case: https://github.com/zellwk/demos/tree/main/fetch-credentials-mdn/cookies-no-need-credentials

@sideshowbarker
Copy link
Collaborator

Regarding the second point: Access-Control-Allow-Credentials is not needed to send credentials between subdomains and domains

Given that Access-Control-Allow-Credentials header is a response header, it doesn’t have any effect on what browsers send in requests. Right?

The browser first sends the request to the server — before the server sends back any response. So if the fetch() call has has include for the credentials option, then the browser goes ahead and includes credentials in the request.

The browser doesn’t — and can’t — somehow wait to send credentials until after it gets backs a response from the server and checks whether or not that response has a Access-Control-Allow-Credentials: true response header.

So, Access-Control-Allow-Credentials doesn’t — and can’t — have any effect on whether the browser includes credentials in the request.

Is there somewhere in the Request»credentials and/or Access-Control-Allow-Credentials articles that states or implies that Access-Control-Allow-Credentials affects whether the browser includes credentials in the request?

Or are you proposing that we update the Request»credentials and/or Access-Control-Allow-Credentials articles to explicitly state that Access-Control-Allow-Credentials doesn’t affect whether browsers include credentials in requests?

If not, what specific changes are you imagining need to be made instead?

Here's a reduced test case: https://github.com/zellwk/demos/tree/main/fetch-credentials-mdn/cookies-no-need-credentials

I see that all of the fetch() calls in the frontend JavaScript code there have include for the credentials option. Therefore, the expected behavior is that credentials will be included in the request.

So it seems unclear what that test case is intended to test.

One of your earlier comments said this:

Authorisation headers were sent regardless whether 'credentials' were set to omit, same-origin, or include

Do you have a test case which demonstrates that?

As far as I can see, the https://github.com/zellwk/demos/tree/main/fetch-credentials-mdn/cookies-no-need-credentials test case is not attempting to demonstrate that — because all of the fetch() calls in the frontend JavaScript code there have include for the credentials option

@zellwk
Copy link
Author

zellwk commented Feb 21, 2022

I'm sorry if I didn't make this clear. Let me make as clear as I can. Thank you for your patience in reading this through and helping me work through this issue that I'm voicing.

Point 1: Authorisation headers were sent regardless whether 'credentials' were set to omit, same-origin, or include.

I want to refer you to this reduced test case: https://github.com/zellwk/demos/tree/main/fetch-credentials-mdn/fetch-omit-credentials.

Please note that the credentials were set to omit in the index.html file, but the Authorization headers were sent to the servers regardless.

This part in Request Credentials mentioned that basic http auth — which I assume is Basic Authorization — is only sent when credentials is set to same-origin or include.

CleanShot 2022-02-21 at 14 30 20

Unfortunately, this is not true as demonstrated in the reduced test case, since I could send Authorization headers with credentials set to omit.

Please feel free to correct me if I misunderstood anything.

Point 2: Access-Control-Allow-Credentials is not needed to send credentials between subdomains and domains

Given that Access-Control-Allow-Credentials header is a response header, it doesn’t have any effect on what browsers send in requests. Right?

This was created by my misunderstanding on what Access-Control-Allow-Credentials were used for.

I was confused because cross-origin requests required this header before cookies can be sent, but they were not required between subdomains and domains.

Technically, a change in domain name (including adding subdomains) constitutes a change in origin, as mentioned in the Origin Glossary page.

CleanShot 2022-02-21 at 15 07 36

But in this case, even though there is a change in "origin", cookies are still sent without the Access-Control-Allow-Credentials header.

The second test case (https://github.com/zellwk/demos/tree/main/fetch-credentials-mdn/cookies-no-need-credentials) was created to prove the point that we don't need the Access-Control-Allow-Credentials header even though we need credentials in the Fetch request.

What makes it confusing is: Cookies work on a cross-site basis, not a cross-origin basis. (Referring to Site) documentation. But most of the docs speak about Credentials as if they're always cross-origin.

If we do not require credentials to use Authorization Headers (back to point 1), what do credentials actually do? Is it only used for Cookies? If credentials is only used for Cookies, then we might want to use talk about it in a cross-site manner, and not a cross-origin manner.

But the Access-Control-Allow-Credentials page mentions that this header is used for cookies, authorization headers, and TLS certs.

  1. Regarding cookies — it's required, but it's cross-site only, not cross-origin
  2. Regarding Authorization headers — not required as mentioned in Point 1. So this seems to be wrong?
  3. Regarding TLS client certs — I'm honestly not sure about this.

If correcting the language (cross-site vs cross-origin) is too huge of a change, then I would suggest writing the fact that the Access-Control-Allow-Credentials header is not required to send cookies between subdomains and domains (which is not cross-site, even though it is cross-origin).

I hope this communicates what I'm trying to say better.

@zellwk
Copy link
Author

zellwk commented Mar 3, 2022

@sideshowbarker Just wondering if you saw the response above?

@sideshowbarker
Copy link
Collaborator

@sideshowbarker Just wondering if you saw the response above?

I did see it, yes

I think what would be most productive at this point would be if you could go ahead and create a pull request with some proposed changes. That’d give something concrete to consider

@zellwk
Copy link
Author

zellwk commented Mar 22, 2022

Gotcha. Just FYI: Lots of work piling up recently, but I'm going to come back to this when I find some time/space to work on it.

@Frederick888
Copy link

I recently also stumbled upon this issue.

First of all, I can confirm @zellwk's testing results about Authorization header. I was confused by some HTTP server implementations and Firefox/Chrome's behaviour but they all make sense now.

However, it seems the current MDN explanation actually aligns with W3C:

https://fetch.spec.whatwg.org/#concept-request-credentials-mode

Request’s credentials mode controls the flow of credentials during a fetch.

Then definition of 'credentials': https://fetch.spec.whatwg.org/#credentials

Credentials are HTTP cookies, TLS client certificates, and authentication entries (for HTTP authentication). [COOKIES] [TLS] [HTTP-AUTH]

Definition of 'authentication entries': https://fetch.spec.whatwg.org/#authentication-entry

An authentication entry and a proxy-authentication entry are tuples of username, password, and realm, used for HTTP authentication and HTTP proxy authentication, and associated with one or more requests.

RFC of 'HTTP authentication': https://httpwg.org/specs/rfc7235.html

This document defines HTTP/1.1 authentication in terms of the architecture defined in "Hypertext Transfer Protocol (HTTP/1.1): Message Syntax and Routing" [RFC7230], including the general framework previously described in "HTTP Authentication: Basic and Digest Access Authentication" [RFC2617] and the related fields and status codes previously defined in "Hypertext Transfer Protocol -- HTTP/1.1" [RFC2616].

And the whole idea of HTTP authentication being part of 'credentials' was reinforced by whatwg/fetch#612 whatwg/fetch#616

I know it must be me who overlooked something other than all the mature server/browser implementations out there, but really I wasn't able to find any supporting documentations on the reason why Authorization header is not affected by fetch's credential mode. :(

@Frederick888
Copy link

By the way I did a quick search in Firefox's code and I was only able to see how request credential mode interacts with cookies [1] at my first glance; though after digging a bit deeper, I'm wondering if things like [2][3] are relevant as well...

[1] https://hg.mozilla.org/mozilla-central/file/a8e5c045d25c0161632683b641c64c7d073fdc91/dom/fetch/FetchDriver.cpp#l593
[2] https://hg.mozilla.org/mozilla-central/file/a8e5c045d25c0161632683b641c64c7d073fdc91/netwerk/protocol/http/nsCORSListenerProxy.cpp#l1013
[3] https://hg.mozilla.org/mozilla-central/file/a8e5c045d25c0161632683b641c64c7d073fdc91/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp#l233

@zellwk
Copy link
Author

zellwk commented Apr 4, 2022

@Frederick888 Thanks for chiming in on this issue. It's reassuring to know I'm not alone in surfacing this issue.

@Frederick888
Copy link

Frederick888 commented Apr 5, 2022

Ah I think I understand where the MDN wording (and W3C) came from now.

'HTTP Authentication' here, I believe, refers to the (usually) challenge-response form of HTTP native authentication, where users are often presented with a pop-up dialogue to provide username and password, then the 'credential' is cached by the browser for a certain period of time. (I've attached a simple python server so anyone who's not familiar with this flow can try it out.)

Indeed in this scenario, credential is passed to server via Authorization header, but Authorization header (and various schemes e.g. Basic, Digest, Bearer, etc.) per se is not only used in the context of 'HTTP Authentication'. And the only Authorization header that's affected by credential: 'omit'|'same-origin'|'include' is the one in 'HTTP Authentication'.

To test this out, still using the attached testing server, first navigate to https://httpbin.org (I'm using httpbin simply cos it hasn't got script security headers, you can use others), then in developer console:

resp = await fetch('http://localhost:9001', { mode: 'cors' } })
await resp.text()
// "needs auth!"

resp = await fetch('http://localhost:9001', { mode: 'cors', credentials: 'include' } })
// you can see a pop-up dialogue here
await resp.text()
// "secret!"

resp = await fetch('http://localhost:9001', { mode: 'cors', credentials: 'include' } })
// this time 'credential' is cached! back-end sees the cached Authorization header
await resp.text()
// "secret!"

resp = await fetch('http://localhost:9001', { mode: 'cors' } })
// cached 'credential' is omitted!
await resp.text()
// "needs auth!"

But then actually you can use Authorization header directly, cos in this way, it's considered a general Authorization header, not one particularly in the context of 'HTTP Authentication':

// you can also modify BE code first to omit Access-Control-Allow-Credentials

resp = await fetch('http://localhost:9001', { mode: 'cors', headers: { 'Authorization': 'Basic foo' } } })
await resp.text()
// "secret!"

In conclusion, instead of saying authorization headers only, it's probably better to say authorization headers used in HTTP Authentication, and link it to https://developer.mozilla.org/en-US/docs/Web/HTTP/Authentication.


(Modified from https://gist.github.com/mdonkers/63e115cc0c79b4f6b8b3a6b797e485c7)

#!/usr/bin/env python3
"""
Very simple HTTP server in python for logging requests
Usage::
    ./server.py [<port>]
"""
from http.server import BaseHTTPRequestHandler, HTTPServer
import logging


class S(BaseHTTPRequestHandler):
    def _set_response(self):
        self.send_response(200)
        self.send_header("Content-type", "text/html")
        self.send_header("Access-Control-Allow-Credentials", "true")
        self.send_header("Access-Control-Allow-Headers", "Content-Type,Authorization")
        self.send_header("Access-Control-Allow-Origin", "https://httpbin.org")
        self.end_headers()

    def _set_challenge(self):
        self.send_response(401)
        self.send_header("WWW-Authenticate", "Basic")
        self.send_header("Content-type", "text/html")
        self.send_header("Access-Control-Allow-Credentials", "true")
        self.send_header("Access-Control-Allow-Headers", "Content-Type,Authorization")
        self.send_header("Access-Control-Allow-Origin", "https://httpbin.org")
        self.end_headers()

    def do_GET(self):
        logging.info(
            "GET request,\nPath: %s\nHeaders:\n%s\n", str(self.path), str(self.headers)
        )
        if self.headers.get("Authorization") is not None:
            # allow any user/pass
            self._set_response()
            self.wfile.write("secret!".encode("utf-8"))
        else:
            self._set_challenge()
            self.wfile.write("needs auth!".encode("utf-8"))

    def do_OPTIONS(self):
        logging.info(
            "OPTIONS request,\nPath: %s\nHeaders:\n%s\n",
            str(self.path),
            str(self.headers),
        )
        self._set_response()
        self.wfile.write("OPTIONS request for {}".format(self.path).encode("utf-8"))

def run(server_class=HTTPServer, handler_class=S, port=8080):
    logging.basicConfig(level=logging.INFO)
    server_address = ("", port)
    httpd = server_class(server_address, handler_class)
    logging.info("Starting httpd...\n")
    try:
        httpd.serve_forever()
    except KeyboardInterrupt:
        pass
    httpd.server_close()
    logging.info("Stopping httpd...\n")


if __name__ == "__main__":
    from sys import argv

    if len(argv) == 2:
        run(port=int(argv[1]))
    else:
        run()
@Josh-Cena Josh-Cena removed the needs info Needs more information to review or act on. label Jul 4, 2023
@wbamberg
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment