fix: improve jwt errors

BREAKING CHANGE

We were exposing internal errors via `Show`. This commit
improves error messages for better user experience.

Author: taimoorzaeem

src/PostgREST/Auth.hs

@@ -54,33 +54,44 @@ import Protolude
 
 -- | Receives the JWT secret and audience (from config) and a JWT and returns a
 -- JSON object of JWT claims.
-parseToken :: AppConfig -> ByteString -> UTCTime -> ExceptT Error IO JSON.Value
-parseToken _ "" _ = return JSON.emptyObject
-parseToken AppConfig{..} token time = do
-  secret <- liftEither . maybeToRight (JwtErr JwtTokenMissing) $ configJWKS
+parseToken :: AppConfig -> Maybe ByteString -> UTCTime -> ExceptT Error IO JSON.Value
+parseToken _ Nothing _ = return JSON.emptyObject
+parseToken _ (Just "") _ = throwE . JwtErr $ JwtDecodeError "Empty JWT is sent in Authorization header" -- should we validate JWT ourselves before decoding or let the library (not very user friendly with errors) handle it?
+parseToken AppConfig{..} (Just token) time = do
+  secret <- liftEither . maybeToRight (JwtErr JwtSecretMissing) $ configJWKS
   eitherContent <- liftIO $ JWT.decode (JWT.keys secret) Nothing token
   content <- liftEither . mapLeft (JwtErr . jwtDecodeError) $ eitherContent
   liftEither $ mapLeft JwtErr $ verifyClaims content
   where
-      -- TODO: Improve errors, those were just taken as-is from hs-jose to avoid
-      -- breaking changes.
       jwtDecodeError :: JWT.JwtError -> JwtError
-      jwtDecodeError (JWT.KeyError _)     = JwtTokenInvalid "JWSError JWSInvalidSignature"
-      jwtDecodeError JWT.BadCrypto        = JwtTokenInvalid "JWSError (CompactDecodeError Invalid number of parts: Expected 3 parts; got 2)"
-      jwtDecodeError (JWT.BadAlgorithm _) = JwtTokenInvalid "JWSError JWSNoSignatures"
-      jwtDecodeError e                    = JwtTokenInvalid $ show e
+      -- The only errors we can get from JWT.decode function are:
+      --   BadAlgorithm
+      --   KeyError
+      --   BadCrypto
+      -- So others should have a generic message because they can't
+      -- be covered by tests?
+      jwtDecodeError (JWT.KeyError _)     = JwtDecodeError "No suitable key or wrong key type"
+      jwtDecodeError (JWT.BadAlgorithm _) = JwtDecodeError "Wrong or unsupported encoding algorithm"
+      jwtDecodeError JWT.BadCrypto        = JwtDecodeError "JWT parsing failed"
+      -- We can't test below cases with current implementation
+      -- TODO: Remove them or replace with a single generic message?
+      jwtDecodeError (JWT.BadDots dots)   = JwtDecodeError ("Wrong number of '.' periods in JWT: Expected 3, got " <> show dots)
+      jwtDecodeError (JWT.BadHeader _)    = JwtDecodeError "Header couldn't be decoded or contains bad data"
+      jwtDecodeError JWT.BadClaims        = JwtClaimsError "JWT claims couldn't be decoded or contains bad data"
+      jwtDecodeError JWT.BadSignature     = JwtDecodeError "The JWT signature couldn't be decoded or is invalid"
+      jwtDecodeError (JWT.Base64Error _)  = JwtDecodeError "A base64 decoding error has occured"
 
       verifyClaims :: JWT.JwtContent -> Either JwtError JSON.Value
       verifyClaims (JWT.Jws (_, claims)) = case JSON.decodeStrict claims of
-        Nothing                    -> Left $ JwtTokenInvalid "Parsing claims failed"
+        Nothing                    -> Left $ JwtClaimsError "Parsing claims failed"
         Just (JSON.Object mclaims)
-          | failedExpClaim mclaims -> Left $ JwtTokenInvalid "JWT expired"
-          | failedNbfClaim mclaims -> Left $ JwtTokenInvalid "JWTNotYetValid"
-          | failedIatClaim mclaims -> Left $ JwtTokenInvalid "JWTIssuedAtFuture"
-          | failedAudClaim mclaims -> Left $ JwtTokenInvalid "JWTNotInAudience"
+          | failedExpClaim mclaims -> Left $ JwtClaimsError "JWT expired"
+          | failedNbfClaim mclaims -> Left $ JwtClaimsError "JWT not yet valid"
+          | failedIatClaim mclaims -> Left $ JwtClaimsError "JWT issued at future"
+          | failedAudClaim mclaims -> Left $ JwtClaimsError "JWT not in audience"
         Just jclaims               -> Right jclaims
       -- TODO: We could enable JWE support here (encrypted tokens)
