986057 - Loop Server — Should accept cross origin requests

Status: VERIFIED FIXED

(Hello (Loop) :: Server, defect, P1)

Description

[Alexis Metaireau (:alexis)]

We should allow cross origin resource sharing for some URLs, that will be called from the WebApp.

To do that, add the following headers:

res.set("Access-Control-Allow-Origin", "*");
res.set("Access-Control-Allow-Methods", "GET,POST");

Comment 1

[Alexis Metaireau (:alexis)]

Attachment: link to github PR

Comment 2

[Eric Rescorla (:ekr)]

Uh, this is generally not a safe practice, since effectively this
removes all CSRF protection. Why can't you identify the correct
friendly origin URLs?

Please address this question prior to landing this patch.

Comment 3

[Alexis Metaireau (:alexis)]

Basically, I was considering this URL unique and as one that can only be accessed by people who know it exists, so CSRF doesn't really seem a treat to me here.

Nevertheless, I agree it is possible to narrow the list of hosts that will expose the pages (theses are the CDNs) and probably that should be good practice, too.

I've updated the patch with a way to define the list of authorized domains in the settings file.

Comment 4

[Rémy Hubscher (:natim)]

Comment on attachment 8394445 [details] [review]
link to github PR

For now it is good enough, but we should add a list to decide how we want to handle multiple domain authorization.

See http://stackoverflow.com/questions/1653308/access-control-allow-origin-multiple-origin-domains

Comment 5

[Rémy Hubscher (:natim)]

https://github.com/mozilla-services/loop-server/commit/7698ebce3cc923fd1fb0fb433649b7692c016ba3

Comment 6

[Alexis Metaireau (:alexis)]

This doesn't seem like the right way to accept multiple domains, so I'm reopening this one until we fix that.

The link you pointed to says we should have the Allow-Origin match the Origin of the request that had been made, and now I remember that's the way it should be implemented.

Comment 7

[Rémy Hubscher (:natim)]

We should define the OPTION HTTP verb as well.

As NiKo` noticed they might be interresting though here: https://gist.github.com/cuppster/2344435

Comment 8

[Alexis Metaireau (:alexis)]

For GET / POST with JSON we should't need a preflight (OPTION) request to be made.

Why are you suggesting to answer to OPTION for this specific case? What's the request that's not working okay?

Comment 9

[Alexis Metaireau (:alexis)]

Actually, after reading back the CORS spec, it seems that we need to support OPTIONS if we want an accept-header different than one of application/x-www-form-urlencoded, multipart/form-data, or text/plain.

Comment 10

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Right, this bug is currently preventing from testing the Loop webapp using the server. Blocker.

Comment 11

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

https://github.com/troygoode/node-cors/ might be just what we need here.

Comment 12

[Rémy Hubscher (:natim)]

Yes looks to be a great as possible :+1:

Comment 13

[Alexis Metaireau (:alexis)]

Attachment: link to github PR

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8395618 [details] [review]
link to github PR

Looks good, r=Standard8

Comment 15

[Alexis Metaireau (:alexis)]

https://github.com/mozilla-services/loop-server/commit/83b58df712d15bab290f51977077b352f52b0423

Comment 16

[James Bonacci [:jbonacci]]

Verified in code.