Using extraUrlParams breaks expansion for requests

[cory-work]

The the extraUrlParams are added to a request the request gets parsed as a URL. When this happens any variables in the host part of the request are url encoded which breaks the variable expansion. Below is an example of an amp-analytics tag that breaks in chrome.

<amp-analytics>
<script type="application/json">
{
  "requests": {
    "pageview": "https://${host}"
  },
  "vars": {
    "host": "metrics.example.com"                                                                                                                                                                                                                                                                                         
  },
  "extraUrlParams": {
    "a": "b"
  },
  "triggers": {
    "pageload": {
      "on": "visible",
      "request": "pageview"
    }
  }
}
</script>
</amp-analytics>

[avimehta]

looking into it...

[rudygalfi]

The request sent is https://%24%7Bhost%7D/?a=b

[rudygalfi]

Thanks @avimehta!

[avimehta]

@cory-work Found the cause for the bug and will be sending out a fix soon. For now, Do you mind moving the host to requests section? So something like this:

{
  "requests": {
    "host": "metrics.example.com",
    "pageview": "https://${host}"
  },
  "extraUrlParams": {
    "a": "b"
  },
  "triggers": {
    "pageload": {
      "on": "visible",
      "request": "pageview"
    }
  }
}

[rudygalfi]

@avimehta Just took a closer look... It looks like the definition of host in vars results in some escaping whereas it wouldn't within requests. Wouldn't that generally be a good approach to handling things? I'm curious to hear more on what you determined is the bug.

[avimehta]

The issue occurs in this line. When the extraUrlParams are present, the request is parsed and additional parameters are added back to the request. During the parsing of the request, browser encodes ${} and that is what you see in the bug report.

I am not sure what the right fix here is.

• One way would be to ask people to keep all components of host and path inside requests section. So for example, in following example, the host and path have to be inside the requests section and the params inside the 'vars' section.

{
  "requests": {
    "pageview": "https://${host}/${path}?${params}"
  },
}

• Another solution would be to let all three to be in any section but you loose the ability to use templates in extraUrlParams. So someone won't be able to do something like:

{
  "extraUrlParams": {
    "param": "AMP_DOC_URL"
  }
}

• The third solution(which has a complex solution and I am looking into simplifying) would be to allow everything everywhere and make our code complex. I am looking into this solution right now and trying to find a simple fix.

[rudygalfi]

I think the third solution seems like the best path.

I'd dismiss (1) because, unlike what I thought earlier, this only breaks with the addition of extraUrlParams, so if the vars are correctly added when not in the presence of extraUrlParams, I think they should also be correct added when extraUrlParams is present.

I'd dismiss (2) because it seems useful to allow extraUrlParams to be defined with substitutions.

It seems like this just looks like an order of operations thing and hence why path (3) is best, but let me know if there's another way to look at the tradeoffs.

[Tonsil]

I came to this page with what I believe to be the same problem. Just posting it here because I'd like to make sure this case is addressed in the fix:

    {
        "requests": {
            "pageview": "https://myamp.com/${account}/${thing}"
        },
        "triggers": {
            "track pageview": {
                "on": "visible",
                "request": "pageview"
            }
        },
        "vars": {
            "account": 1234,
            "thing": "test"
        },
        "extraUrlParams": {
            "herName": "rio",
            "dances": "sand"
        }
    }

[dvoytenko]

@avimehta does #2543 fully resolve this bug?

[avimehta]

Yes, I believe it does. https://github.com/ampproject/amphtml/pull/2543/files#diff-a4664cb4c01931444a010c4d5ea1df63R330 tests the case where vars are used inside path. Checking locally again.

[avimehta]

confirmed. the example from #2245 (comment) results in following request to be sent:

https://myamp.com/1234/test?herName=rio&dances=sand

[Tonsil]

Thank you!