[OAUTH-WG] OAuth for Browser-Based Apps

[Justin Richer <[e-mail]>]

As promised at the last meeting, I have been able to do a full review of the OAuth for Browser Based Applications draft spec, and my notes are attached, indexed by sections and paragraphs where possible.

Even though my notes are extensive, I do want to say that overall the document is in great shape and I think it offers great guidance and necessary support to application developers. Many of my comments focus around clarifying intent, consistency in language, and addressing specific concerns like when the document might be too opinionated (or not opinionated enough).

Thank you to the authors for this momentous work!

— Justin




OAuth for browser-based apps

Title and throughout: should we use "applications" instead of "apps"? The document is not consistent.

§1:

¶1 should be specific here and elsewhere:
    "implementing OAuth 2.0" -> "implementing OAuth 2.0 clients"

¶2 perhaps "is known as" or "is referred to" instead of nicknamed?

BCP for native apps should be the reference, not the RFC

"implement OAuth clients"

¶3 perhaps "This specification, OAuth 2.0 for Browser-Based Apps, ..."

"implementing OAuth <<clients as>> native apps and browser-based apps"

§2: nit, there's a BCP to reference here instead of 2119 directly

§3: if not expanding "app" to "application" throughout, perhaps call out here, even though it's fairly obvious it doesn't hurt

¶4 does "this" refer to the phrase "JavaScript applications" or the document as a whole and its recommendations?

§4:

¶1 the use of "were" is confusing here since OAuth 2.0 is considered one protocol framework; perhaps rephrase:

    "At the time that OAuth 2.0 was initially specified in [RFC6749] and [RFC6750],"

"authorization code flow" -> "authorization code grant type" ... and throughout the rest of the document, generally change "flow" to "grant type" when referring to the official grant names. same applies to "implicit" flow and any others

"this was one" -> "this limitation(?) was one"

¶3 add a reference for CORS?

exposure of tokens is not the problem for refresh tokens historically, it's been the lack of client credentials in SPAs and poor storage mechanisms to go with the refresh token

¶4 second sentence has many asides and parentheticals and is hard to follow, suggest rewording


§5 does "JavaScript" here also refer to other browser-based execution languages, or is this advice JavaScript only? The definition in §3 only calls out "JavaScript application" specifically, not "JavaScript" being used as a generic in general

¶1 remove commas from "vectors, such as ... files, give an"

¶2 "JS" -> "JavaScript" (and elsewhere), or expand and define on first use

is the bold formatting really necessary to make this point?

¶3 perhaps instead of "the first part" and "the second part", link sections directly and number them specifically (5.1, 5.2) so as to prevent the reader from tripping over what constitutes the first or second "part"

¶4 it feels odd to introduce what other sections will do in this section, but I'm not sure where else to put this pointer, which is likely helpful to the reader if they're going all the way through the document

§5.1 suggest removal of "range from trivial to sophisticated" as it implies ranking even though the next sentence says there is no implied ranking; and adversarial toolkits can sometimes make sophisticated attacks trivial to execute in practice.

§5.1.1 why does this scenario underestimate the capabilities of an attacker? It is real and can cause damage. I think what might be intended here is that if a developer focuses only on thwarting this trivial attack they are underestimating an attacker's ability, is that the case? In any event perhaps such value judgement isn't helpful unless it's tied to specific guidance for the reader.

§5.1.2 the attack in §5.1.1 does not depend on theft of token from the payload of the token response but rather from storage; is this difference intended? or is "the payload" meant to refer to the attacker's payload of malicious code?

¶2 I believe the second bullet list is supposed to have a sub-list, but this was not rendered as such in the version I reviewed

§5.1.3

"Setup" -> "set up"

add reference for "Web Messaging"?

It should be noted that a precondition to step (3) is that it's only possible if the authz code can be issued silently and in a frame context, or else the payload would need to manipulate the AS into doing so; this is mentioned in the closing paragraph but it felt out of place not to put it sooner

§5.2.1 nit, it's not impersonation of the user so much as impersonation of the legitimate client application

§5.2.2 same nit as above, not the user but the client; any other place this appears should also be aligned

¶2: suggest "considers the access token to be valid" -> "accepts the valid access token".

¶5 the "additionally" doesn't follow from the preceding text, suggest pulling it out or re-ordering the topics here

§5.2.3

¶2 CORS is expanded here but not above, and both times without a reference; suggest add a reference and expand the first time, and stick to CORS elsewhere throughout

suggest making it clear that it's the CORS configuration of the RS that prevents calls from the client and adversary to that RS, and does not prevent calls elsewhere that have an open policy (or other non-CORS-restricted methods)

§6.1

¶3 what does "Payload" refer to here? The sections above are not referred to as "Payloads" in the titles

the "BFF is a confidential client", but that requirement is not stated until all the way down in 6.1.3.1 where it's specifically under "security considerations". If it's a precondition for the architecture pattern, then state that.

Perhaps move all the preconditions up front as enumerated requirements?

§6.1.1

diagram nit: suggest using aasvg or similar tooling instead of plain ASCII

