REST Endpoints - Prohibit Invalid slashes, duplicate variables, non-singular query definitions, subscriptions

Resolves Issues:

* https://github.com/hasura/graphql-engine-mono/issues/658 - Invalid Slashes
* https://github.com/hasura/graphql-engine-mono/issues/628 - Subscriptions

Implementation:

* Moved some logic from Endpoint.hs to allow reuse of splitting url into PathSegments.
* Additional validation steps alongside checking for overlapping routes
* Logging potential misuse of GET for mutations

Future Work:

* [ ] GET is allowed for mutations (Ignore/Log warning for Now)
* [ ] Add to scInconsistentObjs rather than throwing error
  * Add information to scInconsistentObjs instead of raising errors directly.

TODO:

* [x] Duplicate variable segments with the same name in the location should not be allowed
* [x] We should throw an error on trailing and leading slashes and URLs which contain empty segments
* [x] Endpoints can be created using subscriptions. But the error only shows at the time of the query
* [x] Tests

---

### Kodiak commit message

Prohibit Invalid slashes, duplicate variables, subscriptions for REST endpoints.

GitOrigin-RevId: 86c0d4af97984c8afd02699e6071e9c1658710b8

CHANGELOG.md

@@ -23,6 +23,7 @@ keys in the response body.
 - server: optimize resolving source. Resolving a source would create connection pools every time. Optimize that to re-create connection pools only when necessary. (#609)
 - server: fix issues with remote schema introspection and queries over TLS.
 - console: add support for MS SQL Server
+- server: Prohibit Invalid slashes, duplicate variables, subscriptions, non-singular query definitions for REST endpoints
 
 ## v1.4.0-alpha.1
 

server/src-lib/Hasura/RQL/DDL/Schema/Cache.hs

@@ -28,6 +28,7 @@ import qualified Data.HashMap.Strict.Extended             as M
 import qualified Data.HashMap.Strict.InsOrd               as OMap
 import qualified Data.HashSet                             as HS
 import qualified Data.HashSet.InsOrd                      as HSIns
+import qualified Language.GraphQL.Draft.Syntax            as G
 
 import           Control.Arrow.Extended
 import           Control.Lens                             hiding ((.=))
@@ -163,6 +164,25 @@ buildSchemaCacheRule env = proc (metadata, invalidationKeys) -> do
     , _actNonObjects $ _boCustomTypes resolvedOutputs
     )
 
+  let
+    duplicateVariables :: EndpointMetadata a -> Bool
+    duplicateVariables m = any ((>1) . length) $ group $ sort $ catMaybes $ splitPath Just (const Nothing) (_ceUrl m)
+
+    --  Cases of urls that generate invalid segments:
+    -- * "" -> Error: "Empty URL"
+    -- * "/asdf" -> Error: "Leading slash not allowed"
+    -- * "asdf/" -> Error: "Trailing slash not allowed"
+    -- * "asdf//qwer" -> Error: "Double Slash not allowed"
+    -- * "asdf/:/qwer" -> Error: "Variables must be named"
+    -- * "asdf/:x/qwer/:x" -> Error: "Duplicate Variable: x"
+    invalidSegments m = any (`elem` ["",":"]) (splitPath id id (_ceUrl m))
+
+  bindA -< onJust (nonEmpty $ filter duplicateVariables (M.elems $ _boEndpoints resolvedOutputs)) $ \md ->
+      throw400 BadRequest $ "Duplicate variables found in endpoint paths: " <> commaSeparated (_ceUrl <$> md)
+
+  bindA -< onJust (nonEmpty $ filter invalidSegments (M.elems $ _boEndpoints resolvedOutputs)) $ \md ->
+      throw400 BadRequest $ "Empty segments or unnamed variables are not allowed: " <> commaSeparated (_ceUrl <$> md)
+
   let endpoints = buildEndpointsTrie (M.elems $ _boEndpoints resolvedOutputs)
 
   bindA -< onJust (nonEmpty $ ambiguousPaths endpoints) $ \ambPaths ->
@@ -450,7 +470,16 @@ buildSchemaCacheRule env = proc (metadata, invalidationKeys) -> do
                 <> toTxt queryName
                 <> " does not exist in collection " <> toTxt collName)
                $ find ((== queryName) . _lqName) (_cdQueries (_ccDefinition collection))
