Skip to content

Commit 5b74b2e

Browse files
Antoine Leblanclexi-lambda
Antoine Leblanc
and
lexi-lambda
authored
server: prevent metadata checks in read-only mode (hasura#4250)
* do not perform the metadata check in read-only mode * improve the isAltrDropReplace regex * quote the regex at compile-time to handle syntax errors statically Co-authored-by: Alexis King <lexi.lambda@gmail.com>
1 parent 1264fad commit 5b74b2e

File tree

9 files changed

+163
-39
lines changed

9 files changed

+163
-39
lines changed

‎server/cabal.project.freeze

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -131,8 +131,10 @@ constraints: any.Cabal ==2.4.0.1,
131131
any.free ==5.1.1,
132132
any.generic-arbitrary ==0.1.0,
133133
any.ghc-boot-th ==8.6.5,
134+
any.ghc-heap ==8.6.5,
135+
any.ghc-heap-view ==0.6.0,
136+
ghc-heap-view -prim-supports-any,
134137
any.ghc-prim ==0.5.3,
135-
any.ghc-heap-view ==0.6.0,
136138
any.happy ==1.19.12,
137139
happy +small_base,
138140
any.hashable ==1.2.7.0,
@@ -162,6 +164,7 @@ constraints: any.Cabal ==2.4.0.1,
162164
any.http2 ==1.6.5,
163165
http2 -devel,
164166
any.hvect ==0.4.0.0,
167+
any.immortal ==0.2.2.1,
165168
any.insert-ordered-containers ==0.2.1.0,
166169
any.integer-gmp ==1.0.2.0,
167170
any.integer-logarithms ==1.0.3,
@@ -239,10 +242,9 @@ constraints: any.Cabal ==2.4.0.1,
239242
any.random ==1.1,
240243
any.reflection ==2.1.4,
241244
reflection -slow +template-haskell,
242-
any.regex-base ==0.93.2,
243-
regex-base +newbase +splitbase,
244-
any.regex-tdfa ==1.2.3.1,
245-
regex-tdfa -devel,
245+
any.regex-base ==0.94.0.0,
246+
any.regex-tdfa ==1.3.1.0,
247+
regex-tdfa -force-o2,
246248
any.reroute ==0.5.0.0,
247249
any.resource-pool ==0.2.3.2,
248250
resource-pool -developer,

‎server/graphql-engine.cabal

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ library
165165
, auto-update
166166

167167
-- regex related
168-
, regex-tdfa >= 1.2
168+
, regex-tdfa >=1.3.1 && <1.4
169169

170170
-- pretty printer
171171
, ansi-wl-pprint
@@ -192,10 +192,10 @@ library
192192
-- testing
193193
, QuickCheck
194194
, generic-arbitrary
195-
-- 0.6.1 is supposedly not okay for ghc 8.6:
196-
-- https://github.com/nomeata/ghc-heap-view/issues/27
195+
-- 0.6.1 is supposedly not okay for ghc 8.6:
196+
-- https://github.com/nomeata/ghc-heap-view/issues/27
197197
, ghc-heap-view == 0.6.0
198-
198+
199199
, directory
200200

201201
exposed-modules: Control.Arrow.Extended

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

Lines changed: 41 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ import qualified Data.Text as T
4040
import qualified Data.Text.Encoding as TE
4141
import qualified Database.PG.Query as Q
4242
import qualified Database.PostgreSQL.LibPQ as PQ
43+
import qualified Text.Regex.TDFA as TDFA
4344

4445
import Data.Aeson
4546
import Data.Aeson.Casing
@@ -54,7 +55,7 @@ import Hasura.RQL.DDL.Schema.Rename
5455
import Hasura.RQL.DDL.Schema.Table
5556
import Hasura.RQL.Instances ()
5657
import Hasura.RQL.Types
57-
import Hasura.Server.Utils (matchRegex)
58+
import Hasura.Server.Utils (quoteRegex)
5859

5960
data RunSQL
6061
= RunSQL
@@ -87,22 +88,51 @@ instance ToJSON RunSQL where
8788

8889
runRunSQL :: (MonadTx m, CacheRWM m, HasSQLGenCtx m) => RunSQL -> m EncJSON
8990
runRunSQL RunSQL {..} = do
90-
metadataCheckNeeded <- onNothing rCheckMetadataConsistency $ isAltrDropReplace rSql
91-
bool (execRawSQL rSql) (withMetadataCheck rCascade $ execRawSQL rSql) metadataCheckNeeded
91+
-- see Note [Checking metadata consistency in run_sql]
92+
let metadataCheckNeeded = case rTxAccessMode of
93+
Q.ReadOnly -> False
94+
Q.ReadWrite -> fromMaybe (containsDDLKeyword rSql) rCheckMetadataConsistency
95+
if metadataCheckNeeded
96+
then withMetadataCheck rCascade $ execRawSQL rSql
97+
else execRawSQL rSql
9298
where
9399
execRawSQL :: (MonadTx m) => Text -> m EncJSON
94100
execRawSQL =
95101
fmap (encJFromJValue @RunSQLRes) . liftTx . Q.multiQE rawSqlErrHandler . Q.fromText
96102
where
97103
rawSqlErrHandler txe =
98-
let e = err400 PostgresError "query execution failed"
99-
in e {qeInternal = Just $ toJSON txe}
100-
101-
isAltrDropReplace :: QErrM m => T.Text -> m Bool
102-
isAltrDropReplace = either throwErr return . matchRegex regex False
103-
where
104-
throwErr s = throw500 $ "compiling regex failed: " <> T.pack s
105-
regex = "alter|drop|replace|create function|comment on"
104+
(err400 PostgresError "query execution failed") { qeInternal = Just $ toJSON txe }
105+
106+
-- see Note [Checking metadata consistency in run_sql]
107+
containsDDLKeyword :: Text -> Bool
108+
containsDDLKeyword = TDFA.match $$(quoteRegex
109+
TDFA.defaultCompOpt
110+
{ TDFA.caseSensitive = False
111+
, TDFA.multiline = True
112+
, TDFA.lastStarGreedy = True }
113+
TDFA.defaultExecOpt
114+
{ TDFA.captureGroups = False }
115+
"\\balter\\b|\\bdrop\\b|\\breplace\\b|\\bcreate function\\b|\\bcomment on\\b")
116+
117+
{- Note [Checking metadata consistency in run_sql]
118+
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
119+
SQL queries executed by run_sql may change the Postgres schema in arbitrary
120+
ways. We attempt to automatically update the metadata to reflect those changes
121+
as much as possible---for example, if a table is renamed, we want to update the
122+
metadata to track the table under its new name instead of its old one. This
123+
schema diffing (plus some integrity checking) is handled by withMetadataCheck.
124+
125+
But this process has overhead---it involves reloading the metadata, diffing it,
126+
and rebuilding the schema cache---so we don’t want to do it if it isn’t
127+
necessary. The user can explicitly disable the check via the
128+
check_metadata_consistency option, and we also skip it if the current
129+
transaction is in READ ONLY mode, since the schema can’t be modified in that
130+
case, anyway.
131+
132+
However, even if neither read_only or check_metadata_consistency is passed, lots
133+
of queries may not modify the schema at all. As a (fairly stupid) heuristic, we
134+
check if the query contains any keywords for DDL operations, and if not, we skip
135+
the metadata check as well. -}
106136

107137
data RunSQLRes
108138
= RunSQLRes

‎server/src-lib/Hasura/RQL/Instances.hs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ import qualified Data.HashSet as S
99
import qualified Data.URL.Template as UT
1010
import qualified Language.GraphQL.Draft.Syntax as G
1111
import qualified Language.Haskell.TH.Syntax as TH
12+
import qualified Text.Regex.TDFA as TDFA
13+
import qualified Text.Regex.TDFA.Pattern as TDFA
1214

1315
import Data.Functor.Product
1416
import Data.GADT.Compare
@@ -53,6 +55,15 @@ instance (TH.Lift k, TH.Lift v) => TH.Lift (M.HashMap k v) where
5355
instance TH.Lift a => TH.Lift (S.HashSet a) where
5456
lift s = [| S.fromList $(TH.lift $ S.toList s) |]
5557

58+
deriving instance TH.Lift TDFA.CompOption
59+
deriving instance TH.Lift TDFA.DoPa
60+
deriving instance TH.Lift TDFA.ExecOption
61+
deriving instance TH.Lift TDFA.Pattern
62+
deriving instance TH.Lift TDFA.PatternSet
63+
deriving instance TH.Lift TDFA.PatternSetCharacterClass
64+
deriving instance TH.Lift TDFA.PatternSetCollatingElement
65+
deriving instance TH.Lift TDFA.PatternSetEquivalenceClass
66+
5667
instance (GEq f, GEq g) => GEq (Product f g) where
5768
Pair a1 a2 `geq` Pair b1 b2
5869
| Just Refl <- a1 `geq` b1

‎server/src-lib/Hasura/Server/Utils.hs

Lines changed: 13 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
{-# LANGUAGE TypeApplications #-}
22
module Hasura.Server.Utils where
33

4+
import Hasura.Prelude
5+
46
import Control.Lens ((^..))
57
import Data.Aeson
68
import Data.Char
79
import Data.List (find)
8-
import Language.Haskell.TH.Syntax (Lift)
10+
import Language.Haskell.TH.Syntax (Lift, Q, TExp)
911
import System.Environment
1012
import System.Exit
1113
import System.Process
@@ -15,7 +17,6 @@ import qualified Data.CaseInsensitive as CI
1517
import qualified Data.HashSet as Set
1618
import qualified Data.List.NonEmpty as NE
1719
import qualified Data.Text as T
18-
import qualified Data.Text.Encoding as TE
1920
import qualified Data.Text.IO as TI
2021
import qualified Data.UUID as UUID
2122
import qualified Data.UUID.V4 as UUID
@@ -24,9 +25,10 @@ import qualified Network.HTTP.Client as HC
2425
import qualified Network.HTTP.Types as HTTP
2526
import qualified Network.Wreq as Wreq
2627
import qualified Text.Regex.TDFA as TDFA
27-
import qualified Text.Regex.TDFA.ByteString as TDFA
28+
import qualified Text.Regex.TDFA.ReadRegex as TDFA
29+
import qualified Text.Regex.TDFA.TDFA as TDFA
2830

29-
import Hasura.Prelude
31+
import Hasura.RQL.Instances ()
3032

3133
newtype RequestId
3234
= RequestId { unRequestId :: Text }
@@ -72,15 +74,15 @@ getRequestId headers =
7274
Just reqId -> return $ RequestId $ bsToTxt reqId
7375

7476
-- Get an env var during compile time
75-
getValFromEnvOrScript :: String -> String -> TH.Q (TH.TExp String)
77+
getValFromEnvOrScript :: String -> String -> Q (TExp String)
7678
getValFromEnvOrScript n s = do
7779
maybeVal <- TH.runIO $ lookupEnv n
7880
case maybeVal of
7981
Just val -> [|| val ||]
8082
Nothing -> runScript s
8183

8284
-- Run a shell script during compile time
83-
runScript :: FilePath -> TH.Q (TH.TExp String)
85+
runScript :: FilePath -> Q (TExp String)
8486
runScript fp = do
8587
TH.addDependentFile fp
8688
fileContent <- TH.runIO $ TI.readFile fp
@@ -97,19 +99,11 @@ duplicates = mapMaybe greaterThanOne . group . sort
9799
where
98100
greaterThanOne l = bool Nothing (Just $ head l) $ length l > 1
99101

100-
-- regex related
101-
matchRegex :: B.ByteString -> Bool -> T.Text -> Either String Bool
102-
matchRegex regex caseSensitive src =
103-
fmap (`TDFA.match` TE.encodeUtf8 src) compiledRegexE
104-
where
105-
compOpt = TDFA.defaultCompOpt
106-
{ TDFA.caseSensitive = caseSensitive
107-
, TDFA.multiline = True
108-
, TDFA.lastStarGreedy = True
109-
}
110-
execOption = TDFA.defaultExecOpt {TDFA.captureGroups = False}
111-
compiledRegexE = TDFA.compile compOpt execOption regex
112-
102+
-- | Quotes a regex using Template Haskell so syntax errors can be reported at compile-time.
103+
quoteRegex :: TDFA.CompOption -> TDFA.ExecOption -> String -> Q (TExp TDFA.Regex)
104+
quoteRegex compOption execOption regexText = do
105+
regex <- TDFA.parseRegex regexText `onLeft` (fail . show)
106+
[|| TDFA.patternToRegex regex compOption execOption ||]
113107

114108
fmapL :: (a -> a') -> Either a b -> Either a' b
115109
fmapL fn (Left e) = Left (fn e)

‎server/tests-py/queries/v1/run_sql/setup.yaml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,15 @@ args:
2626
bool_col boolean not null default false
2727
);
2828
insert into test (name) values ('name_1'), ('name_2');
29+
create table malicious(
30+
id serial primary key,
31+
alterable boolean,
32+
droppable boolean
33+
);
34+
insert into malicious (alterable, droppable) values
35+
(true, false),
36+
(false, true);
37+
create view malicious_view as select * from malicious;
2938
3039
- type: track_table
3140
args:

‎server/tests-py/queries/v1/run_sql/sql_select_query_read_only.yaml

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
- url: /v1/query
2+
status: 200
3+
response:
4+
result_type: TuplesOk
5+
result:
6+
- - id
7+
- name
8+
- - '1'
9+
- Author 1
10+
- - '2'
11+
- Author 2
12+
query:
13+
type: run_sql
14+
args:
15+
read_only: True
16+
sql: |
17+
SELECT * from author
18+
19+
- url: /v1/query
20+
status: 200
21+
response:
22+
result_type: TuplesOk
23+
result:
24+
- - alterable
25+
- droppable
26+
- - t
27+
- f
28+
- - f
29+
- t
30+
query:
31+
type: run_sql
32+
args:
33+
read_only: True
34+
sql: |
35+
SELECT alterable, droppable FROM malicious
36+
37+
- url: /v1/query
38+
status: 200
39+
response:
40+
result_type: TuplesOk
41+
result:
42+
- - drop
43+
- - ' alter '
44+
query:
45+
type: run_sql
46+
args:
47+
read_only: True
48+
sql: |
49+
SELECT ' alter ' as drop
50+
51+
- url: /v1/query
52+
status: 400
53+
response:
54+
internal:
55+
statement: "DROP TABLE malicious\n"
56+
prepared: false
57+
error:
58+
exec_status: FatalError
59+
hint:
60+
message: cannot execute DROP TABLE in a read-only transaction
61+
status_code: '25006'
62+
description:
63+
arguments: []
64+
path: $.args
65+
error: query execution failed
66+
code: postgres-error
67+
query:
68+
type: run_sql
69+
args:
70+
read_only: True
71+
sql: |
72+
DROP TABLE malicious

‎server/tests-py/queries/v1/run_sql/teardown.yaml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,4 +6,6 @@ args:
66
drop table article;
77
drop table author;
88
drop table test;
9+
drop view malicious_view;
10+
drop table malicious;
911
cascade: true

‎server/tests-py/test_v1_queries.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -543,6 +543,10 @@ def test_select_query(self, hge_ctx):
543543
check_query_f(hge_ctx, self.dir() + '/sql_select_query.yaml')
544544
hge_ctx.may_skip_test_teardown = True
545545

546+
def test_select_query_read_only(self, hge_ctx):
547+
check_query_f(hge_ctx, self.dir() + '/sql_select_query_read_only.yaml')
548+
hge_ctx.may_skip_test_teardown = True
549+
546550
def test_set_timezone(self, hge_ctx):
547551
check_query_f(hge_ctx, self.dir() + '/sql_set_timezone.yaml')
548552
hge_ctx.may_skip_test_teardown = True

0 commit comments

Comments
 (0)