From 62272b0be696e3e06fe34b9aa8eb406ebc6605a0 Mon Sep 17 00:00:00 2001 From: Mohammad Khatib Date: Thu, 8 Sep 2016 17:59:35 -0700 Subject: [PATCH 1/8] Update CORS and Add AMP-Same-Origin when Origin is missing on same origin requests --- build-system/server.js | 103 +++++++++++++++++++----- examples/forms.amp.html | 99 ++++++++++++++++++++--- spec/amp-cors-requests.md | 9 ++- src/document-submit.js | 23 +++--- src/service/xhr-impl.js | 33 +++++++- test/functional/test-document-submit.js | 26 +++--- test/functional/test-xhr.js | 74 ++++++++++++++--- 7 files changed, 296 insertions(+), 71 deletions(-) diff --git a/build-system/server.js b/build-system/server.js index 354e49a7cf0a..a03326fa36fa 100644 --- a/build-system/server.js +++ b/build-system/server.js @@ -98,20 +98,30 @@ 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; + if (req.method != 'POST') { + res.statusCode = 405; res.end(JSON.stringify({ - message: 'Origin header is invalid.' + message: req.method + ' method is not allowed. Use POST.' })); 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; + if (req.headers['amp-same-origin'] != 'true') { + 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; + } } var form = new formidable.IncomingForm(); @@ -132,21 +142,31 @@ 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; + if (req.method != 'POST') { + res.statusCode = 405; res.end(JSON.stringify({ - message: 'Origin header is invalid.' + message: req.method + ' method is not allowed. Use POST.' })); return; } + if (req.headers['amp-same-origin'] != 'true') { + 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; + 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; + } } var form = new formidable.IncomingForm(); @@ -155,6 +175,7 @@ app.use('/form/echo-json/post', function(req, res) { if (fields['email'] == 'already@subscribed.com') { res.statusCode = 500; } + res.setHeader('Access-Control-Allow-Credentials', 'true'); res.setHeader('Access-Control-Allow-Origin', req.headers.origin); res.setHeader('Access-Control-Expose-Headers', @@ -165,6 +186,52 @@ app.use('/form/echo-json/post', function(req, res) { }); }); + +app.use('/form/search-html/get', function(req, res) { + res.setHeader('Content-Type', 'text/html'); + res.end(` +

Here's results for your search

+ + `); +}); + + +app.use('/form/search-json/get', function(req, res) { + if (req.headers['amp-same-origin'] != 'true') { + 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; + } + res.setHeader('Access-Control-Allow-Credentials', 'true'); + res.setHeader('Access-Control-Allow-Origin', + req.headers.origin); + } + res.setHeader('Content-Type', 'application/json'); + 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.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); diff --git a/examples/forms.amp.html b/examples/forms.amp.html index 7dde7a3bbae8..c48c1239a364 100644 --- a/examples/forms.amp.html +++ b/examples/forms.amp.html @@ -71,10 +71,71 @@ -

Subscribe to our weekly Newsletter (non-xhr)

+

Search (GET non-xhr same origin)

+
+
+ + +
+
+ +

Search (GET non-xhr cross origin - current host > httpbin.org)

+
+
+ + +
+
+ +

Search (GET xhr same origin)

+
+
+ + +
+ +
+ +
+
+ +

Search (GET xhr cross origin current host > httpbin.org)

+
+
+ + +
+ +
+ +
+
+

Subscribe to our weekly Newsletter (POST xhr same origin)

-
- There are some input that needs fixing. Please fix them. +
+
-
- Everything looks good, you can submit now! +
+
-

Subscribe to our weekly Newsletter (xhr)

+

Subscribe to our weekly Newsletter (xhr POST cross origin - current host > amp.localhost:8000)

+

NOT ALLOWED (POST non-xhr - submission prevented)

+ +
+ + +
+ + diff --git a/spec/amp-cors-requests.md b/spec/amp-cors-requests.md index eca601e69d5a..11563fb7ff49 100644 --- a/spec/amp-cors-requests.md +++ b/spec/amp-cors-requests.md @@ -53,7 +53,12 @@ The resulting HTTP response has to also contain the following headers: #### Note on non-idempotent 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. -2. Check the `__amp_source_origin` query parameter. If it's not the publisher's origin stop and return an error response. +1. 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). +2. Else, check the `Origin` header. If the origin was not `*.ampproject.org` or the publisher's origin, stop and return an error response. +3. Check the `__amp_source_origin` query parameter. If it's not the publisher's origin stop and return an error response. + +**Important Note**: Only use POST for non-idempotent requests. +**Important Note**: 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. Use non-XHR GET for navigational purposes only, e.g. Search. 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. diff --git a/src/document-submit.js b/src/document-submit.js index a79954da18db..66fe0eb7edd8 100644 --- a/src/document-submit.js +++ b/src/document-submit.js @@ -16,7 +16,7 @@ import {startsWith} from './string'; import {dev, user} from './log'; -import {assertHttpsUrl, getCorsUrl, SOURCE_ORIGIN_PARAM} from './url'; +import {assertHttpsUrl, checkCorsUrl, SOURCE_ORIGIN_PARAM} from './url'; import {urls} from './config'; @@ -59,21 +59,20 @@ 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(); 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)); + if (!actionXhr && method != 'GET') { + e.preventDefault(); + user().assert(false, + 'action-xhr attribute is required for non-GET form submissions. %s', + form); + } + checkCorsUrl(action); const target = form.getAttribute('target'); user().assert(target, 'form target attribute is required: %s', form); diff --git a/src/service/xhr-impl.js b/src/service/xhr-impl.js index a7fd189d5286..bb37f3aa0e57 100644 --- a/src/service/xhr-impl.js +++ b/src/service/xhr-impl.js @@ -21,6 +21,7 @@ import { getCorsUrl, } from '../url'; import {isArray, isObject, isFormData} from '../types'; +import {platformFor} from '../platform'; /** @@ -112,8 +113,16 @@ export class Xhr { * @private */ fetchAmpCors_(input, opt_init) { + const init = opt_init || {}; input = this.getCorsUrl(this.win, input); - return this.fetch_(input, opt_init).then(response => { + // For some same origin requests, add AMP-Same-Origin: true header to allow + // publishers to validate that this request came from their own origin + // when Origin header is not going to be set by the browser. + if (!this.willSetOriginHeader_(input, init)) { + init['headers'] = init['headers'] || {}; + init['headers']['AMP-Same-Origin'] = 'true'; + } + return this.fetch_(input, init).then(response => { const allowSourceOriginHeader = response.headers.get( ALLOW_SOURCE_ORIGIN_HEADER); if (allowSourceOriginHeader) { @@ -124,7 +133,7 @@ export class Xhr { `Returned ${ALLOW_SOURCE_ORIGIN_HEADER} is not` + ` equal to the current: ${allowSourceOriginHeader}` + ` vs ${sourceOrigin}`); - } else if (opt_init && opt_init.requireAmpResponseSourceOrigin) { + } else if (init.requireAmpResponseSourceOrigin) { // If the `AMP-Access-Control-Allow-Source-Origin` header is not // returned but required, return error. user().assert(false, `Response must contain the` + @@ -246,6 +255,26 @@ export class Xhr { return getCorsUrl(win, url); } + /** + * Checks if the current browsers will set the origin header for the request. + * @param {string} url + * @param {?Object} opt_init + * @return {boolean} + * @private + */ + willSetOriginHeader_(url, opt_init) { + const init = opt_init || {}; + const platform = platformFor(this.win); + const sourceOrigin = getSourceOrigin(this.win.location.href); + const targetOrigin = getSourceOrigin(url); + const isSameOrigin = sourceOrigin == targetOrigin; + const isGet = (init['method'] || 'GET').toUpperCase() == 'GET'; + const isIEOrEdge = platform.isIe() || platform.isEdge(); + // All same origin GET requests will not set Origin. + // Same Origin GET and POST requests on IE or Edge will not set Origin. + return !((isSameOrigin && isGet) || (isSameOrigin && isIEOrEdge)); + } + } diff --git a/test/functional/test-document-submit.js b/test/functional/test-document-submit.js index 8f218b89e7ca..de064c591a49 100644 --- a/test/functional/test-document-submit.js +++ b/test/functional/test-document-submit.js @@ -85,27 +85,23 @@ describe('test-document-submit onDocumentFormSubmit_', () => { /Illegal input name, __amp_source_origin found/); }); - it('should add __amp_source_origin to action before submit', () => { + it('should assert __amp_source_origin is not set in action', () => { evt.target.setAttribute('action', 'https://example.com/?__amp_source_origin=12'); expect(() => onDocumentFormSubmit_(evt)) .to.throw(/Source origin is not allowed in/); + }); - evt.target.__AMP_INIT_ACTION__ = null; - evt.target.setAttribute('action', 'https://example.com/'); - onDocumentFormSubmit_(evt); - expect(evt.target.getAttribute('action')).to.contain( - '__amp_source_origin'); - expect(() => onDocumentFormSubmit_(evt)).to.not.throw( - /Source origin is not allowed in/); + it('should fail when POST and action-xhr is not set', () => { + evt.target.setAttribute('method', 'post'); + expect(() => onDocumentFormSubmit_(evt)) + .to.throw(/action-xhr attribute is required for non-GET/); + expect(preventDefaultSpy.callCount).to.equal(1); - // Should not throw on any subsequent submits and use initial - // value of action. - evt.target.setAttribute('action', - 'https://www.example.com/?a=1&__amp_source_origin=example1.com&b=2'); - onDocumentFormSubmit_(evt); - expect(evt.target.getAttribute('action')).to.contain( - '__amp_source_origin=http%3A%2F%2Flocalhost%3A9876'); + evt.target.setAttribute('method', 'post'); + evt.target.setAttribute('action-xhr', 'https://example.com'); + expect(() => onDocumentFormSubmit_(evt)).to.not.throw(); + expect(preventDefaultSpy.callCount).to.equal(2); }); it('should do nothing if already prevented', () => { diff --git a/test/functional/test-xhr.js b/test/functional/test-xhr.js index a7f224e02e0b..5b8993c64c94 100644 --- a/test/functional/test-xhr.js +++ b/test/functional/test-xhr.js @@ -23,28 +23,41 @@ import { assertSuccess, } from '../../src/service/xhr-impl'; import {getCookie} from '../../src/cookies'; +import {platformFor} from '../../src/platform'; describe('XHR', function() { let sandbox; let requests; + let platform; + let isIe; + let isEdge; + let location = {href: 'https://acme.com/path'}; + let nativeWin = { + navigator: { + userAgent: window.navigator.userAgent, + }, + fetch: window.fetch, + location: location, + }; + + let polyfillWin = { + navigator: { + userAgent: window.navigator.userAgent, + }, + fetch: fetchPolyfill, + location: location, + }; // Given XHR calls give tests more time. this.timeout(5000); const scenarios = [ { - xhr: installXhrService({ - fetch: window.fetch, - location: {href: 'https://acme.com/path'}, - }), + xhr: installXhrService(nativeWin), desc: 'Native', - }, - { - xhr: installXhrService({ - fetch: fetchPolyfill, - location: {href: 'https://acme.com/path'}, - }), + }, { + xhr: installXhrService(polyfillWin), desc: 'Polyfill', }, ]; @@ -68,6 +81,15 @@ describe('XHR', function() { beforeEach(() => { sandbox = sinon.sandbox.create(); + location.href = 'https://acme.com/path'; + nativePlatform = platformFor(nativeWin); + polyfillPlatform = platformFor(polyfillWin); + isIe = false; + isEdge = false; + sandbox.stub(nativePlatform, 'isIe', () => isIe); + sandbox.stub(nativePlatform, 'isEdge', () => isEdge); + sandbox.stub(polyfillPlatform, 'isIe', () => isIe); + sandbox.stub(polyfillPlatform, 'isEdge', () => isEdge); }); afterEach(() => { @@ -225,6 +247,38 @@ describe('XHR', function() { }); } + describe('AMP-Same-Origin', () => { + it('should not be set for cross origin requests', () => { + const init = {}; + xhr.fetchJson('/whatever', init); + expect(init['headers']['AMP-Same-Origin']).to.be.undefined; + }); + + it('should not set for same origin POST on non-IE/Edge', () => { + const init = {method: 'POST', body: {}}; + isEdge = false; + isIe = false; + location.href = '/path'; + xhr.fetchJson('/whatever', init); + expect(init['headers']['AMP-Same-Origin']).to.be.undefined; + }); + + it('should be set for all same origin GET requests', () => { + const init = {}; + location.href = '/path'; + xhr.fetchJson('/whatever', init); + expect(init['headers']['AMP-Same-Origin']).to.equal('true'); + }); + + it('should be set for same origin POST requests on IE/Edge', () => { + const init = {method: 'post', body: {}}; + location.href = '/path'; + isIe = true; + xhr.fetchJson('/whatever', init); + expect(init['headers']['AMP-Same-Origin']).to.equal('true'); + }); + }); + describe(test.desc, () => { describe('assertSuccess', () => { From 7a59a3ef5b66656c240ede837849871d9f663aaa Mon Sep 17 00:00:00 2001 From: Mohammad Khatib Date: Thu, 8 Sep 2016 18:38:21 -0700 Subject: [PATCH 2/8] fix lint and address comments --- extensions/amp-form/amp-form.md | 20 ++++++++++++++++++-- spec/amp-cors-requests.md | 3 --- src/document-submit.js | 3 ++- test/functional/test-xhr.js | 13 +++++++------ 4 files changed, 27 insertions(+), 12 deletions(-) diff --git a/extensions/amp-form/amp-form.md b/extensions/amp-form/amp-form.md index 33d8da08a186..5363241bd0f2 100644 --- a/extensions/amp-form/amp-form.md +++ b/extensions/amp-form/amp-form.md @@ -76,12 +76,13 @@ __required__ Action must be provided, `https` and is non-cdn link (does **NOT** link to https://cdn.ampproject.org). **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 [Protecting against XSRF attacks]( All other [form attributes](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/form) are optional. @@ -181,3 +182,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 [non-idempotent requests note]() + +In general, keep in mind the following points when accepting input from the user: + +* Only use POST for non-idempotent (will change state in your backend) 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. diff --git a/spec/amp-cors-requests.md b/spec/amp-cors-requests.md index 11563fb7ff49..c962d5cc3107 100644 --- a/spec/amp-cors-requests.md +++ b/spec/amp-cors-requests.md @@ -58,7 +58,4 @@ When making CORS requests that would change the state of your system (e.g. user 2. Else, check the `Origin` header. If the origin was not `*.ampproject.org` or the publisher's origin, stop and return an error response. 3. Check the `__amp_source_origin` query parameter. If it's not the publisher's origin stop and return an error response. -**Important Note**: Only use POST for non-idempotent requests. -**Important Note**: 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. Use non-XHR GET for navigational purposes only, e.g. Search. - 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. diff --git a/src/document-submit.js b/src/document-submit.js index 66fe0eb7edd8..6d576e3085ff 100644 --- a/src/document-submit.js +++ b/src/document-submit.js @@ -69,7 +69,8 @@ export function onDocumentFormSubmit_(e) { if (!actionXhr && method != 'GET') { e.preventDefault(); user().assert(false, - 'action-xhr attribute is required for non-GET form submissions. %s', + 'Only XHR based (via action-xhr attribute) submissions are support ' + + 'for POST requests. %s', form); } checkCorsUrl(action); diff --git a/test/functional/test-xhr.js b/test/functional/test-xhr.js index 5b8993c64c94..19cb328dbbf6 100644 --- a/test/functional/test-xhr.js +++ b/test/functional/test-xhr.js @@ -29,24 +29,25 @@ import {platformFor} from '../../src/platform'; describe('XHR', function() { let sandbox; let requests; - let platform; + let nativePlatform; + let polyfillPlatform; let isIe; let isEdge; - let location = {href: 'https://acme.com/path'}; - let nativeWin = { + const location = {href: 'https://acme.com/path'}; + const nativeWin = { + location, navigator: { userAgent: window.navigator.userAgent, }, fetch: window.fetch, - location: location, }; - let polyfillWin = { + const polyfillWin = { + location, navigator: { userAgent: window.navigator.userAgent, }, fetch: fetchPolyfill, - location: location, }; // Given XHR calls give tests more time. From 25562ee78d043d08e999f0c0da748dc8a2c099a0 Mon Sep 17 00:00:00 2001 From: Mohammad Khatib Date: Fri, 9 Sep 2016 11:49:41 -0700 Subject: [PATCH 3/8] address comments, send AMP-Same-Origin on all same origin requests. --- extensions/amp-form/amp-form.md | 8 +++++--- spec/amp-cors-requests.md | 8 ++++++-- src/service/xhr-impl.js | 29 ++++------------------------- test/functional/test-xhr.js | 30 +----------------------------- 4 files changed, 16 insertions(+), 59 deletions(-) diff --git a/extensions/amp-form/amp-form.md b/extensions/amp-form/amp-form.md index 5363241bd0f2..2461d9be529c 100644 --- a/extensions/amp-form/amp-form.md +++ b/extensions/amp-form/amp-form.md @@ -75,6 +75,8 @@ __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)__ 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. @@ -82,7 +84,7 @@ You can also provide an action-xhr attribute, if provided, the form will be subm This attribute can be the same or a different endpoint than `action` and has the same action requirements above. -**Important**: See [Protecting against XSRF attacks]( +**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. @@ -188,11 +190,11 @@ See the [full example here](https://github.com/ampproject/amphtml/blob/master/ex 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]() +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 non-idempotent (will change state in your backend) requests. +* 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. diff --git a/spec/amp-cors-requests.md b/spec/amp-cors-requests.md index c962d5cc3107..8def7822751c 100644 --- a/spec/amp-cors-requests.md +++ b/spec/amp-cors-requests.md @@ -37,7 +37,9 @@ 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. + +On same origin requests, AMP will set `AMP-Same-Origin: true` custom header. If this header is set it indicates the request is coming from same origin. CORS endpoints receive requesting origin via "Origin" HTTP header. This header has to be restricted to only allow the following origins: - *.ampproject.org @@ -50,7 +52,9 @@ The resulting HTTP response has to also contain the following headers: - `AMP-Access-Control-Allow-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 that AMP CORS never requires preflighted requests so it's safe to assume that requests with the `OPTIONS` method are **not** coming from a legitimate AMP page and an error response should be returned. + +#### 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 if the request has `AMP-Same-Origin: true` header. If yes, proceed to process the request safely (skip next steps). diff --git a/src/service/xhr-impl.js b/src/service/xhr-impl.js index bb37f3aa0e57..8b4d14c46cd2 100644 --- a/src/service/xhr-impl.js +++ b/src/service/xhr-impl.js @@ -21,7 +21,6 @@ import { getCorsUrl, } from '../url'; import {isArray, isObject, isFormData} from '../types'; -import {platformFor} from '../platform'; /** @@ -116,9 +115,10 @@ export class Xhr { const init = opt_init || {}; input = this.getCorsUrl(this.win, input); // For some same origin requests, add AMP-Same-Origin: true header to allow - // publishers to validate that this request came from their own origin - // when Origin header is not going to be set by the browser. - if (!this.willSetOriginHeader_(input, init)) { + // publishers to validate that this request came from their own origin. + const sourceOrigin = getSourceOrigin(this.win.location.href); + const targetOrigin = getSourceOrigin(input); + if (sourceOrigin == targetOrigin) { init['headers'] = init['headers'] || {}; init['headers']['AMP-Same-Origin'] = 'true'; } @@ -126,7 +126,6 @@ export class Xhr { const allowSourceOriginHeader = response.headers.get( ALLOW_SOURCE_ORIGIN_HEADER); if (allowSourceOriginHeader) { - const sourceOrigin = getSourceOrigin(this.win.location.href); // If the `AMP-Access-Control-Allow-Source-Origin` header is returned, // ensure that it's equal to the current source origin. user().assert(allowSourceOriginHeader == sourceOrigin, @@ -255,26 +254,6 @@ export class Xhr { return getCorsUrl(win, url); } - /** - * Checks if the current browsers will set the origin header for the request. - * @param {string} url - * @param {?Object} opt_init - * @return {boolean} - * @private - */ - willSetOriginHeader_(url, opt_init) { - const init = opt_init || {}; - const platform = platformFor(this.win); - const sourceOrigin = getSourceOrigin(this.win.location.href); - const targetOrigin = getSourceOrigin(url); - const isSameOrigin = sourceOrigin == targetOrigin; - const isGet = (init['method'] || 'GET').toUpperCase() == 'GET'; - const isIEOrEdge = platform.isIe() || platform.isEdge(); - // All same origin GET requests will not set Origin. - // Same Origin GET and POST requests on IE or Edge will not set Origin. - return !((isSameOrigin && isGet) || (isSameOrigin && isIEOrEdge)); - } - } diff --git a/test/functional/test-xhr.js b/test/functional/test-xhr.js index 19cb328dbbf6..a731d83219dd 100644 --- a/test/functional/test-xhr.js +++ b/test/functional/test-xhr.js @@ -29,24 +29,14 @@ import {platformFor} from '../../src/platform'; describe('XHR', function() { let sandbox; let requests; - let nativePlatform; - let polyfillPlatform; - let isIe; - let isEdge; const location = {href: 'https://acme.com/path'}; const nativeWin = { location, - navigator: { - userAgent: window.navigator.userAgent, - }, fetch: window.fetch, }; const polyfillWin = { location, - navigator: { - userAgent: window.navigator.userAgent, - }, fetch: fetchPolyfill, }; @@ -83,14 +73,6 @@ describe('XHR', function() { beforeEach(() => { sandbox = sinon.sandbox.create(); location.href = 'https://acme.com/path'; - nativePlatform = platformFor(nativeWin); - polyfillPlatform = platformFor(polyfillWin); - isIe = false; - isEdge = false; - sandbox.stub(nativePlatform, 'isIe', () => isIe); - sandbox.stub(nativePlatform, 'isEdge', () => isEdge); - sandbox.stub(polyfillPlatform, 'isIe', () => isIe); - sandbox.stub(polyfillPlatform, 'isEdge', () => isEdge); }); afterEach(() => { @@ -255,15 +237,6 @@ describe('XHR', function() { expect(init['headers']['AMP-Same-Origin']).to.be.undefined; }); - it('should not set for same origin POST on non-IE/Edge', () => { - const init = {method: 'POST', body: {}}; - isEdge = false; - isIe = false; - location.href = '/path'; - xhr.fetchJson('/whatever', init); - expect(init['headers']['AMP-Same-Origin']).to.be.undefined; - }); - it('should be set for all same origin GET requests', () => { const init = {}; location.href = '/path'; @@ -271,10 +244,9 @@ describe('XHR', function() { expect(init['headers']['AMP-Same-Origin']).to.equal('true'); }); - it('should be set for same origin POST requests on IE/Edge', () => { + it('should be set for all same origin POST requests', () => { const init = {method: 'post', body: {}}; location.href = '/path'; - isIe = true; xhr.fetchJson('/whatever', init); expect(init['headers']['AMP-Same-Origin']).to.equal('true'); }); From 919c950b97f095c539c772f04e5cff318adb9811 Mon Sep 17 00:00:00 2001 From: Mohammad Khatib Date: Fri, 9 Sep 2016 11:52:48 -0700 Subject: [PATCH 4/8] fix test --- test/functional/test-document-submit.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/functional/test-document-submit.js b/test/functional/test-document-submit.js index de064c591a49..a4f70ad56eec 100644 --- a/test/functional/test-document-submit.js +++ b/test/functional/test-document-submit.js @@ -95,7 +95,7 @@ describe('test-document-submit onDocumentFormSubmit_', () => { it('should fail when POST and action-xhr is not set', () => { evt.target.setAttribute('method', 'post'); expect(() => onDocumentFormSubmit_(evt)) - .to.throw(/action-xhr attribute is required for non-GET/); + .to.throw(/Only XHR based \(via action-xhr attribute\) submissions/); expect(preventDefaultSpy.callCount).to.equal(1); evt.target.setAttribute('method', 'post'); From 04f7ebc7450551cc71767fe9a4bbd7f59e01e7ae Mon Sep 17 00:00:00 2001 From: Mohammad Khatib Date: Sat, 10 Sep 2016 16:06:49 -0700 Subject: [PATCH 5/8] address comments --- build-system/server.js | 107 ++++++++---------------- examples/forms.amp.html | 2 +- spec/amp-cors-requests.md | 13 ++- src/document-submit.js | 19 +++-- src/service/xhr-impl.js | 8 +- test/functional/test-document-submit.js | 8 ++ 6 files changed, 68 insertions(+), 89 deletions(-) diff --git a/build-system/server.js b/build-system/server.js index a03326fa36fa..cccf026b60ef 100644 --- a/build-system/server.js +++ b/build-system/server.js @@ -98,31 +98,7 @@ const SOURCE_ORIGIN_REGEX = new RegExp('^http://localhost:8000|' + '^https?://.+\.herokuapp\.com:8000/'); app.use('/form/html/post', function(req, res) { - if (req.method != 'POST') { - res.statusCode = 405; - res.end(JSON.stringify({ - message: req.method + ' method is not allowed. Use POST.' - })); - return; - } - - if (req.headers['amp-same-origin'] != 'true') { - 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) { @@ -142,46 +118,57 @@ app.use('/form/html/post', function(req, res) { }); }); +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; -app.use('/form/echo-json/post', function(req, res) { - if (req.method != 'POST') { + if (validMethods.indexOf(req.method) == -1) { res.statusCode = 405; - res.end(JSON.stringify({ - message: req.method + ' method is not allowed. Use POST.' - })); - return; + res.end(JSON.stringify({message: invalidMethod})); + throw invalidMethod; } - if (req.headers['amp-same-origin'] != 'true') { + + if (req.headers.origin) { + origin = req.headers.origin; if (!ORIGIN_REGEX.test(req.headers.origin)) { res.statusCode = 500; - res.end(JSON.stringify({ - message: 'Origin header is invalid.' - })); - return; + 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: '__amp_source_origin parameter is invalid.' - })); - return; + 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-Credentials', 'true'); - 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)); }); }); @@ -201,31 +188,7 @@ app.use('/form/search-html/get', function(req, res) { app.use('/form/search-json/get', function(req, res) { - if (req.headers['amp-same-origin'] != 'true') { - 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; - } - res.setHeader('Access-Control-Allow-Credentials', 'true'); - res.setHeader('Access-Control-Allow-Origin', - req.headers.origin); - } - res.setHeader('Content-Type', 'application/json'); - 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); + assertCors(req, res, ['GET']); res.json({ results: [{title: 'Result 1'}, {title: 'Result 2'}, {title: 'Result 3'}] }); diff --git a/examples/forms.amp.html b/examples/forms.amp.html index c48c1239a364..bf863a544b1c 100644 --- a/examples/forms.amp.html +++ b/examples/forms.amp.html @@ -195,7 +195,7 @@

Subscribe to our weekly Newsletter (xhr POST cross origin - current host > a

Subscribe to our weekly Newsletter (xhr + submit events)

diff --git a/spec/amp-cors-requests.md b/spec/amp-cors-requests.md index 8def7822751c..ba633f8b10f6 100644 --- a/spec/amp-cors-requests.md +++ b/spec/amp-cors-requests.md @@ -39,12 +39,12 @@ by default. This security protocol consists of three components: `AMP-Same-Origin`, CORS origin and source origin restrictions. -On same origin requests, AMP will set `AMP-Same-Origin: true` custom header. If this header is set it indicates the request is coming from same origin. - 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: @@ -52,14 +52,13 @@ The resulting HTTP response has to also contain the following headers: - `AMP-Access-Control-Allow-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 that AMP CORS never requires preflighted requests so it's safe to assume that requests with the `OPTIONS` method are **not** coming from a legitimate AMP page and an error response should be returned. - #### 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 if the request has `AMP-Same-Origin: true` header. If yes, proceed to process the request safely (skip next steps). +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). -2. Else, check the `Origin` header. If the origin was not `*.ampproject.org` or the publisher's origin, stop and return an error response. -3. Check the `__amp_source_origin` query parameter. If it's not the publisher's origin stop and return an error response. +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. diff --git a/src/document-submit.js b/src/document-submit.js index 6d576e3085ff..b27f95acfa60 100644 --- a/src/document-submit.js +++ b/src/document-submit.js @@ -62,18 +62,25 @@ export function onDocumentFormSubmit_(e) { const action = form.getAttribute('action'); const actionXhr = form.getAttribute('action-xhr'); const method = (form.getAttribute('method') || 'GET').toUpperCase(); - 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); - if (!actionXhr && method != 'GET') { + if (method == 'GET') { + user().assert(action, + 'form action attribute is required for method=GET: %s', form); + assertHttpsUrl(action, 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, 'Only XHR based (via action-xhr attribute) submissions are support ' + 'for POST requests. %s', form); } - checkCorsUrl(action); + checkCorsUrl(actionXhr); const target = form.getAttribute('target'); user().assert(target, 'form target attribute is required: %s', form); diff --git a/src/service/xhr-impl.js b/src/service/xhr-impl.js index 8b4d14c46cd2..3568d254083b 100644 --- a/src/service/xhr-impl.js +++ b/src/service/xhr-impl.js @@ -19,6 +19,7 @@ import {fromClass} from '../service'; import { getSourceOrigin, getCorsUrl, + parseUrl, } from '../url'; import {isArray, isObject, isFormData} from '../types'; @@ -116,9 +117,9 @@ export class Xhr { input = this.getCorsUrl(this.win, input); // For some same origin requests, add AMP-Same-Origin: true header to allow // publishers to validate that this request came from their own origin. - const sourceOrigin = getSourceOrigin(this.win.location.href); - const targetOrigin = getSourceOrigin(input); - if (sourceOrigin == targetOrigin) { + const currentOrigin = parseUrl(this.win.location.href).origin; + const targetOrigin = parseUrl(input).origin; + if (currentOrigin == targetOrigin) { init['headers'] = init['headers'] || {}; init['headers']['AMP-Same-Origin'] = 'true'; } @@ -126,6 +127,7 @@ export class Xhr { const allowSourceOriginHeader = response.headers.get( ALLOW_SOURCE_ORIGIN_HEADER); if (allowSourceOriginHeader) { + const sourceOrigin = getSourceOrigin(this.win.location.href); // If the `AMP-Access-Control-Allow-Source-Origin` header is returned, // ensure that it's equal to the current source origin. user().assert(allowSourceOriginHeader == sourceOrigin, diff --git a/test/functional/test-document-submit.js b/test/functional/test-document-submit.js index a4f70ad56eec..f8d9e02ee681 100644 --- a/test/functional/test-document-submit.js +++ b/test/functional/test-document-submit.js @@ -93,6 +93,7 @@ describe('test-document-submit onDocumentFormSubmit_', () => { }); it('should fail when POST and action-xhr is not set', () => { + evt.target.removeAttribute('action'); evt.target.setAttribute('method', 'post'); expect(() => onDocumentFormSubmit_(evt)) .to.throw(/Only XHR based \(via action-xhr attribute\) submissions/); @@ -104,6 +105,13 @@ describe('test-document-submit onDocumentFormSubmit_', () => { expect(preventDefaultSpy.callCount).to.equal(2); }); + it('should fail when action is provided for POST', () => { + evt.target.setAttribute('method', 'post'); + expect(() => onDocumentFormSubmit_(evt)) + .to.throw(/form action attribute is invalid for method=POST/); + expect(preventDefaultSpy.callCount).to.equal(1); + }); + it('should do nothing if already prevented', () => { evt.defaultPrevented = true; onDocumentFormSubmit_(evt); From 68775e56517d40ae42caf2bc92723bf1b27c1917 Mon Sep 17 00:00:00 2001 From: Mohammad Khatib Date: Tue, 13 Sep 2016 13:25:59 -0700 Subject: [PATCH 6/8] fix lint --- src/document-submit.js | 2 +- test/functional/test-xhr.js | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/document-submit.js b/src/document-submit.js index b27f95acfa60..4badb3921308 100644 --- a/src/document-submit.js +++ b/src/document-submit.js @@ -15,7 +15,7 @@ */ import {startsWith} from './string'; -import {dev, user} from './log'; +import {user} from './log'; import {assertHttpsUrl, checkCorsUrl, SOURCE_ORIGIN_PARAM} from './url'; import {urls} from './config'; diff --git a/test/functional/test-xhr.js b/test/functional/test-xhr.js index a731d83219dd..c8bd3abcccb2 100644 --- a/test/functional/test-xhr.js +++ b/test/functional/test-xhr.js @@ -23,7 +23,6 @@ import { assertSuccess, } from '../../src/service/xhr-impl'; import {getCookie} from '../../src/cookies'; -import {platformFor} from '../../src/platform'; describe('XHR', function() { From bd69c73149340559504bb47b97f50059a49e5e9e Mon Sep 17 00:00:00 2001 From: Mohammad Khatib Date: Tue, 13 Sep 2016 13:53:26 -0700 Subject: [PATCH 7/8] fix type --- src/document-submit.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/document-submit.js b/src/document-submit.js index 4badb3921308..319df8378942 100644 --- a/src/document-submit.js +++ b/src/document-submit.js @@ -65,7 +65,7 @@ export function onDocumentFormSubmit_(e) { if (method == 'GET') { user().assert(action, 'form action attribute is required for method=GET: %s', form); - assertHttpsUrl(action, form, 'action'); + assertHttpsUrl(action, /** @type {!Element} */ (form), 'action'); user().assert(!startsWith(action, urls.cdn), 'form action should not be on AMP CDN: %s', form); checkCorsUrl(action); From 3b43eef176749a3545f0a5eba3d43eabc2c0f15b Mon Sep 17 00:00:00 2001 From: Mohammad Khatib Date: Tue, 13 Sep 2016 14:33:15 -0700 Subject: [PATCH 8/8] add origin/source origin test case --- test/functional/test-xhr.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/functional/test-xhr.js b/test/functional/test-xhr.js index c8bd3abcccb2..410833f23c63 100644 --- a/test/functional/test-xhr.js +++ b/test/functional/test-xhr.js @@ -249,6 +249,18 @@ describe('XHR', function() { xhr.fetchJson('/whatever', init); expect(init['headers']['AMP-Same-Origin']).to.equal('true'); }); + + it('should check origin not source origin', () => { + let init = {method: 'post', body: {}}; + location.href = 'https://cdn.ampproject.org/c/s/example.com/hello/path'; + xhr.fetchJson('https://example.com/what/ever', init); + expect(init['headers']['AMP-Same-Origin']).to.be.undefined; + + init = {method: 'post', body: {}}; + location.href = 'https://example.com/hello/path'; + xhr.fetchJson('https://example.com/what/ever', init); + expect(init['headers']['AMP-Same-Origin']).to.equal('true'); + }); }); describe(test.desc, () => {