-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add support for credentials requests #434
base: master
Are you sure you want to change the base?
Conversation
@indexzero this is for dev purposes |
@senaev would you mind merging/rebasing master? |
# Conflicts: # README.md # bin/http-server # lib/http-server.js
@indexzero @thornjad done |
test/http-server-test.js
Outdated
credentials: true, | ||
corsHeaders: 'X-Test' | ||
}); | ||
server.listen(8083); |
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.
This port is already being used by username/password tests, so we should do 8085 here (8084 is also claimed). This is causing the tests to fail.
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.
👌
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.
✅ fixed
test/http-server-test.js
Outdated
credentials: true, | ||
corsHeaders: 'X-Test' | ||
}); | ||
server.listen(8083); |
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.
👌
@thornjad check this please |
This pull request has been inactive for 360 days |
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.
Besides this one question, I really like this! Here's what's left to do:
- documentation in doc/http-server.1
- Sorry, more merge conflicts to fix up
' --credentials Cookie credentials can be transferred as part of a CORS request', | ||
' Enables --cors automatically', |
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 concerned that the name of this switch is too general for its use case, how would you feel about --cookie-credentials
or shorten to --cookie-creds
or something like that?
To serve files with credentials support cross domains we ned to not just add some headers in request, we should also change Access-Control-Allow-Origin header to Origin value from request.
I added a new flag for it, because is is different from --cors logic and may be not safe for users.