Fix a bug with url encoded parameters in url.parse #1480

erikdubbelboer · 2017-08-11T10:33:21Z

Type of change

Bugfix

Description of change

When you pass an url with url encoded parameters to Ajax.ajax but also pass extra parameters in the data argument, such as the atomx adapter does, it will break the url.

Ajax.ajax will parse the url and format it again:

Prebid.js/src/ajax.js

Lines 78 to 82 in 6956a56

    
           if (method === 'GET' && data) { 
        
             let urlInfo = parseURL(url); 
        
             Object.assign(urlInfo.search, data); 
        
             url = formatURL(urlInfo); 
        
           }

But url.parse decodes the whole url without url.format encoding it again:

Prebid.js/src/url.js

Line 29 in 6956a56

parsed.href = decodeURIComponent(url);

So Ajax.ajax('http://example.com?foo=foo%26foo%3Dxxx', null, {ignore: 123}) will load the url http://example.com?foo=foo&foo=xxx&ignore=123 instead of http://example.com?foo=foo%26foo%3Dxxx&ignore=123 which is just plain wrong.

This can be fixed by not just decoding the whole url but instead decoding and encoding the separate query parameters.

This pull request right now only removes the decodeURIComponent in url.parse as that doesn't seem to break any tests. Adding decodeURIComponent and encodeURIComponent to url.parseQS and url.formatQS seems to break some tests for adapters that already depend on this broken behavior.

erikdubbelboer · 2017-08-18T11:57:38Z

Any updates? Right now Prebid.js is impossible to use for us and I'm pretty sure other adapters suffer from this bug as well but just don't notice it because they might not always have url encoded values in the urls they use with Ajax.ajax.

mkendall07 · 2017-08-18T12:05:00Z

@erikdubbelboer why can't you just pass the query params into the data argument? Is it because of needing to use the same key more than once (so you need an array of key/values instead of a just plain object? )

erikdubbelboer · 2017-08-18T12:09:35Z

@mkalafior the base url is build by external javascript code that isn't included in Prebid.js. The only way we could fix this at the moment is to encodeURIComponent all urls we pass to Ajax.ajax which would be a super ugly fix that will break things in the future for sure.

Don't you think url.parse is really broken by always doing decodeURIComponent on the whole url?

mkendall07 · 2017-08-18T12:19:04Z

@erikdubbelboer yes I agree it's broken performing decodeURIComponent on the whole URL. The contract on all the url functions is not clear though, if we expect to decode/encode. So changing the behavior without any such clarification doesn't seem correct to me either. I think what we need is a param that says explicitly to perform encoding/decoding on each function tbh.

erikdubbelboer · 2017-08-18T15:54:55Z

@mkendall07 I can modify the pull request to add an options dictionary to url.parse to change this behavior. I'll make it enabled by default like it is now. This way everything keeps working the way it is now. The Ajax.ajax just needs to pass this option from its own options down to url.parse. Does that sound acceptable?

mkendall07 · 2017-08-18T15:57:39Z

Sure that sounds ok in the short term. We discussed this a bit earlier and might end up pulling in a lightweight ajax library to be more standardized. That might not be right away tho.

erikdubbelboer · 2017-08-18T16:01:49Z

I'll modify the pull request next week.

All other Ajax libraries will obviously not run decodeURIComponent on the whole URL meaning all adapters that depend on this behavior will break.

See: prebid#1480

erikdubbelboer · 2017-08-21T14:02:24Z

@mkendall07 I changed the pull request. I'm not sure if noDecodeWholeURL is a nice name for the option. If you have any other suggestions like me know and I'll change it.

erikdubbelboer · 2017-08-21T14:03:32Z

src/ajax.js

