Page MenuHomePhabricator
Differential D8862

Write search bolding in a way which is certainly HTML-safe
ClosedPublic
Actions

Authored by epriestley on Apr 25 2014, 7:20 PM.
Tags
None
Referenced Files
F14033357: D8862.id21043.diff
Sat, Nov 9, 5:32 PM
F14033356: D8862.id21023.diff
Sat, Nov 9, 5:31 PM
F14033355: D8862.id21022.diff
Sat, Nov 9, 5:31 PM
F14033354: D8862.id.diff
Sat, Nov 9, 5:31 PM
F14020535: D8862.diff
Wed, Nov 6, 12:41 AM
F14016052: D8862.id21023.diff
Mon, Nov 4, 4:44 AM
F14016050: D8862.id21022.diff
Mon, Nov 4, 4:44 AM
F14016049: D8862.id21043.diff
Mon, Nov 4, 4:44 AM
View All Files
Subscribers
epriestley
rugabarbo

Details

Reviewers
btrahan
Commits
Restricted Diffusion Commit
rP88ae24659396: Write search bolding in a way which is certainly HTML-safe
Summary

This algorithm is tricky, and uses phutil_safe_html() directly, which makes it potentially unsafe.

In particular, D8859 fixes a bug with it which caused it to produce non-utf8 output. This doesn't guarantee it's a security problem, but does make it suspicious.

I don't actually see a way to break it, but rewrite it so that it's absolutely bulletproof and does not need to call phutil_safe_html().

Test Plan

Screen_Shot_2014-04-25_at_12.17.32_PM.png (1×1 px, 166 KB)

@rugabarbo, if you have a chance, can you check if this still works for you?

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

epriestley updated this revision to Diff 21022.Apr 25 2014, 7:20 PM
epriestley retitled this revision from to Write search bolding in a way which is certainly HTML-safe.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: btrahan.
epriestley added a subscriber: rugabarbo.
Herald added a subscriber: epriestley. · View Herald TranscriptApr 25 2014, 7:20 PM
Harbormaster completed remote builds in B69: Diff 21022.Apr 25 2014, 7:21 PM
epriestley updated this revision to Diff 21023.Apr 25 2014, 7:21 PM
  • Remove unnecessary line.
Harbormaster completed remote builds in B70: Diff 21023.Apr 25 2014, 7:22 PM
rugabarbo added a comment.Apr 25 2014, 7:39 PM

@epriestley, now I have no access to iMac, where I found bug D8859 (it appeared only for Mac OS X phabricator instance).
I get access to it only two days later :-(

If you can wait I will apply this patch for Mac OS X phabricator two days later...

Now I only have a laptop with "Ubuntu 14.04 LTS".
But here I couldn't reproduce this bug.

Phabricator instance on ubuntu works fine without D8859.
And it's magic for me.

btrahan accepted this revision.Apr 26 2014, 2:31 AM
btrahan edited edge metadata.

Seems unit testable if you anticipate more changes / swapping around implementations.

This revision is now accepted and ready to land.Apr 26 2014, 2:31 AM
epriestley closed this revision.Apr 26 2014, 7:44 PM
epriestley updated this revision to Diff 21043.

Closed by commit rP88ae24659396 (authored by @epriestley).

epriestley added a comment.Apr 26 2014, 7:46 PM

(I'm going with a gut feeling that unit tests on this probably won't pay for themselves, but I'll write some to repent if this gets touched again.)

rugabarbo added a comment.EditedApr 28 2014, 4:12 PM

@epriestley, I checked it for Mac OS X Phabricator installation (where I found this bug first time).
Now it works fine (search results aren't corrupted any more and bolding works fine too).

Thanks!

Revision Contents
Changeset List

PathSize
src/
applications/
search/
view/
101 lines

Diff 21043

View Options

src/applications/search/view/PhabricatorSearchResultView.php

Loading...
Phabricator · Built by Phacility