Added support for extraUrlParams in trigger blocks and URL vars. #3168
Conversation
| @@ -192,6 +170,36 @@ export class AmpAnalytics extends AMP.BaseElement { | |||
| return Promise.all(promises); | |||
| } | |||
|
|
|||
| processExtraUrlParams_(params, replaceMap) { | |||
| @@ -67,7 +67,7 @@ export function parseUrl(url) { | |||
| * @param {string} paramString | |||
| * @return {string} | |||
| */ | |||
| function appendParamStringToUrl(url, paramString) { | |||
| export function appendParamStringToUrl(url, paramString) { | |||
There was a problem hiding this comment.
Doing this is risky and can introduce XSS issues. For example if the paramString had a "><script>js code</script> it could execute arbitrary code on the page.
This PR does not introduce the risks though. I construct the paramString safely and then append it in the right place.
I see two ways forward:
- We make the change but add documentation to make note of this.
- I copy the function into amp-analytics. That way, the exposure is contained and someone can't inadvertently introduce XSS risk.
A third solution that I don't want to do is to deconstruct the paramString and then reconstruct it.
@cramforce Do you know anyone I can involve to check for XSS risks?
|
I don't understand why this can't just use the existing API. Why does encoding here need to happen separately from appending? |
|
#2423 asks for extraUrlParams to be inserted inside the url instead of at the end. The existing methods in url.js do not allow for this kind of behavior (it doesn't have any concept for variable substitution either). To support this, I create the paramString first and then either append it at the end or replace the variable inside the URL. The other option would be to look for the var in the URL and then call the function that puts the values in the right place. Will try doing that. |
|
I'd prefer if you keep this special logic in your private code. |
|
The special logic is in the extension code. The change to the platform is just about exporting a function. Let me see if I can avoid that as well. |
|
@avimehta @cramforce I'd say, ideally we'd avoid it altogether (i.e. deconstruct/reconstruct). But if we can't and have an absolute trust in the injected string, we can do this operation in the extension. It certainly shouldn't be part of the core API. I'd very much prefer the former. |
|
Updated to contain the changes just to extension code. |
|
ptal. |
| @@ -193,6 +170,44 @@ export class AmpAnalytics extends AMP.BaseElement { | |||
| } | |||
|
|
|||
| /** | |||
| * Replace the names of keys in params object with the values in replace map. | |||
| * | |||
| * @param {Object<string, string>} params The params that need to be renamed. | |||
|
Yeah, this looks good. Just a couple of comments. |
- params in trigger block override the ones at top level config. ampproject#2422 - If the extraUrlParams variable is used in request URL, it is replaces with extra params and the params are not appended to the url. ampproject#2423
|
LGTM |
with extra params and the params are not appended to the url. Make extraUrlParams a substitution variable #2423
/cc @rudygalfi