-      verifyClaims _                = Left $ JwtTokenInvalid "Unsupported token type"
+      verifyClaims _                = Left $ JwtDecodeError "Unsupported token type"
 
       allowedSkewSeconds = 30 :: Int64
       now = floor . nominalDiffTimeToSeconds $ utcTimeToPOSIXSeconds time
@@ -148,7 +159,7 @@ middleware appState app req respond = do
   conf <- getConfig appState
   time <- getTime appState
 
-  let token  = fromMaybe "" $ Wai.extractBearerAuth =<< lookup HTTP.hAuthorization (Wai.requestHeaders req)
+  let token  = Wai.extractBearerAuth =<< lookup HTTP.hAuthorization (Wai.requestHeaders req)
       parseJwt = runExceptT $ parseToken conf token time >>= parseClaims conf
       jwtCacheState = getJwtCacheState appState
 
@@ -160,15 +171,19 @@ middleware appState app req respond = do
           return $ req { Wai.vault = Wai.vault req & Vault.insert authResultKey authResult & Vault.insert jwtDurKey dur }
 
     (True, maxLifetime)  -> do
-          (dur, authResult) <- timeItT $ lookupJwtCache jwtCacheState token maxLifetime parseJwt time
+          (dur, authResult) <- timeItT $ case token of
+            Just tkn -> lookupJwtCache jwtCacheState tkn maxLifetime parseJwt time
+            Nothing  -> parseJwt
           return $ req { Wai.vault = Wai.vault req & Vault.insert authResultKey authResult & Vault.insert jwtDurKey dur }
 
     (False, 0)           -> do
           authResult <- parseJwt
           return $ req { Wai.vault = Wai.vault req & Vault.insert authResultKey authResult }
 
     (False, maxLifetime) -> do
-          authResult <- lookupJwtCache jwtCacheState token maxLifetime parseJwt time
+          authResult <- case token of
+            Just tkn -> lookupJwtCache jwtCacheState tkn maxLifetime parseJwt time
+            Nothing -> parseJwt
           return $ req { Wai.vault = Wai.vault req & Vault.insert authResultKey authResult }
 
   app req' respond

src/PostgREST/Error.hs

@@ -577,9 +577,10 @@ data Error
   | PgErr PgError
 
 data JwtError
-  = JwtTokenInvalid Text
-  | JwtTokenMissing
+  = JwtDecodeError Text
+  | JwtSecretMissing
   | JwtTokenRequired
+  | JwtClaimsError Text
 
 instance PgrstError Error where
   status (ApiRequestError err) = status err
@@ -593,13 +594,15 @@ instance PgrstError Error where
   headers _                     = mempty
 
 instance PgrstError JwtError where
-  status JwtTokenInvalid{} = HTTP.unauthorized401
-  status JwtTokenMissing   = HTTP.status500
-  status JwtTokenRequired  = HTTP.unauthorized401
+  status JwtDecodeError{} = HTTP.unauthorized401
+  status JwtSecretMissing = HTTP.status500
+  status JwtTokenRequired = HTTP.unauthorized401
+  status JwtClaimsError{} = HTTP.unauthorized401
 
-  headers (JwtTokenInvalid m) = [invalidTokenHeader m]
-  headers JwtTokenRequired    = [requiredTokenHeader]
-  headers _                   = mempty
+  headers (JwtDecodeError m) = [invalidTokenHeader m]
+  headers JwtTokenRequired   = [requiredTokenHeader]
+  headers (JwtClaimsError m) = [invalidTokenHeader m]
+  headers _                  = mempty
 
 instance JSON.ToJSON Error where
   toJSON (ApiRequestError err) = JSON.toJSON err
@@ -608,16 +611,20 @@ instance JSON.ToJSON Error where
   toJSON NoSchemaCacheError    = toJsonPgrstError
       ConnectionErrorCode02 "Could not query the database for the schema cache. Retrying." Nothing Nothing
 
+-- Should we provide hints and description or explain the error in docs or both?
 instance JSON.ToJSON JwtError where
-  toJSON JwtTokenMissing = toJsonPgrstError
+  toJSON JwtSecretMissing = toJsonPgrstError
       JWTErrorCode00 "Server lacks JWT secret" Nothing Nothing
 
-  toJSON (JwtTokenInvalid message) = toJsonPgrstError
+  toJSON (JwtDecodeError message) = toJsonPgrstError
       JWTErrorCode01 message Nothing Nothing
 
   toJSON JwtTokenRequired = toJsonPgrstError
       JWTErrorCode02 "Anonymous access is disabled" Nothing Nothing
 
+  toJSON (JwtClaimsError message) = toJsonPgrstError
+      JWTErrorCode03 message Nothing Nothing
+
 
 invalidTokenHeader :: Text -> Header
 invalidTokenHeader m =
@@ -710,6 +717,7 @@ data ErrorCode
   | JWTErrorCode00
   | JWTErrorCode01
   | JWTErrorCode02
+  | JWTErrorCode03
   -- Internal errors related to the Hasql library
   | InternalErrorCode00
 
