feat: Fail schema cache lookup with invalid db-schemas config

Previously, we'd silently report "200 OK" on the root endpoint, but would never return any endpoints from the schema cache. Now the schema cache query fails because of the ::regnamespace cast.
PostgREST · Jul 9, 2024 · 86c3257 · 86c3257
1 parent f31848f
commit 86c3257
Show file tree

Hide file tree

Showing 6 changed files with 33 additions and 88 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -25,6 +25,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
  - #2052, Dropped support for PostgreSQL 11 - @wolfgangwalther
  - #3508, PostgREST now fails to start when `server-port` and `admin-server-port` config options are the same - @develop7
  - #3607, PostgREST now fails to start when the JWT secret is less than 32 characters long - @laurenceisla
+ - #3644, Fail schema cache lookup with invalid db-schemas config - @wolfgangwalther
+   - Previously, this would silently return 200 - OK on the root endpoint, but don't provide any usable endpoints.
 
 ## [12.2.1] - 2024-06-27
 

diff --git a/src/PostgREST/Query/SqlFragment.hs b/src/PostgREST/Query/SqlFragment.hs
@@ -157,10 +157,10 @@ pgBuildArrayLiteral vals =
 
 -- TODO: refactor by following https://github.com/PostgREST/postgrest/pull/1631#issuecomment-711070833
 pgFmtIdent :: Text -> SQL.Snippet
-pgFmtIdent x = SQL.sql $ escapeIdent x
+pgFmtIdent x = SQL.sql . encodeUtf8 $ escapeIdent x
 
-escapeIdent :: Text -> ByteString
-escapeIdent x = encodeUtf8 $ "\"" <> T.replace "\"" "\"\"" (trimNullChars x) <> "\""
+escapeIdent :: Text -> Text
+escapeIdent x = "\"" <> T.replace "\"" "\"\"" (trimNullChars x) <> "\""
 
 -- Only use it if the input comes from the database itself, like on `jsonb_build_object('column_from_a_table', val)..`
 pgFmtLit :: Text -> Text
@@ -181,7 +181,7 @@ trimNullChars = T.takeWhile (/= '\x0')
 -- >>> escapeIdentList ["schema_1", "schema_2", "SPECIAL \"@/\\#~_-"]
 -- "\"schema_1\", \"schema_2\", \"SPECIAL \"\"@/\\#~_-\""
 escapeIdentList :: [Text] -> ByteString
-escapeIdentList schemas = BS.intercalate ", " $ escapeIdent <$> schemas
+escapeIdentList schemas = BS.intercalate ", " $ encodeUtf8 . escapeIdent <$> schemas
 
 asCsvF :: SQL.Snippet
 asCsvF = asCsvHeaderF <> " || '\n' || " <> asCsvBodyF

diff --git a/src/PostgREST/SchemaCache.hs b/src/PostgREST/SchemaCache.hs
@@ -46,6 +46,7 @@ import NeatInterpolation          (trimming)
 import PostgREST.Config                      (AppConfig (..))
 import PostgREST.Config.Database             (TimezoneNames,
                                               toIsolationLevel)
+import PostgREST.Query.SqlFragment           (escapeIdent)
 import PostgREST.SchemaCache.Identifiers     (AccessSet, FieldName,
                                               QualifiedIdentifier (..),
                                               RelIdentifier (..),
@@ -363,7 +364,7 @@ allFunctions :: Bool -> SQL.Statement AppConfig RoutineMap
 allFunctions = SQL.Statement funcsSqlQuery params decodeFuncs
   where
     params =
-      (toList . configDbSchemas >$< arrayParam HE.text) <>
+      (map escapeIdent . toList . configDbSchemas >$< arrayParam HE.text) <>
       (configDbHoistedTxSettings >$< arrayParam HE.text)
 
 accessibleFuncs :: Bool -> SQL.Statement ([Schema], [Text]) RoutineMap
@@ -464,7 +465,7 @@ funcsSqlQuery = encodeUtf8 [trimming|
   ) func_settings ON TRUE
   WHERE t.oid <> 'trigger'::regtype AND COALESCE(a.callable, true)
   AND prokind = 'f'
-  AND pn.nspname = ANY($$1) |]
+  AND p.pronamespace = ANY($$1::regnamespace[]) |]
 
 schemaDescription :: Bool -> SQL.Statement Schema (Maybe Text)
 schemaDescription =
@@ -477,20 +478,21 @@ schemaDescription =
         pg_namespace n
         left join pg_description d on d.objoid = n.oid
       where
-        n.nspname = $$1 |]
+        n.oid = $$1::regnamespace |]
 
 accessibleTables :: Bool -> SQL.Statement [Schema] AccessSet
 accessibleTables =
-  SQL.Statement sql (arrayParam HE.text) decodeAccessibleIdentifiers
+  SQL.Statement sql params decodeAccessibleIdentifiers
  where
