unnesessary encodeURI call in antie.URLBuilder

[solntcev]

https://github.com/fmtvp/tal/blob/master/static/script/urlbuilder.js

return encodeURI(url).replace(/'/g, "%27");

By default all media links is encoded by urlbuilder. This can cause issues with media sources url that include Base64 query parameters. (for example wowza session parameters).

Method getURL should not encode whole url, if it must, it should encode replaced params, but leave original url as is.

[tsadler1988]

Hi Ivan,

Have you got an example URL that will be incorrectly encoded by urlbuilder?

Thanks,

Tom

[tsadler1988]

Hi Ivan,

We will close this issue for now, but feel free to reopen this or raise a new issue when you have an example for us.

Thanks,

Tom

[solntcev]

Hello, sorry for late response.

Any url, containing symbol %, for example

http://example.com/media.m3u8?param=%3D

If this url passed to URLBuilder, output will ecape percent sign:

http://example.com/media.m3u8?param=%253D

[subsidel]

Suggested fix:

getURL: function(href, tags) {
  var url = this._urlTemplate.replace(/^%href%/, href);

  url = url.replace(/%[a-z]+%/g, function(match) {
    var v;
    if((v = tags[match]) !== undefined) {
        return encodeURI(v);
    }
    return match;
  });

  return url.replace(/'/g, "%27");
}

We would need to check whether this breaks any of our tests before pursuing this further.

[solntcev]

For encoding parameters in url it is better to use encodeURIComponent() instead of encodeURI(). It will also escape & symbols.

[johnbeech]

Ok, so:

getURL: function(href, tags) {
  var url = this._urlTemplate.replace(/^%href%/, href);

  url = url.replace(/%[a-z]+%/g, function(match) {
    var v;
    if((v = tags[match]) !== undefined) {
        return encodeURIComponent(v);
    }
    return match;
  });

  return url.replace(/'/g, "%27");
}

Next steps: create pull request with this change, and drive out some new tests based on the sample URLs provided:

Good: http://example.com/media.m3u8?param=%3D
Bad: http://example.com/media.m3u8?param=%253D

Not sure if/when we'll do this, but @solntcev feel free to open a pull request for this, we have no immediate requirement to fix this but would be happy to take it in as a change.

[kukulaka]

Closing issue due to immenent deprecation.