OAuthServer - harden things up a bit

Summary: This is the hardening work mentioned in T887#86529. Also take a documentation pass for accuracy about these changes and formatting. Ref T4593.

Test Plan: unit tests...! generated diviner docs and oauthserver doc looked good

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T4593

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

src/applications/oauthserver/PhabricatorOAuthServer.php

@@ -215,8 +215,8 @@ final class PhabricatorOAuthServer {
 
   /**
    * If there's a URI specified in an OAuth request, it must be validated in
-   * its own right. Further, it must have the same domain and (at least) the
-   * same query parameters as the primary URI.
+   * its own right. Further, it must have the same domain, the same path, the
+   * same port, and (at least) the same query parameters as the primary URI.
    */
   public function validateSecondaryRedirectURI(
     PhutilURI $secondary_uri,
@@ -232,6 +232,16 @@ final class PhabricatorOAuthServer {
       return false;
     }
 
+    // Both URIs must have the same path
+    if ($secondary_uri->getPath() != $primary_uri->getPath()) {
+      return false;
+    }
+
+    // Both URIs must have the same port
+    if ($secondary_uri->getPort() != $primary_uri->getPort()) {
+      return false;
+    }
+
     // Any query parameters present in the first URI must be exactly present
     // in the second URI.
     $need_params = $primary_uri->getQueryParams();

src/applications/oauthserver/__tests__/PhabricatorOAuthServerTestCase.php

@@ -24,11 +24,11 @@ final class PhabricatorOAuthServerTestCase
 
   public function testValidateSecondaryRedirectURI() {
     $server      = new PhabricatorOAuthServer();
-    $primary_uri = new PhutilURI('http://www.google.com');
+    $primary_uri = new PhutilURI('http://www.google.com/');
     static $test_domain_map = array(
-      'http://www.google.com'               => true,
+      'http://www.google.com'               => false,
       'http://www.google.com/'              => true,
-      'http://www.google.com/auth'          => true,
+      'http://www.google.com/auth'          => false,
       'http://www.google.com/?auth'         => true,
       'www.google.com'                      => false,
       'http://www.google.com/auth#invalid'  => false,
@@ -76,12 +76,12 @@ final class PhabricatorOAuthServerTestCase
 
     $primary_uri = new PhutilURI('http://example.com/?z=2&y=3');
     $tests = array(
-      'http://example.com?z=2&y=3'      => true,
-      'http://example.com?y=3&z=2'      => true,
-      'http://example.com?y=3&z=2&x=1'  => true,
-      'http://example.com?y=2&z=3'      => false,
-      'http://example.com?y&x'          => false,
-      'http://example.com?z=2&x=3'      => false,
+      'http://example.com/?z=2&y=3'      => true,
+      'http://example.com/?y=3&z=2'      => true,
+      'http://example.com/?y=3&z=2&x=1'  => true,
+      'http://example.com/?y=2&z=3'      => false,
+      'http://example.com/?y&x'          => false,
+      'http://example.com/?z=2&x=3'      => false,
     );
     foreach ($tests as $input => $expected) {
       $uri = new PhutilURI($input);

src/docs/contributor/using_oauthserver.diviner

@@ -18,20 +18,20 @@ clients that implement OAuth 2.0.
 
 = Vocabulary =
 
-- **Access token** - a token which allows a client to ask for data on
-behalf of a resource owner. A given client will only be able to access
-data included in the scope(s) the resource owner authorized that client for.
+- **Access token** - a token which allows a client to ask for data on behalf
+ of a resource owner. A given client will only be able to access data included
+ in the scope(s) the resource owner authorized that client for.
 - **Authorization code** - a short-lived code which allows an authenticated
-client to ask for an access token on behalf of some resource owner.
+ client to ask for an access token on behalf of some resource owner.
 - **Client** - this is the application or system asking for data from the
-OAuth Server on behalf of the resource owner.
+ OAuth Server on behalf of the resource owner.
 - **Resource owner** - this is the user the client and OAuth Server are
-concerned with on a given request.
+ concerned with on a given request.
 - **Scope** - this defines a specific piece of granular data a client can
-or can not access on behalf of a user. For example, if authorized for the
-"whoami" scope on behalf of a given resource owner, the client can get the
-results of Conduit.whoami for that resource owner when authenticated with
-a valid access token.
+ or can not access on behalf of a user. For example, if authorized for the
+ "whoami" scope on behalf of a given resource owner, the client can get the
+ results of Conduit.whoami for that resource owner when authenticated with
+ a valid access token.
 
 = Setup - Creating a Client =
 
@@ -46,16 +46,16 @@ following parameters:
 
 - Required - **client_id** - the id of the newly registered client.
 - Required - **response_type** - the desired type of authorization code
-response. Only code is supported at this time.
+ response. Only code is supported at this time.
 - Optional - **redirect_uri** - override the redirect_uri the client
-registered. This redirect_uri must have the same fully-qualified domain
-and have at least the same query parameters as the redirect_uri the client
-registered, as well as have no fragments.
+ registered. This redirect_uri must have the same fully-qualified domain,
+ path, port and have at least the same query parameters as the redirect_uri
+ the client registered, as well as have no fragments.
 - Optional - **scope** - specify what scope(s) the client needs access to
-in a space-delimited list.
+ in a space-delimited list.
 - Optional - **state** - an opaque value the client can send to the server
-for programmatic excellence. Some clients use this value to implement XSRF
-protection or for debugging purposes.
+ for programmatic excellence. Some clients use this value to implement XSRF
+ protection or for debugging purposes.
 
 If done correctly and the resource owner has not yet authorized the client
 for the desired scope, then the resource owner will be presented with an
@@ -81,14 +81,14 @@ with the following parameters:
 
 - Required - **client_id** - the id of the client
 - Required - **client_secret** - the secret of the client.
-This is used to authenticate the client.
+ This is used to authenticate the client.
 - Required - **code** - the authorization code obtained earlier.
 - Required - **grant_type** - the desired type of access grant.
-Only token is supported at this time.
+ Only token is supported at this time.
 - Optional - **redirect_uri** - should be the exact same redirect_uri as
-the redirect_uri specified to obtain the authorization code. If no
-redirect_uri was specified to obtain the authorization code then this
-should not be specified.
+ the redirect_uri specified to obtain the authorization code. If no
+ redirect_uri was specified to obtain the authorization code then this
+ should not be specified.
 
 If done correctly, the OAuth Server will redirect to the pertinent
 redirect_uri with an access token.
@@ -115,6 +115,6 @@ currently exposed through the OAuth Server.
 There are only two scopes supported at this time.
 
 - **offline_access** - allows an access token to work indefinitely without
-expiring.
+ expiring.
 - **whoami** - allows the client to access the results of Conduit.whoami on
-behalf of the resource owner.
+ behalf of the resource owner.