loggerhead

Merge lp://qastaging/~wgrant/loggerhead/bug-740142 into lp://qastaging/loggerhead

bug-740142
Merge into trunk-rich

Proposed by William Grant on 2011-03-23

Status:

Merged

Approved by:

Robert Collins on 2011-03-23

Approved revision:

448

Merged at revision:

442

Proposed branch:

lp://qastaging/~wgrant/loggerhead/bug-740142

Merge into:

lp://qastaging/loggerhead

Diff against target:

247 lines (+96/-21)

6 files modified

loggerhead/controllers/view_ui.py (+1/-2)
loggerhead/templatefunctions.py (+20/-12)
loggerhead/tests/__init__.py (+1/-0)
loggerhead/tests/test_simple.py (+7/-3)
loggerhead/tests/test_util.py (+33/-0)
loggerhead/util.py (+34/-4)

To merge this branch:

bzr merge lp://qastaging/~wgrant/loggerhead/bug-740142

Critical

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Robert Collins		2011-03-23	Approve on 2011-03-23
Review via email: mp+54463@code.qastaging.launchpad.net

Commit message

Properly escape filenames throughout loggerhead.templatefunctions.

Description of the change

loggerhead.templatefunctions uses cgi.escape to do all its escaping: for element values, attribute values, and URLs. But cgi.escape(foo) is only OK for element values; it fails to escape double or single quotes, allowing content to break out of quoted attributes. It also doesn't do much for URLs at all.

This branch fixes all the holes I could find after a quickish examination. I've introduced a new html_format function which does safe HTML template formatting. All of loggerhead.templatefunction's HTML generation now uses html_format. SimpleTAL will only let content through unescaped when "structure" is used in the template, and all referenced functions seem to be safe now.

templatefunctions also failed to URL-encode some URL fragments. I can't think of any significant damage that could be done here besides breaking the page, but it was an easy and relevant fix necessary for testing.

lp://qastaging/~wgrant/loggerhead/bug-740142 updated on 2011-03-23

447. By William Grant on 2011-03-23: Add forgotten test_util.

Revision history for this message

Robert Collins (lifeless) wrote on 2011-03-23:

Two things...
firstly, you probably want to copy the xml serialiser regex bzrlib has - its perf tested (we may render 10K filenames on a single page...).

And have you checked for performance impacts?

lp://qastaging/~wgrant/loggerhead/bug-740142 updated on 2011-03-23

448. By William Grant on 2011-03-23: Use str.replace instead of lots of dict lookups.

Revision history for this message

Robert Collins (lifeless) wrote on 2011-03-23:

cool

review: Approve

Revision history for this message

John A Meinel (jameinel) wrote on 2011-03-23:

I did some testing of this branch with non-ascii filenames and all the URLs still worked. (at least of what I tested.)

I think it would be good to have more tests that assert we don't include the raw content of filenames and file content in all the pages we serve, but this is certainly a good start.