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

Fixes #16971 - CVE-2016-7077 remove unauthorized checkboxes #3955

Merged
merged 1 commit into from Nov 9, 2016

Conversation

ares
Copy link
Member

@ares ares commented Oct 19, 2016

This is one way to fix it - add authorization for checkboxes including hidden field to keep existing associations that user can't modify. The other way would be removing the checkboxes completely and always use multiple_selects, which I find better and more safe.

I haven't find any recommendation for this in patternfly, @Rohoover your input would be appreciated. When we have n:m associations we use checkboxes if there are less than 6 objects, and more sophisticated multiple selector otherwise. Would it be better/worse if we used only one form element, the multiple select one?

Illustration of current form elements for less than 6 options
checkboxes1

for 6 and more
multiple1

possible change to have the same thing for less items too
multiple2

@Rohoover
Copy link

@ares One other option that may work for both use case scenarios found in PatternFly, is the multiple select option.
http://www.patternfly.org/pattern-library/widgets/#bootstrap-select

screen shot 2016-10-19 at 8 01 21 am

Thoughts?

@ares
Copy link
Member Author

ares commented Oct 19, 2016

Thanks for prompt response. I like the patternfly multiple select bootstrap-select also seems to support live search and select all features. Since this is security fix, we'd like to keep the patch minimal, which means I'd like to avoid changing all forms in Foreman in this PR. That does not mean we can't open a separate issue to migrate to patternfly multiple select if others agree. If I read it correctly, you also suggest using one widget for any number of items, correct?

@Rohoover
Copy link

@ares In the name of consistency, I would prefer to use one widget for the same type of interaction, but also understand that incremental steps may be necessary for full implementation. Fixing it in this one instance, and opening another PR for all others sounds reasonable. Thanks!

@ares
Copy link
Member Author

ares commented Oct 20, 2016

Thanks Roxanne! I'll change this PR so we use only one of two widgets we have which gets us closer to migrate to patternfly widget too.

@ares ares changed the title Fixes #16971 - CVE-2016-7077 authorize associations checkboxes Fixes #16971 - CVE-2016-7077 remove unauthorized checkboxes Oct 20, 2016
@dLobatog
Copy link
Member

@ares Tests are failing, other than that 👍 at first glance, all this does is to remove the paths for < 6 elements. Thanks !

@ares
Copy link
Member Author

ares commented Oct 25, 2016

I fixed the integration tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants