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

Confirm vendor BC break has no impact #3856

Closed
craigh opened this Issue Nov 17, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@craigh
Copy link
Member

craigh commented Nov 17, 2017

Q A
Zikula Version 1.5 -> 2.0

Ensure this BC break does not impact the zikula core. or take corrective action to ensure a bug is not created affecting end users.

http://symfony.com/blog/cve-2017-16653-csrf-protection-does-not-use-different-tokens-for-http-and-https

Note that this patch introduces a small BC break (the token ID can change, and isn't the one specified by the user in some cases) but has the benefit of fixing all use cases:

Form component;
Login and logout security listeners;
Direct usage of the security.csrf.token_manager service;
Direct usage of the CsrfTokenManager (component without the full stack framework) even if not used with HttpFoundation.
The previous behavior can be restored by using '' as namespace.

@craigh craigh added this to the 1.5.4 milestone Nov 17, 2017

@craigh craigh added the Blocker label Nov 17, 2017

@Guite

This comment has been minimized.

Copy link
Member

Guite commented Dec 8, 2017

From looking at the patch I think the behaviour shouldn't change unless very rare edge cases where the scheme is switched between two pages, for example:

  • a non-ssl page has a link including a token to a ssl page
  • a ssl page has a form pointing to a non-ssl page

The core does not enforce any schemes by default (for example by route conditions), so I think we are fine.

@craigh

This comment has been minimized.

Copy link
Member

craigh commented Dec 8, 2017

thanks for evaluating. Should we note the BC break somehow?

@Guite

This comment has been minimized.

Copy link
Member

Guite commented Dec 8, 2017

probably worth a changelog entry

@Guite

This comment has been minimized.

Copy link
Member

Guite commented Dec 11, 2017

Added in 2638095 and da0d949

@Guite Guite closed this Dec 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment