Add __amp_source_origin to amp-form non-xhr action and update example. #4546
Conversation
| // Update the form non-xhr action to add `__amp_source_origin` parameter. | ||
| // This allows publishers to understand where the request is coming from. | ||
| const action = this.form_.getAttribute('action'); | ||
| user().assert(action, 'form action is required: %s', |
There was a problem hiding this comment.
Should this be in the validator instead?
There was a problem hiding this comment.
It's in the validator, but I think we also like to do validation here for an early error indicator.
|
PTAL |
mkhatib
force-pushed
the
amp-form-cors-server-example
branch
from
f175980 to
f1ed185
Compare
mkhatib
force-pushed
the
amp-form-cors-server-example
branch
from
f1ed185 to
d6c1ba8
Compare
|
PTAL |
| const action = form.getAttribute('action'); | ||
| user().assert(action, 'form action attribute is required: %s', form); | ||
| assertHttpsUrl(action, form, 'action'); | ||
| user().assert(!startsWith(action, urls.cdn), | ||
| 'form action should not be on AMP CDN: %s', form); | ||
| // Update the form non-xhr action to add `__amp_source_origin` parameter. | ||
| // This allows publishers to understand where the request is coming from. | ||
| if (action.indexOf('__amp_source_origin=') == -1) { |
There was a problem hiding this comment.
We need to overwrite the existing __amp_source_origin, or fail, to avoid someone trying to pretend to have request from another origin.
There was a problem hiding this comment.
I guess override then cause otherwise it'll error with every time it'll try to subsequently submit.
|
LGTM. Except for the issue with overriding the param - that's important. |
|
@dvoytenko mind taking one last look 👀 with the last change? |
| @@ -79,7 +79,28 @@ app.use('/api/echo/post', function(req, res) { | |||
| res.end(JSON.stringify(req.body, null, 2)); | |||
| }); | |||
|
|
|||
| const ORIGIN_REGEX = new RegExp('^http://localhost:8000|' + | |||
There was a problem hiding this comment.
Please add a comment saying that in practice, this would be *.ampproject.org and the publisher's origin.
|
@dvoytenko re-wrote the override logic as discussed. Let me know if this looks good. 👀 |
|
LGTM |
Follow up to #4485 and part of security review for
amp-form.ITI: #3343