Add h() for html escaping (Trac #4397)

[elgg-gitbot]

Original ticket http://trac.elgg.org/ticket/4397 on 42168601-11-04 by ewinslow, assigned to unknown.

Elgg version: 1.8

htmlspecialchars($text, ENT_QUOTES, 'UTF-8')??? Who wants to type all that?

h($text) is awesome.

[elgg-gitbot]

cash wrote on 42178713-02-12

h() is a bad function name.

[elgg-gitbot]

ewinslow wrote on 42178823-05-12

Its pretty common to use h for html escaping in templates, in my experience.
Why do you think its bad? In terms of descriptive, definitely definitely the exception.

Do you have an alternative?

[elgg-gitbot]

cash wrote on 42189245-01-09

I believe one of our goals is to properly namespace Elgg's functions and classes so that external libraries can be used without conflicts.

[elgg-gitbot]

ewinslow wrote on 42189411-01-17

if(!function_exists())?

[elgg-gitbot]

cash wrote on 42189449-04-28

1. you lose control over functionality when you do that
2. it only works if the library also does a function_exists() call

[elgg-gitbot]

ewinslow wrote on 42189657-06-03

Mmm. Good points.

So... alternatives?

[elgg-gitbot]

ewinslow wrote on 42277756-11-02

elgg_h?

[elgg-gitbot]

ewinslow wrote on 42365161-08-09

elgg_h feels like proper namespacing to me. Slating this for 1.9.

[elgg-gitbot]

Milestone changed to Elgg 1.9.0 by ewinslow on 42365161-08-09

[elgg-gitbot]

brettp wrote on 42365167-01-21

I really don't like elgg_h(). It has no intrinsic meaning. If we're aiming for brevity, elgg_html() at least carries some meaning as to the function's purpose.

[elgg-gitbot]

cash wrote on 42370198-11-16

Agree with Brett on not liking elgg_h(). Why do we care about brevity? I almost never type more than "elgg_xx" to get the function that I want due to function hinting. I prefer elgg_escape().

[elgg-gitbot]

ewinslow wrote on 42378459-11-28

It's not just for brevity, but precedent.

[elgg-gitbot]

ewinslow wrote on 42378467-01-26

This may be a moot issue because really the only viable way to fight against XSS is by using context-aware, auto-escaped templates, and PHP does not fit that bill.

[elgg-gitbot]

trac user sembrestels wrote on 42532477-12-02

We also need cover

htmlspecialchars($text, ENT_NOQUOTES, 'UTF-8');

or

htmlspecialchars($text, ENT_QUOTES, 'UTF-8', false);

(+1 to elgg_escape)

[elgg-gitbot]

cash wrote on 42534323-05-08

I think we are going to need context aware escaping functions.

I was thinking something like this:

$title = ElggFormatter::escapeHTML($title);

$attribute = ElggFormatter::escapeAttribute($attribute);

It is more verbose, but I'm ok with that. The attribute escape function understands its context and uses the appropriate escaping which is different from the escaping needed for plain html text.

[elgg-gitbot]

ewinslow wrote on 42534708-11-03

Meh. Now I'm getting nervous. We've been down this road at Google and providing escaping functions is basically hopeless. Context-aware template systems are much more effective at actually mitigating the xss attacks.

[elgg-gitbot]

cash wrote on 42534724-11-27

Hopeless because it relies on the developers to use them correctly or hopeless for some other reason?

[elgg-gitbot]

ewinslow wrote on 42534748-12-04

Relies on devs to use them right.

[elgg-gitbot]

cash wrote on 42534789-08-01

Our options for Elgg 1.9 are

1. sprinkle htmlspecialchars() calls through our views with different parameters based on context
2. add some functions that make the context that they should be used in explicit and sprinkle those in our views
3. do nothing
4. rip out the current view system and replace with a context-aware template system
5. use Content Security Policy and do away with all inline CSS/JavaScript

Any other options?

[elgg-gitbot]

ewinslow wrote on 42535106-01-11

Use a context aware template system without replacing the current views system.

[elgg-gitbot]

cash wrote on 42536685-06-04

Any of the context aware template systems that I have seen in PHP involve a preprocessor that compiles the templates to PHP. That may not involve ripping out the current views system, but does mean rewriting all the views - correct?

[elgg-gitbot]

ewinslow wrote on 42537172-12-04

I don't see it being an all or nothing approach. We can convert views to autoescape gradually as we discover vulnerabilities.

[elgg-gitbot]

cash wrote on 42537893-11-27

I think we mean different things by autoescaping - sounds like you may mean that we add code to a view so that the input is escaped by default (say for the title field of the module view). I'm talking about a template language other than PHP that gets compiled to PHP with escaping built in.

[elgg-gitbot]

ewinslow wrote on 42540092-09-21

We mean the same thing. We convert views to use the template language gradually.

[elgg-gitbot]

cash wrote on 43101183-11-20

See #4989

[elgg-gitbot]

Milestone changed to Long Term Discussion by cash on 43101183-11-20

[mrclay]

FTR my votes for general purpose would be elgg_escape_body and elgg_escape_attr. Inside views I have a few ideas to make this more brief:

• In views, permit short echo tag <?=. This is now always enabled in 5.4 and I've come around to it.
• Add body() and attr() methods to ElggViewService. This makes them available via $this in view scripts.
• Before including the view script, set $b_ = 'elgg_escape_body'. This makes $b_(...) available in views. Yes this is kind of a hack :)

[cash]

I'd really like to get something like elgg_escape_body and elgg_escape_attr into core. We can deal with the best short cuts for usage later.

[ewinslow]

Not going to do this as proposed since I am so thoroughly convinced that contextually auto-escaped templates are the only reasonable way to actually ensure safety.

[mrclay]

FYI in the past I started work on this a few times and ran into the dilemma of whether or not to allow double escaping. Auto-escaped templates are going to hit the same problem. The frustration of dealing with occasional problems with over-escaping might be worth the savings of very rare XSS bugs.