Page MenuHomePhabricator

Security review of OOjs UI's PHP implementation
Closed, ResolvedPublic

Description

Nothing too concerning with what you're doing. Security is roughly the same as using Html/Xml classes at this point.

The only thing I'd really like to see changed is in php/widgets/InputWidget.php, the "sanitizeValue" function doesn't do any (security) sanitization, which I think that could cause confusion later on. If the name can't be changed, maybe make the comments explicit that it's not security sanitization?

It would also be nice to have some extra sanitization built in from the start, which we can't do in the Html/Xml classes since they're abused in odd ways, but have bitten some developers (SemanticForms had bunch of issues because they assumed these happened):

  • Validate tag name will be parsed in html as a single tag name-- so doesn't contain whitespace, /, >, or null.
  • Validate attribute names don't contain whitespace, /, =, >
  • Validate that form actions and button hrefs aren't javascript: urls

There are also a couple of places you're adding style attributes directly. Is it possible to avoid that? My long-term plan is to have MediaWiki set a Content Security Policy that doesn't allow inline css, so I'd prefer to not introduce new uses of it, if possible.


Version: wmf-deployment
Severity: normal

Details

Reference
bz73156

Event Timeline

bzimport raised the priority of this task from to Needs Triage.Nov 22 2014, 3:58 AM
bzimport set Reference to bz73156.

Thanks for the review. Good points, let's do this.

(In reply to Chris Steipp from comment #0)

There are also a couple of places you're adding style attributes directly.
Is it possible to avoid that? My long-term plan is to have MediaWiki set a
Content Security Policy that doesn't allow inline css, so I'd prefer to not
introduce new uses of it, if possible.

It can't be avoided in all cases, for example GridLayout depends on them – can't be fixed without a major refactoring or even just killing the things. We also want to have the same behavior in PHP as in JS (or a subset), whenever possible.

We could probably get rid of them in at least some places, such as LabelElement. I'll look into them.

Change 174814 had a related patch set uploaded by Bartosz Dziewoński:
LinkTargetInputWidget: Update for #sanitizeValue → #cleanUpValue OOUI change

https://gerrit.wikimedia.org/r/174814

Change 174815 had a related patch set uploaded by Bartosz Dziewoński:
[BREAKING CHANGE] Rename InputWidget#sanitizeValue → #cleanUpValue

https://gerrit.wikimedia.org/r/174815

Change 174835 had a related patch set uploaded by Bartosz Dziewoński:
PHP: Reject malformed and potentially evil input when outputting HTML

https://gerrit.wikimedia.org/r/174835

(In reply to Chris Steipp from comment #0)

The only thing I'd really like to see changed is in
php/widgets/InputWidget.php, the "sanitizeValue" function doesn't do any
(security) sanitization, which I think that could cause confusion later on.
If the name can't be changed, maybe make the comments explicit that it's not
security sanitization?

https://gerrit.wikimedia.org/r/174815
https://gerrit.wikimedia.org/r/174814

It would also be nice to have some extra sanitization built in from the
start, which we can't do in the Html/Xml classes since they're abused in odd
ways, but have bitten some developers (SemanticForms had bunch of issues
because they assumed these happened):

  • Validate tag name will be parsed in html as a single tag name-- so doesn't

contain whitespace, /, >, or null.

  • Validate attribute names don't contain whitespace, /, =, >
  • Validate that form actions and button hrefs aren't javascript: urls

https://gerrit.wikimedia.org/r/174835
https://gerrit.wikimedia.org/r/174833 (dependency)
https://gerrit.wikimedia.org/r/174834 (dependency)

There are also a couple of places you're adding style attributes directly.
Is it possible to avoid that? My long-term plan is to have MediaWiki set a
Content Security Policy that doesn't allow inline css, so I'd prefer to not
introduce new uses of it, if possible.

So in the end, there are only three such places in PHP code:

  • LabelElement. This one already came up earlier as a code quality issue, actually. Should be fixable, but preferably not right now :) Filed bug 73677 so it's not forgotten.
  • GridLayout. Alas, inline styles are a core part of how it works and the only way to fix it would be to remove the class entirely.
  • widgets.php demo, where it's used on a GridLayout (and is also necessary).

Change 174844 had a related patch set uploaded by Bartosz Dziewoński:
LabelElement: Kill inline styles

https://gerrit.wikimedia.org/r/174844

Jdforrester-WMF assigned this task to matmarex.
Jdforrester-WMF triaged this task as High priority.
Jdforrester-WMF added a project: OOUI.
Jdforrester-WMF set Security to None.