SDI-1659: support for CORS header #1509

candysmurf · 2017-02-07T04:55:13Z

Fixes #998

Summary of changes:

Added CORS support.
Cors origins have to be specified in the global config. Otherwise it's the same origin.

Testing done:

swagger-ur which is in a different origin of Snap.

@intelsdi-x/snap-maintainers

andrzej-k · 2017-02-07T08:08:29Z

mgmt/rest/server.go

+
+	// Deals with CORS
+	c := cors.New(cors.Options{
+		AllowedOrigins: []string{"*"},


I don't think that it's safe to allow unrestricted access from any domain. Is there a possibility to constrain allowed origins - via snapteld configuration maybe?

@andrzej-k, agreed your concerns. Should we allow localhost for now until we know whoch domains are allowed?

Configurable with 'localhost' as the default?

@jcooklin, sounds good. I'll add it as a global option.

marcintao · 2017-02-07T10:45:32Z

mgmt/rest/server.go

+	// Deals with CORS
+	c := cors.New(cors.Options{
+		AllowedOrigins: []string{"*"},
+		AllowedMethods: []string{"GET, POST, DELETE, PUT, PATCH, OPTIONS"},


Not all of the HTTP methods were allowed previously. I believe it would be the safest to keep the list of allowed methods to a necessary minimum.
PATCH - I didn't find it declared for any of REST controllers. Are you sure we need to allow it?
OPTIONS - the same as for PATCH. It's not needed for pre-flight requests. Negroni allows it implicitly only for the purpose of CORS. On the other hand, if it's needed for Snap API, it should be enabled with a flag OptionsPassthrough.

OPTIONS is needed for preflight. I can remove PATCH for now.

Yes, you're right - OPTIONS are needed for preflight. But isn't it handled before the actual request? I'm thinking about this: https://github.com/rs/cors/blob/master/cors.go#L189. I guess the actual request handling and method checking takes place later.

@marcintao, thanks for looking into it. However, Snap has two places to be patched. One place cannot use the lib. I added maxAge to ease your concerns. Please let me know any other suggestions you have.

candysmurf · 2017-02-08T18:44:43Z

@andrzej-k, @marcintao, @jcooklin, CORS PR is updated. Please review when you have a chance.

andrzej-k · 2017-02-09T07:42:23Z

docs/SNAPTELD_CONFIGURATION.md

@@ -192,6 +192,9 @@ restapi:

  # port sets the port to start the REST API server on. Default is 8181
  port: 8181
+
+  # corsd sets the allowed origins in a comma separated list. It defaults to the same origin if the value is empty.
+  corsd: http://127.0.0.1:8080, http://snap.example.io, http://example.com


"corsd" sounds cryptic - how about changing to, for example: "allowed_origins"?

Will it work with https origins? If so, maybe add to the list https example also

andrzej-k · 2017-02-09T07:49:54Z

mgmt/rest/flags.go

@@ -60,7 +60,11 @@ var (
 		Name:  "pprof",
 		Usage: "Enables profiling tools",
 	}
+	flCorsd = cli.StringFlag{
+		Name:  "corsd",


Consider changing to, for example "allowed_origins"

andrzej-k · 2017-02-09T07:52:27Z

mgmt/rest/server.go

@@ -60,6 +70,7 @@ type Server struct {
 	// the following instance variables are used to cleanly shutdown the server
 	serverListener net.Listener
 	closingChan    chan bool
+	allowedOrigins map[string]bool


It doesn't seem that "allowedOrigins" is used to "cleanly shutdown the server" - consider moving new variable before this comment.

not sure your comment here. It's used in authMiddleware method.

I'm just saying that "allowedOrigins" variable is added in a section which is described with a comment (3 lines above):
// the following instance variables are used to cleanly shutdown the server

This may be misleading, so I just proposed to move "allowedOrigins" before that comment, i.e. 4 lines up.

andrzej-k · 2017-02-09T08:06:36Z

mgmt/rest/server_test.go

+		Convey("Origins match results", func() {
+			So(s.allowedOrigins, ShouldResemble, map[string]bool{"http://127.0.0.1:80": true, "http://example.com": true})
+			So(len(o), ShouldEqual, 2)
+		})


Consider adding more rainy day scenarios, ie. incorrect origin format, incorrect separator, etc.

andrzej-k · 2017-02-09T08:10:46Z

mgmt/rest/server.go

+	for _, o := range os {
+		s.allowedOrigins[strings.TrimSpace(o)] = true
+	}
+	return os


Consider adding more validation, for example if origin format is correct, something like:

httpa://domain.com

127.0.0.0.1

http://?domain.com

...

should not pass, and user should be warned early that configuration is not valid.

candysmurf · 2017-02-10T04:26:55Z

@andrzej-k, thanks for your awesome review. I changed the name, added URL validation and more negative tests as you suggested. Please let me know if anything else.

andrzej-k

LGTM after addressing all comments.

andrzej-k · 2017-02-10T07:58:29Z

docs/SNAPTELD_CONFIGURATION.md

@@ -195,6 +195,9 @@ restapi:

  # port sets the port to start the REST API server on. Default is 8181
  port: 8181
+
+  # corsd sets the allowed origins in a comma separated list. It defaults to the same origin if the value is empty.


"corsd" -> "allowed_origins"

andrzej-k · 2017-02-10T08:04:27Z

mgmt/rest/server.go

@@ -60,6 +70,7 @@ type Server struct {
 	// the following instance variables are used to cleanly shutdown the server
 	serverListener net.Listener
 	closingChan    chan bool
+	allowedOrigins map[string]bool


I'm just saying that "allowedOrigins" variable is added in a section which is described with a comment (3 lines above):
// the following instance variables are used to cleanly shutdown the server

This may be misleading, so I just proposed to move "allowedOrigins" before that comment, i.e. 4 lines up.

candysmurf · 2017-02-10T16:54:30Z

good catch! Changed. Thanks @andrzej-k!

jcooklin · 2017-02-10T18:07:09Z

LGTM

added global config, tests, docs for cors added url validation, more nagtive tests changed typo and variable localtion

addressed his comments.

candysmurf force-pushed the cors branch from e33d7ed to 73f756f Compare February 7, 2017 06:17

andrzej-k suggested changes Feb 7, 2017

View reviewed changes

marcintao suggested changes Feb 7, 2017

View reviewed changes

candysmurf force-pushed the cors branch 2 times, most recently from 4abab8f to e431cc7 Compare February 8, 2017 18:32

marcintao approved these changes Feb 9, 2017

View reviewed changes

andrzej-k previously requested changes Feb 9, 2017

View reviewed changes

candysmurf force-pushed the cors branch from e431cc7 to f85bb9b Compare February 10, 2017 04:05

candysmurf force-pushed the cors branch 2 times, most recently from 3c8774a to 7ed7e38 Compare February 10, 2017 04:35

andrzej-k reviewed Feb 10, 2017

View reviewed changes

SDI-1659: support for CORS header

09ddbf9

added global config, tests, docs for cors added url validation, more nagtive tests changed typo and variable localtion

candysmurf force-pushed the cors branch from af93c93 to 09ddbf9 Compare February 10, 2017 21:49

candysmurf merged commit 89bf6c0 into intelsdi-x:master Feb 10, 2017

candysmurf deleted the cors branch February 10, 2017 22:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SDI-1659: support for CORS header #1509

SDI-1659: support for CORS header #1509

candysmurf commented Feb 7, 2017 •

edited

andrzej-k Feb 7, 2017

candysmurf Feb 7, 2017

jcooklin Feb 7, 2017

candysmurf Feb 7, 2017

marcintao Feb 7, 2017 •

edited

candysmurf Feb 7, 2017

marcintao Feb 8, 2017

candysmurf Feb 8, 2017

candysmurf commented Feb 8, 2017 •

edited

andrzej-k Feb 9, 2017

andrzej-k Feb 9, 2017

andrzej-k Feb 9, 2017

candysmurf Feb 10, 2017

andrzej-k Feb 10, 2017

andrzej-k Feb 9, 2017

andrzej-k Feb 9, 2017

candysmurf commented Feb 10, 2017

andrzej-k left a comment

andrzej-k Feb 10, 2017

andrzej-k Feb 10, 2017

candysmurf commented Feb 10, 2017

jcooklin commented Feb 10, 2017

SDI-1659: support for CORS header #1509

SDI-1659: support for CORS header #1509

Conversation

candysmurf commented Feb 7, 2017 • edited

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

marcintao Feb 7, 2017 • edited

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

candysmurf commented Feb 8, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

candysmurf commented Feb 10, 2017

andrzej-k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

candysmurf commented Feb 10, 2017

jcooklin commented Feb 10, 2017

candysmurf commented Feb 7, 2017 •

edited

marcintao Feb 7, 2017 •

edited

candysmurf commented Feb 8, 2017 •

edited