fixed the xss issue in layoutTitle #244

Merged
merged 2 commits into from Jul 31, 2012

Conversation

Projects
None yet
4 participants
@bobbywarner

This comment has been minimized.

Show comment
Hide comment
@bobbywarner

bobbywarner Jul 31, 2012

Member

I was trying to figure out how I forgot the ? like everyone was asking, but it makes sense now. No ? needed if I would have put it in the right place, ha! :)

Member

bobbywarner commented Jul 31, 2012

I was trying to figure out how I forgot the ? like everyone was asking, but it makes sense now. No ? needed if I would have put it in the right place, ha! :)

burtbeckwith added a commit that referenced this pull request Jul 31, 2012

Merge pull request #244 from bobbywarner/GRAILS-9052
fixed the xss issue in layoutTitle

@burtbeckwith burtbeckwith merged commit 6bc7dd8 into grails:master Jul 31, 2012

@marcpalmer

This comment has been minimized.

Show comment
Hide comment
@marcpalmer

marcpalmer Aug 1, 2012

Contributor

Guys this is a breaking change. People who have encoded it manually in their GSPs because they know the problem will now get broken apps that double encode the titles.

Contributor

marcpalmer commented Aug 1, 2012

Guys this is a breaking change. People who have encoded it manually in their GSPs because they know the problem will now get broken apps that double encode the titles.

@pledbrook

This comment has been minimized.

Show comment
Hide comment
@pledbrook

pledbrook Aug 1, 2012

Member

Also, what happens if a user has GSP defaulting to 'html', which is kind of what we want to head towards anyway?

Member

pledbrook commented Aug 1, 2012

Also, what happens if a user has GSP defaulting to 'html', which is kind of what we want to head towards anyway?

@bobbywarner

This comment has been minimized.

Show comment
Hide comment
@bobbywarner

bobbywarner Aug 2, 2012

Member

@marcpalmer Yes, there's certainly the possibility of double encoding. I didn't realize double encoding would be a breaking change though. I thought this would be good to fix as it's potentially something that could be easily missed by users. If it's a big problem to possibly double encode, then obviously it should be removed. Please let me know.

@pledbrook Right, default html encoding would be the best, but given previous discussions, it was decided that the default setting in config.groovy won't be changed until at least 3.0. I was thinking that we probably don't want there to be a potential xss vulnerability by default until then (most users probably have no idea they need to encode the title). Please let me know.

Member

bobbywarner commented Aug 2, 2012

@marcpalmer Yes, there's certainly the possibility of double encoding. I didn't realize double encoding would be a breaking change though. I thought this would be good to fix as it's potentially something that could be easily missed by users. If it's a big problem to possibly double encode, then obviously it should be removed. Please let me know.

@pledbrook Right, default html encoding would be the best, but given previous discussions, it was decided that the default setting in config.groovy won't be changed until at least 3.0. I was thinking that we probably don't want there to be a potential xss vulnerability by default until then (most users probably have no idea they need to encode the title). Please let me know.

@pledbrook

This comment has been minimized.

Show comment
Hide comment
@pledbrook

pledbrook Aug 2, 2012

Member

@bobbywarner The problem is if the user sets the default codec to "html" (which is probably recommended) and then has something like <title>${someTitle}</title> in their GSP views. I think this is a problem that needs to be solved holistically with a root & branch overhaul of default codecs. Without it, you end up with inconsistencies and breakages.

For example, if we change <g:layoutTitle>, shouldn't we also change <g:layoutHead> and <g:layoutBody>?

Member

pledbrook commented Aug 2, 2012

@bobbywarner The problem is if the user sets the default codec to "html" (which is probably recommended) and then has something like <title>${someTitle}</title> in their GSP views. I think this is a problem that needs to be solved holistically with a root & branch overhaul of default codecs. Without it, you end up with inconsistencies and breakages.

For example, if we change <g:layoutTitle>, shouldn't we also change <g:layoutHead> and <g:layoutBody>?

@bobbywarner

This comment has been minimized.

Show comment
Hide comment
@bobbywarner

bobbywarner Aug 3, 2012

Member

Ok, should I send a pull request to revert the change? Please let me know.

Member

bobbywarner commented Aug 3, 2012

Ok, should I send a pull request to revert the change? Please let me know.

@bobbywarner

This comment has been minimized.

Show comment
Hide comment
@bobbywarner

bobbywarner Aug 6, 2012

Member

Change reverted in pull request #246.

Member

bobbywarner commented Aug 6, 2012

Change reverted in pull request #246.

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