Remove the developer-specific CSRF help in phabricator_form()

@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge

phorge phabricator

fork

Summary:
Fixes T4802. For context, see T1921.

Originally (in T1921), a developer ran into an issue where rendering `phabricator_form()` with an absolute URI confusingly dropped CSRF tokens, and it wasn't obvious why. This is a security measure, but at the time it wasn't very clear how all the pieces fit together. To make it more clear, we:

# expanded the exception text in developer mode to include a description of this issue; and
# added an exception in developer mode when rendering a form like this.

However, (2) causes some undesirable interactions for file downloads. In particular, if:

- developer mode is on; and
- there's no alternate file domain configured; and
- you try to download a file...

...we produce CDN URIs that are fully-qualified, and you get the exception from (2) above.

This is kind of a mess, and producing fully-qualified CDN URIs in all cases is simple and clear and desirable. To resolve this, just revert (2). We still have the clarification from (1) above and this hasn't caused further issues, so I think that's sufficient. This is a rare issue anyway and not particularly serious or error prone (at worst, a bit confusing and annoying, but hopefully easy to understand and resolve after the changes in (1)).

Test Plan: With develper mode and no alternate file domain, downloaded files from Files.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4802

Differential Revision: https://secure.phabricator.com/D8776

epriestley 12 years ago 04c07a7a cb545856

+19 -18

1 changed file

expand all

src

infrastructure

javelin

markup.php

+19 -18

src/infrastructure/javelin/markup.php

··· 49 49 $is_absolute_uri = preg_match('#^(https?:|//)#', $http_action); 50 50 51 51 if ($is_post) { 52 - if ($is_absolute_uri) { 53 - $is_dev = PhabricatorEnv::getEnvConfig('phabricator.developer-mode'); 54 - if ($is_dev) { 55 - $form_domain = id(new PhutilURI($http_action)) 56 - ->getDomain(); 57 - $host_domain = id(new PhutilURI(PhabricatorEnv::getURI('/'))) 58 - ->getDomain(); 52 + 53 + // NOTE: We only include CSRF tokens if a URI is a local URI on the same 54 + // domain. This is an important security feature and prevents forms which 55 + // submit to foreign sites from leaking CSRF tokens. 56 + 57 + // In some cases, we may construct a fully-qualified local URI. For example, 58 + // we can construct these for download links, depending on configuration. 59 + 60 + // These forms do not receive CSRF tokens, even though they safely could. 61 + // This can be confusing, if you're developing for Phabricator and 62 + // manage to construct a local form with a fully-qualified URI, since it 63 + // won't get CSRF tokens and you'll get an exception at the other end of 64 + // the request which is a bit disconnected from the actual root cause. 65 + 66 + // However, this is rare, and there are reasonable cases where this 67 + // construction occurs legitimately, and the simplest fix is to omit CSRF 68 + // tokens for these URIs in all cases. The error message you receive also 69 + // gives you some hints as to this potential source of error. 59 70 60 - if (strtolower($form_domain) == strtolower($host_domain)) { 61 - throw new Exception( 62 - pht( 63 - "You are building a <form /> that submits to Phabricator, but ". 64 - "has an absolute URI in its 'action' attribute ('%s'). To avoid ". 65 - "leaking CSRF tokens, Phabricator does not add CSRF information ". 66 - "to forms with absolute URIs. Instead, use a relative URI.", 67 - $http_action)); 68 - } 69 - } 70 - } else { 71 + if (!$is_absolute_uri) { 71 72 $body[] = phutil_tag( 72 73 'input', 73 74 array(

Configure Feed

Configure Feed