Update CORS, add AMP-Same-Origin when Origin is missing on same origin requests #4879
Conversation
|
|
||
| <h4>Subscribe to our weekly Newsletter (POST xhr same origin)</h4> | ||
| <form method="post" | ||
| action="/form/html/post" |
There was a problem hiding this comment.
FMI: Why both action and action-xhr?
There was a problem hiding this comment.
We always require action as a fallback in case amp-form extension failed to load for any reason the form would still be submittable.
There was a problem hiding this comment.
I see. Unfortunately we don't have a security story in that case, right? Maybe only allow fallback for same origin?
There was a problem hiding this comment.
We have document-submit that gets bundled part of amp runtime. It also handles the cases where amp-form might fail. For example, the POST + non-XHR case is prevented by document-submit. GET requests are ok to fallback as long as we clearly communicate they shouldn't be used for state changing requests.
There was a problem hiding this comment.
For POST this is very misleading. We should just remove action. We need to find a way to block un-upgraded submission in our submit handler so it doesn't post to itself (although this is probably already happening).
There was a problem hiding this comment.
For POST this is very misleading. We should just remove action
I am assuming this is just for the examples? sgtm
We need to find a way to block un-upgraded submission in our submit handler so it doesn't post to itself
document-submit does assert action is provided, https and not on cdn for GET. We do stop un-upgraded POST submissions in this PR.
|
Please document the submission rules in the amp-form docs. |
|
Done. PTAL 👀 |
| You can also provide an action-xhr attribute, if provided, the form will be submitted in an XHR fashion. | ||
|
|
||
| This attribute can be the same or a different endpoint than `action` and has the same action requirements above. | ||
|
|
||
| **Important**: Your XHR endpoints need to follow and implement [CORS Requests in AMP spec](https://github.com/ampproject/amphtml/blob/master/spec/amp-cors-requests.md). | ||
|
|
||
| **Important**: See [Protecting against XSRF attacks]( |
|
It's worth mentioning that target=_blank won't work with XHR. |
| Your XHR endpoints need to follow and implement [CORS Requests in AMP spec](https://github.com/ampproject/amphtml/blob/master/spec/amp-cors-requests.md). | ||
|
|
||
| ### Protecting against XSRF | ||
| In addition to following AMP CORS spec, please pay extra attention to [non-idempotent requests note]() |
There was a problem hiding this comment.
I think idempotence is the wrong concept here. Maybe just "mutating" or "state changing"
There was a problem hiding this comment.
True, "state changing" would be better and simpler.
|
Done. 👀 PTAL |
| message: '__amp_source_origin parameter is invalid.' | ||
| })); | ||
| return; | ||
| if (req.headers['amp-same-origin'] != 'true') { |
There was a problem hiding this comment.
This is a good time to move this test in a separate function. E.g. checkAmpCors or such.
There was a problem hiding this comment.
In light of the discussion below, we should reverse the checks and process Origin header first.
There was a problem hiding this comment.
Let's discuss this in the other thread, I think there's no change needed here.
|
PTAL 👀 |
|
LGTM |
mkhatib
force-pushed
the
update-cors-forms
branch
from
1c0a27d
to
68775e5
Compare
| const init = {method: 'post', body: {}}; | ||
| location.href = '/path'; | ||
| xhr.fetchJson('/whatever', init); | ||
| expect(init['headers']['AMP-Same-Origin']).to.equal('true'); |
There was a problem hiding this comment.
Let's add a test to ensure it uses actual origins and NOT source origins.
|
LGTM with one more test requested. Please add the test and merge. |
ITI: #3343