-      pure (_lqQuery listedQuery)
+
+      let lq@(GQLQueryWithText lqq) = _lqQuery listedQuery
+
+      for_ (G.getExecutableDefinitions $ unGQLQuery $ snd lqq) $ \case
+          G.ExecutableDefinitionOperation (G.OperationDefinitionTyped d)
+            | G._todType d == G.OperationTypeSubscription ->
+                throw405 $ "query with name " <> toTxt queryName <> " is a subscription"
+          _ -> pure ()
+
+      pure lq
 
     mkEventTriggerMetadataObject (_, source, _, table, eventTriggerConf) =
       let objectId = MOSourceObjId source $

server/src-lib/Hasura/RQL/Types/Endpoint.hs

@@ -1,6 +1,7 @@
 module Hasura.RQL.Types.Endpoint
   ( EndpointName(..)
   , EndpointMethod(..)
+  , EndpointUrl()
   , CreateEndpoint
   , EndpointDef(..)
   , QueryReference(..)
@@ -18,6 +19,8 @@ module Hasura.RQL.Types.Endpoint
   , ceName
   , ceUrl
   , deName
+  , splitPath
+  , mkEndpointUrl
   ) where
 
 import           Hasura.Prelude
@@ -61,6 +64,9 @@ newtype EndpointUrl = EndpointUrl { unEndpointUrl :: NonEmptyText }
            , Generic, Arbitrary
            )
 
+mkEndpointUrl :: ToTxt a => a -> Maybe EndpointUrl
+mkEndpointUrl s = EndpointUrl <$> mkNonEmptyText (toTxt s)
+
 instance FromHttpApiData EndpointUrl where
   parseQueryParam s = parseQueryParam s >>= \t ->
     case mkNonEmptyText t of
@@ -89,15 +95,16 @@ buildEndpointsTrie = foldl' insert mempty
   where
     insert t q =
       let endpointMap = foldMap (`singletonMultiMap` q) $ _ceMethods q
-      in insertPath (split (_ceUrl q)) endpointMap t
+      in insertPath (splitPath (const PathParam) PathLiteral (_ceUrl q)) endpointMap t
 
-    split :: EndpointUrl -> Path Text
-    split = map toPathComponent . T.split (=='/') . toTxt
-
-    toPathComponent :: T.Text -> PathComponent Text
-    toPathComponent x
-      | ":" `T.isPrefixOf` x = PathParam
-      | otherwise            = PathLiteral x
+-- | Split a path and construct PathSegments based on callbacks for variables and literals
+--   Var callback is passed the ":" prefix as part of the text.
+splitPath :: (T.Text -> a) -> (T.Text -> a) -> EndpointUrl -> [a]
+splitPath var lit = map toPathComponent . T.split (=='/') . toTxt
+  where
+  toPathComponent x
+    | ":" `T.isPrefixOf` x = var x
+    | otherwise            = lit x
 
 type CreateEndpoint = EndpointMetadata QueryReference
 

server/tests-py/queries/endpoints/endpoint_duplicate_param.yaml

@@ -0,0 +1,19 @@
+description: Try to add an endpoint with duplicate params
+url: /v1/query
+status: 400
+response:
+  path: $.args
+  error: |-
+    Duplicate variables found in endpoint paths: foo/:id/bar/:id
+  code: bad-request
+query:
+  type: create_rest_endpoint
+  args:
+    url: "foo/:id/bar/:id"
+    name: duplicate
+    methods:
+      - GET
+    definition:
+      query:
+        collection_name: test_collection
+        query_name: simple_query

server/tests-py/queries/endpoints/endpoint_empty_path.yaml

@@ -0,0 +1,19 @@
+description: Tries to create an endpoint with empty path
+url: /v1/query
+status: 400
+response:
+  path: $.url
+  error: |-
+    empty string not allowed
+  code: parse-failed
+query:
+  type: create_rest_endpoint
+  args:
+    url: ""
+    name: duplicate
+    methods:
+      - GET
+    definition:
+      query:
+        collection_name: test_collection
+        query_name: simple_query

server/tests-py/queries/endpoints/endpoint_empty_path_param.yaml

