Amphtml: amp-mustache-0.2: Warn on sanitized attributes

[meistudioli]

To Whom it may concern:

I found amp-mustache-0.2 will trimed input attribute name once the value is "action". You may check the following code.

<form id="myForm" class="infinites" method="post" target="_top"
    action-xhr="web-service.php"
    on="submit-success:AMP.setState({
        likes: event.response.likes,
        ssStatus: {
            whatelse: event.response.isLast,
            rSlide: event.response.rSlide
        }
    })">
    <a class="infinites__a" on="tap:myForm.submit">check more</a>
    <div submit-success template="template-listing"></div>
</form>

<template id="template-listing" type="amp-mustache">
    <form action-xhr="web-service2.php" method="post" target="_top">
        <input type="hidden" name="action" value="like" />
        <input type="hidden" name="merchandiseId" value="{{id}}" />
        <input type="hidden" name="_crumb" value="AUTHDATA(crumb)" data-amp-replace="AUTHDATA" />
    </form>
</template>

Expect input tag render result:

<input type="hidden" name="action" value="like" />

But it will be

<input type="hidden"  value="like" />

I wonder is "action" reserved word in amp-mustache-0.2 ? Once I switch to amp-mustache-0.1, everything will be fine as usual.

Paul

[kevinkassimo]

(Not in charge of the component but played with it anyways)
It seems that this was somehow sanitized by DOMPurify (0.1 used Caja).
Tested on the demo page for DOMPurify itself, https://cure53.de/purify

[kevinkassimo]

Brief investigation found that DOMPurify purifies more than simply "action", but also the following (name value be form attribute names):

<input name="action">
<input name="method">
<input name="name">
<input name="target">
<input name="enctype">

and something like this (from document)

<input name="querySelectorAll">

This is caused by the following lines in DOMPurify (inside _isValidAttribute()):

https://github.com/cure53/DOMPurify/blob/875d955b681a0166047ca88c1d02736ebcc14eba/src/purify.js#L597-L603

Not sure if this is intended behavior. I would suggest for now avoid these values.
/cc @choumx

[choumx]

Thanks for the report. Kevin is right that this is a DOMPurify feature to prevent DOM clobbering attack.

We should print out a meaningful error when such attributes are stripped and consider documenting this caveat.

[meistudioli]

Hi, @choumx:

Thanks for response. Looks like there will be some api interface need to modify. orz

Will this check affect other HTML structure (which not use mustache to render) ?

Paul

[choumx]

No, it should only affect markup that lives inside <template>.

[meistudioli]

All right. But I still need to make them same for maintenance.

[choumx]

Sorry about that Paul and thanks for your continued efforts and bearing with us.

[meistudioli]

Don't mention it. AMP is really a good tech.

[cure53]

Hey all, just FYI, there is also a setting for that, in case you are certain that no clobbering can happen:

// disable DOM Clobbering protection on output (default is true, handle with care!)
var clean = DOMPurify.sanitize(dirty, {SANITIZE_DOM: false});

[jakewantulok]

I also ran into a similar issue where the name attribute is stripped from the <input> if given a value of "id" - which is stripped because 'id' is an API member of the <form> element.

Example:

<template id="checkout" type="amp-mustache">
    <form action="/cart/add" method="get" target="_top">
        <input type="hidden" name="id"  value="{{variants.0.id}}">
        <button type="submit" class="add-to-cart">
            <span class="btn__text">Add to Cart</span>
        </button>
    </form>
</template>

This attribute is then sanitized from the form by AMP.

<template id="checkout" type="amp-mustache">
    <form action="/cart/add" method="get" target="_top">
        <input type="hidden" value="{{variants.0.id}}">
        <button type="submit" class="add-to-cart">
            <span class="btn__text">Add to Cart</span>
        </button>
    </form>
</template>

Sample URL that demonstrates the issue: https://android.cmphys.com/temp/amp-strips-name-attr.html

It would be much appreciated there was a custom warning about the stripped attribute.

[cure53]

Heya, adding a warning makes sense. We expose a property that might be useful for that, DOMPurify.removed.

This collection right now contains info about what was removed and where it was removed from.

Would it make sense to also add info about what part of DOMPurify removed it? Or why it was removed? Or is it already sufficient like that?

[choumx]

Yep, we actually did that in #20285 but it was rolled back since it can generate a huge amount of console messages in some usages and crash devtools. The plan is to roll forward with a throttling mechanism.

[cure53]

Oh, I can imagine :D Let me know if we can help somehow.

[mattwomple]

As a first step, how about a console warning when viewing an AMP page with the #development fragment?