From 86c3257f5496688196be33a465d6cacaf89f62cf Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Sat, 22 Jun 2024 21:10:14 +0200 Subject: [PATCH] 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. --- CHANGELOG.md | 2 ++ src/PostgREST/Query/SqlFragment.hs | 8 ++--- src/PostgREST/SchemaCache.hs | 50 ++++++++++++++------------ test/spec/Feature/Query/ErrorSpec.hs | 53 ---------------------------- test/spec/Main.hs | 5 --- test/spec/SpecHelper.hs | 3 -- 6 files changed, 33 insertions(+), 88 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c263acdc92..b5a28985f4 100644 --- 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 index 9329fb438d..39b869d5d9 100644 --- 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 index bcf5a287ab..7db9808ca5 100644 --- 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,12 +478,13 @@ 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, @@ -490,7 +492,7 @@ accessibleTables = 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,13 +981,13 @@ 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 @@ -992,16 +995,17 @@ allViewsKeyDependencies = ), -- 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 index d004e5c5cc..a41f371fea 100644 --- 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 index 070c6905de..526bd45dcc 100644 --- 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 index 7e64d9e4a4..992cb735dc 100644 --- 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 \"@/\\#~_-"] }