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

Sanitize dojo components #849

Merged
merged 2 commits into from
Dec 2, 2014

Conversation

LukasReschke
Copy link
Contributor

First bunch of fixes… More likely to come later…

@thz @VicDeo @karlitschek FYI

@kogmbh-ci
Copy link

Can one of the admins verify this patch?

@kossebau
Copy link
Contributor

kossebau commented Dec 1, 2014

ok to test

@kogmbh-ci
Copy link

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/2283/

@kossebau
Copy link
Contributor

kossebau commented Dec 1, 2014

Hi @LukasReschke

More likely to come later…

So each day the next 24 days a new PR? :) That would be a nice Advent calendar for WebODF devs :)

Please make a separate PR for 3db09e5 given it is a separate topic (and also might mean separate discussion and time to find resolution, at least I would already comment that supporting only http: and https: might be too little).

@LukasReschke
Copy link
Contributor Author

So each day the next 24 days a new PR? :) That would be a nice Advent calendar for WebODF devs :)

Nice try ;-)

Please make a separate PR for 3db09e5 given it is a separate topic (and also might mean separate discussion and time to find resolution, at least I would already comment that supporting only http: and https: might be too little).

Absolutely. - I'll create a branch for this commit later (maybe even tomorrow so we have already 2/24 ;-))

@kogmbh-ci
Copy link

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/2284/

@LukasReschke LukasReschke changed the title Sanitize dojo components and also clickable links Sanitize dojo components Dec 1, 2014
@peitschie
Copy link
Contributor

Just for good record keeping, this is a partial fix for some of the issues highlighted in #724

@peitschie
Copy link
Contributor

The only (small) style issue I have with this patch is that dojox.html.entities is not a class. We'd generally use a lowercase first letter to indicate this (e.g., htmlEntities).

Other than that, the patch looks good from my end. @kossebau ?

@LukasReschke
Copy link
Contributor Author

Changed to lower-case.

@kogmbh-ci
Copy link

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/2285/

@@ -97,6 +97,7 @@ add_custom_command(
COMMAND ${CMAKE_COMMAND} -E make_directory dojox/
COMMAND ${CMAKE_COMMAND} -E copy_directory dojo-deps/dist/dojox/layout/resources/ dojox/layout/resources/
COMMAND ${CMAKE_COMMAND} -E copy_directory dojo-deps/dist/dojox/widget/ColorPicker/ dojox/widget/ColorPicker/
COMMAND ${CMAKE_COMMAND} -E copy_directory dojo-deps/dist/dojox/html/ dojox/html/
Copy link
Contributor

Choose a reason for hiding this comment

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

This copying is not needed, as in this section only non-JavaScript files are copied. And unless I missed something when I just tested and had a look, html/entities.js is self-contained and does have all data inside. So adding 'dojox/html/entities' to the profile as done abovebelow is all that is needed to include that 'entities' unit.

"dojox/html/entities" will be used to sanitize the strings.
@kogmbh-ci
Copy link

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/2286/

@kossebau
Copy link
Contributor

kossebau commented Dec 2, 2014

sniff I will miss the marquee fun with style names :)
Otherwise looks good to me and worked for what I tested. So please remove that unneeded copying as mentioned, and also please add an entry to ChangeLog.md for this fix, either as separate commit or with the last, as a short release-notes-like entry. This spares us collecting changes at release time :)
Guess I will be more than tempted to press the "Merge" button then :)

@LukasReschke
Copy link
Contributor Author

So please remove that unneeded copying as mentioned, and also please add an entry to ChangeLog.md for this fix, either as separate commit or with the last, as a short release-notes-like entry.

Where would that match? - Under # Changes between 0.5.4 and 0.5.5 or does this require a new section?

@kossebau
Copy link
Contributor

kossebau commented Dec 2, 2014

The next release will be 0.5.5, so below the most top ## WebODF you would add

### Fixes

* Prevent JS injection from style names and font names ([#849](https://github.com/kogmbh/WebODF/pull/849)))

or similar.

@kossebau
Copy link
Contributor

kossebau commented Dec 2, 2014

Right, not only JS injection :)

@kogmbh-ci
Copy link

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/2287/

@kossebau
Copy link
Contributor

kossebau commented Dec 2, 2014

I see you stood the Turing test and adapted my wrong instructions and placed the entry at the right place :)
Thanks for the fix, and welcome to the club of WebODF committers, Lukas! 🎉

To frame things, 0.5.5 should be kicked out this year still, I personally target for Dec. 18, hoping to get some other things in until then, so would/could also be the window for anything you plan.
Next thing will be 0.6 then, as a port from Closure to Dojo has been started in the background, and hopefully should be done in January, and that would be worth a version bump.

kossebau pushed a commit that referenced this pull request Dec 2, 2014
@kossebau kossebau merged commit 4b06c28 into webodf:master Dec 2, 2014
@LukasReschke
Copy link
Contributor Author

🎉 - thanks! :-)

@LukasReschke LukasReschke deleted the sanitize-dojo-components branch December 2, 2014 15:02
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.

4 participants