Last modified: 2014-11-20 23:10:52 UTC
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.
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
