thatguystone / flowplayer-ima

A simple Interactive Media Ads plugin for FP
GNU General Public License v3.0
24 stars 8 forks source link

encoding of special chars #11

Closed CBCNewMedia closed 10 years ago

CBCNewMedia commented 10 years ago

This is such a fundamental issue that I'm surprised it hasn't been discussed here already.

My VAST URL looks like this (I've anonymized the URL a bit):

http://pubads.g.doubleclick.net/gampad/ads?sz=576x324;iu=/NETWORKID/ADUNIT;ciu_szs=300x250;impl=s;gdfp_req=1;env=vp;output=xml_vast2;unviewed_position_start=1;url=URLOFVIDEOPAGE;correlator=1388428539;cust_params=city%3DChicago%26county%3DCook%26

flowplayer.js mangles this when it passes it into the flash player. It escapes the URL with this line:

return url.replace(/&/g, '%26').replace(/&/g, '%26').replace(/=/g, '%3D');

With no code in the flowplayer-ima to reverse this encoding, you get an unusuable URL that will result in a 400 status in the response from DFP:

http://pubads.g.doubleclick.net/gampad/ads?sz%3D576x324;iu%3D/NETWORKID/ADUNIT;ciu_szs%3D300x250;impl%3Ds;gdfp_req%3D1;env%3Dvp;output%3Dxml_vast2;unviewed_position_start%3D1;url%3DURLOFVIDEOPAGE;correlator%3D1388428539;cust_params%3Dcity%3DChicago%26county%3DCook%26

I'm not sure how your example code could even work. The URL you provide

http://ad.doubleclick.net/pfadx/N270.132652.1516607168321/B3442378.3;dcadv=1379578;sz=0x0;ord=79879;dcmt=text/xml

Should be received by flowplayer-ima as:

http://ad.doubleclick.net/pfadx/N270.132652.1516607168321/B3442378.3;dcadv%3D1379578;sz%3D0x0;ord%3D79879;dcmt%3Dtext/xml

And I don't see any code in flowplayer-ima to decode the encoded characters.

If you do add code to flowplayer-ima to reverse the encoding, you hit a trickier problem when the URL looks like mine. The cust_params string is supposed to contain a set of key/value pairs, with "=" and "&" url-encoded.

But the code that reverses the flowplayer.js encoding will also decode all the encoded values in the custom_params string:

http://pubads.g.doubleclick.net/gampad/ads?sz=576x324;iu=/NETWORKID/ADUNIT;ciu_szs=300x250;impl=s;gdfp_req=1;env=vp;output=xml_vast2;unviewed_position_start=1;url=URLOFVIDEOPAGE;correlator=1388428539;cust_params=city=Chicago&county=Cook&

So instead of DFP getting a series of values in the custom_params, it gets cust_params=city (and then probably ignores the rest of the URL).

adammada commented 10 years ago

You anonimised it too much (no URLs at all).

CBCNewMedia commented 10 years ago

I think that's beside the point. Obviously, I have a real network ID, ad unit, and page URL in my test pages.

There are two points:

The fix that I implemented was this:

  1. changed queryescape() in flowplayer.js to do this instead:

return url.replace (/%/g, '%25').replace(/&/g, '%26').replace(/&/g, '%26').replace(/=/g, '%3D');

(first escape all "%" characters)

  1. in load() in InteractiveMediaAdsProvider.as:

clip.url = clip.url.replace(/%3D/g, '=').replace(/%26/g, '&').replace (/%25/g, '%');

Still not perfect. I'm not sure why the flowplayer authors didn't just call encodeURI() on the entire URL in flowplayer.js, and then decodeURI() inside flowplayer. But I decided not to change the flowplayer core to make my fix, and I think that my change won't break anything that wasn't already broken.

thatguystone commented 10 years ago

That's rather strange as we rely on key-value pairs in the URLs for targeting, and we haven't seen any issues with it. While you're right that it would be an issue if the URL didn't get unescaped properly, that would be FP's responsibility, not the the plugins, as the plugin only reads out the clip's URL, which it assumes is correct. I'll take a look at this a bit more on Monday, but in the meantime: what version of FP are you using, and are you using (just want to make sure it's not a new bug that FP introduced)?

thatguystone commented 10 years ago

Just tested this out: I'm seeing URLs coming in exactly as expected. Can you check your FP version and report back?

thatguystone commented 10 years ago

Closing this since I haven't heard back.