From 65a56c6ce09266cd0bae0b4a3290ca91306ace75 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 11 Jan 2012 13:36:28 -0800
Subject: [PATCH] Improve mailing list edit form

Summary:
  - Add some captions to make it more clear what these fields mean.
  - Require "name", since tokenizers use it exclusively.
  - Limit URI to allowed protocols, since admins can currently XSS users by
entering a "javascript:" URI and then tricking the user into clicking the
mailing list name. This exploit is dumb, but technically privilege escallation.

Test Plan:
  - Created a new mailing list.
  - Edited a mailing list.
  - Tested URI: valid, invalid, omitted.
  - Tested name: valid, omitted.

Reviewers: btrahan, jungejason, davidreuss

Reviewed By: btrahan

CC: aran, btrahan

Differential Revision: https://secure.phabricator.com/D1365
---
 ...icatorMetaMTAMailingListEditController.php | 29 ++++++++++++++++++-
 .../controller/mailinglistedit/__init__.php   |  3 ++
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/src/applications/metamta/controller/mailinglistedit/PhabricatorMetaMTAMailingListEditController.php b/src/applications/metamta/controller/mailinglistedit/PhabricatorMetaMTAMailingListEditController.php
index 65fd88ec45..395cf483de 100644
--- a/src/applications/metamta/controller/mailinglistedit/PhabricatorMetaMTAMailingListEditController.php
+++ b/src/applications/metamta/controller/mailinglistedit/PhabricatorMetaMTAMailingListEditController.php
@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -37,6 +37,8 @@ class PhabricatorMetaMTAMailingListEditController
     }
 
     $e_email = true;
+    $e_uri = null;
+    $e_name = true;
     $errors = array();
 
     $request = $this->getRequest();
@@ -50,6 +52,26 @@ class PhabricatorMetaMTAMailingListEditController
         $errors[] = 'Email is required.';
       }
 
+      if (!strlen($list->getName())) {
+        $e_name = 'Required';
+        $errors[] = 'Name is required.';
+      }
+
+      if ($list->getURI()) {
+        $uri = new PhutilURI($list->getURI());
+        $proto = $uri->getProtocol();
+        $allowed_protocols = PhabricatorEnv::getEnvConfig(
+          'uri.allowed-protocols');
+        if (empty($allowed_protocols[$proto])) {
+          $e_uri = 'Invalid';
+          $protocol_list = implode(', ', array_keys($allowed_protocols));
+          $protocol_list = phutil_escape_html($protocol_list);
+          $errors[] =
+            'URI must use one of the allowed protocols: '.
+            $protocol_list.'.';
+        }
+      }
+
       if (!$errors) {
         $list->save();
         return id(new AphrontRedirectResponse())
@@ -78,16 +100,21 @@ class PhabricatorMetaMTAMailingListEditController
           ->setLabel('Email')
           ->setName('email')
           ->setValue($list->getEmail())
+          ->setCaption('Email will be delivered to this address.')
           ->setError($e_email))
       ->appendChild(
         id(new AphrontFormTextControl())
           ->setLabel('Name')
           ->setName('name')
+          ->setError($e_name)
+          ->setCaption('Human-readable display and autocomplete name.')
           ->setValue($list->getName()))
       ->appendChild(
         id(new AphrontFormTextControl())
           ->setLabel('URI')
           ->setName('uri')
+          ->setError($e_uri)
+          ->setCaption('Optional link to mailing list archives or info.')
           ->setValue($list->getURI()))
       ->appendChild(
         id(new AphrontFormStaticControl())
diff --git a/src/applications/metamta/controller/mailinglistedit/__init__.php b/src/applications/metamta/controller/mailinglistedit/__init__.php
index 3e2b746785..1b3a0db970 100644
--- a/src/applications/metamta/controller/mailinglistedit/__init__.php
+++ b/src/applications/metamta/controller/mailinglistedit/__init__.php
@@ -10,6 +10,7 @@ phutil_require_module('phabricator', 'aphront/response/404');
 phutil_require_module('phabricator', 'aphront/response/redirect');
 phutil_require_module('phabricator', 'applications/metamta/controller/base');
 phutil_require_module('phabricator', 'applications/metamta/storage/mailinglist');
+phutil_require_module('phabricator', 'infrastructure/env');
 phutil_require_module('phabricator', 'view/form/base');
 phutil_require_module('phabricator', 'view/form/control/static');
 phutil_require_module('phabricator', 'view/form/control/submit');
@@ -17,6 +18,8 @@ phutil_require_module('phabricator', 'view/form/control/text');
 phutil_require_module('phabricator', 'view/form/error');
 phutil_require_module('phabricator', 'view/layout/panel');
 
+phutil_require_module('phutil', 'markup');
+phutil_require_module('phutil', 'parser/uri');
 phutil_require_module('phutil', 'utils');