Page MenuHomePhabricator

let pht expect PhutilSafeHTMLProducerInterface
ClosedPublic

Authored by avivey on Jul 7 2016, 12:05 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 16, 1:50 PM
Unknown Object (File)
Mon, Dec 16, 11:48 AM
Unknown Object (File)
Sun, Dec 15, 5:59 PM
Unknown Object (File)
Thu, Dec 12, 1:35 AM
Unknown Object (File)
Tue, Dec 10, 8:21 PM
Unknown Object (File)
Sat, Dec 7, 11:57 AM
Unknown Object (File)
Fri, Dec 6, 1:32 PM
Unknown Object (File)
Thu, Dec 5, 4:34 PM
Subscribers

Details

Summary

phutil_escape_html can handle these objects, but pht needs to know to call it.

Test Plan

Render a couple of things? I'm not sure how much damage this can do.

Diff Detail

Repository
rPHU libphutil
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

avivey retitled this revision from to let pht expect PhutilSafeHTMLProducerInterface.
avivey updated this object.
avivey edited the test plan for this revision. (Show Details)
avivey added a reviewer: epriestley.

You've mentioned we might need this in D16236, and my quick test worked, but that was because I also had a renderAuthor() in my test input, which triggered the "this is html" flag.

Alternatively, I can just add another ->render() in D16236, which is there in the existing Transaction's renderHandle() (User's renderHandle() doesn't "render" HTML, but produces a PHUIView).

Or maybe just pretend everything is HTML in this function, so we escape all inputs?

epriestley edited edge metadata.

We should possibly make PhutilSafeHTML implement PhutilSafeHTMLProducerInterface (just as return $this;, I think) to simplify this and a couple of other checks, but that's just a tiny implementation detail.

Calling render() means we have to load the handle immediately, so that's not desirable.

I believe escaping only when necessary is a (possibly significant?) optimization.

The data access pattern here is probably still not great -- I think pht(..., $this->renderHandle()) will end up loading the handle before pht() returns. But that's fixable (by making pht() return an object which evaluates lazily, at least some of the time) as long as we don't call render() inline.

This revision is now accepted and ready to land.Jul 7 2016, 12:15 AM
This revision was automatically updated to reflect the committed changes.