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 <> 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
