[
https://issues.apache.org/jira/browse/FEDIZ-232?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16699348#comment-16699348
]
ASF GitHub Bot commented on FEDIZ-232:
--------------------------------------
coheigea commented on a change in pull request #35: FEDIZ-232 added flag to
disable RequestState verification
URL: https://github.com/apache/cxf-fediz/pull/35#discussion_r236355604
##########
File path:
plugins/spring/src/main/java/org/apache/cxf/fediz/spring/web/FederationAuthenticationFilter.java
##########
@@ -132,20 +132,24 @@ private String getState(ServletRequest request) {
private RequestState verifySavedState(HttpServletRequest request) {
HttpSession session = request.getSession(false);
+ boolean enforceValidState =
+ federationConfig == null ||
federationConfig.getFedizContext().isRequestStateValidation();
- if (session == null) {
- logger.warn("The received state does not match the state saved in
the context");
- throw new BadCredentialsException("The received state does not
match the state saved in the context");
- }
-
- RequestState savedRequestState = (RequestState)
session.getAttribute(SAVED_CONTEXT);
- String state = getState(request);
- if (savedRequestState == null ||
!savedRequestState.getState().equals(state)) {
+ if (session == null && enforceValidState) {
logger.warn("The received state does not match the state saved in
the context");
throw new BadCredentialsException("The received state does not
match the state saved in the context");
+ } else if (session != null) {
+ RequestState savedRequestState = (RequestState)
session.getAttribute(SAVED_CONTEXT);
+ session.removeAttribute(SAVED_CONTEXT);
+ String state = getState(request);
+
+ if (enforceValidState && (savedRequestState == null ||
!savedRequestState.getState().equals(state))) {
Review comment:
I think the logic here could be improved a bit (and for the other plugins).
If we have !enforceValidState then I still think we should throw an exception
if savedRequestState is non null +
!savedRequestState.getState().equals(state))).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> 'wctx' parameter mandatory but protocol does not require
> --------------------------------------------------------
>
> Key: FEDIZ-232
> URL: https://issues.apache.org/jira/browse/FEDIZ-232
> Project: CXF-Fediz
> Issue Type: Bug
> Reporter: Christian Fischer
> Priority: Major
>
> For logins which are not initiated by a valid session on the RP side the user
> cannot be authenticated because the wctx parameter is missing or has the
> wrong value.
> There are at least two scenarios in which this causes a unwanted behaviour of
> the system.
> * First is if the IDP/login page is bookmarked and returns only later after
> the session on the RP is timed out.
> * Second is something similar to a IDP initiated login flow. It's not in the
> WS federation protocol specification but according to our tests fediz could
> easily allow that if the 'wctx' check is removed.
> In the protocol specification the 'wctx' parameter is also only optional,
> where fediz expects it to be always present. There is a comment with respect
> to CSRF prevention but our security team didn't see the case for this since
> there is no passive way of authentication is used. In fact it's the actual
> authentication request that is supposed to be protected, but we don't see the
> need.
>
> One option (if the CSRF case is valid) would be to at least disable the
> 'wctx' state validation by setting a flag.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
