Handled review comments

[philippederyck]

This PR includes the following changes

• Handled review comments from Rifaat Shekh-Yusef (mail 13/11/2024), details included below
• Resolved Clarify why a BFF belongs to a BFF #56

Detailed changes from review Rifaat

I have the following comments/questions:

Section 6.1.1
“This response to the browser will also trigger the reloading of the JavaScript application (H).”
I am assuming that there is a reason for this reload, but that is not clear to me. Can you elaborate on why a reload is needed?

Clarified.

Section 6.1.2.1
• There are few places where the word “initialize” is used, but I guess you meant to say “initiate”?
• “Initialization URI” should that be “authorization URI”?

Fixed.

Section 6.1.2.2
• “These steps are not shown in the diagram, but would occur between step J and K. ”
Should the BFF mint a new access token as soon as the existing access token has expired to allow for faster response to the request in step J?

Reworded.

• “Therefore, it is recommended to configure the lifetime of the cookie-based session managed by the BFF to be equal to the maximum lifetime of the refresh token. Additionally, when the BFF learns that a refresh token for an active session is no longer valid, it is recommended to invalidate the session.”
“recommended” -> “RECOMMENDED”?
Since this is a “recommended”, should there be some text to explain to the implementers when this recommendation might be ignored?

Changed "recommended" to "makes sense" since this is not really a RECOMMENDATION, but mainly an observation for the implementer. All security hinges on the validity of the access/refresh token, and the session of the BFF is just to keep track of them. Keeping it around after a token expires is mainly ineffecient and pointless, but not really problematic.

Section 6.1.3.2
“
• The BFF SHOULD enable the SameSite=Strict flag for its cookies
• The BFF SHOULD set its cookie path to /
• The BFF SHOULD NOT set the Domain attribute for cookies
• The BFF SHOULD start the name of its cookies with the __Host- prefix ([CookiePrefixes])
”
The above statements use “SHOULD”, which implies that in some cases these can be ignored. Section 6.1.3.3.1 then elaborates on the “sameSite” flag. Should there be some text to elaborate on the others?

I added a sentence to clarify this a bit. In my opinion, these can definitely all be MUSTs, but that may conflict with certain corner cases or implementation strategies. We can continue this discussion if the current fix is not sufficient.

Section 6.2.2.1
“The endpoint that initializes the Authorization Code flow (step C) is …”
“initializes” -> “initiates”?

Fixed.

Section 6.2.2.2
“Therefore, it is recommended to configure the lifetime of the cookie-based session to be equal to the maximum lifetime of the refresh token if such information is known upfront. Additionally, when the token-mediating backend learns that a refresh token for an active session is no longer valid, it is recommended to invalidate the session.”
“recommended” -> “RECOMMENDED”?
Since this is a “recommended”, should there be some text to explain to the implementers when this recommendation might be ignored?

Fixed like before.

Section 6.3.2.2
• “using PKCE, and confirming that the authorization server supports PKCE”
How would the browser-based app “confirm” that the authorization server supports PKCE?

The developer would do that, but I've reworded this to avoid any confusion.

Section 6.3.2.3
• “At this point, when the application attempts to use the refresh token after 8 hours, the request will fail and the application will have to re-initialize an Authorization Code flow that relies on the user's authentication or previously established session”
“re-initialize” -> “re-initiate”?

Fixed.

Section 6.3.3.1

“For this reason, and those stated in Section 5.3.1 of [RFC6819], it is NOT RECOMMENDED for authorization servers to require client authentication of browser-based applications using a shared secret, as this serves no value beyond client identification which is already provided by the client_id parameter.”
Since this is considered a bad practice, should we be more forceful here and try to change “NOT RECOMMENDED” to “MUST NOT”?

Done.

Section 6.3.4.2.3
“even when it is associated with a flow that was initialized by the attacker.”
“initialized” -> “initiated”?

Fixed.