From c9ef7aeaa34a32042a3974e88b8bd1b650f47221 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sun, 5 Jun 2016 10:23:28 -0700
Subject: [PATCH] Validate select/option settings more strictly when reading
 them

Summary:
Ref T4103. If the database has `""` (empty string) for select/option settings, we can let that value be effective in the UI right now.

One consequence is that timestamps can vanish from the UI.

Instead, be stricter and discard it as an invalid value.

Test Plan:
  - Forced `time-format` setting to `''`.
  - Saw timestamps vanish before change.
  - Saw timestamps return to the default value after change.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4103

Differential Revision: https://secure.phabricator.com/D16047
---
 .../setting/PhabricatorSelectSetting.php      | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/src/applications/settings/setting/PhabricatorSelectSetting.php b/src/applications/settings/setting/PhabricatorSelectSetting.php
index 44369baa46..bccf450454 100644
--- a/src/applications/settings/setting/PhabricatorSelectSetting.php
+++ b/src/applications/settings/setting/PhabricatorSelectSetting.php
@@ -27,6 +27,25 @@ abstract class PhabricatorSelectSetting
       ->setOptions($options);
   }
 
+  public function assertValidValue($value) {
+    // This is a slightly stricter check than the transaction check. It's
+    // OK for empty string to go through transactions because it gets converted
+    // to null later, but we shouldn't be reading the empty string from
+    // storage.
+    if ($value === null) {
+      return;
+    }
+
+    if (!strlen($value)) {
+      throw new Exception(
+        pht(
+          'Empty string is not a valid setting for "%s".',
+          $this->getSettingName()));
+    }
+
+    $this->validateTransactionValue($value);
+  }
+
   final public function validateTransactionValue($value) {
     if (!strlen($value)) {
       return;