Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update CORS, add AMP-Same-Origin when Origin is missing on same origin requests #4879

Merged
merged 8 commits into from
Sep 13, 2016
Merged

Update CORS, add AMP-Same-Origin when Origin is missing on same origin requests #4879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
62272b0
Update CORS and Add AMP-Same-Origin when Origin is missing on same or…
Sep 9, 2016
7a59a3e
fix lint and address comments
Sep 9, 2016
25562ee
address comments, send AMP-Same-Origin on all same origin requests.
Sep 9, 2016
919c950
fix test
Sep 9, 2016
04f7ebc
address comments
Sep 10, 2016
68775e5
fix lint
Sep 13, 2016
bd69c73
fix type
Sep 13, 2016
3b43eef
add origin/source origin test case
Sep 13, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 64 additions & 34 deletions build-system/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,21 +98,7 @@ const SOURCE_ORIGIN_REGEX = new RegExp('^http://localhost:8000|' +
'^https?://.+\.herokuapp\.com:8000/');

app.use('/form/html/post', function(req, res) {
if (!ORIGIN_REGEX.test(req.headers.origin)) {
res.statusCode = 500;
res.end(JSON.stringify({
message: 'Origin header is invalid.'
}));
return;
}

if (!SOURCE_ORIGIN_REGEX.test(req.query.__amp_source_origin)) {
res.statusCode = 500;
res.end(JSON.stringify({
message: '__amp_source_origin parameter is invalid.'
}));
return;
}
assertCors(req, res, ['POST']);

var form = new formidable.IncomingForm();
form.parse(req, function(err, fields) {
Expand All @@ -132,39 +118,83 @@ app.use('/form/html/post', function(req, res) {
});
});

app.use('/form/echo-json/post', function(req, res) {
if (!ORIGIN_REGEX.test(req.headers.origin)) {
res.statusCode = 500;
res.end(JSON.stringify({
message: 'Origin header is invalid.'
}));
return;
function assertCors(req, res, opt_validMethods) {
const validMethods = opt_validMethods || ['GET', 'POST', 'OPTIONS'];
const invalidMethod = req.method + ' method is not allowed. Use POST.';
const invalidOrigin = 'Origin header is invalid.';
const invalidSourceOrigin = '__amp_source_origin parameter is invalid.';
const unauthorized = 'Unauthorized Request';
var origin;

if (validMethods.indexOf(req.method) == -1) {
res.statusCode = 405;
res.end(JSON.stringify({message: invalidMethod}));
throw invalidMethod;
}

if (!SOURCE_ORIGIN_REGEX.test(req.query.__amp_source_origin)) {
res.statusCode = 500;
res.end(JSON.stringify({
message: '__amp_source_origin parameter is invalid.'
}));
return;
if (req.headers.origin) {
origin = req.headers.origin;
if (!ORIGIN_REGEX.test(req.headers.origin)) {
res.statusCode = 500;
res.end(JSON.stringify({message: invalidOrigin}));
throw invalidOrigin;
}

if (!SOURCE_ORIGIN_REGEX.test(req.query.__amp_source_origin)) {
res.statusCode = 500;
res.end(JSON.stringify({message: invalidSourceOrigin}));
throw invalidSourceOrigin;
}
} else if (req.headers['amp-same-origin'] == 'true') {
origin = getUrlPrefix(req);
} else {
res.statusCode = 401;
res.end(JSON.stringify({message: unauthorized}));
throw unauthorized;
}

res.setHeader('Access-Control-Allow-Credentials', 'true');
res.setHeader('Access-Control-Allow-Origin', origin);
res.setHeader('Access-Control-Expose-Headers',
'AMP-Access-Control-Allow-Source-Origin')
res.setHeader('AMP-Access-Control-Allow-Source-Origin',
req.query.__amp_source_origin);
}

app.use('/form/echo-json/post', function(req, res) {
assertCors(req, res, ['POST']);
var form = new formidable.IncomingForm();
form.parse(req, function(err, fields) {
res.setHeader('Content-Type', 'application/json');
if (fields['email'] == 'already@subscribed.com') {
res.statusCode = 500;
}
res.setHeader('Access-Control-Allow-Origin',
req.headers.origin);
res.setHeader('Access-Control-Expose-Headers',
'AMP-Access-Control-Allow-Source-Origin')
res.setHeader('AMP-Access-Control-Allow-Source-Origin',
req.query.__amp_source_origin);
res.end(JSON.stringify(fields));
});
});


app.use('/form/search-html/get', function(req, res) {
res.setHeader('Content-Type', 'text/html');
res.end(`
<h1>Here's results for your search<h1>
<ul>
<li>Result 1</li>
<li>Result 2</li>
<li>Result 3</li>
</ul>
`);
});


app.use('/form/search-json/get', function(req, res) {
assertCors(req, res, ['GET']);
res.json({
results: [{title: 'Result 1'}, {title: 'Result 2'}, {title: 'Result 3'}]
});
});


app.use('/share-tracking/get-outgoing-fragment', function(req, res) {
res.setHeader('AMP-Access-Control-Allow-Source-Origin',
req.protocol + '://' + req.headers.host);
Expand Down
101 changes: 88 additions & 13 deletions examples/forms.amp.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,71 @@
</head>
<body>

<h2>Subscribe to our weekly Newsletter (non-xhr)</h2>
<h4>Search (GET non-xhr same origin)</h4>
<form method="get" action="/form/search-html/get" target="_blank">
<fieldset>
<label>
<span>Search for</span>
<input type="search" name="term" required>
</label>
<input type="submit" value="Search">
</fieldset>
</form>

<h4>Search (GET non-xhr cross origin - current host > httpbin.org)</h4>
<form method="get" action="https://httpbin.org/html" target="_blank">
<fieldset>
<label>
<span>Search for</span>
<input type="search" name="term" required>
</label>
<input type="submit" value="Search">
</fieldset>
</form>

<h4>Search (GET xhr same origin)</h4>
<form method="get" action="/form/search-html/get" action-xhr="/form/search-json/get" target="_blank">
<fieldset>
<label>
<span>Search for</span>
<input type="search" name="term" required>
</label>
<input type="submit" value="Search">
</fieldset>

<div submit-success>
<template type="amp-mustache">
<h1>Here are the results for the search:</h1>
<ul>
{{#results}}<li>{{title}}</li>{{/results}}
</ul>
</template>
</div>
</form>

<h4>Search (GET xhr cross origin current host > httpbin.org)</h4>
<form method="get"
action="https://httpbin.org/response-headers?AMP-Access-Control-Allow-Source-Origin=http%3A%2F%2Flocalhost%3A8000&access-control-expose-headers=AMP-Access-Control-Allow-Source-Origin"
action-xhr="https://httpbin.org/response-headers?AMP-Access-Control-Allow-Source-Origin=http%3A%2F%2Flocalhost%3A8000&access-control-expose-headers=AMP-Access-Control-Allow-Source-Origin"
target="_blank">
<fieldset>
<label>
<span>Search for</span>
<input type="search" name="term" required>
</label>
<input type="submit" value="Search">
</fieldset>

<div submit-success>
<template type="amp-mustache">
<h1>You searched for: {{term}}</h1>
</template>
</div>
</form>

<h4>Subscribe to our weekly Newsletter (POST xhr same origin)</h4>
<form method="post"
action="https://example.com/form/html/post"
action-xhr="/form/echo-json/post"
target="_blank">
<fieldset>
<label>
Expand All @@ -88,19 +149,23 @@ <h2>Subscribe to our weekly Newsletter (non-xhr)</h2>
<input type="submit" value="Subscribe">
</fieldset>

<div class="form-message invalid-message">
There are some input that needs fixing. Please fix them.
<div submit-success>
<template type="amp-mustache">
Success! Thanks {{name}} for subscribing! Please make sure to check your email {{email}}
to confirm!
</template>
</div>
<div class="form-message valid-message">
Everything looks good, you can submit now!
<div submit-error>
<template type="amp-mustache">
Oops! {{name}}, We apologies something went wrong. Please try again later.
</template>
</div>
</form>

<h2>Subscribe to our weekly Newsletter (xhr)</h2>

<h4>Subscribe to our weekly Newsletter (xhr POST cross origin - current host > amp.localhost:8000)</h4>
<form method="post"
action="https://example.com/form/html/post"
action-xhr="https://example.com/form/echo-json/post"
action-xhr="http://amp.localhost:8000/form/echo-json/post"
target="_blank">
<fieldset>
<label>
Expand All @@ -117,7 +182,7 @@ <h2>Subscribe to our weekly Newsletter (xhr)</h2>
<div submit-success>
<template type="amp-mustache">
Success! Thanks {{name}} for subscribing! Please make sure to check your email {{email}}
to confirm!
to confirm!
</template>
</div>
<div submit-error>
Expand All @@ -127,11 +192,10 @@ <h2>Subscribe to our weekly Newsletter (xhr)</h2>
</div>
</form>

<h2>Subscribe to our weekly Newsletter (xhr + submit events)</h2>
<h4>Subscribe to our weekly Newsletter (xhr + submit events)</h4>

<form method="post"
action="https://example.com/form/html/post"
action-xhr="https://example.com/form/echo-json/post"
action-xhr="/form/echo-json/post"
target="_blank"
on="submit-success:success-lightbox;submit-error:error-lightbox">
<fieldset>
Expand Down Expand Up @@ -189,5 +253,16 @@ <h1>Oops! Kittens!</h1>
</div>
</amp-lightbox>

<h4>NOT ALLOWED (POST non-xhr - submission prevented)</h4>
<form method="post" action="/regardless" target="_blank">
<fieldset>
<label>
<span>Subscribe</span>
<input type="email" name="email" required>
</label>
<input type="submit" value="Subscribe">
</fieldset>
</form>

</body>
</html>
22 changes: 20 additions & 2 deletions extensions/amp-form/amp-form.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,16 @@ __required__

Action must be provided, `https` and is non-cdn link (does **NOT** link to https://cdn.ampproject.org).

__Note__: `target` and `action` will only be used for non-xhr GET requests. AMP runtime will use `action-xhr` to make the request and will ignore `action` and `target`. When `action-xhr` is not provided AMP would make a GET request to `action` endpoint and use `target` to open a new window (if `_blank`). AMP runtime might also fallback to using action and target in cases where `amp-form` extension fails to load.

**action-xhr**
__(optional)__
__(optional)__ for `GET` __required__ for `POST` requests
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 [Security Considerations](#security-considerations) for notes on how to secure your forms endpoints.

All other [form attributes](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/form) are optional.

Expand Down Expand Up @@ -181,3 +184,18 @@ One of the main differences between `:invalid` and `:user-invalid` is when are t
`.user-valid` and `.user-invalid` classes are a polyfill for the pseudo classes as described above. Publishers can use these to style their inputs and fieldsets to be responsive to user actions (e.g. highlighting an invalid input with a red border after user blurs from it).

See the [full example here](https://github.com/ampproject/amphtml/blob/master/examples/forms.amp.html) on using these.


## Security Considerations
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 [state changing requests note](https://github.com/ampproject/amphtml/blob/master/spec/amp-cors-requests.md#note-on-state-changing-requests).

In general, keep in mind the following points when accepting input from the user:

* Only use POST for state changing requests.
* Use non-XHR GET for navigational purposes only, e.g. Search.
* non-XHR GET requests are not going to receive accurate origin/headers and backends won't be able to protect against XSRF with the above mechanism.
* In general use XHR/non-XHR GET requests for navigational or information retrieval only.
* non-XHR POST requests are not allowed in AMP documents. This is due to inconsistencies of setting `Origin` header on these requests across browsers. And the complications supporting it would introduce in protecting against XSRF. This might be reconsidered and introduced later, please file an issue if you think this is needed.
11 changes: 8 additions & 3 deletions spec/amp-cors-requests.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,23 +37,28 @@ by default.

## CORS Security in AMP

This security protocol consists of two components: CORS origin and source origin restrictions.
This security protocol consists of three components: `AMP-Same-Origin`, CORS origin and source origin restrictions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @andreban @sebastianbenz for changes proposed to AMP CORS, to support in sample publisher as well.


CORS endpoints receive requesting origin via "Origin" HTTP header. This header has to be restricted to only allow the following origins:
- *.ampproject.org
- The Publisher’s own origins

If `Origin` header is missing, AMP will set `AMP-Same-Origin: true` custom header. If this header is set it indicates the request is coming from same origin. And should, as rule, be allowed.

Source origin restrictions has to be implemented by requiring "__amp_source_origin" URL parameter to be within a set of the Publisher's own origins. The "__amp_source_origin" parameter is passed from AMP Runtime in all fetch requests and contains the source origin, e.g. "https://publisher1.com".

The resulting HTTP response has to also contain the following headers:
- `Access-Control-Allow-Origin: <origin>`. Here "origin" refers to the requesting origin that was allowed via "Origin" request header above. Ex: "https://cdn.ampproject.org". This is a CORS spec requirement. Notice that while CORS spec allows the value of '\*' to be returned in this header, AMP strongly discourages use of '\*'. Instead the value of the "Origin" header should be validated and echoed for improved security.
- `AMP-Access-Control-Allow-Source-Origin: <source-origin>`. Here "source-origin" indicates the source origin that is allowed to read the authorization response as was verified via "__amp_source_origin" URL parameter. Ex: "https://publisher1.com".
- `Access-Control-Expose-Headers: AMP-Access-Control-Allow-Source-Origin`. This header simply allows CORS response to contain the "AMP-Access-Control-Allow-Source-Origin" header.

#### Note on non-idempotent Requests
#### Note on State Changing Requests
When making CORS requests that would change the state of your system (e.g. user subscribes to or unsubscribes from a mailing list) the first two steps you need to make sure to do:

1. Check the `Origin` header. If the origin was not `*.ampproject.org` or the publisher's origin, stop and return an error response.
1. Check the `Origin` header, if it doesn't exist move to step 3. If the origin was not `*.ampproject.org` or the publisher's origin, stop and return an error response.
2. Check the `__amp_source_origin` query parameter. If it's not the publisher's origin stop and return an error response.
3. Check if the request has `AMP-Same-Origin: true` header. If yes, proceed to process the request safely (skip next steps).
* This custom request header is sent by AMP runtime when making an XHR request on sameorigin (document served from non-cache URL).
4. Otherwise reject the request and return an error response.

It's very important that these are done first before processing the request, this provides protection against CSRF attacks and avoids processing untrusted sources requests.
39 changes: 23 additions & 16 deletions src/document-submit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
*/

import {startsWith} from './string';
import {dev, user} from './log';
import {assertHttpsUrl, getCorsUrl, SOURCE_ORIGIN_PARAM} from './url';
import {user} from './log';
import {assertHttpsUrl, checkCorsUrl, SOURCE_ORIGIN_PARAM} from './url';
import {urls} from './config';


Expand Down Expand Up @@ -59,21 +59,28 @@ export function onDocumentFormSubmit_(e) {
'Illegal input name, %s found: %s', SOURCE_ORIGIN_PARAM, inputs[i]);
}

const win = form.ownerDocument.defaultView;
let action = form.getAttribute('action');
if (!form.__AMP_INIT_ACTION__) {
form.__AMP_INIT_ACTION__ = action;
} else {
action = form.__AMP_INIT_ACTION__;
const action = form.getAttribute('action');
const actionXhr = form.getAttribute('action-xhr');
const method = (form.getAttribute('method') || 'GET').toUpperCase();
if (method == 'GET') {
user().assert(action,
'form action attribute is required for method=GET: %s', form);
assertHttpsUrl(action, /** @type {!Element} */ (form), 'action');
user().assert(!startsWith(action, urls.cdn),
'form action should not be on AMP CDN: %s', form);
checkCorsUrl(action);
} else if (action) {
e.preventDefault();
user().assert(false,
'form action attribute is invalid for method=POST: %s', form);
} else if (!actionXhr) {
e.preventDefault();
user().assert(false,
Copy link
Contributor

@dvoytenko dvoytenko Sep 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be done in the document-submit-handler.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, got my eyes crossed. It's already here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

'Only XHR based (via action-xhr attribute) submissions are support ' +
'for POST requests. %s',
form);
}
user().assert(action, 'form action attribute is required: %s', form);
assertHttpsUrl(action, dev().assertElement(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.
form.setAttribute('action', getCorsUrl(win, action));
checkCorsUrl(actionXhr);

const target = form.getAttribute('target');
user().assert(target, 'form target attribute is required: %s', form);
Expand Down
Loading