API Added CORS pre-flight handler (fixes #66) #69

PapaBearNZ · 2017-02-06T22:24:45Z

Added code to index() to detect a CORS preflight OPTIONS request.
Added getCorsResponse() to append the correct headers for a CORS request.
Also checks the origin is an allowed domain for cross-origin requests.
Included unittests for invalid origin, valid origin, OPTIONS request type.

PapaBearNZ · 2017-02-08T02:10:15Z

Found the bug in this pull request. Fixing. I'll submit a new PR with the fix in place.

PapaBearNZ · 2017-02-08T02:11:59Z

Oops - hit close and comment in error.

PapaBearNZ · 2017-02-08T03:18:36Z

I think CORS should be disabled by default. This is in line with the idea of assuming all things are as locked down as possible unless the dev chooses to open them up. If no one has any complaints, I'll update the default config. This should then (in conjunction with the above modification) fix the travis build issues.

chillu · 2017-02-08T09:34:29Z

src/Controller.php

-            ->addHeader('Content-Type', 'application/json');
+        $response = new HTTPResponse(json_encode($result));
+        if ($corsEnabled) {
+            $response = $this->getCorsResponse($request, $response);


Good catch about including CORS headers in the actual GraphQL response, that seems to be best practice. If you follow https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS#Preflighted_requests, the actual HTTP response (not the OPTIONS call) only has the Access-Control-Allow-Origin header - but the spec says "An HTTP response to a CORS request can include the following headers", listing all available headers (https://fetch.spec.whatwg.org/#http-responses) - so I think we're good

chillu · 2017-02-08T09:38:49Z

src/Controller.php

+     * @param HTTPResponse $response
+     * @return HTTPResponse
+     */
+    public function getCorsResponse(HTTPRequest $request, HTTPResponse $response = null)


The dual use with/without a $response argument is a bit confusing. Can you create new HTTPResponse() outside of this method, make $response a mandatory argument, and rename the method to addCorsHeaders()?

Understood. I'll make the necessary changes.

chillu · 2017-02-08T09:39:39Z

Could you have a look at the failed tests please? Just restarted the build, in case they were temporary

chillu · 2017-02-08T09:41:27Z

I think CORS should be disabled by default. This is in line with the idea of assuming all things are as locked down as possible unless the dev chooses to open them up. If no one has any complaints, I'll update the default config. This should then (in conjunction with the above modification) fix the travis build issues.

Agreed. So you're planning to set Enabled: false, meaning an OPTIONS request won't receive any special responses in the controller by default, right?

PapaBearNZ · 2017-02-08T18:55:57Z

@chillu Re the CORS Enabled comment. You are correct regarding the response headers. The errors in the tests are due to CORS being enable by default but having no Allowed Origins defined, the CORS response was correctly rejecting the request. By setting CORS to disabled by default, This check never happens and the response is processed normally without the 403 status.

codecov-io · 2017-02-08T21:15:40Z

Codecov Report

Merging #69 into master will increase coverage by 0.08%.

@@            Coverage Diff             @@
##           master      #69      +/-   ##
==========================================
+ Coverage   90.51%   90.59%   +0.08%     
==========================================
  Files          36       36              
  Lines        1402     1436      +34     
==========================================
+ Hits         1269     1301      +32     
- Misses        133      135       +2

Impacted Files	Coverage Δ
src/Controller.php	`85.18% <94.44%> (+6.46%)`	✅

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16752a3...cc0a672. Read the comment docs.

chillu · 2017-02-08T21:29:10Z

src/Controller.php

@@ -51,6 +51,10 @@ public function index(HTTPRequest $request)
            // Process the CORS config and add appropriate headers.
            $response = new HTTPResponse();
            return $this->addCorsHeaders($request, $response);
+        } elseif (!$corsEnabled && $request->httpMethod() == 'OPTIONS') {
+            // CORS is disabled but we have received an OPTIONS request.  This is not a valid request method in this
+            // situation.  Return a 405 Method Not Allowed response.


The comments are getting a little bit verbose (repeating the status code), but otherwise all good :)

chillu · 2017-02-08T21:30:17Z

Sweet, now it's just missing some docs for the README!

chillu · 2017-02-08T23:13:23Z

README.md

+       Enabled: true
+```
+
+Once you have enabled CORS you can then control 4 new headers in the HTTP Response.


"4" should be spelled out as "four" in most common grammar guidelines

noted and corrected.

chillu · 2017-02-08T23:13:47Z

README.md

+```
+
+Once you have enabled CORS you can then control 4 new headers in the HTTP Response.
+(please refer to the YAML below for examples)


"please refer" sentence is too verbose, given the YAML is two lines below it

noted and removed.

chillu · 2017-02-08T23:15:13Z

Can you rebase on master please? There’s conflicts. Can you please link the first mention of CORS to https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS? I found that guide to be helpful. Otherwise the README looks great!

chillu · 2017-02-09T00:35:55Z

README.md

@@ -1449,6 +1440,126 @@ php -r 'echo base64_encode("hello:world");'
 # aGVsbG86d29ybGQ=
 ```

+### Defining your own authenticators


Why did you add this section? Merge error?

Argh - That was to resolve a merge conflict after the rebase. It's actually @robbieaverill 's work that's somehow gotten credited to me :( Not sure how to fix that. (lines 1443-1461)

Looks like a dodgy merge

The rebase off upstream master worked well but then when trying to push up my branch there were conflicts there as well. This section got caught in the crossfire :(

What do you suggest to resolve this guys? Should I create a new branch off master, squash my commit tree and then do one single commit as part of a replacement PR?

Whatever works for you - the end goal is that only relevant patches make it into this PR. We can squash on merge, or you can squash - either way is fine.

How does this look? Better?

master...PapaBearNZ:66-cors-handling

* Added addCorsHeaders() function to append appropriate headers to response if necessary. * Added cors section to SilverStripe\GraphQL config object. * Added UnitTests. * Added README documentation for CORS handling.

…fixup BUG Ensure i18n detects locale from <html lang>

PapaBearNZ mentioned this pull request Feb 7, 2017

Make CORS configurable #66

Closed

PapaBearNZ closed this Feb 8, 2017

PapaBearNZ reopened this Feb 8, 2017

chillu reviewed Feb 8, 2017

View reviewed changes

chillu requested changes Feb 8, 2017

View reviewed changes

chillu reviewed Feb 8, 2017

View reviewed changes

chillu reviewed Feb 9, 2017

View reviewed changes

FEATURE: CORS Request Handler (silverstripe#66)

cc0a672

* Added addCorsHeaders() function to append appropriate headers to response if necessary. * Added cors section to SilverStripe\GraphQL config object. * Added UnitTests. * Added README documentation for CORS handling.

PapaBearNZ force-pushed the 66-cors-preflight-handling branch from bc5dcb5 to cc0a672 Compare February 9, 2017 02:57

chillu merged commit f2190da into silverstripe:master Feb 9, 2017

unclecheese pushed a commit to unclecheese/silverstripe-graphql that referenced this pull request Jan 27, 2021

Merge pull request silverstripe#69 from open-sausages/pulls/4.0/i18n-…

08c25e3

…fixup BUG Ensure i18n detects locale from <html lang>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Added CORS pre-flight handler (fixes #66) #69

API Added CORS pre-flight handler (fixes #66) #69

PapaBearNZ commented Feb 6, 2017 •

edited

Loading

PapaBearNZ commented Feb 8, 2017

PapaBearNZ commented Feb 8, 2017

PapaBearNZ commented Feb 8, 2017

chillu Feb 8, 2017

chillu Feb 8, 2017

PapaBearNZ Feb 8, 2017

chillu commented Feb 8, 2017

chillu commented Feb 8, 2017

PapaBearNZ commented Feb 8, 2017 •

edited

Loading

codecov-io commented Feb 8, 2017 •

edited

Loading

chillu Feb 8, 2017

chillu commented Feb 8, 2017

chillu Feb 8, 2017

PapaBearNZ Feb 8, 2017

chillu Feb 8, 2017

PapaBearNZ Feb 8, 2017

chillu commented Feb 8, 2017

chillu Feb 9, 2017

PapaBearNZ Feb 9, 2017

robbieaverill Feb 9, 2017

PapaBearNZ Feb 9, 2017

chillu Feb 9, 2017

PapaBearNZ Feb 9, 2017 •

edited

Loading

API Added CORS pre-flight handler (fixes #66) #69

API Added CORS pre-flight handler (fixes #66) #69

Conversation

PapaBearNZ commented Feb 6, 2017 • edited Loading

PapaBearNZ commented Feb 8, 2017

PapaBearNZ commented Feb 8, 2017

PapaBearNZ commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chillu commented Feb 8, 2017

chillu commented Feb 8, 2017

PapaBearNZ commented Feb 8, 2017 • edited Loading

codecov-io commented Feb 8, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

chillu commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chillu commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PapaBearNZ Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

PapaBearNZ commented Feb 6, 2017 •

edited

Loading

PapaBearNZ commented Feb 8, 2017 •

edited

Loading

codecov-io commented Feb 8, 2017 •

edited

Loading

PapaBearNZ Feb 9, 2017 •

edited

Loading