Skip to content

Commit 7b68e1e

Browse files
alexbiehlalexbiehl
committed
Forward request query parameters on file package redirects
1 parent 01b382f commit 7b68e1e

10 files changed

+95
-17
lines changed

‎src/Scarf/Gateway/Rule.hs

+61-8
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,8 @@ import Data.HashSet qualified as HashSet
4343
import Data.Text (Text)
4444
import Data.Text qualified as Text
4545
import Data.Text.Encoding qualified as Text
46-
import Network.HTTP.Types.URI (Query, parseQuery, parseQueryText)
47-
import Network.URI (URI (..), parseRelativeReference)
46+
import Network.HTTP.Types.URI (Query, parseQuery, parseQueryText, renderQuery)
47+
import Network.URI (URI (..), parseRelativeReference, parseURI)
4848
import Network.Wai
4949
( pathInfo,
5050
rawQueryString,
@@ -549,7 +549,7 @@ matchFlatfile ::
549549
FlatfileRule ->
550550
Request ->
551551
m (Maybe RedirectOrProxy)
552-
matchFlatfile FlatfileRule {..} request@Request {requestPath, requestQueryString} = do
552+
matchFlatfile FlatfileRule {..} request@Request {requestWai, requestPath, requestQueryString} = do
553553
let -- If the public facing template captures query parameters we include the query string
554554
-- in the path to match, else we just check on the path.
555555
pathToMatch =
@@ -563,7 +563,8 @@ matchFlatfile FlatfileRule {..} request@Request {requestPath, requestQueryString
563563
let extracted = HashMap.fromList xs,
564564
HashSet.isSubsetOf ruleExpectedVariables (HashMap.keysSet extracted),
565565
Just !expanded <- URLTemplate.expand (`HashMap.lookup` extracted) ruleBackendTemplate -> do
566-
let !absoluteUrl = Text.encodeUtf8 expanded
566+
-- Rewrite the query string, if necessary
567+
let !absoluteUrl = rewriteQueryString expanded requestWai
567568
pure
568569
( Just
569570
( RedirectTo
@@ -600,7 +601,8 @@ matchFlatfile FlatfileRule {..} request@Request {requestPath, requestQueryString
600601
Just !ruleBackendExpanded <- URLTemplate.expand (\_ -> Nothing) ruleBackendTemplate,
601602
-- check that the publicPathRule matches our request including queryStrings if they are present
602603
pathAndQueryValidation rulePublicTemplate request -> do
603-
let !absoluteUrl = Text.encodeUtf8 ruleBackendExpanded
604+
-- Rewrite the query string, if necessary
605+
let !absoluteUrl = rewriteQueryString ruleBackendExpanded requestWai
604606
pure
605607
( Just
606608
( RedirectTo
@@ -630,15 +632,65 @@ matchFlatfile FlatfileRule {..} request@Request {requestPath, requestQueryString
630632
_ ->
631633
pure Nothing
632634

635+
-- | It's a requirement to forward all query parameters to the redirect location.
636+
-- This function merges the query strings from the incoming request and the rendered
637+
-- redirect location.
638+
rewriteQueryString ::
639+
-- | Absolute URL to redirect to
640+
Text ->
641+
-- | Incoming request
642+
Wai.Request ->
643+
ByteString
644+
rewriteQueryString redirectTarget request
645+
-- Fast path: In case there are no query parameters on the request
646+
-- there is no need for rewriting.
647+
| [] <- Wai.queryString request =
648+
Text.encodeUtf8 redirectTarget
649+
-- Parse the redirect target as a URI to extract, combine and replace
650+
-- the query part of it.
651+
-- TODO: there are an aweful lot of string conversions going on,
652+
-- maybe there's a more direct way.
653+
| Just uri@URI {uriQuery} <- parseURI (Text.unpack redirectTarget) =
654+
let redirectTargetQuery =
655+
parseQuery (Text.encodeUtf8 (Text.pack uriQuery))
656+
657+
requestQuery =
658+
Wai.queryString request
659+
660+
-- This is tricky: In case the redirect target already has query
661+
-- parameters there is a chance that when combining the queries that
662+
-- we have duplicate query parameters. Duplicates means they will
663+
-- be counted twice in our main application. But if we nub them, we might
664+
-- lose duplicate query parametes that were expected to occurr multiple
665+
-- times -- after all, query strings may contain more than one occurrence of
666+
-- a variable.
667+
combinedQuery =
668+
redirectTargetQuery <> requestQuery
669+
670+
renderedQuery =
671+
Text.decodeUtf8
672+
( renderQuery
673+
True -- add a leading '?'
674+
combinedQuery
675+
)
676+
in Text.encodeUtf8 $
677+
Text.pack $
678+
show (uri {uriQuery = Text.unpack renderedQuery})
679+
-- In case the redirectTarget didn't parse as a URI we are not doing anything
680+
-- and ideally shouldn't happen.
681+
| otherwise =
682+
Text.encodeUtf8 redirectTarget
683+
633684
matchFileRuleV2 :: (MonadMatch m) => FileRuleV2 -> Request -> m (Maybe RedirectOrProxy)
634-
matchFileRuleV2 FileRuleV2 {..} Request {requestPath} =
685+
matchFileRuleV2 FileRuleV2 {..} Request {requestWai, requestPath} =
635686
case Regex.match ruleIncomingPathRegex requestPath of
636687
Just xs
637688
| let extracted = HashMap.fromList xs,
638689
HashSet.isSubsetOf ruleExpectedVariables (HashMap.keysSet extracted),
639690
Just ruleBackendTemplate <- ruleBackendTemplate,
640691
Just !expanded <- URLTemplate.expand (`HashMap.lookup` extracted) ruleBackendTemplate -> do
641-
let !absoluteUrl = Text.encodeUtf8 expanded
692+
-- Rewrite the query string, if necessary
693+
let !absoluteUrl = rewriteQueryString expanded requestWai
642694
pure
643695
( Just
644696
( RedirectTo
@@ -658,7 +710,8 @@ matchFileRuleV2 FileRuleV2 {..} Request {requestPath} =
658710
| HashSet.null ruleExpectedVariables,
659711
Just ruleBackendTemplate <- ruleBackendTemplate,
660712
Just !ruleBackendExpanded <- URLTemplate.expand (\_ -> Nothing) ruleBackendTemplate -> do
661-
let !absoluteUrl = Text.encodeUtf8 ruleBackendExpanded
713+
-- Rewrite the query string, if necessary
714+
let !absoluteUrl = rewriteQueryString ruleBackendExpanded requestWai
662715
pure
663716
( Just
664717
( RedirectTo

‎test/golden/file-package-with-variables-1.output.yaml

+1-1
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ capture: |-
33
FlatfileCapture
44
{ fileAbsoluteUrl =
55
Just
6-
"https://datausa.io/api/data?drilldowns=Nation&measures=Population"
6+
"https://datausa.io/api/data?drilldowns=Nation&measures=Population&drilldowns=Nation&measures=Population"
77
, fileVariables =
88
fromList
99
[ ( "drilldowns" , "Nation" ) , ( "measures" , "Population" ) ]

‎test/golden/file-package-with-variables-2.output.yaml

+1-1
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ capture: |-
33
FlatfileCapture
44
{ fileAbsoluteUrl =
55
Just
6-
"https://datausa.io/api/data?drilldowns=Nation&measures=Population"
6+
"https://datausa.io/api/data?drilldowns=Nation&measures=Population&measures=Population"
77
, fileVariables =
88
fromList
99
[ ( "drilldowns" , "Nation" ) , ( "measures" , "Population" ) ]

‎test/golden/file-package-without-variable-2.output.yaml

+3-1
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
capture: |-
22
Just
33
FlatfileCapture
4-
{ fileAbsoluteUrl = Just "https://datausa.io/api/data"
4+
{ fileAbsoluteUrl =
5+
Just
6+
"https://datausa.io/api/data?drilldowns=Nation&measures=Population"
57
, fileVariables = fromList [ ( "x" , "xxx" ) ]
68
, filePackage = "a1b331fa-1539-49e5-bdc4-dc8a48e586d1"
79
}

‎test/golden/file-package-without-variable.output.yaml

+4-2
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
capture: |-
22
Just
33
FlatfileCapture
4-
{ fileAbsoluteUrl = Just "https://datausa.io/api/data"
4+
{ fileAbsoluteUrl =
5+
Just
6+
"https://datausa.io/api/data?drilldowns=Nation&measures=Population"
57
, fileVariables = fromList []
68
, filePackage = "a1b331fa-1539-49e5-bdc4-dc8a48e586d1"
79
}
810
headers:
9-
Location: https://datausa.io/api/data
11+
Location: https://datausa.io/api/data?drilldowns=Nation&measures=Population
1012
query: ?drilldowns=Nation&measures=Population
1113
status: 302

‎test/golden/file-package-without-varible-1.output.yaml

+2-2
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,11 @@ capture: |-
33
FlatfileCapture
44
{ fileAbsoluteUrl =
55
Just
6-
"https://datausa.io/api/data?drilldowns=Nation&measures=Population"
6+
"https://datausa.io/api/data?drilldowns=Nation&measures=Population&drilldowns=Nation&measures=Population"
77
, fileVariables = fromList []
88
, filePackage = "a1b331fa-1539-49e5-bdc4-dc8a48e586d1"
99
}
1010
headers:
11-
Location: https://datausa.io/api/data?drilldowns=Nation&measures=Population
11+
Location: https://datausa.io/api/data?drilldowns=Nation&measures=Population&drilldowns=Nation&measures=Population
1212
query: ?drilldowns=Nation&measures=Population
1313
status: 302

‎test/golden/file-package-without-varible-2.output.yaml

+2-2
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,11 @@ capture: |-
33
FlatfileCapture
44
{ fileAbsoluteUrl =
55
Just
6-
"https://datausa.io/api/data?drilldowns=Nation&measures=Population"
6+
"https://datausa.io/api/data?drilldowns=Nation&measures=Population&drilldowns=Nation&measures=Population"
77
, fileVariables = fromList []
88
, filePackage = "a1b331fa-1539-49e5-bdc4-dc8a48e586d1"
99
}
1010
headers:
11-
Location: https://datausa.io/api/data?drilldowns=Nation&measures=Population
11+
Location: https://datausa.io/api/data?drilldowns=Nation&measures=Population&drilldowns=Nation&measures=Population
1212
query: ?drilldowns=Nation&measures=Population
1313
status: 302

‎test/golden/issue-2024-02-06.output.body

Whitespace-only changes.

‎test/golden/issue-2024-02-06.output.yaml

+11
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
capture: |-
2+
Just
3+
FlatfileCapture
4+
{ fileAbsoluteUrl = Just "https://some.host/blah/blah?prefix=foo"
5+
, fileVariables = fromList [ ( "path" , "blah/blah" ) ]
6+
, filePackage = "a1b331fa-1539-49e5-bdc4-dc8a48e586d1"
7+
}
8+
headers:
9+
Location: https://some.host/blah/blah
10+
query: ?prefix=foo
11+
status: 302

‎test/golden/issue-2024-02-06.yaml

+10
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
path: /blah/blah?prefix=foo
2+
headers:
3+
Host: test-org.gateway.scarf.sh
4+
manifest:
5+
rules:
6+
- type: file-v1
7+
incoming-path: /{+path}
8+
domain: test-org.gateway.scarf.sh
9+
package-id: a1b331fa-1539-49e5-bdc4-dc8a48e586d1
10+
outgoing-url: https://some.host/{+path}

0 commit comments

Comments
 (0)