fix: malformed redirect url when return_url has query params

[gausam]

Fixes #1598

Type of Change

• Bugfix
• New feature
• Enhancement
• Refactoring
• Dependency updates
• Documentation
• CI/CD

Description

Fixes malformed post-poll-status redirect URLs in Elements when return_url already contains query parameters.

Problem

In handleRetrievePaymentResponse (Elements.res), the redirect URL was built with string concatenation:

`${returnUrl}?payment_intent_client_secret=...&status=...`

[semanticdiff-com]

Review changes with  

[XyneSpaces]

Review Summary

Classification: fix · redirect-url · query-parameter-handling
Risk: Low
Verdict: ✅ LGTM — clean fix for malformed redirect URLs

---

Analysis

This PR fixes a URL construction bug where appending query parameters via string concatenation produced malformed URLs when the return_url already contained query parameters.

Before:

let redirectUrl = `${returnUrl}?payment_intent_client_secret=${clientSecretRef.contents}&status=${status}`
// With returnUrl = "https://example.com/checkout?session=abc"
// Result: "https://example.com/checkout?session=abc?payment_intent_client_secret=...&status=..." 
// ^^ Double ? makes this an invalid URL

After:

let url = makeUrl(returnUrl)
url.searchParams.set("payment_intent_client_secret", clientSecretRef.contents)
url.searchParams.set("status", status)
let redirectUrl = url.href
// Properly handles existing query params: "https://example.com/checkout?session=abc&payment_intent_client_secret=...&status=..."

---

Tier 1 Checks

Check	Status	Notes
Silent-null onFailure	N/A	No error handling changes
Card data in logs	N/A	No logging changes
Endpoint routing	N/A	URL construction only, not API endpoint definition
postMessage targets	N/A	No outbound messages
Amount arithmetic	N/A	No financial calculations

---

Tier 2 Checks

• ✅ No v1/v2 parity concerns — Elements.res is a shared core file
• ✅ No submodule changes
• ✅ No shared-code/sdk-utils changes

---

Suggestions

None — the fix correctly uses URLModule to handle query parameter appending safely. Using searchParams.set() will overwrite any existing param with the same key, which is appropriate here (the SDK controls these specific params).

---

[XyneSpaces]

Review Summary

PR Type: Bugfix — Fix malformed redirect URLs when return_url has query params
Files Changed: 1 file (Elements.res)
Scope: Post-poll-status redirect URL construction
Risk Level: Low — Targeted fix using existing URLModule

---

Verification

✅ URLModule.makeUrl binding exists in codebase (src/Utilities/URLModule.res)
✅ searchParams.set is the correct API for appending query parameters
✅ Using url.href preserves all URL components correctly
✅ Fixes the reported issue (#1598) where ?payment_intent_client_secret=... was appended blindly

---

Findings

⚠️ Missing error handling for invalid/empty returnUrl

The code assumes returnUrl is always a valid URL:

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

If return_url is missing or malformed, `URLModule.makeUrl(

[XyneSpaces]

Detailed Review Comments for PR #1599

After reviewing the full diff against the tiered checklist for hyperswitch-web:

Tier 1 — Payment correctness & card-data safety:

• ✅ No card data exposure in logs/console
• ✅ No silent-null error handling changes
• ✅ No 3DS code modified
• ✅ No endpoint routing changes (existing redirect flow only)
• ✅ Uses existing URLModule for URL construction

Code Quality:

• ✅ Proper use of URLModule.makeUrl to construct URLs
• ✅ searchParams.set correctly appends parameters without breaking existing query strings
• ✅ url.href preserves all URL components

Issue Identified:

⚠️ Missing error handling for invalid/empty returnUrl

On line where makeUrl(returnUrl) is called:

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

If return_url is empty or malformed, `URLModule.makeUrl(

[XyneSpaces]

Detailed Review Summary for PR #1599

PR Type: Bugfix — Fix malformed redirect URLs when return_url has query params
Files Changed: 1 file (Elements.res)
Scope: Post-poll-status redirect URL construction
Risk Level: Low — Targeted fix using existing URLModule

---

Verification Against Checklist

Tier 1 — Payment correctness & card-data safety:

• ✅ No card data exposure in logs/console
• ✅ No silent-null error handling changes
• ✅ No 3DS code modified
• ✅ No endpoint routing changes (existing redirect flow only)
• ✅ Uses existing URLModule for URL construction

Tier 2 — Parity, routing, and release hygiene:

• ✅ No v1/v2 parity concerns (single file change)
• ✅ No submodule pointer moves
• ✅ No shared-code/sdk-utils changes
• ✅ No webpack/env matrix changes

---

Code Quality Assessment

✅ Proper use of URLModule.makeUrl — Correctly leverages existing URL bindings
✅ searchParams.set — Safely appends parameters without breaking existing query strings
✅ url.href — Preserves all URL components including protocol, host, path, and existing params
✅ Fixes reported issue — Resolves #1598 where blind string concatenation broke URLs with existing params

---

Findings

⚠️ Missing error handling for invalid/empty returnUrl

The new code assumes returnUrl is always a valid URL:

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

If return_url is empty string (the default) or malformed, URLModule.makeUrl will throw a JavaScript TypeError when the native URL constructor receives an invalid URL string.

Suggested improvement:

let returnUrl = dict->getString("return_url", "")
if returnUrl->String.length > 0 {
  try {
    let url = makeUrl(returnUrl)
    url.searchParams.set("payment_intent_client_secret", clientSecretRef.contents)
    url.searchParams.set("status", status)
    let redirectUrl = url.href
    // ... redirect logic
  } catch {
    // Fallback to original behavior or handle gracefully
    let redirectUrl = `${returnUrl}?payment_intent_client_secret=${clientSecretRef.contents}&status=${status}`
  }
}

Alternatively, consider using a utility like URLModule.makeUrlSafe that returns an option type, if available.

Note: The original code had the same vulnerability (blind string concatenation with potentially empty returnUrl), so this is not a new issue introduced by the fix. However, since you're touching this code, consider adding basic validation.

---

Verdict

💬 Comment — Clean, targeted fix. Addresses the core issue correctly. Minor suggestion to add error handling for edge cases where return_url may be empty or invalid.

---

team:hyperswitch-web

[XyneSpaces]

⚠️ 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]

Review Summary

Classification: URL-construction bugfix
Risk Level: Low-Medium
Scope: Fix for malformed redirect URLs when return_url contains query parameters

---

🔍 Findings

⚠️ Warning: Missing error handling for makeUrl

The makeUrl(returnUrl) call on line 815 assumes returnUrl is always a valid, non-empty URL. If returnUrl is malformed, empty, or contains unexpected characters, this will throw an unhandled exception in the promise chain, potentially leaving users without a redirect.

Consider wrapping in a try-catch with a fallback to the original string-concatenation behavior:

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}`
}

💡 Note on searchParams.set() behavior

Using set() will overwrite any existing payment_intent_client_secret or status parameters in the original URL. Confirm this is the intended behavior (likely yes), vs. wanting to preserve or merge with existing values.

---

✅ Positive Notes

• Clean, surgical fix addressing the exact bug described
• Proper abstraction via URLModule instead of manual string manipulation
• Conventional commit title and linked issue (#1598)

Verdict: Approve with minor suggestion to add defensive error handling.

[XyneSpaces]

⚠️ 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]

@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.