Skip to content
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 HTTP Basic auth via Authorization header #167

Merged
merged 8 commits into from Dec 11, 2015

Conversation

@idan
Copy link
Contributor

commented Dec 9, 2015

Fixes #78.

This is a resurrection of #130 with test and docs; it seems that effort stalled. I've rebased @lachenmayer's branch on top of latest master and dealt with conflicts, this should merge cleanly.

Most critically, it does not rely on URL embedded identities (aka http://user:pass@example.com). Chrome 19 dropped support of URL embedded identities in XMLHttpRequest for security reasons. Basic auth which relies on the trailing user and pass arguments to XMLHttpRequest.open will fail on some platforms. By contrast, Authorization headers work everywhere.

Thanks to @lachenmayer for the initial effort, and @mzabriskie for the lovely axios.

lachenmayer and others added some commits Oct 20, 2015

Abandoning URL embedded identities for Basic auth
Use an `Authorization` header instead, which is a safer choice than URL embedded identities (aka `http://user:pass@example.com`). [Chrome 19 dropped support][chromium128323] for URL embedded identities in XMLHttpRequest for security reasons.

Added documentation note about how this will overwrite any existing `Authorization` header that the user may have set.

[chromium128323]: https://code.google.com/p/chromium/issues/detail?id=128323
@laander

This comment has been minimized.

Copy link

commented Dec 9, 2015

Good stuff! Although, be aware that this will fail in IE8 + IE9 as they don't support window.btoa, see http://caniuse.com/#feat=atob-btoa

@idan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2015

@laander herp derp, didn't notice that. Fixing

@idan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2015

@laander do you think it's legit to call out a polyfill dependency similar to promises? Specifically, https://github.com/davidchambers/Base64.js

@laander

This comment has been minimized.

Copy link

commented Dec 9, 2015

@idan Seeing that the source for that is only ~500 bytes, I think it would be fine 👍
@mzabriskie Your thoughts?

@idan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2015

Yup, also seeing as ES2015 promises are required and not even supported in any flavor of IE, seems justified to say that older IE needs a polyfill for btoa.

@mzabriskie

This comment has been minimized.

Copy link
Member

commented Dec 10, 2015

I think given the size of btoa polyfill lets just add it as a helper function and import it locally. So we're not actually polyfilling on window. Something like this:

module.exports = window.btoa || function () { /* ... */ };

Then in xhr.js just import and use the local helper.

Also I think I'd prefer to just provide one naming convention. Either username and password or user and pass. I don't see the need for supporting both.

Thanks for the PR!

@idan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2015

Roger.

idan added some commits Dec 10, 2015

Dropping support for auth.user/pass
Only accept `username` and `password` as arguments
Polyfilling btoa where appropriate
Includes testing of the polyfills.
@idan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2015

@mzabriskie Done. I did the polyfill check in xhr.js to facilitate testing of the polyfill module independently.

Also removed support for user and pass in favor of the non-abbreviated flavors.

mzabriskie added a commit that referenced this pull request Dec 11, 2015

Merge pull request #167 from idan/add-basic-auth
Add support for HTTP Basic auth via Authorization header

@mzabriskie mzabriskie merged commit d81db4a into axios:master Dec 11, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mzabriskie

This comment has been minimized.

Copy link
Member

commented Dec 11, 2015

@idan many thanks for the PR!

@idan idan deleted the idan:add-basic-auth branch Dec 11, 2015

@idan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2015

❤️ 🍰 !

@mzabriskie can you please cut a new release for NPM too?

Many many thanks for Axios. 😄

@samtheson

This comment has been minimized.

Copy link

commented Jan 10, 2017

Many thanks for this addition @idan. What would it take for Axios, given a url like https://jane:doe@example.com, to pick out the auth info and add it to the config automatically? 💥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.