@@ -758,5 +766,6 @@ buildErrorCode code = case code of
   JWTErrorCode00         -> "PGRST300"
   JWTErrorCode01         -> "PGRST301"
   JWTErrorCode02         -> "PGRST302"
+  JWTErrorCode03         -> "PGRST303"
 
   InternalErrorCode00    -> "PGRSTX00"

test/io/test_io.py

@@ -92,7 +92,7 @@ def test_jwt_errors(defaultenv):
         headers = jwtauthheader({}, "other secret")
         response = postgrest.session.get("/", headers=headers)
         assert response.status_code == 401
-        assert response.json()["message"] == "JWSError JWSInvalidSignature"
+        assert response.json()["message"] == "No suitable key or wrong key type"
 
         headers = jwtauthheader({"role": "not_existing"}, SECRET)
         response = postgrest.session.get("/", headers=headers)
@@ -110,35 +110,32 @@ def test_jwt_errors(defaultenv):
         headers = jwtauthheader({"nbf": relativeSeconds(31)}, SECRET)
         response = postgrest.session.get("/", headers=headers)
         assert response.status_code == 401
-        assert response.json()["message"] == "JWTNotYetValid"
+        assert response.json()["message"] == "JWT not yet valid"
 
         # 31 seconds, because we allow clock skew of 30 seconds
         headers = jwtauthheader({"iat": relativeSeconds(31)}, SECRET)
         response = postgrest.session.get("/", headers=headers)
         assert response.status_code == 401
-        assert response.json()["message"] == "JWTIssuedAtFuture"
+        assert response.json()["message"] == "JWT issued at future"
 
         headers = jwtauthheader({"aud": "not set"}, SECRET)
         response = postgrest.session.get("/", headers=headers)
         assert response.status_code == 401
-        assert response.json()["message"] == "JWTNotInAudience"
+        assert response.json()["message"] == "JWT not in audience"
 
         # partial token, no signature
         headers = authheader("eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.bm90IGFuIG9iamVjdA")
         response = postgrest.session.get("/", headers=headers)
         assert response.status_code == 401
-        assert (
-            response.json()["message"]
-            == "JWSError (CompactDecodeError Invalid number of parts: Expected 3 parts; got 2)"
-        )
+        assert response.json()["message"] == "JWT parsing failed"
 
         # token with algorithm "none"
         headers = authheader(
             "eyJ0eXAiOiJKV1QiLCJhbGciOiJub25lIn0.e30.yOBhlOIqn56T-4NvyEXCjfi3UmyQZ-BzXtePMO2NgRI"
         )
         response = postgrest.session.get("/", headers=headers)
         assert response.status_code == 401
-        assert response.json()["message"] == "JWSError JWSNoSignatures"
+        assert response.json()["message"] == "Wrong or unsupported encoding algorithm"
 
 
 def test_fail_with_invalid_password(defaultenv):

test/spec/Feature/Auth/AuthSpec.hs

@@ -88,7 +88,7 @@ spec = describe "authorization" $ do
   it "fails with an expired token" $ do
     let auth = authHeaderJWT "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjE0NDY2NzgxNDksInJvbGUiOiJwb3N0Z3Jlc3RfdGVzdF9hdXRob3IiLCJpZCI6Impkb2UifQ.f8__E6VQwYcDqwHmr9PG03uaZn8Zh1b0vbJ9DYS0AdM"
     request methodGet "/authors_only" [auth] ""
-      `shouldRespondWith` [json| {"message":"JWT expired","code":"PGRST301","hint":null,"details":null} |]
+      `shouldRespondWith` [json| {"message":"JWT expired","code":"PGRST303","hint":null,"details":null} |]
         { matchStatus = 401
         , matchHeaders = [
             "WWW-Authenticate" <:>
@@ -99,11 +99,11 @@ spec = describe "authorization" $ do
   it "hides tables from users with invalid JWT" $ do
     let auth = authHeaderJWT "ey9zdGdyZXN0X3Rlc3RfYXV0aG9yIiwiaWQiOiJqZG9lIn0.y4vZuu1dDdwAl0-S00MCRWRYMlJ5YAMSir6Es6WtWx0"
     request methodGet "/authors_only" [auth] ""
-      `shouldRespondWith` [json| {"message":"JWSError (CompactDecodeError Invalid number of parts: Expected 3 parts; got 2)","code":"PGRST301","hint":null,"details":null} |]
+      `shouldRespondWith` [json| {"message":"JWT parsing failed","code":"PGRST301","hint":null,"details":null} |]
         { matchStatus = 401
         , matchHeaders = [
             "WWW-Authenticate" <:>
-            "Bearer error=\"invalid_token\", error_description=\"JWSError (CompactDecodeError Invalid number of parts: Expected 3 parts; got 2)\""
+            "Bearer error=\"invalid_token\", error_description=\"JWT parsing failed\""
           ]
         }