Skip to content

Add ordered params support and fix url encoding for Uplynk signature validation#109

Closed
rubenglobant wants to merge 2 commits intoTHEOplayer:mainfrom
rubenglobant:fix/uplynk-signature-encoding
Closed

Add ordered params support and fix url encoding for Uplynk signature validation#109
rubenglobant wants to merge 2 commits intoTHEOplayer:mainfrom
rubenglobant:fix/uplynk-signature-encoding

Conversation

@rubenglobant
Copy link

Description

This PR addresses critical issues with Uplynk signature validation caused by parameter handling.

Changes

  1. Ordered Parameters API: Added orderedPreplayParameters to UplynkSSAIConfiguration. This assumes the caller provides the correct order required by the signature, preventing Dictionary from shuffling them.
  2. URL Encoding Fix: Updated UplynkSSAIConfiguration+Extensions.swift to apply robust percent encoding to all values. This prevents double-decoding issues on the server (e.g., preserving npa%3D1 as npa%253D1) and handles commas in cid correctly.
  3. Ping Fix: pingParameters now returns an empty string when .noPing is configured, preventing injection of unsigned parameters.

Motivation

Without these fixes, valid signatures generated server-side or via KMM logic were failing validation due to reordering or double-decoding of special characters.

Copy link
Contributor

@MaartenRimaux MaartenRimaux left a comment

Choose a reason for hiding this comment

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

This change aligns pingParameters with the android implementation

The orderedPreplayParameters is indeed necessary to maintain the order of the keys and values.

This looks good to me ✅

@therama
Copy link
Contributor

therama commented Feb 5, 2026

@rubenglobant We maintain a changelog file at the root of the repository for public changes that are worth logging. Since adding orderedPreplayParameters to UplynkSSAIConfiguration is a public API change then we should have an entry in the changelog file notifying it. Please add an entry under the Unreleased tag. You don't need to worry about the date/versioning in the changelog file as that will be taken care of by the release process.

@rubenglobant
Copy link
Author

Added the changelog entry

@therama
Copy link
Contributor

therama commented Feb 5, 2026

@rubenglobant Since it's a repo containing multiple connectors, we specify the title of the connector before the change. In this case it should be Uplynk. Please check the format for other entries and apply the same title to your entry.

@therama
Copy link
Contributor

therama commented Feb 5, 2026

@rubenglobant Also make sure to add Added or Fixed tags. Again see other entries.

@ceyhun-o ceyhun-o self-assigned this Feb 6, 2026
@ceyhun-o ceyhun-o requested review from ceyhun-o and removed request for Danesz February 6, 2026 10:45
@ceyhun-o
Copy link
Contributor

ceyhun-o commented Feb 6, 2026

Hi @rubenglobant,

This PR as is is incomplete without proper changelog entries and documentation updates, and therefore does not meet our standards to merge. Since we also do not have access to make changes to your fork, in order to speed up the process I am closing this PR in favor of #110, where I have cherry-picked your changes.

Once again, thank you for your contribution.

@ceyhun-o ceyhun-o closed this Feb 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants