Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IBX-833: As a Developer I want to configure CSRF validation in REST endpoints #76

Merged
merged 1 commit into from
Aug 2, 2021

Conversation

adamwojs
Copy link
Member

@adamwojs adamwojs commented Jul 30, 2021

Question Answer
JIRA issue IBX-833
Type improvement
Target version 3.3.7+
BC breaks no
Tests pass yes
Doc needed yes

Allow to configure CSRF validation in REST endpoints using 

csrf_protection: false

attribute in route definition.

Currently to achive the same efect \EzSystems\EzPlatformRestBundle\EventListener\CsrfListener::isSessionRoute needs to be overriden:

class CsrfListener implements EventSubscriberInterface
   # ...
    protected function isSessionRoute($route)
    {
        return in_array(
            $route,
            ['ezpublish_rest_createSession', 'ezpublish_rest_refreshSession', 'ezpublish_rest_deleteSession']
        );
    }
    # ...
}

Example use cases:

  • Forgot password implementation via REST

TODO:

  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@adamwojs adamwojs requested review from kmadejski and a team July 30, 2021 05:58
@adamwojs adamwojs self-assigned this Jul 30, 2021
@adamwojs adamwojs added the Doc needed The changes require some documentation label Jul 30, 2021
@adamwojs adamwojs requested a review from Nattfarinn July 30, 2021 05:59
@sonarcloud
Copy link

sonarcloud bot commented Jul 30, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@@ -143,6 +147,8 @@ protected function isLoginRequest($route)
* @param string $route
*
* @return bool
*
* @deprecated since Ibexa DXP 3.3.7. Add csrf_protection: false attribute to route definition instead.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use trigger_deprecation from Symfony contracts to log situations where isSessionRoute returns true? Does it make sense?

@adamwojs adamwojs requested a review from a team July 30, 2021 06:37
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: doesn't this open us to potential security issue?

@adamwojs
Copy link
Member Author

Q: doesn't this open us to potential security issue?

No, CSRF protection is enabled by default for all POST/PUT/etc endpoints. If you needs to disable CSRF protection (in mentioned use case is required) then you must configure it explicitly.

@alongosz
Copy link
Member

Q: doesn't this open us to potential security issue?

No, CSRF protection is enabled by default for all POST/PUT/etc endpoints. If you needs to disable CSRF protection (in mentioned use case is required) then you must configure it explicitly.

My question was rather about is it safe to disable that for session endpoints?

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarified during internal sync.

@bogusez bogusez self-assigned this Aug 2, 2021
@adamwojs adamwojs merged commit 6022a51 into 1.3 Aug 2, 2021
@adamwojs adamwojs deleted the ibx_833 branch August 2, 2021 11:50
@DominikaK DominikaK removed the Doc needed The changes require some documentation label Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants