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

Security: XSS can be injected in the group edit view #151

Merged
merged 1 commit into from Jul 31, 2014
Merged

Security: XSS can be injected in the group edit view #151

merged 1 commit into from Jul 31, 2014

Conversation

stojg
Copy link

@stojg stojg commented Jul 31, 2014

No description provided.

// Prevent XSS injection
foreach($subsiteMap as $key => $map) {
$subsiteMap[$key] = Convert::raw2xml($map);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need the loop. Just $subsiteMap = Convert::raw2xml($subsiteMap);

Copy link
Author

Choose a reason for hiding this comment

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

True, I forgot that it can take an array type as well

@stojg
Copy link
Author

stojg commented Jul 31, 2014

Rebased.Removed the convert from SiteTreeSubsites since it's already converted, courtesy of the dropdown where the subsites are showing up.

@tractorcow
Copy link
Contributor

Perhaps I'm wrong, but why isn't this a bug with the CheckboxSetField and DropdownField classes? Why don't they safely escape field labels?

@halkyon
Copy link
Contributor

halkyon commented Jul 31, 2014

The templates should be escaping in CheckboxSetField.ss and DropdownField.ss. It seems DropdownField is already doing that, so escaping Title in this PR is redundant.

CheckboxSetField might have a bug where it doesn't escape the label properly, though.

@stojg
Copy link
Author

stojg commented Jul 31, 2014

Removed the escaping from the DropDownField, checkboxsetfield doesn't. For now I'm escaping the checkbox fields until we get this in framework.

@halkyon
Copy link
Contributor

halkyon commented Jul 31, 2014

I'll merge this for now, and will have a look at fixing it in the core.

halkyon added a commit that referenced this pull request Jul 31, 2014
Security: XSS can be injected in the group edit view
@halkyon halkyon merged commit ccf125a into silverstripe:master Jul 31, 2014
@tractorcow
Copy link
Contributor

We can always change the behaviour in 3.2, since this module will need a separate for for that anyway.

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

Successfully merging this pull request may close these issues.

None yet

4 participants