-
Notifications
You must be signed in to change notification settings - Fork 17
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.
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
Make CORS configurable / optional #372
Conversation
readonly http?: { | ||
readonly cors_middleware?: boolean; | ||
readonly credentials?: boolean; | ||
readonly allowed_origins?: string[]; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be part of the runtime config and passed all the way down to the httpServer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by that?
@@ -58,6 +58,11 @@ export interface DBOSConfig { | |||
readonly application?: object; | |||
readonly dbClientMetadata?: any; | |||
readonly debugProxy?: string; | |||
readonly http?: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other option is to remove this completely and ask for a decorator if you want to configure CORS at all, like what we do for the custom body parser or custom middleware. Personally, I think that's better because I prefer having information in code than in YAML files, but either way works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this particular case, I suspect that the CORS whitelist may be dependent on deployment, and for that reason I don't think you'd want to bake it into the code. (Though it could be that the application provides code that checks the application section of yaml.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Body parser of course is determined by the code.
I9 was trying to deploy a DBOS app with a react frontend. Calls w/ credentials to DBOS failed for CORS objections.
This PR makes CORS optional (so you could do your own CORS middleware; as it sits the main CORS will prevent anything else from running), based on the config .yaml file. It also allows credentials by default, if not configured.
Includes a
@KoaCors
decorator that can override CORS policy for the class. (Sort of like we are doing for body parser).Once we have this PR settled, documentation will be needed. That is being drafted now.