Page MenuHomePhabricator
Create Task
Maniphest T11331

Explicitly specifying allowed protocols breaks viewing raw files
Closed, InvalidPublic
Actions

Description

I haven't reproduced on a test instance because I don't think I can get full admin access, but I reproduced and root caused on my local setup, so hopefully this is helpful. Here goes:

  1. Set up uri.allowed-protocols to be something super simple like {"myprotocol", true}. Omit standard ones
  2. Do not set up alternate file domain, such that your files are served from the same domain as the rest of your content
  3. Enable HTTPS for everything and automatically redirect to HTTPS
  4. Navigate to a file in your repository and click "View Raw File"

At this point you'll get an error that says:

Refusing to redirect to external URI "https://phabricator.example.com/<a bunch of stuff here>". This URI is not a valid remote web resource.'

I did some digging and it looks like PhabricatorEnv::requireValidRemoteURIForLink checks for a presence of protocol, and makes sure that the protocol specifies is included in uri.allowed-protocols. Since HTTPS is not on the list, the check fails.
Adding HTTPS to the list of allowed URI works around the problem, but it seems like the right solution here is really:

  1. Check fully-qualified links against the base URI
  2. Assume HTTP and HTTPS are allowed by default, even if they are not explicitly specified in uri.allowed-protocols

Event Timeline

anton.vladimirov created this task.Jul 14 2016, 10:36 PM
epriestley closed this task as Invalid.Jul 14 2016, 10:41 PM
epriestley added a subscriber: epriestley.

This isn't a bug. List HTTP and HTTPS if you want to use these protocols.

anton.vladimirov added a comment.Jul 14 2016, 10:57 PM

I guess I can phrase that differently, then.
Consider that this services as an exception and the associated error message is extremely confusing:

  • It specifies a URI that is valid and is not external to my server and calls it both remote and invalid, which is odd
  • The URI that is surfaced is a long and very internal-looking one, clearly generated by Phabricator itself
  • There is no explanation given as to why this is considered invalid.

That last one is important, because requireValidRemoteURIForLink actually provides a reasonable error message that is swallowed down the stack in favor of this less clear one.

public static function requireValidRemoteURIForLink($raw_uri) {
  $uri = new PhutilURI($raw_uri);

  $proto = $uri->getProtocol();
  if (!strlen($proto)) {
    throw new Exception(
      pht(
        'URI "%s" is not a valid linkable resource. A valid linkable '.
        'resource URI must specify a protocol.',
        $raw_uri));
  }

  $protocols = self::getEnvConfig('uri.allowed-protocols');
  if (!isset($protocols[$proto])) {
    throw new Exception(
      pht(
        'URI "%s" is not a valid linkable resource. A valid linkable '.
        'resource URI must use one of these protocols: %s.',
        $raw_uri,
        implode(', ', array_keys($protocols))));
  }

  $domain = $uri->getDomain();
  if (!strlen($domain)) {
    throw new Exception(
      pht(
        'URI "%s" is not a valid linkable resource. A valid linkable '.
        'resource URI must specify a domain.',
        $raw_uri));
  }
}

If the error message from the validation itself was surfaced, this would make sense to me, but as it stands, I had to go dig though a bunch of source code to figure out what I need to do to fix this.

anton.vladimirov added a comment.Jul 14 2016, 11:00 PM

But I still kind of think that if you generated a link to your own content you should be able to navigate to it, so it seems reasonable that either:

  • remote URI that start with pre-configured base URI are considered valid
  • "View Raw File" generates a local URI relative to base URI to avoid this check
Phabricator · Built by Phacility