Thanks for addressing my comments. On Thu, Jul 3, 2025 at 8:06 PM Aaron Parecki <aaron= [email protected]> wrote:
> Thanks for your review Orie. Responses inline. You can also see the > diff with the changes here: > > https://github.com/oauth-wg/oauth-browser-based-apps/pull/109/files > > > ---------------------------------------------------------------------- > > COMMENT: > > ---------------------------------------------------------------------- > > > # Orie Steele, ART AD, comments for > draft-ietf-oauth-browser-based-apps-24 > > CC @OR13 > > > ## Comments > > > Thanks to Marc Blanchet for the ART review. > > > ### 1. Introduction > > > The ordering of the introduction feels a bit awkward, you might consider > > starting with: > > > ``` > > This document focuses on JavaScript frontend applications acting as the > OAuth > > client,... ``` > > > Then relate to "OAuth 2.0 for Native Apps" and then end with the comment > about > > OpenID Connect: > > > ``` > > Such a scenario, (which only uses OAuth 2.0 as the underlying > specification of > > OpenID Connect), is not within scope of this specification. ``` > > Agreed, I've rearranged this accordingly. > > > I was not familiar with the term "first-party frontend" before. > > > Having read the full document, I might try to more clearly distinguish > this > > case from BFF up front. > > I changed this to "common domain" and added a reference to section 7.1 > where this is discussed further. > > > ### Revoking of refresh tokens > > > ``` > > 530 The attack is only stopped when the authorization server > refuses a > > 531 refresh token because it has expired or rotated, or when the > refresh > > 532 token is revoked. In a typical browser-based OAuth client, > it is not > > 533 uncommon for a refresh token to remain valid for multiple > hours, or > > 534 even days. > > ``` > > > Given the comments about refresh token reuse, is it common to > automatically > > revoke a refresh token where reuse is detected or attempted? > > Yes, described in Section 4.14.2 of RFC 9700. > > > ### Reference for session fixation > > > ``` > > 576 sending requests to backend systems. Alternatively, the > attacker can > > 577 also abuse their access to the application to launch > additional > > 578 attacks, such as tricking the client into acting on behalf of > the > > 579 attacker using an attack such as session fixation. > > ``` > > > Consider a reference for session fixation, possibly OWASP. > > Thanks, added. > > > ### confidential client > > > Consider a reference or definition for "confidential client". > > Added. > > > ``` > > 861 The main benefit of using a BFF is the BFF's ability to act > as a > > 862 confidential client. Therefore, the BFF MUST act as a > confidential > > 863 client. Furthermore, the BFF MUST use the OAuth 2.0 > Authorization > > 864 Code grant as described in Section 2.1.1 of [RFC9700] to > initiate a > > 865 request for an access token. > > ``` > > > Making it easier to implement this MUST, I'm not sure what is required > to act > > as a confidential client. > > I added "by establishing credentials with the authorization server" to > the end of that sentence. > > > ### Cookie security, why NOT MUST > > > ``` > > 867 6.1.3.2. Cookie Security > > ``` > > > This section contains several SHOULDs and SHOULD NOTs. > > While it is nice that the guidance is so compact, it raises the question > of why > > these are not MUSTs, and under which conditions they ought to be > required. > > Consider providing a reference if the answer lies elsewhere, or a brief > comment > > after the compact list. > > The paragraph below the list goes in to some of this detail. I tried > to make this more explicit by adding a sentence in between. > > > ## Nits > > > ### refresh tokens and correlation of requests > > > ``` > > 367 reduce the scope and lifetime of the token. For refresh > tokens, the > > 368 use of refresh token rotation offers a detection and > correction > > 369 mechanism. Sender-constrained tokens (Section 9.2) offer an > > ``` > > > A few extra words here, or a pointer might make it clearer what > recommendation > > is being made wrt refresh tokens. > > Refresh token rotation is defined in RFC9700, I've added a reference to > that. > > > Are you recommending a specific refresh token rotation policy here ? > > No. > > > Later you have: > > > ``` > > 410 is not sufficient to prevent abuse of a refresh token. An > attacker > > 411 can easily wait until the user closes the application or their > > 412 browser goes offline before using the latest refresh token, > thereby > > 413 ensuring that the latest refresh token is not reused. > > ``` > > > Consider forshadowing this comment. > > This paragraph has been rephrased based on some of the other feedback. > > > ### Confused Deputy > > > ``` > > 921 Site Request Forgery (CSRF) attacks. A successful CSRF > attack could > > 922 transform the BFF into a confused deputy, allowing the > attacker's > > 923 request to the BFF to trigger outgoing calls to a protected > resource > > 924 on behalf of the user. > > ``` > > > Consider providing a reference for this term. > > I simplified the sentence by removing the reference entirely: > > "A successful CSRF attack could allow the attacker's request to the > BFF to trigger outgoing calls to a protected resource." > > > ### Extracting CryptoKeyPair > > > ``` > > 1851 extractable [CryptoKeyPair] is stored using > [W3C.IndexedDB]). As a > > 1852 result, the use of DPoP effectively prevents scenarios where > the > > 1853 attacker exfiltrates the application's tokens (See Section > 5.1.1 and > > 1854 Section 5.1.2). > > ``` > > > IIRC, the non extractable crypto key pair could potentially still be > extracted > > by other processes running on the same machine. The context makes it > clear you > > are referring to attackers with the ability to execute javascript in the > > browser, but there are other kinds of attackers. > > > I see you comment on this later in Section 8.6 & 9.2, consider if you > want to > > make the text here a little more narrowly focused on attackers with XSS > > capabilities. > > The "attack scenarios" and "additional defenses" headers preceding > this text already scope the discussion to XSS attackers. Just for good > measure I updated this to "where the XSS attacker" as a reminder. > > > --- > Aaron Parecki > >
_______________________________________________ OAuth mailing list -- [email protected] To unsubscribe send an email to [email protected]