+  params = map escapeIdent >$< arrayParam HE.text
   sql = encodeUtf8 [trimming|
     SELECT
       n.nspname AS table_schema,
       c.relname AS table_name
     FROM pg_class c
     JOIN pg_namespace n ON n.oid = c.relnamespace
     WHERE c.relkind IN ('v','r','m','f','p')
-    AND n.nspname = ANY($$1)
+    AND c.relnamespace = ANY($$1::regnamespace[])
     AND (
       pg_has_role(c.relowner, 'USAGE')
       or has_table_privilege(c.oid, 'SELECT, INSERT, UPDATE, DELETE, TRUNCATE, REFERENCES, TRIGGER')
@@ -608,7 +610,7 @@ addViewPrimaryKeys tabs keyDeps =
 allTables :: Bool -> SQL.Statement AppConfig TablesMap
 allTables = SQL.Statement tablesSqlQuery params decodeTables
   where
-    params = toList . configDbSchemas >$< arrayParam HE.text
+    params = map escapeIdent . toList . configDbSchemas >$< arrayParam HE.text
 
 -- | Gets tables with their PK cols
 tablesSqlQuery :: SqlQuery
@@ -657,7 +659,7 @@ tablesSqlQuery =
               ON d.objoid = a.attrelid and d.objsubid = a.attnum
           LEFT JOIN pg_attrdef ad
               ON a.attrelid = ad.adrelid AND a.attnum = ad.adnum
-          JOIN (pg_class c JOIN pg_namespace nc ON c.relnamespace = nc.oid)
+          JOIN pg_class c
               ON a.attrelid = c.oid
           JOIN pg_type t
               ON a.atttypid = t.oid
@@ -670,7 +672,7 @@ tablesSqlQuery =
           AND a.attnum > 0
           AND NOT a.attisdropped
           AND c.relkind in ('r', 'v', 'f', 'm', 'p')
-          AND nc.nspname = ANY($$1)
+          AND c.relnamespace = ANY($$1::regnamespace[])
   ),
   columns_agg AS (
     SELECT
@@ -856,8 +858,8 @@ allViewsKeyDependencies =
   --  * json transformation: https://gist.github.com/wolfgangwalther/3a8939da680c24ad767e93ad2c183089
   where
     params =
-      (toList . configDbSchemas >$< arrayParam HE.text) <>
-      (configDbExtraSearchPath >$< arrayParam HE.text)
+      (map escapeIdent . toList . configDbSchemas >$< arrayParam HE.text) <>
+      (map escapeIdent . toList . configDbExtraSearchPath >$< arrayParam HE.text)
     sql = encodeUtf8 [trimming|
       with recursive
       pks_fks as (
@@ -887,18 +889,19 @@ allViewsKeyDependencies =
       ),
       views as (
         select
-          c.oid       as view_id,
-          n.nspname   as view_schema,
-          c.relname   as view_name,
-          r.ev_action as view_definition
+          c.oid          as view_id,
+          c.relnamespace as view_schema_id,
+          n.nspname      as view_schema,
+          c.relname      as view_name,
+          r.ev_action    as view_definition
         from pg_class c
         join pg_namespace n on n.oid = c.relnamespace
         join pg_rewrite r on r.ev_class = c.oid
-        where c.relkind in ('v', 'm') and n.nspname = ANY($$1 || $$2)
+        where c.relkind in ('v', 'm') and c.relnamespace = ANY($$1::regnamespace[] || $$2::regnamespace[])
       ),
       transform_json as (
         select
-          view_id, view_schema, view_name,
+          view_id, view_schema_id, view_schema, view_name,
           -- the following formatting is without indentation on purpose
           -- to allow simple diffs, with less whitespace noise
           replace(
@@ -978,30 +981,31 @@ allViewsKeyDependencies =
       ),
       target_entries as(
         select
-          view_id, view_schema, view_name,
+          view_id, view_schema_id, view_schema, view_name,
           json_array_elements(view_definition->0->'targetList') as entry
         from transform_json
       ),
       results as(
         select
-          view_id, view_schema, view_name,
+          view_id, view_schema_id, view_schema, view_name,
           (entry->>'resno')::int as view_column,
           (entry->>'resorigtbl')::oid as resorigtbl,
           (entry->>'resorigcol')::int as resorigcol
         from target_entries
       ),
       -- CYCLE detection according to PG docs: https://www.postgresql.org/docs/current/queries-with.html#QUERIES-WITH-CYCLE
       -- Can be replaced with CYCLE clause once PG v13 is EOL.
-      recursion(view_id, view_schema, view_name, view_column, resorigtbl, resorigcol, is_cycle, path) as(
+      recursion(view_id, view_schema_id, view_schema, view_name, view_column, resorigtbl, resorigcol, is_cycle, path) as(
         select
           r.*,
           false,
           ARRAY[resorigtbl]
         from results r
-        where view_schema = ANY ($$1)
+        where view_schema_id = ANY ($$1::regnamespace[])
         union all
         select
           view.view_id,
+          view.view_schema_id,
           view.view_schema,
           view.view_name,
           view.view_column,
@@ -1060,7 +1064,7 @@ mediaHandlers :: Bool -> SQL.Statement AppConfig MediaHandlerMap
 mediaHandlers =
   SQL.Statement sql params decodeMediaHandlers
   where
-    params = toList . configDbSchemas >$< arrayParam HE.text
+    params = map escapeIdent . toList . configDbSchemas >$< arrayParam HE.text
     sql = encodeUtf8 [trimming|
       with
       all_relations as (
@@ -1101,7 +1105,7 @@ mediaHandlers =
         join pg_type      arg_name     on arg_name.oid = proc.proargtypes[0]
         join pg_namespace arg_schema   on arg_schema.oid = arg_name.typnamespace
       where
-        proc_schema.nspname = ANY($$1) and
+        proc.pronamespace = ANY($$1::regnamespace[]) and
         proc.pronargs = 1 and
         arg_name.oid in (select reltype from all_relations)
       union
@@ -1117,7 +1121,7 @@ mediaHandlers =
         join media_types mtype on proc.prorettype = mtype.oid
         join pg_namespace typ_sch     on typ_sch.oid = mtype.typnamespace
       where
-        pro_sch.nspname = ANY($$1) and NOT proretset
+        proc.pronamespace = ANY($$1::regnamespace[]) and NOT proretset
         and prokind = 'f'|]
 
 decodeMediaHandlers :: HD.Result MediaHandlerMap

diff --git a/test/spec/Feature/Query/ErrorSpec.hs b/test/spec/Feature/Query/ErrorSpec.hs
@@ -9,59 +9,6 @@ import Test.Hspec.Wai.JSON
 
 import Protolude hiding (get)
 
-nonExistentSchema :: SpecWith ((), Application)
-nonExistentSchema = do
-  describe "Non existent api schema" $ do
-    it "succeeds when requesting root path" $
-      get "/" `shouldRespondWith` 200
-
-    it "gives 404 when requesting a nonexistent table in this nonexistent schema" $
-      get "/nonexistent_table" `shouldRespondWith` 404
-
-  describe "Non existent URL" $ do
-    it "gives 404 on a single nested route" $
-      get "/projects/nested" `shouldRespondWith` 404
-
-    it "gives 404 on a double nested route" $
-      get "/projects/nested/double" `shouldRespondWith` 404
-
-  describe "Unsupported HTTP methods" $ do
-    it "should return 405 for CONNECT method" $
-      request methodConnect "/"
-          []
-          ""
-        `shouldRespondWith`
-          [json|
-            {"hint": null,
-             "details": null,
-             "code": "PGRST117",
-             "message":"Unsupported HTTP method: CONNECT"}|]
-          { matchStatus = 405 }
-
-    it "should return 405 for TRACE method" $
-      request methodTrace "/"
-          []
-          ""
-        `shouldRespondWith`
-          [json|
-            {"hint": null,
-             "details": null,
-             "code": "PGRST117",
-             "message":"Unsupported HTTP method: TRACE"}|]
-          { matchStatus = 405 }
-
-    it "should return 405 for OTHER method" $
-      request "OTHER" "/"
-          []
-          ""
-        `shouldRespondWith`
-          [json|
-            {"hint": null,
-             "details": null,
-             "code": "PGRST117",
-             "message":"Unsupported HTTP method: OTHER"}|]
-          { matchStatus = 405 }
-
 pgErrorCodeMapping :: SpecWith ((), Application)
 pgErrorCodeMapping = do
   describe "PostreSQL error code mappings" $ do

diff --git a/test/spec/Main.hs b/test/spec/Main.hs
@@ -126,7 +126,6 @@ main = do
 
       extraSearchPathApp   = appDbs testCfgExtraSearchPath
       unicodeApp           = appDbs testUnicodeCfg
-      nonexistentSchemaApp = appDbs testNonexistentSchemaCfg
       multipleSchemaApp    = appDbs testMultipleSchemaCfg
       ignorePrivOpenApi    = appDbs testIgnorePrivOpenApiCfg
 
@@ -221,10 +220,6 @@ main = do
     parallel $ before asymJwkSetApp $
       describe "Feature.Auth.AsymmetricJwtSpec" Feature.Auth.AsymmetricJwtSpec.spec
 
-    -- this test runs with a nonexistent db-schema
-    parallel $ before nonexistentSchemaApp $
-      describe "Feature.Query.NonExistentSchemaErrorSpec" Feature.Query.ErrorSpec.nonExistentSchema
-
     -- this test runs with an extra search path
     parallel $ before extraSearchPathApp $ do
       describe "Feature.ExtraSearchPathSpec" Feature.ExtraSearchPathSpec.spec

diff --git a/test/spec/SpecHelper.hs b/test/spec/SpecHelper.hs
@@ -227,9 +227,6 @@ testCfgAsymJWKSet =
   , configJWKS = rightToMaybe $ parseSecret secret
   }
 
-testNonexistentSchemaCfg :: AppConfig
-testNonexistentSchemaCfg = baseCfg { configDbSchemas = fromList ["nonexistent"] }
-
 testCfgExtraSearchPath :: AppConfig
 testCfgExtraSearchPath = baseCfg { configDbExtraSearchPath = ["public", "extensions", "EXTRA \"@/\\#~_-"] }