Page MenuHomePhabricator

Improve monospaced font preference validation
AbandonedPublic

Authored by asherkin on Oct 9 2013, 3:07 PM.

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Summary

Currently, various valid font names are rejected even when quoted, and any bad character just get silently stripped. Attempt to improve UX by allowing all characters when properly quoted, and provide proper error messages upon failure.

Test Plan

Set various different font strings.

Diff Detail

Branch
fonterror
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

The validation here is still a lot stricter than the CSS spec (no single quotes being the major one), but it seemed a reasonable compromise - an alternative would be to just strip the "very bad" characters ({}<>) which would also cover the intended purpose.

One thing I couldn't decide on when implementing was whether the other fields should get saved when there is an error - it's implemented like that here, but thinking about it more it's probably a bad idea (and the user can hit 'back' in their browser to 'refill' the form, which makes it less painful) - let me know which is preferred!

epriestley requested changes to this revision.Oct 9 2013, 3:35 PM

I was able to break out of the sandbox and write arbitrary CSS. I suspect I can write arbitrary JS, too, although my working copy is now so broken that I can't easily edit the setting anymore. :P

{F66290}

I'll accept the addition of literal period to the regexp. I'm worried about the security of solutions which blacklist "bad" characters or try to parse arbitrary expressions. For example, if we strip {}<> users can still include remote resources, and use CSS expressions, and break CSS and write arbitrary rules. The primary purpose of stripping is security, but a secondary purpose is that the user shouldn't be able to enter any value in to the field which renders their account unusable. With enough refinement, we can likely come up with some blacklist which I can't find any counterexamples for, but as a general matter of policy this should be a whitelist. See http://www.phabricator.com/docs/phabricator/article/Things_You_Should_Do_Now.html#never-design-a-blacklist. (I'm not really even confident the existing regexp is strict enough after seeing how difficult this problem was at Facebook.)

For consistency, we shouldn't save() if there are errors. The pattern generally looks like this:

if ($request->isDialogFormPost()) {
  $errors[] = ...
  if (!$errors) {
    $object->save();
    redirect;
  }
}

render the form;

We should also echo back the bad input so it's easier to correct when showing errors. For instance, if the user enters "X>Y" and we reject it, the input should be prefilled with "X>Y" when the page loads.

For what it's worth, the trick I used to break out was:

"\"x"; } <I can type whatever I want here> /* "

D10163 adds a literal period to the regexp.

asherkin abandoned this revision.Nov 16 2014, 2:55 AM
epriestley changed the visibility from "All Users" to "Public (No Login Required)".Feb 27 2018, 1:23 PM

(Relevant to T4340.)