@@ -76,7 +76,7 @@ export function ajax(url, callback, data, options = {}) {
    }

    if (method === 'GET' && data) {
-      let urlInfo = parseURL(url);
+      let urlInfo = parseURL(url, options);


Not sure if passing the whole options object here is the best or if it should extract the options that apply to url.parse. This would result in a lot more code.

erikdubbelboer · 2017-08-21T14:04:26Z

test/spec/url_spec.js

@@ -28,8 +28,9 @@ describe('helpers.url', () => {
    it('extracts the search query', () => {
      expect(parsed).to.have.property('search');
      expect(parsed.search).to.eql({
-        foo: 'bar',
-        search: 'test'
+        foo: 'xxx',


I left this here to show that the current url.parse is quite broken with foo being set to xxx instead of the expected bar.

erikdubbelboer · 2017-09-07T10:49:44Z

@mkendall07 any update? We really need this as currently our adapter is broken because if this bug.

erikdubbelboer · 2017-09-09T22:52:59Z

@snapwich @ndhimehta @mkendall07 anyone?

jaiminpanchal27

LGTM

…built * 'master' of https://github.com/prebid/Prebid.js: (46 commits) Serverbid alias (prebid#1560) Add user sync to process for approving adapter PRs (prebid#1457) fix travis build (prebid#1595) Rubicon project improvement/user sync (prebid#1549) Adding Orbitsoft adapter (prebid#1378) Fix renderer test for new validation rule (prebid#1592) Allow SET_TARGETING to be used in AnalyticsAdapter (prebid#1577) Add support for video stream context (prebid#1483) Invalidate bid if matching bid request not found (prebid#1575) allow adapters to set default adserverTargeting for specific bid (prebid#1568) Custom granularity precision should honor 0 if it is passed in closes prebid#1479 (prebid#1591) BaseAdapter for the Prebid 0.x -> 1.x transition (prebid#1494) Add a version to the Criteo adapter (prebid#1573) Allow bundling from node.js or with new gulp task bundle-to-stdout (prebid#1570) Add url.parse option to not decode the whole URL (prebid#1480) Tremor Video Bid Adapter (prebid#1552) Yieldmo bid adapter (prebid#1415) Switch `gulp docs` to build its output using documentation.js (prebid#1545) Increment pre version Prebid 0.28.0 Release ...

See: prebid#1480

dbemiller assigned ndhimehta Aug 14, 2017

ndhimehta assigned snapwich Aug 14, 2017

erikdubbelboer force-pushed the urlparse branch from cec9abe to 5342c43 Compare August 21, 2017 13:57

erikdubbelboer added a commit to atomx/Prebid.js that referenced this pull request Aug 21, 2017

Add url.parse option to not decode the whole URL

5342c43

See: prebid#1480

Add url.parse option to not decode the whole URL

13a98a8

See: prebid#1480

erikdubbelboer force-pushed the urlparse branch from 5342c43 to 13a98a8 Compare August 21, 2017 14:02

erikdubbelboer commented Aug 21, 2017

View reviewed changes

mkendall07 approved these changes Sep 9, 2017

View reviewed changes

mkendall07 added the LGTM label Sep 9, 2017

mkendall07 assigned jaiminpanchal27 and unassigned snapwich and ndhimehta Sep 9, 2017

jaiminpanchal27 approved these changes Sep 11, 2017

View reviewed changes

jaiminpanchal27 merged commit d3e06a4 into prebid:master Sep 11, 2017

jbAdyoulike pushed a commit to jbAdyoulike/Prebid.js that referenced this pull request Sep 21, 2017

Add url.parse option to not decode the whole URL (prebid#1480)

4963d01

See: prebid#1480

dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018

Add url.parse option to not decode the whole URL (prebid#1480)

05aed4c

See: prebid#1480

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a bug with url encoded parameters in url.parse #1480

Fix a bug with url encoded parameters in url.parse #1480

erikdubbelboer commented Aug 11, 2017

erikdubbelboer commented Aug 18, 2017

mkendall07 commented Aug 18, 2017

erikdubbelboer commented Aug 18, 2017

mkendall07 commented Aug 18, 2017

erikdubbelboer commented Aug 18, 2017 •

edited

Loading

mkendall07 commented Aug 18, 2017

erikdubbelboer commented Aug 18, 2017

erikdubbelboer commented Aug 21, 2017

erikdubbelboer Aug 21, 2017

erikdubbelboer Aug 21, 2017

erikdubbelboer commented Sep 7, 2017

erikdubbelboer commented Sep 9, 2017

jaiminpanchal27 left a comment

	if (method === 'GET' && data) {
	let urlInfo = parseURL(url);
	Object.assign(urlInfo.search, data);
	url = formatURL(urlInfo);
	}

Fix a bug with url encoded parameters in url.parse #1480

Fix a bug with url encoded parameters in url.parse #1480

Conversation

erikdubbelboer commented Aug 11, 2017

Type of change

Description of change

erikdubbelboer commented Aug 18, 2017

mkendall07 commented Aug 18, 2017

erikdubbelboer commented Aug 18, 2017

mkendall07 commented Aug 18, 2017

erikdubbelboer commented Aug 18, 2017 • edited Loading

mkendall07 commented Aug 18, 2017

erikdubbelboer commented Aug 18, 2017

erikdubbelboer commented Aug 21, 2017

erikdubbelboer Aug 21, 2017

Choose a reason for hiding this comment

erikdubbelboer Aug 21, 2017

Choose a reason for hiding this comment

erikdubbelboer commented Sep 7, 2017

erikdubbelboer commented Sep 9, 2017

jaiminpanchal27 left a comment

Choose a reason for hiding this comment

erikdubbelboer commented Aug 18, 2017 •

edited

Loading