Skip to content
This repository has been archived by the owner on Aug 1, 2024. It is now read-only.

closure: Fix uri query parameter encoding #434

Closed
wants to merge 1 commit into from

Conversation

irock
Copy link

@irock irock commented Mar 29, 2015

This fixes handling of URIs with special characters. Without this patch, segment URIs such as e.g. http://example.com/0.webm?param=%26 may be translated to http://example.com/0.webm?param=&.

Fixed in shaka-player: shaka-project/shaka-player#40

@joeltine
Copy link
Contributor

I'm pretty sure this is working as intended. If someone tries to resolve a relative URI that contains encoded query data, this will create a new absolute URI with the encoded params exactly as they are. Then, if someone tries to call a method like getEncodedQuery later on the absolute URI, it will double encode the query data.

@joeltine joeltine closed this Jul 25, 2015
@irock
Copy link
Author

irock commented Oct 11, 2015

@joeltine Sorry for the late response.

If someone tries to resolve a relative URI that contains encoded query data, this will create a new absolute URI with the encoded params exactly as they are. - This is what this patch set is supposed to fix. Currently, the resolve() function willl decode the query data, which seem like a strange behavior to me.

I would have expected that both of the expressions below would do the same thing:

goog.Uri.parse('http://www.google.com/search')
  .resolve(goog.Uri.parse('?q=%26')).toString());
goog.Uri.parse('http://www.google.com/search?q=%26').toString();

This is not the case in the current version of the library.

@joeltine
Copy link
Contributor

Thanks for the code snippet, I see the issue now. Unfortunately, I can't reopen the PR. It looks like the branch changed.

My concern is that this change could be hard to land internally. People may have been coding based on the current behavior for resolve.

I'm going to run some tests on the change to get a sense for the breakages.

@joeltine
Copy link
Contributor

Moved the issue to #585

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants