From 6bd8542abb387df34fb73e18f579bce6f653934a Mon Sep 17 00:00:00 2001
From: vrana <jakubv@fb.com>
Date: Thu, 2 Feb 2012 18:09:51 -0800
Subject: [PATCH] Avoid sending CSRF token in GET and external forms

Summary:
Sending CSRF token in GET forms is dangerous because if there are external links
on the target page then the token could leak through Referer header.
The token is not required for anything because GET forms are used only to
display data, not to perform operations.
Sending CSRF tokens to external URLs leaks the token immediately.

Please note that <form action> defaults to GET.

PhabricatorUserOAuthSettingsPanelController suffered from this problem for both
reasons.

Test Plan: Save my settings (POST form).

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1558
---
 .../javelin/markup/__init__.php               |  1 +
 src/infrastructure/javelin/markup/markup.php  | 39 ++++++++++---------
 src/view/form/base/AphrontFormView.php        | 20 ++++------
 src/view/form/base/__init__.php               |  1 -
 4 files changed, 30 insertions(+), 31 deletions(-)

diff --git a/src/infrastructure/javelin/markup/__init__.php b/src/infrastructure/javelin/markup/__init__.php
index eee8eff821..5b3867c478 100644
--- a/src/infrastructure/javelin/markup/__init__.php
+++ b/src/infrastructure/javelin/markup/__init__.php
@@ -10,6 +10,7 @@ phutil_require_module('phabricator', 'aphront/request');
 phutil_require_module('phabricator', 'infrastructure/celerity/api');
 
 phutil_require_module('phutil', 'markup');
+phutil_require_module('phutil', 'utils');
 
 
 phutil_require_source('markup.php');
diff --git a/src/infrastructure/javelin/markup/markup.php b/src/infrastructure/javelin/markup/markup.php
index f6feb76595..1836eeb6ff 100644
--- a/src/infrastructure/javelin/markup/markup.php
+++ b/src/infrastructure/javelin/markup/markup.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.
@@ -49,22 +49,25 @@ function javelin_render_tag(
 
 
 function phabricator_render_form(PhabricatorUser $user, $attributes, $content) {
-  return javelin_render_tag(
-    'form',
-    $attributes,
-    phutil_render_tag(
-      'input',
-      array(
-        'type' => 'hidden',
-        'name' => AphrontRequest::getCSRFTokenName(),
-        'value' => $user->getCSRFToken(),
-      )).
-    phutil_render_tag(
-      'input',
-      array(
-        'type' => 'hidden',
-        'name' => '__form__',
-        'value' => true,
-      )).$content);
+  if (strcasecmp(idx($attributes, 'method'), 'POST') == 0 &&
+      !preg_match('#^(https?:|//)#', idx($attributes, 'action'))) {
+    $content =
+      phutil_render_tag(
+        'input',
+        array(
+          'type' => 'hidden',
+          'name' => AphrontRequest::getCSRFTokenName(),
+          'value' => $user->getCSRFToken(),
+        )).
+      phutil_render_tag(
+        'input',
+        array(
+          'type' => 'hidden',
+          'name' => '__form__',
+          'value' => true,
+        )).
+      $content;
+  }
+  return javelin_render_tag('form', $attributes, $content);
 }
 
diff --git a/src/view/form/base/AphrontFormView.php b/src/view/form/base/AphrontFormView.php
index cfef0f1f64..6f95dbb07e 100644
--- a/src/view/form/base/AphrontFormView.php
+++ b/src/view/form/base/AphrontFormView.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.
@@ -73,8 +73,12 @@ final class AphrontFormView extends AphrontView {
       ->appendChild($this->renderDataInputs())
       ->appendChild($this->renderChildren());
 
-    return javelin_render_tag(
-      'form',
+    if (!$this->user) {
+      throw new Exception('You must pass the user to AphrontFormView.');
+    }
+
+    return phabricator_render_form(
+      $this->user,
       array(
         'action'  => $this->action,
         'method'  => $this->method,
@@ -86,16 +90,8 @@ final class AphrontFormView extends AphrontView {
   }
 
   private function renderDataInputs() {
-    if (!$this->user) {
-      throw new Exception('You must pass the user to AphrontFormView.');
-    }
-
-    $data = $this->data + array(
-      '__form__' => 1,
-      AphrontRequest::getCSRFTokenName() => $this->user->getCSRFToken(),
-    );
     $inputs = array();
-    foreach ($data as $key => $value) {
+    foreach ($this->data as $key => $value) {
       if ($value === null) {
         continue;
       }
diff --git a/src/view/form/base/__init__.php b/src/view/form/base/__init__.php
index ff4720b1d4..2b61fad475 100644
--- a/src/view/form/base/__init__.php
+++ b/src/view/form/base/__init__.php
@@ -6,7 +6,6 @@
 
 
 
-phutil_require_module('phabricator', 'aphront/request');
 phutil_require_module('phabricator', 'infrastructure/celerity/api');
 phutil_require_module('phabricator', 'infrastructure/javelin/api');
 phutil_require_module('phabricator', 'infrastructure/javelin/markup');