@@ -0,0 +1,19 @@
+description: Tries to create an endpoint with empty path param
+url: /v1/query
+status: 400
+response:
+  path: $.args
+  error: |-
+    Empty segments or unnamed variables are not allowed: foo/:/bar
+  code: bad-request
+query:
+  type: create_rest_endpoint
+  args:
+    url: "foo/:/bar"
+    name: foo
+    methods:
+      - GET
+    definition:
+      query:
+        collection_name: test_collection
+        query_name: simple_query

server/tests-py/queries/endpoints/endpoint_empty_path_segment.yaml

@@ -0,0 +1,19 @@
+description: Tries to create an endpoint with empty path component
+url: /v1/query
+status: 400
+response:
+  path: $.args
+  error: |-
+    Empty segments or unnamed variables are not allowed: foo//bar
+  code: bad-request
+query:
+  type: create_rest_endpoint
+  args:
+    url: "foo//bar"
+    name: foo
+    methods:
+      - GET
+    definition:
+      query:
+        collection_name: test_collection
+        query_name: simple_query

server/tests-py/queries/endpoints/endpoint_subscription.yaml

@@ -0,0 +1,31 @@
+description: Try to add an endpoint with duplicate params
+url: /v1/query
+status: 400
+response:
+  internal:
+  - definition:
+      definition:
+        query:
+          collection_name: test_collection
+          query_name: simple_subscription
+      url: foo/bar/subscribe
+      methods:
+      - GET
+      name: foo
+      comment:
+    reason: 'in endpoint foo: query with name simple_subscription is a subscription'
+    type: endpoint
+  path: $.args
+  error: cannot continue due to new inconsistent metadata
+  code: unexpected
+query:
+  type: create_rest_endpoint
+  args:
+    url: "foo/bar/subscribe"
+    name: foo
+    methods:
+      - GET
+    definition:
+      query:
+        collection_name: test_collection
+        query_name: simple_subscription

server/tests-py/queries/endpoints/endpoint_trailing_slash.yaml

@@ -0,0 +1,19 @@
+description: Tries to create an endpoint with trailing slash
+url: /v1/query
+status: 400
+response:
+  path: $.args
+  error: |-
+    Empty segments or unnamed variables are not allowed: foo/
+  code: bad-request
+query:
+  type: create_rest_endpoint
+  args:
+    url: "foo/"
+    name: duplicate
+    methods:
+      - GET
+    definition:
+      query:
+        collection_name: test_collection
+        query_name: simple_query

server/tests-py/queries/endpoints/setup.yaml

@@ -13,6 +13,8 @@ args:
         query: "query ($first_name:String!) { test_table(where: {first_name: { _eq: $first_name } }) { first_name last_name } }"
       - name: query_with_args
         query: "query ($first_name: String!, $last_name:String!) { test_table(where: {first_name: { _eq: $first_name } last_name: { _eq: $last_name }}) { first_name last_name } }"
+      - name: simple_subscription
+        query: "subscription { test_table { first_name last_name } }"
 
 - type: create_rest_endpoint
   args:

server/tests-py/test_endpoints.py

@@ -58,3 +58,20 @@ class TestCustomEndpoints:
     def test_endpoint_conflicting(self, hge_ctx, transport):
         check_query_f(hge_ctx, self.dir() + '/endpoint_conflicting.yaml', transport)
 
+    def test_endpoint_duplicate_param(self, hge_ctx, transport):
+        check_query_f(hge_ctx, self.dir() + '/endpoint_duplicate_param.yaml', transport)
+
+    def test_endpoint_empty_path(self, hge_ctx, transport):
+        check_query_f(hge_ctx, self.dir() + '/endpoint_empty_path.yaml', transport)
+        
+    def test_endpoint_trailing_slash(self, hge_ctx, transport):
+        check_query_f(hge_ctx, self.dir() + '/endpoint_trailing_slash.yaml', transport)
+        
+    def test_endpoint_empty_path_segment(self, hge_ctx, transport):
+        check_query_f(hge_ctx, self.dir() + '/endpoint_empty_path_segment.yaml', transport)
+
+    def test_endpoint_empty_path_param(self, hge_ctx, transport):
+        check_query_f(hge_ctx, self.dir() + '/endpoint_empty_path_param.yaml', transport)
+
+    def test_endpoint_subscription(self, hge_ctx, transport):
+        check_query_f(hge_ctx, self.dir() + '/endpoint_subscription.yaml', transport)