Skip to content
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.

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

Turning on CMCD Query Args corrupts existing query args containing certain URLEncoded values. #4577

Open
5 tasks done
wilaw opened this issue Sep 27, 2024 · 2 comments
Open
5 tasks done
Assignees
Labels
Milestone

Comments

@wilaw
Copy link
Member

wilaw commented Sep 27, 2024

Environment
  • Link to playable MPD file:
  • Dash.js version: latest
  • Browser name/version:
  • OS name/version:
Steps to reproduce

I can see a bug where, when CMCD is being used with an existing URL-encoded query arg that includes %20 (spaces), these are changed to '+'.  This can be illustrated with the following manifest playback

https://reference.dashif.org/dash.js/latest/samples/dash-if-reference-player/index.html?mpd=https%3A%2F%2Fdash.akamaized.net%2Fakamai%2Fbbb_30fps%2Fbbb_30fps.mpd%3Fa%3Dsomehting%20with%20spaces+&debug.logLevel=4&streaming.delay.liveDelayFragmentCount=NaN&streaming.delay.liveDelay=NaN&streaming.buffer.initialBufferLevel=NaN&streaming.liveCatchup.maxDrift=NaN&streaming.liveCatchup.playbackRate.min=NaN&streaming.liveCatchup.playbackRate.max=NaN

which shows in the javascript panel the correct request to the server of

https://dash.akamaized.net/akamai/bbb_30fps/bbb_30fps.mpd?a=somehting%20with%20spaces

However, if we turn on CMCD with query-arg mode and try the same manifest

https://reference.dashif.org/dash.js/latest/samples/dash-if-reference-player/index.html?mpd=https%3A%2F%2Fdash.akamaized.net%2Fakamai%2Fbbb_30fps%2Fbbb_30fps.mpd%3Fa%3Dsomehting%20with%20spaces+&debug.logLevel=4&streaming.delay.liveDelayFragmentCount=NaN&streaming.delay.liveDelay=NaN&streaming.buffer.initialBufferLevel=NaN&streaming.liveCatchup.maxDrift=NaN&streaming.liveCatchup.playbackRate.min=NaN&streaming.liveCatchup.playbackRate.max=NaN&streaming.cmcd.enabled=true

then we see that the request to the server looks like this:

https://dash.akamaized.net/akamai/bbb_30fps/bbb_30fps.mpd?a=somehting+with+spaces&CMCD=ot%3Dm%2Csid%3D%22d336fd7a-178d-4dfe-b180-809fa58af3c6%22

where the original query arg has been modified.

We can also see with a change in capitalization. If we try to play the URL

https://dash.akamaized.net/akamai/bbb_30fps/bbb_30fps.mpd?a=%3d

without CMCD-query args active, then the player makes the correct request to the server. If however we turn on CMCD query args, then it makes this request to the server

https://dash.akamaized.net/akamai/bbb_30fps/bbb_30fps.mpd?a=%3D&CMCD=ot%3Dm%2Csid%3D%22b7b542be-8f66-4834-aedf-4fcd4883a449%22

in which the %3d had been converted to %3D. This breaks any server which is hashing the URL in order to decide if an access token is valid.

@wilaw wilaw added the Bug label Sep 27, 2024
@wilaw wilaw changed the title Turning on CMCD Query Args corrupts existing query args that contain %20 URLEncoded values. Turning on CMCD Query Args corrupts existing query args certain URLEncoded values. Sep 27, 2024
@wilaw wilaw changed the title Turning on CMCD Query Args corrupts existing query args certain URLEncoded values. Turning on CMCD Query Args corrupts existing query args containing certain URLEncoded values. Sep 27, 2024
@dsilhavy dsilhavy added this to the 5.0.0 milestone Sep 27, 2024
@dsilhavy
Copy link
Collaborator

Just had a quick look, and it is related to the function addAditionalQueryParameterToUrl in Utils.js:

    static addAditionalQueryParameterToUrl(url, params) {
        try {
            if (!params || params.length === 0) {
                return url;
            }

            let modifiedUrl = new URL(url);

            params.forEach((param) => {
                if (param.key && param.value) {
                    modifiedUrl.searchParams.set(param.key, param.value);
                }
            });

            return modifiedUrl.href;


        } catch (e) {
            return url;
        }
    }

The native URLSearchParams implementation automatically replaces whitespaces with + signs as soon as we add a new query parameters using modifiedUrl.searchParams.set(param.key, param.value).

In addition, I found that (https://http.dev/percent-encoding) :

Percent-encoding uses three characters to represent a reserved character. The first is the percent sign %, and this is followed by the two-character hexadecimal value of the reserved ASCII character. The two-character hexadecimal value is not case sensitive.

The conversion from %3d to %3D also happens automatically once we call modifiedUrl.searchParams.set(param.key, param.value).

To support the behavior described in this issue we probably need to replace the logic that sets the query parameters. It is to be highlighted though that URLSearchParams (https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams) and URL (https://developer.mozilla.org/en-US/docs/Web/API/URL/URL) are native browser implementation and other clients might use it as well.

@dsilhavy
Copy link
Collaborator

I also checked in Shaka player:

@dsilhavy dsilhavy self-assigned this Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In Progress
Development

No branches or pull requests

2 participants