Skip to content

fix: malformed redirect url when return_url has query params #1599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gausam wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: malformed redirect url when return_url has query params #1599

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/hyper-loader/Elements.res
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ open ErrorUtils
open Identity
open Utils
open EventListenerManager
open URLModule

@XyneSpaces XyneSpaces Jun 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential runtime error if returnUrl is empty or invalid

URLModule.makeUrl calls the native JavaScript URL constructor, which throws a TypeError on invalid URLs. The code:

let returnUrl = dict->getString("return_url", "")
let url = makeUrl(returnUrl)

Will throw if return_url is empty or malformed. Consider adding a guard:

if returnUrl->String.length > 0 {
  let url = makeUrl(returnUrl)
  // ...
}

Or wrap in try-catch if invalid URLs should be handled gracefully.

@XyneSpaces XyneSpaces Jun 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Missing error handling for makeUrl

If returnUrl is empty, malformed, or contains invalid characters, makeUrl() may throw an exception. This would cause an unhandled rejection in the promise chain and leave the user without a redirect.

Consider adding a try-catch fallback:

let redirectUrl = try {
  let url = makeUrl(returnUrl)
  url.searchParams.set("payment_intent_client_secret", clientSecretRef.contents)
  url.searchParams.set("status", status)
  url.href
} catch {
| _ => `${returnUrl}?payment_intent_client_secret=${clientSecretRef.contents}&status=${status}`
}

@gausam gausam Jun 3, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@XyneSpaces Thanks for flagging this.

On makeUrl throwing: that behavior is indeed introduced by this change. The previous string-concat path wouldn’t throw for empty/malformed return_url; it would still produce a redirect URL (often incorrect, especially when query params already existed, which this PR fixes).

On try/catch + concat fallback: I’m hesitant to add that. If return_url is empty or invalid, falling back to ${returnUrl}?payment_intent_client_secret=... doesn’t make the redirect correct; it only avoids the exception. We’d still end up with a bad URL (e.g. ?secret=... for empty input, or broken query merging for URLs that fail parsing). So we’d be trading a visible failure for a silent broken redirect, and in some cases reintroducing the original query-string bug on the fallback path.

If return_url is missing or malformed, that’s a merchant/config issue, perhaps? The same assumption existed before this PR (and elsewhere in the codebase, e.g. PaymentHelpers). Happy to add a small guard for the empty-string case if you’d like, but I don’t think a try/catch that forces the success path is the right fix for invalid URLs.

makeUrl throwing is caught by the existing .catch on the retrieve chain (here and here), so it is not an unhandled rejection. It does, however, route through the error path (submitSuccessful: false + error payload), not the success redirect with merged query params. A try/catch fallback to string concat would force the success path with a URL that is still wrong for invalid input, which does not seem like an improvement.

Thanks for the review. Let me know what you prefer.


type trustPayFunctions = {
finishApplePaymentV2: (string, ApplePayTypes.paymentRequestData, string) => promise<JSON.t>,
Expand Down Expand Up @@ -811,7 +812,10 @@ let make = (
let dict = json->getDictFromJson
let status = dict->getString("status", "")
let returnUrl = dict->getString("return_url", "")
let redirectUrl = `${returnUrl}?payment_intent_client_secret=${clientSecretRef.contents}&status=${status}`
let url = makeUrl(returnUrl)
url.searchParams.set("payment_intent_client_secret", clientSecretRef.contents)
url.searchParams.set("status", status)
let redirectUrl = url.href
if redirect.contents === "always" {
Utils.replaceRootHref(redirectUrl, redirectionFlags)
resolve(JSON.Encode.null)
Expand Down
Loading