Maintain POST requests when following redirects
AbandonedPublic
Actions

Authored by joshuaspence on May 28 2017, 11:03 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Summary

We have just moved our Phabricator installation to a new domain and setup 301 redirects from the old domain to the new domain. It seems, however, that arc commands are now failing with the following error:

> arc patch --nobranch --skip-dependencies --trace D67921
 ARGV  '/home/josh/workspace/github.com/phacility/arcanist/bin/../scripts/arcanist.php' 'patch' '--nobranch' '--skip-dependencies' '--trace' 'D67921'
 LOAD  Loaded "phutil" from "/home/josh/workspace/github.com/phacility/libphutil/src".
 LOAD  Loaded "arcanist" from "/home/josh/workspace/github.com/phacility/arcanist/src".
Config: Reading user configuration file "/home/josh/.arcrc"...
Config: Did not find system configuration at "/etc/arcconfig".
Working Copy: Reading .arcconfig from "REDACTED/.arcconfig".
Working Copy: Path "REDACTED" is part of `git` working copy "REDACTED".
Working Copy: Project root is at "REDACTED".
Config: Did not find local configuration at "REDACTED/.git/arc/config".
Loading phutil library from 'REDACTED/src'...
Loading phutil library from '/home/josh/workspace/github.com/phacility/phabricator/src'...
>>> [0] <conduit> differential.querydiffs() <bytes = 76>
>>> [1] <http> https://phabricator.REDACTED/api/differential.querydiffs
<<< [1] <http> 3,206,007 us
<<< [0] <conduit> 3,206,916 us
>>> [2] <conduit> user.whoami() <bytes = 117>
>>> [3] <http> https://phabricator.REDACTED/api/user.whoami
<<< [3] <http> 832,867 us
<<< [2] <conduit> 834,450 us

[2017-05-28 22:49:50] EXCEPTION: (ConduitClientException) ERR-INVALID-SESSION: Session key is not present. at [<phutil>/src/conduit/ConduitFuture.php:58]
arcanist(head=master, ref.master=3c4735795a29), phabricator(head=master, ref.master=b164d2d04bcb, custom=1), phlab(head=master, ref.master=0ea38bb44fef), phutil(head=stable, ref.master=310a831f97ab, ref.stable=8326890bd2b9)
  #0 ConduitFuture::didReceiveResult(array) called at [<phutil>/src/future/FutureProxy.php:58]
  #1 FutureProxy::getResult() called at [<phutil>/src/future/FutureProxy.php:35]
  #2 FutureProxy::resolve() called at [<phutil>/src/conduit/ConduitClient.php:64]
  #3 ConduitClient::callMethodSynchronous(string, array) called at [<arcanist>/src/workflow/ArcanistWorkflow.php:332]
  #4 ArcanistWorkflow::authenticateConduit() called at [<arcanist>/src/workflow/ArcanistPatchWorkflow.php:396]
  #5 ArcanistPatchWorkflow::run() called at [<arcanist>/scripts/arcanist.php:394]

The issue here is that, by default, curl will change GET requests to POST requests when following redirects.

Test Plan

Ran arc patch and saw no error.

Diff Detail

Branch

stable

Lint

Lint Errors

Severity	Location	Code	Message
Error	src/future/http/HTTPSFuture.php:211	XHP45	PHP Compatibility

Unit

Tests Passed

Build Status

Buildable 17303
Build 23180: arc lint + arc unit

Event Timeline

joshuaspence created this revision.May 28 2017, 11:03 PM

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptMay 28 2017, 11:03 PM

Herald added a subscriber: Korvin. · View Herald Transcript

@epriestley, if you had any thoughts on this it would be appreciated as this issue is heavily affecting our users. There is a workaround (update our .arcconfig files to point to the new domain), but we have a lot of repositories...

I think we should probably not follow redirects from arc at all.

In theory, they allow an attacker who can force the server to 301 (which might reasonably be easier than actually gaining meaningful control of the server) to trick arc into submitting credentials for wholesome-good-company.com to evil-attacker-domain.com. Although getting a server to 301 is similar in difficulty to controlling it entirely, we might imagine an attacker who gains access to an internal load balancer inside something like a VPC with no access to the public internet might face a lower barrier to escalation by making the LB issue a 301 than by escalating to attack application hosts, etc.

Keeping this working silently also means that some configuration may never be updated. In the best case, this means a long-lasting performance penalty for all users. It could also mean that live credentials get sent somewhere bad a year later when config changes because everyone assumes old-domain.com is no longer used.

To somewhat buoy this argument, RFC2616 says (although maybe this has been errata'd or whatever):

If the 301 status code is received in response to a request other
than GET or HEAD, the user agent MUST NOT automatically redirect the
request unless it can be confirmed by the user, since this might
change the conditions under which the request was issued.

I think a better behavior would probably be something like this (broadly: notify the user of the redirect, but refuse to follow it, and instruct them to fix the configuration problem):

$ arc diff
This server at "phabricator.old-company.com" is redirecting to "phabricator.new-company.com".
If you trust this redirect, update ".arcconfig" to point at this new install.

Yep, that's a better solution. In the mean time, we just updated all of our .arcconfig files.

Revision Contents
Changeset List

Path

Size

src/

future/

http/

HTTPSFuture.php

4 lines

Diff 43390

View Options

src/future/http/HTTPSFuture.php

Maintain POST requests when following redirectsAbandonedPublicActions