Forward request query parameters on file package redirects

alexbiehl · alexbiehl · commit dae893eb16a1 · 2024-02-09T12:43:18.000+01:00
diff --git a/src/Scarf/Gateway/Rule.hs b/src/Scarf/Gateway/Rule.hs
@@ -43,8 +43,8 @@ import Data.HashSet qualified as HashSet
 import Data.Text (Text)
 import Data.Text qualified as Text
 import Data.Text.Encoding qualified as Text
-import Network.HTTP.Types.URI (Query, parseQuery, parseQueryText)
-import Network.URI (URI (..), parseRelativeReference)
+import Network.HTTP.Types.URI (Query, parseQuery, parseQueryText, renderQuery)
+import Network.URI (URI (..), parseRelativeReference, parseURI)
 import Network.Wai
   ( pathInfo,
     rawQueryString,
@@ -549,7 +549,7 @@ matchFlatfile ::
   FlatfileRule ->
   Request ->
   m (Maybe RedirectOrProxy)
-matchFlatfile FlatfileRule {..} request@Request {requestPath, requestQueryString} = do
+matchFlatfile FlatfileRule {..} request@Request {requestWai, requestPath, requestQueryString} = do
   let -- If the public facing template captures query parameters we include the query string
       -- in the path to match, else we just check on the path.
       pathToMatch =
@@ -563,7 +563,8 @@ matchFlatfile FlatfileRule {..} request@Request {requestPath, requestQueryString
         let extracted = HashMap.fromList xs,
         HashSet.isSubsetOf ruleExpectedVariables (HashMap.keysSet extracted),
         Just !expanded <- URLTemplate.expand (`HashMap.lookup` extracted) ruleBackendTemplate -> do
-          let !absoluteUrl = Text.encodeUtf8 expanded
+          -- Rewrite the query string, if necessary
+          let !absoluteUrl = rewriteQueryString expanded requestWai
           pure
             ( Just
                 ( RedirectTo
@@ -573,7 +574,7 @@ matchFlatfile FlatfileRule {..} request@Request {requestPath, requestQueryString
                           fileVariables = extracted
                         }
                     )
-                    (Text.encodeUtf8 expanded)
+                    absoluteUrl
                 )
             )
       | Nothing <- ruleBackendTemplate,
@@ -600,7 +601,8 @@ matchFlatfile FlatfileRule {..} request@Request {requestPath, requestQueryString
         Just !ruleBackendExpanded <- URLTemplate.expand (\_ -> Nothing) ruleBackendTemplate,
         -- check that the publicPathRule matches our request including queryStrings if they are present
         pathAndQueryValidation rulePublicTemplate request -> do
-          let !absoluteUrl = Text.encodeUtf8 ruleBackendExpanded
+          -- Rewrite the query string, if necessary
+          let !absoluteUrl = rewriteQueryString ruleBackendExpanded requestWai
           pure
             ( Just
                 ( RedirectTo
@@ -630,15 +632,65 @@ matchFlatfile FlatfileRule {..} request@Request {requestPath, requestQueryString
     _ ->
       pure Nothing
 
+-- | It's a requirement to forward all query parameters to the redirect location.
+-- This function merges the query strings from the incoming request and the rendered
+-- redirect location.
+rewriteQueryString ::
+  -- | Absolute URL to redirect to
+  Text ->
+  -- | Incoming request
+  Wai.Request ->
+  ByteString
+rewriteQueryString redirectTarget request
+  -- Fast path: In case there are no query parameters on the request
+  -- there is no need for rewriting.
+  | [] <- Wai.queryString request =
+      Text.encodeUtf8 redirectTarget
+  -- Parse the redirect target as a URI to extract, combine and replace
+  -- the query part of it.
+  -- TODO: there are an aweful lot of string conversions going on,
+  -- maybe there's a more direct way.
+  | Just uri@URI {uriQuery} <- parseURI (Text.unpack redirectTarget) =
+      let redirectTargetQuery =
+            parseQuery (Text.encodeUtf8 (Text.pack uriQuery))
+
+          requestQuery =
+            Wai.queryString request
+
+          -- This is tricky: In case the redirect target already has query
+          -- parameters there is a chance that when combining the queries that
+          -- we have duplicate query parameters. Duplicates means they will
+          -- be counted twice in our main application. But if we nub them, we might
+          -- lose duplicate query parametes that were expected to occurr multiple
+          -- times -- after all, query strings may contain more than one occurrence of
+          -- a variable.
+          combinedQuery =
+            redirectTargetQuery <> requestQuery
+
+          renderedQuery =
+            Text.decodeUtf8
+              ( renderQuery
+                  True -- add a leading '?'
+                  combinedQuery
+              )
+       in Text.encodeUtf8 $
+            Text.pack $
+              show (uri {uriQuery = Text.unpack renderedQuery})
+  -- In case the redirectTarget didn't parse as a URI we are not doing anything
+  -- and ideally shouldn't happen.
+  | otherwise =
+      Text.encodeUtf8 redirectTarget
+
 matchFileRuleV2 :: (MonadMatch m) => FileRuleV2 -> Request -> m (Maybe RedirectOrProxy)
-matchFileRuleV2 FileRuleV2 {..} Request {requestPath} =
+matchFileRuleV2 FileRuleV2 {..} Request {requestWai, requestPath} =
   case Regex.match ruleIncomingPathRegex requestPath of
     Just xs
       | let extracted = HashMap.fromList xs,
         HashSet.isSubsetOf ruleExpectedVariables (HashMap.keysSet extracted),
         Just ruleBackendTemplate <- ruleBackendTemplate,
         Just !expanded <- URLTemplate.expand (`HashMap.lookup` extracted) ruleBackendTemplate -> do
-          let !absoluteUrl = Text.encodeUtf8 expanded
+          -- Rewrite the query string, if necessary
+          let !absoluteUrl = rewriteQueryString expanded requestWai
           pure
             ( Just
                 ( RedirectTo
@@ -648,7 +700,7 @@ matchFileRuleV2 FileRuleV2 {..} Request {requestPath} =
                           fileVariables = extracted
                         }
                     )
-                    (Text.encodeUtf8 expanded)
+                    absoluteUrl
                 )
             )
 
@@ -658,7 +710,8 @@ matchFileRuleV2 FileRuleV2 {..} Request {requestPath} =
       | HashSet.null ruleExpectedVariables,
         Just ruleBackendTemplate <- ruleBackendTemplate,
         Just !ruleBackendExpanded <- URLTemplate.expand (\_ -> Nothing) ruleBackendTemplate -> do
-          let !absoluteUrl = Text.encodeUtf8 ruleBackendExpanded
+          -- Rewrite the query string, if necessary
+          let !absoluteUrl = rewriteQueryString ruleBackendExpanded requestWai
           pure
             ( Just
                 ( RedirectTo
diff --git a/test/golden/file-package-with-variables-1.output.yaml b/test/golden/file-package-with-variables-1.output.yaml
@@ -3,13 +3,13 @@ capture: |-
     FlatfileCapture
       { fileAbsoluteUrl =
           Just
-            "https://datausa.io/api/data?drilldowns=Nation&measures=Population"
+            "https://datausa.io/api/data?drilldowns=Nation&measures=Population&drilldowns=Nation&measures=Population"
       , fileVariables =
           fromList
             [ ( "drilldowns" , "Nation" ) , ( "measures" , "Population" ) ]
       , filePackage = "a1b331fa-1539-49e5-bdc4-dc8a48e586d1"
       }
 headers:
-  Location: https://datausa.io/api/data?drilldowns=Nation&measures=Population
+  Location: https://datausa.io/api/data?drilldowns=Nation&measures=Population&drilldowns=Nation&measures=Population
 query: ?drilldowns=Nation&measures=Population
 status: 302
diff --git a/test/golden/file-package-with-variables-2.output.yaml b/test/golden/file-package-with-variables-2.output.yaml
@@ -3,13 +3,13 @@ capture: |-
     FlatfileCapture
       { fileAbsoluteUrl =
           Just
-            "https://datausa.io/api/data?drilldowns=Nation&measures=Population"
+            "https://datausa.io/api/data?drilldowns=Nation&measures=Population&measures=Population"
       , fileVariables =
           fromList
             [ ( "drilldowns" , "Nation" ) , ( "measures" , "Population" ) ]
       , filePackage = "a1b331fa-1539-49e5-bdc4-dc8a48e586d1"
       }
 headers:
-  Location: https://datausa.io/api/data?drilldowns=Nation&measures=Population
+  Location: https://datausa.io/api/data?drilldowns=Nation&measures=Population&measures=Population
 query: ?measures=Population
 status: 302
diff --git a/test/golden/file-package-without-variable-2.output.yaml b/test/golden/file-package-without-variable-2.output.yaml
@@ -1,11 +1,13 @@
 capture: |-
   Just
     FlatfileCapture
-      { fileAbsoluteUrl = Just "https://datausa.io/api/data"
+      { fileAbsoluteUrl =
+          Just
+            "https://datausa.io/api/data?drilldowns=Nation&measures=Population"
       , fileVariables = fromList [ ( "x" , "xxx" ) ]
       , filePackage = "a1b331fa-1539-49e5-bdc4-dc8a48e586d1"
       }
 headers:
-  Location: https://datausa.io/api/data
+  Location: https://datausa.io/api/data?drilldowns=Nation&measures=Population
 query: ?drilldowns=Nation&measures=Population
 status: 302
diff --git a/test/golden/file-package-without-variable.output.yaml b/test/golden/file-package-without-variable.output.yaml
@@ -1,11 +1,13 @@
 capture: |-
   Just
     FlatfileCapture
-      { fileAbsoluteUrl = Just "https://datausa.io/api/data"
+      { fileAbsoluteUrl =
+          Just
+            "https://datausa.io/api/data?drilldowns=Nation&measures=Population"
       , fileVariables = fromList []
       , filePackage = "a1b331fa-1539-49e5-bdc4-dc8a48e586d1"
       }
 headers:
-  Location: https://datausa.io/api/data
+  Location: https://datausa.io/api/data?drilldowns=Nation&measures=Population
 query: ?drilldowns=Nation&measures=Population
 status: 302
diff --git a/test/golden/file-package-without-varible-1.output.yaml b/test/golden/file-package-without-varible-1.output.yaml
@@ -3,11 +3,11 @@ capture: |-
     FlatfileCapture
       { fileAbsoluteUrl =
           Just
-            "https://datausa.io/api/data?drilldowns=Nation&measures=Population"
+            "https://datausa.io/api/data?drilldowns=Nation&measures=Population&drilldowns=Nation&measures=Population"
       , fileVariables = fromList []
       , filePackage = "a1b331fa-1539-49e5-bdc4-dc8a48e586d1"
       }
 headers:
-  Location: https://datausa.io/api/data?drilldowns=Nation&measures=Population
+  Location: https://datausa.io/api/data?drilldowns=Nation&measures=Population&drilldowns=Nation&measures=Population
 query: ?drilldowns=Nation&measures=Population
 status: 302
diff --git a/test/golden/file-package-without-varible-2.output.yaml b/test/golden/file-package-without-varible-2.output.yaml
@@ -3,11 +3,11 @@ capture: |-
     FlatfileCapture
       { fileAbsoluteUrl =
           Just
-            "https://datausa.io/api/data?drilldowns=Nation&measures=Population"
+            "https://datausa.io/api/data?drilldowns=Nation&measures=Population&drilldowns=Nation&measures=Population"
       , fileVariables = fromList []
       , filePackage = "a1b331fa-1539-49e5-bdc4-dc8a48e586d1"
       }
 headers:
-  Location: https://datausa.io/api/data?drilldowns=Nation&measures=Population
+  Location: https://datausa.io/api/data?drilldowns=Nation&measures=Population&drilldowns=Nation&measures=Population
 query: ?drilldowns=Nation&measures=Population
 status: 302
diff --git a/test/golden/issue-2024-02-06.output.body b/test/golden/issue-2024-02-06.output.body
diff --git a/test/golden/issue-2024-02-06.output.yaml b/test/golden/issue-2024-02-06.output.yaml
@@ -0,0 +1,11 @@
+capture: |-
+  Just
+    FlatfileCapture
+      { fileAbsoluteUrl = Just "https://some.host/blah/blah?prefix=foo"
+      , fileVariables = fromList [ ( "path" , "blah/blah" ) ]
+      , filePackage = "a1b331fa-1539-49e5-bdc4-dc8a48e586d1"
+      }
+headers:
+  Location: https://some.host/blah/blah?prefix=foo
+query: ?prefix=foo
+status: 302
diff --git a/test/golden/issue-2024-02-06.yaml b/test/golden/issue-2024-02-06.yaml
@@ -0,0 +1,10 @@
+path: /blah/blah?prefix=foo
+headers: 
+  Host: test-org.gateway.scarf.sh
+manifest:
+  rules:
+    - type: file-v1
+      incoming-path: /{+path}
+      domain: test-org.gateway.scarf.sh
+      package-id: a1b331fa-1539-49e5-bdc4-dc8a48e586d1
+      outgoing-url: https://some.host/{+path}
