Update CORS, add AMP-Same-Origin when Origin is missing on same origin requests by mkhatib · Pull Request #4879 · ampproject/amphtml

mkhatib · 2016-09-09T01:01:55Z

cramforce · 2016-09-09T01:14:40Z

examples/forms.amp.html

+
+<h4>Subscribe to our weekly Newsletter (POST xhr same origin)</h4>
+<form method="post"
+      action="/form/html/post"


FMI: Why both action and action-xhr?

We always require action as a fallback in case amp-form extension failed to load for any reason the form would still be submittable.

I see. Unfortunately we don't have a security story in that case, right? Maybe only allow fallback for same origin?

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.

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).

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.

Examples and validator.

cramforce · 2016-09-09T01:17:41Z

Please document the submission rules in the amp-form docs.

mkhatib · 2016-09-09T01:43:39Z

Done. PTAL 👀

molnarg · 2016-09-09T10:42:07Z

extensions/amp-form/amp-form.md


-**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](


Broken link.

molnarg · 2016-09-09T10:44:23Z

It's worth mentioning that target=_blank won't work with XHR.

molnarg · 2016-09-09T10:53:44Z

extensions/amp-form/amp-form.md

+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]()


Nit: dot at the end of the sentence.

Also, missing link.

I think idempotence is the wrong concept here. Maybe just "mutating" or "state changing"

True, "state changing" would be better and simpler.

mkhatib · 2016-09-09T18:53:52Z

Done. 👀 PTAL

dvoytenko · 2016-09-09T23:08:33Z

build-system/server.js

-      message: '__amp_source_origin parameter is invalid.'
-    }));
-    return;
+  if (req.headers['amp-same-origin'] != 'true') {


This is a good time to move this test in a separate function. E.g. checkAmpCors or such.

In light of the discussion below, we should reverse the checks and process Origin header first.

Let's discuss this in the other thread, I think there's no change needed here.

mkhatib · 2016-09-11T01:56:51Z

PTAL 👀

molnarg · 2016-09-13T12:52:14Z

LGTM

…igin requests

dvoytenko · 2016-09-13T21:29:07Z

test/functional/test-xhr.js

+        const init = {method: 'post', body: {}};
+        location.href = '/path';
+        xhr.fetchJson('/whatever', init);
+        expect(init['headers']['AMP-Same-Origin']).to.equal('true');


Let's add a test to ensure it uses actual origins and NOT source origins.

dvoytenko · 2016-09-13T21:29:30Z

LGTM with one more test requested. Please add the test and merge.

…n requests (ampproject#4879)

mkhatib added the NEEDS REVIEW label Sep 9, 2016

mkhatib assigned cramforce and dvoytenko Sep 9, 2016

cramforce reviewed Sep 9, 2016
View reviewed changes

molnarg reviewed Sep 9, 2016
View reviewed changes

dvoytenko reviewed Sep 9, 2016
View reviewed changes

Mohammad Khatib added 6 commits September 13, 2016 13:22

Update CORS and Add AMP-Same-Origin when Origin is missing on same or…

62272b0

…igin requests

fix lint and address comments

7a59a3e

address comments, send AMP-Same-Origin on all same origin requests.

25562ee

fix test

919c950

address comments

04f7ebc

fix lint

68775e5

mkhatib force-pushed the update-cors-forms branch from 1c0a27d to 68775e5 Compare September 13, 2016 20:26

fix type

bd69c73

dvoytenko reviewed Sep 13, 2016
View reviewed changes

dvoytenko added LGTM and removed NEEDS REVIEW labels Sep 13, 2016

add origin/source origin test case

3b43eef

mkhatib merged commit 96fd6cb into ampproject:master Sep 13, 2016

mkhatib deleted the update-cors-forms branch September 13, 2016 21:59

dreamofabear pushed a commit to dreamofabear/amphtml that referenced this pull request Sep 16, 2016

Update CORS, add AMP-Same-Origin when Origin is missing on same origi…

b40ac0d

…n requests (ampproject#4879)

mityaha pushed a commit to brightcove-archive/ooyala_amphtml that referenced this pull request Nov 30, 2016

Update CORS, add AMP-Same-Origin when Origin is missing on same origi…

ab371da

…n requests (ampproject#4879)

ampprojectbot mentioned this pull request Jan 10, 2022

🚨 Error: i.toggleAttribute is not a function ampproject/error-reporting#120

Closed

This was referenced Jan 18, 2022

🚨 Error: Cannot read properties of null (reading 'style') ampproject/error-reporting#131

Open

🚨 Error: i.toggleAttribute is not a function ampproject/error-reporting#135

Closed


		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](

Conversation

mkhatib commented Sep 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cramforce commented Sep 9, 2016

Uh oh!

mkhatib commented Sep 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

molnarg commented Sep 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkhatib commented Sep 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkhatib commented Sep 11, 2016

Uh oh!

molnarg commented Sep 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvoytenko commented Sep 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants