@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
Fix a redirect-on-login issue by allowing logged-out users to view 404 pages
Summary:
See T2102 and inline for discussion. This seems like the least-bad approach until we have something better.
The utility of next_uri seems much greater than the minor exposure of routable URIs.
Note that attackers can //not// detect if routable URIs are //valid// (e.g., "/D999" will always hit the login page whether it exists or not), just that they're routable. So you can only really tell if apps are installed or not.
Test Plan: Hit `/alsdknlkasnbla` while logged out, got 404 instead of login.
Reviewers: vrana, codeblock, btrahan
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2102
Differential Revision: https://secure.phabricator.com/D4012
+29
+29
src/applications/base/controller/Phabricator404Controller.php
+29
src/applications/base/controller/Phabricator404Controller.php
···
2
2
3
3
final class Phabricator404Controller extends PhabricatorController {
4
4
5
+
public function shouldRequireLogin() {
6
+
7
+
// NOTE: See T2102 for discussion. When a logged-out user requests a page,
8
+
// we give them a login form and set a `next_uri` cookie so we send them
9
+
// back to the page they requested after they login. However, some browsers
10
+
// or extensions request resources which may not exist (like
11
+
// "apple-touch-icon.png" and "humans.txt") and these requests may overwrite
12
+
// the stored "next_uri" after the login page loads. Our options for dealing
13
+
// with this are all bad:
14
+
//
15
+
// 1. We can't put the URI in the form because some login methods (OAuth2)
16
+
// issue redirects to third-party sites. After T1536 we might be able
17
+
// to.
18
+
// 2. We could set the cookie only if it doesn't exist, but then a user who
19
+
// declines to login will end up in the wrong place if they later do
20
+
// login.
21
+
// 3. We can blacklist all the resources browsers request, but this is a
22
+
// mess.
23
+
// 4. We can just allow users to access the 404 page without login, so
24
+
// requesting bad URIs doesn't set the cookie.
25
+
//
26
+
// This implements (4). The main downside is that an attacker can now detect
27
+
// if a URI is routable (i.e., some application is installed) by testing for
28
+
// 404 vs login. If possible, we should implement T1536 in such a way that
29
+
// we can pass the next URI through the login process.
30
+
31
+
return false;
32
+
}
33
+
5
34
public function processRequest() {
6
35
return new Aphront404Response();
7
36
}