diagram nit: this might read better as a vertical time sequence diagram, with concise labels on the arrows

There seem to be several holes in this process, and I think the text is more opinionated about implementation of the pattern than it ought to be:

how is "an active session" communicated between the BFF and the javascript? in other words, why do we expect that the BFF does not immediately detect the scenario and go from (B) to (D) and instead waits for the javascript to initiate (C)? While that's also a valid pattern I would like to understand the reason behind the flow

Is there a need for the front end to handle the redirect in (E) and not have the BFF process the redirect itself?

what is "relevant information" in step (G) and why does this pattern seem to presume a cookie format?

why does the application fully reload itself in (H) instead of managing state internally? It would be trivial for many applications to respond to (G) without a full application reload, especially since the application would have reloaded at the start of (E) on the tail end of the redirect anyway.

the request in step (J) need not be a direct proxy to the RS, as it could be transformed by the BFF in ways beyond the simple credential swap between (J) and (K)

§6.1.2.1 are these items "recommended" or "RECOMMENDED"? ie, is this intentionally informative?

formatting/consistency nit, above the steps are listed as "(F)" but here they are "step F" (no parens, include "step").

formatting/consistency nit: capitalization of Refresh Token is inconsistent throughout this section (and likely elsewhere in the document); is this significant?

¶3 what does "recover" mean in this context? continue to call the RS?

§6.1.2.2

¶1 suggest remove "traditional" unless you have an idea of a "nontraditional" browser cookie :)

add reference for HTTP cookies RFC(s) (either here or earlier in the description of the architecture)

¶2 "only expose" -> "expose only"

§6.1.2.3 it should be noted that if the frontend catches the redirect, then the frontend ALSO gets the ID token -- even though it's implied that it should ignore that, it doesn't need to (and importantly, an attacker in the frontend will see it)

§6.1.3.1 here we finally see a requirement for being a confidential client, while the past few sections are built around this assumption (which is pretty much stated here but not earlier). Additionally, a major benefit is not exposing access and refresh tokens to the browser environment, which has nothing to do with the confidential client.

The SHOULD here is confusing -- is authz code w/o PKCE OK? is some other grant OK? under what circumstances can the SHOULD be relaxed?

§6.1.3.2 I feel like more attention should be paid to the two different types of cookie storage and their resulting requirements.

Why is AEAD the only encryption method called out here? What's particularly special about this algorithm?

There are no requirements for key management. It's obvious to a seasoned developer or security engineer that the BFF holds the decryption keys and doesn't give them out but that's never talked about. Are there any other considerations? Like, is it OK for the BFF to use the same encryption keys with each client? Does the key need to be identified?

§6.1.3.3.1

¶3 remove the last sentence, it's unnecessary commentary and value judgement ("Technically, this attack...")

§6.1.3.3.2

¶5 I got lost as to which components are protected by the CORS methods. If the system is using a BFF then the RS should never be called by the browser and CORS doesn't matter there.

¶6 it is considered bad practice to create `X-` header field names, and probably even worse practice to recommend making up an unregistered and ignored header field name just to force CORS. Especially if the BFF is fielding all requests to the RS in the first place (see previous point), I don't think this is good and actionable advice and will likely run afoul of the HTTP directorate review during publication. If this actually does need to be kept, perhaps seek out advice from HTTP WG?

§6.1.3.4 it's not a matter of power so much as a matter of secure key management and hopefully better control over the runtime environment of the application code. Neither of these have to do with power, as I can guarantee you that I've deployed web servers with less computational power than the browser on my laptop has today. :)

§6.1.4 see previous notes about "Payload"

§6.1.4.1

¶5 suggest remove "unfortunately" as it's a value judgement

¶6 this aspect also makes the BFF a prime target for attackers. One can argue that it's a harder target, fine -- but any proxy architecture has privacy and security implications that are absolutely tradeoffs. I accept that attacks against non-browser-systems are out of scope for this draft, but I feel like it would be irresponsible to at least mention this and place it out of scope. Basically, tell people that the BFF needs to be extra secure and that just having one doesn't solve security

§6.1.4.2

¶5 suggest "Because of the nature of the BFF" -> "Since refresh and access tokens are managed by the BFF not exposed to the browser,"

§6.1.4.3

¶1 this isn't a summary, this is a new point being made here for the first time. Suggest remove "To summarize" from this paragraph.

§6.2.1 same notes on diagrams and step-label formatting as above

same note as above on returning the code through the front end instead of directly to the backend

same note as above on relevant information (this seems to presume storing state in the cookie as the solution)

same note as above on reloading the application

question, is the "Editor's note" intended to survive to the full RFC? it seems useful reference if sufficiently caveated

§6.2.2.1 same comment as before about assuming a confidential client but not requiring it in the text until much later (§6.2.3.1 here)

same comment as above on "recover"

same comment as above on non-normative "recommended" vs "RECOMMENDED" intent here

§6.2.2.3 same note as above on "traditional" and cookie reference

§6.2.2.5 the patterns do affect the relationship, though, as it helps determine whether the token proxy needs to have CORS or not and how that should be deployed

