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
Branch
qbold
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 70
Build 70: [Placeholder Plan] Wait for 30 Seconds

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 21023

View Options

src/applications/search/view/PhabricatorSearchResultView.php

Loading...
Phabricator · Built by Phacility