Repository navigation
[OID4VCI-FAPI2] Pass fapi2-security-profile-final-state-only-outside-request-object-not-used - #49788
Conversation
b1c2a58 to
60d8d4e
Compare
98c8f49 to
be7fb87
Compare
There was a problem hiding this comment.
@tdiesler Thanks!
It seems there is also alternative approach that request will not be rejected, but state parameter (or other parameters included in the query) would be ignored. However I am rather +1 to the approach from your PR as it is safer.
Added minor comment inline related to the error message.
| for (String queryParam : queryKeys) { | ||
| if (!requestParametersNameFromPAR.contains(queryParam)) { | ||
| AuthzEndpointParParser.removeRequestObject(session, requestUri); | ||
| throw new ClientPolicyException(OAuthErrorException.INVALID_REQUEST_OBJECT, "PAR request did not include query parameter: " + queryParam); |
There was a problem hiding this comment.
Is it please possible to add value of queryParam directly to this exception message?
The possible problem is, that this is user-provided parameter and user can possibly use some strange parameter. And the exception message from here is returned back in the response, which in some corner cases may cause issues like XSS.
If you prefer, it is OK to display log.warn and add the name of missing parameter there? Or eventually add it to the event (but not sure if it is easily possible).
There was a problem hiding this comment.
I'm not sure what you mean. The error description indicates that there is a param on the url query that is not part of the PAR object. The name of the conflicting parameter is part of the error description, the value of which should not matter.
There was a problem hiding this comment.
Oh, sorry for the confusion... What I wanted to write was : Is it please possible to avoid adding name of queryParam directly to this exception message?
With the change code similar to this:
| throw new ClientPolicyException(OAuthErrorException.INVALID_REQUEST_OBJECT, "PAR request did not include query parameter: " + queryParam); | |
| throw new ClientPolicyException(OAuthErrorException.INVALID_REQUEST_OBJECT, "PAR request did not included query parameter"); | |
| log.warnf("PAR request did not included query parameter: %s", queryParam); |
Signed-off-by: Thomas Diesler <tdiesler@proton.me>
…request-object-not-used closes keycloak#48047 Signed-off-by: Thomas Diesler <tdiesler@proton.me>
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.broker.KcOidcBrokerTest#loginWithExistingUserWithBruteForceEnabledKeycloak CI - Java Distribution IT (windows-latest - temurin - 21) |
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.broker.KcOidcBrokerTest#loginWithExistingUserWithBruteForceEnabledKeycloak CI - Java Distribution IT (windows-latest - temurin - 21) |
…request-object-not-used (keycloak#49788) closes keycloak#48047 Signed-off-by: Thomas Diesler <tdiesler@proton.me>
closes #48047
Fixes