§6.2.3.1 an additional major benefit is not exposing refresh tokens to the browser environment

§6.2.3.4 sure it CAN, but is that a recommendation or requirement for proper use of this pattern? Possibility does not imply action.

§6.2.4.3.1 this "note" is pretty serious and deserves more than a note in terms of attention

§6.2.4.3.2 DPoP could also be solved by having the backend deliver a keypair alongside the token to the front end, which would come with its own security issues. Should this be covered here?

§6.2.4.4 similar note as above on "summary"

suggest remove aphorism "go the extra mile", and the advice is questionable as the complexities and downsides of a full proxied system are significant and should not be ignored by implementors

§6.3.1 same comments as above on diagrams and step labeling

in-browser applications are fully capable of doing dynamic client registration per RFC7591. There are some pretty severe management tradeoffs when doing this, but it's disingenuous to say "There's no way". A second pattern, which I haven't seen in the wild and only in a PoC, is to programmatically register an instance of the SPA and pack credentials in at the time of download. In both of these cases, though, malicious code still has the ability to get access tokens as if it were the client. Yet another option is the in-line registration method used in OIDC Federation.

§6.3.2.1 suggest remove the two "in summary" as they aren't summarizing anything from before.

suggest splitting the inner bullet list into three items instead of 2, and suggest stating that the techniques can be safely and effectively used in combination, as is done in §6.3.2.6

the advice on refresh token expiration seems confusing at first read, particularly if my lifetime is based on use in the first place. What is this advice in the last bullet trying to prevent?

§6.3.2.2

¶1 again this restriction to public clients predates several specs that remove it and the advice shouldn't be parroted here. better to say that they usually are going to be public because of the problem of registering a client ID and storing a secret of any kind at configuration time associated with that client ID.

¶2 the "or more" seems like it is asking for trouble with this kind of client.

§6.3.2.3 again "cannot contain" is strictly too strong a statement. Also, this repeats text in the previous section without adding.

§6.2.3.5

¶1 the last sentence currently reads as though it's saying "to avoid page refreshes or to avoid run silent frame-based flows" which I don't think is the intent, suggest adding the infinitive with "to run frame-based flows silently" or something similar.

§6.2.3.2 details of what, exactly, should the reader be looking for there?

§6.3.2.7 I would expect a reader at this point of the document to not need an introduction to what a refresh token is and what it does. Maybe skip the intro sentence or two?

aren't these the same requirements as above in §6.3.2.1? Does it need to be listed as requirements in both places? Are they identical?

§6.3.2.8

¶2 what is a "proper CORS configuration" as per this spec? I don't think we have quite enough details to require that as such.

§6.3.3.2.1 and §6.3.3.2.2 add references for local storage, web worker, indexdb, etc

§6.3.3.2.2 the note on dpop should also note that since the attacker can control which key the new token is bound to, the attacker can count on being able to exfiltrate the token to their own system that also has a copy of the key and use it outside of the original application context

§7 ¶1 who is "our", the authors, the readers, the oauth community, someone else?

§7.2

the whole of ¶2 seems out of place as first we're discussing the implicit flow but then give requirements, in that context, that contradict what the flow requires one to do. At first it seems like we're reading requirements FOR the implicit flow, not to prevent its use. Perhaps this can be introduced to make the requirements less jarring, such as "The security properties of the implicit flow make it no longer a recommended best practice. To effectively prevent the implicit flow, the as MUST NOT ..."

§7.2.1

¶1 as I recall, the fragment was used primarily to prevent the access token from leaking to whatever static backend was serving the SPA. If it's in query parameters it lands in the back end. If it's in the fragment, it is not sent. this is a problem that remains with the code flow and browser-based apps, but I don't see that mentioned in this document (especially since this document recommends serving the redirect URI from the front end in all architectures).

§7.2.3.x suggest removing "Threat:" from headings (it's not used elsewhere in the doc)

§7.2.4

¶1 remove "standard" before "authorization code" since implicit is also just as standard per OAuth 2.0

§7.4.1.1

suggest change "Just note the that the service worker" -> "The service worker..." and combine last two sentences into one.

§8

¶1 previously the token-mediated pattern is said to not require storage of tokens b/c the frontend can just go get a new token from the mediator, this seems to contradict with what's stated here

§8.2

¶3 is it true that the service worker obtains tokens FROM the application, or should this be FOR the application? or is it the application obtains them FROM the service worker, instead? I might be missing the intent of this advice

§8.4 this section seems explicitly JavaScript specific, is that the case?

§8.5 provide references for each storage type's specification

§9 security considerations sections should be about "why" and really shouldn't have normative requirements, please remove them from the subsequent sections

§9.1 recommend removing "straightforward"

§9.2

last sentence of ¶2 repeats the first sentence of ¶3, suggest removing one or combining both

§9+ this draft should add privacy considerations, particularly for BFF pattern's proxy architecture.e

§A it feels wrong to have normative requirements in an appendix (point 7), and points 2 and 3 both have a weird uppercase NOT which isn't a keyword