-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
feat: add allowHeaders to Options #6044
feat: add allowHeaders to Options #6044
Conversation
This allows developers to use custom headers in their API requests, and they will be accepted by their mounted app.
return `${this.publicServerURL}/apps/${ | ||
this.applicationId | ||
}/request_password_reset`; | ||
return `${this.publicServerURL}/apps/${this.applicationId}/request_password_reset`; |
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.
Not sure what happened here - maybe the pre-commit ran a formatter through it!
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.
Yeah it’s prettier
Nice! Can you add a few test cases? |
src/middlewares.js
Outdated
@@ -280,12 +284,17 @@ function decodeBase64(str) { | |||
} | |||
|
|||
export function allowCrossDomain(req, res, next) { | |||
const config = Config.get( | |||
req.get('X-Parse-Application-Id', getMountForRequest(req)) |
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'm making an assumption here that Config.get
will gracefully handle empty parameters and return an undefined config. If not, I'll need to add some checks to ensure we have an appId
and mount
before looking for the config.
@dplewis Yup just looking for the best spec file to put them! |
Can you add it to the definitions? Thats how options are generated. A test to check that the option is actually set. You can check here; |
This is necessary as the middleware may run in OPTIONS request that do not contain the appId within the header.
Codecov Report
@@ Coverage Diff @@
## master #6044 +/- ##
==========================================
- Coverage 93.75% 83.2% -10.56%
==========================================
Files 164 164
Lines 11128 11144 +16
==========================================
- Hits 10433 9272 -1161
- Misses 695 1872 +1177
Continue to review full report at Codecov.
|
* feat: add allowHeaders to Options This allows developers to use custom headers in their API requests, and they will be accepted by their mounted app. * refactor: convert allowCrossDomain to generator to add appId in scope This is necessary as the middleware may run in OPTIONS request that do not contain the appId within the header. * chore: update Definitions and docs * fix: update test to use new allowCrossDomain params * chore: add tests for allowCustomDomain middleware re: allowHeadrs
This allows developers to use custom headers in their API requests, and they will be accepted by their mounted app.
Currently, if you try to send an API request with any custom headers, you receive a preflight error due to this static function. With this optional setting, our users can add their own headers to the allowed list.