From 0a96a0933c10012b18ad33f37a0115301abfc407 Mon Sep 17 00:00:00 2001 From: Paolo Capriotti Date: Wed, 30 Aug 2023 13:52:09 +0200 Subject: [PATCH 1/4] Check validity of notification IDs --- integration/test/SetupHelpers.hs | 7 +++++-- integration/test/Test/Notifications.hs | 19 +++++++++++++++++++ libs/wire-api/src/Wire/API/Notification.hs | 9 +++++++++ services/gundeck/src/Gundeck/Monad.hs | 6 +++--- services/gundeck/src/Gundeck/Notification.hs | 11 +++++++++++ 5 files changed, 47 insertions(+), 5 deletions(-) diff --git a/integration/test/SetupHelpers.hs b/integration/test/SetupHelpers.hs index 8851e258900..d5e324ea41c 100644 --- a/integration/test/SetupHelpers.hs +++ b/integration/test/SetupHelpers.hs @@ -10,6 +10,7 @@ import Data.Aeson.Types qualified as Aeson import Data.Default import Data.Function import Data.List qualified as List +import Data.UUID.V1 (nextUUID) import Data.UUID.V4 (nextRandom) import GHC.Stack import Testlib.Prelude @@ -83,8 +84,10 @@ resetFedConns owndom = do Internal.deleteFedConn' owndom `mapM_` rdoms randomId :: HasCallStack => App String -randomId = do - liftIO (show <$> nextRandom) +randomId = liftIO (show <$> nextRandom) + +randomUUIDv1 :: HasCallStack => App String +randomUUIDv1 = liftIO (show . fromJust <$> nextUUID) randomUserId :: (HasCallStack, MakesValue domain) => domain -> App Value randomUserId domain = do diff --git a/integration/test/Test/Notifications.hs b/integration/test/Test/Notifications.hs index 5e6de9f3a59..ee17b0da485 100644 --- a/integration/test/Test/Notifications.hs +++ b/integration/test/Test/Notifications.hs @@ -61,3 +61,22 @@ testLastNotification = do lastNotif <- getLastNotification user "c" >>= getJSON 200 lastNotif %. "payload" `shouldMatch` [object ["client" .= "c"]] + +testInvalidNotification :: HasCallStack => App () +testInvalidNotification = do + user <- randomUserId OwnDomain + let client = "deadbeef" + + -- test uuid v4 as "since" + do + notifId <- randomId + void $ + getNotifications user client def {since = Just notifId} + >>= getJSON 400 + + -- test arbitrary uuid v1 as "since" + do + notifId <- randomUUIDv1 + void $ + getNotifications user client def {since = Just notifId} + >>= getJSON 404 diff --git a/libs/wire-api/src/Wire/API/Notification.hs b/libs/wire-api/src/Wire/API/Notification.hs index 3e808c02ef2..b404e05db61 100644 --- a/libs/wire-api/src/Wire/API/Notification.hs +++ b/libs/wire-api/src/Wire/API/Notification.hs @@ -20,6 +20,7 @@ module Wire.API.Notification ( NotificationId, + isValidNotificationId, RawNotificationId (..), Event, @@ -41,6 +42,7 @@ import Control.Lens (makeLenses, (.~)) import Control.Lens.Operators ((?~)) import Data.Aeson (FromJSON (..), ToJSON (..)) import Data.Aeson.Types qualified as Aeson +import Data.Bits import Data.HashMap.Strict.InsOrd qualified as InsOrdHashMap import Data.Id import Data.Json.Util @@ -50,6 +52,7 @@ import Data.Schema import Data.Swagger (ToParamSchema (..)) import Data.Swagger qualified as S import Data.Time.Clock (UTCTime) +import Data.UUID qualified as UUID import Imports import Servant import Wire.API.Routes.MultiVerb @@ -80,6 +83,12 @@ eventSchema = mkSchema sdoc Aeson.parseJSON (Just . Aeson.toJSON) ) ] +isValidNotificationId :: NotificationId -> Bool +isValidNotificationId (Id uuid) = + -- check that the version bits are set to 1 + case UUID.toWords uuid of + (_, w, _, _) -> (w `shiftR` 12) .&. 0xf == 1 + -------------------------------------------------------------------------------- -- QueuedNotification diff --git a/services/gundeck/src/Gundeck/Monad.hs b/services/gundeck/src/Gundeck/Monad.hs index 4ef03216436..f3a3a9512b3 100644 --- a/services/gundeck/src/Gundeck/Monad.hs +++ b/services/gundeck/src/Gundeck/Monad.hs @@ -45,9 +45,9 @@ where import Bilge hiding (Request, header, options, statusCode) import Bilge.RPC import Cassandra -import Control.Error hiding (err) +import Control.Error import Control.Exception (throwIO) -import Control.Lens hiding ((.=)) +import Control.Lens import Control.Monad.Catch hiding (tryJust) import Data.Aeson (FromJSON) import Data.Default (def) @@ -61,7 +61,7 @@ import Network.Wai import Network.Wai.Utilities import System.Logger qualified as Log import System.Logger qualified as Logger -import System.Logger.Class hiding (Error, info) +import System.Logger.Class import UnliftIO (async) -- | TODO: 'Client' already has an 'Env'. Why do we need two? How does this even work? We should diff --git a/services/gundeck/src/Gundeck/Notification.hs b/services/gundeck/src/Gundeck/Notification.hs index 5f41a7ba5cf..615ed79c29d 100644 --- a/services/gundeck/src/Gundeck/Notification.hs +++ b/services/gundeck/src/Gundeck/Notification.hs @@ -25,6 +25,8 @@ import Bilge.IO hiding (options) import Bilge.Request import Bilge.Response import Control.Lens (view) +import Control.Monad.Catch +import Control.Monad.Except import Data.ByteString.Conversion import Data.Id import Data.Misc (Milliseconds (..)) @@ -35,10 +37,13 @@ import Gundeck.Monad import Gundeck.Notification.Data qualified as Data import Gundeck.Options hiding (host, port) import Imports hiding (getLast) +import Network.HTTP.Types hiding (statusCode) +import Network.Wai.Utilities.Error import System.Logger.Class import System.Logger.Class qualified as Log import Util.Options hiding (host, port) import Wire.API.Internal.Notification +import Wire.API.Notification data PaginateResult = PaginateResult { paginateResultGap :: Bool, @@ -47,6 +52,7 @@ data PaginateResult = PaginateResult paginate :: UserId -> Maybe NotificationId -> Maybe ClientId -> Range 100 10000 Int32 -> Gundeck PaginateResult paginate uid since mclt size = do + traverse_ validateNotificationId since for_ mclt $ \clt -> updateActivity uid clt time <- posixTime @@ -60,6 +66,11 @@ paginate uid since mclt size = do (Just (millisToUTC time)) millisToUTC = posixSecondsToUTCTime . fromIntegral . (`div` 1000) . ms + validateNotificationId :: NotificationId -> Gundeck () + validateNotificationId n = + unless (isValidNotificationId n) $ + throwM (mkError status400 "bad-request" "Invalid Notification ID") + -- | Update last_active property of the given client by making a request to brig. updateActivity :: UserId -> ClientId -> Gundeck () updateActivity uid clt = do From 7769183b3c8920b01bc84554979aacf829b3351d Mon Sep 17 00:00:00 2001 From: Paolo Capriotti Date: Wed, 30 Aug 2023 14:06:41 +0200 Subject: [PATCH 2/4] Add CHANGELOG entry --- changelog.d/5-internal/notification-500 | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/5-internal/notification-500 diff --git a/changelog.d/5-internal/notification-500 b/changelog.d/5-internal/notification-500 new file mode 100644 index 00000000000..7af7198c513 --- /dev/null +++ b/changelog.d/5-internal/notification-500 @@ -0,0 +1 @@ +Check validity of notification IDs in the notification API From f42786dbc5101bc656cc4e008d090d30a78dac47 Mon Sep 17 00:00:00 2001 From: Paolo Capriotti Date: Thu, 31 Aug 2023 09:38:32 +0200 Subject: [PATCH 3/4] fixup! Add CHANGELOG entry From bdaa690791ace2e2d6e76385c5e0524e67bebed3 Mon Sep 17 00:00:00 2001 From: Paolo Capriotti Date: Thu, 31 Aug 2023 13:49:40 +0200 Subject: [PATCH 4/4] fixup! fixup! Add CHANGELOG entry