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.

Sign up for GitHub

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 withCredentials option. Fix #29 #33

Merged
merged 2 commits into from
Jul 1, 2014
+36 −2
Merged

Add withCredentials option. Fix #29 #33

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
62ccd95
Add withCredentials option. Fix #29
Jun 30, 2014
b2a49ce
True by default when cors is true. #29
Jul 1, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ type XhrOptions = String | {
timeout: Number?,
headers: Object?,
body: String?,
json: Object?
json: Object?,
withCredentials: Boolean?
}
xhr := (XhrOptions, Callback<Response>) => Request
```
Expand Down Expand Up @@ -100,6 +101,12 @@ A valid JSON serializable value to be send to the server. If this

Additionally the response body is parsed as JSON

### `options.withCredentials`

Specify whether user credentials are to be included in a cross-origin
request. Sets [`xhr.withCredentials`][10].


## MIT Licenced

[1]: http://xhr.spec.whatwg.org/#the-send()-method
Expand All @@ -111,3 +118,4 @@ Additionally the response body is parsed as JSON
[7]: http://xhr.spec.whatwg.org/#the-responsetext-attribute
[8]: http://xhr.spec.whatwg.org/#the-responsexml-attribute
[9]: http://xhr.spec.whatwg.org/#the-setrequestheader()-method
[10]: http://xhr.spec.whatwg.org/#the-withcredentials-attribute
6 changes: 4 additions & 2 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,11 @@ function createXHR(options, callback) {
// hate IE
xhr.ontimeout = noop
xhr.open(method, uri, !sync)
if (options.cors) {
xhr.withCredentials = true

if ("withCredentials" in options) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we default it to true.

We can use if (options.withCredentials !== false) { ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that would be bad actually. What we want is

if (options.cors && options.withCredentials !== false) {

Meaning opt into cors & opt out of withCredentials

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See browserify/http-browserify#47, which I mentioned in #29. The spec says withCredentials should default to false, so that's what people expect. Also, the wildcard is not allowed in Access-Control-Allow-Origin when it's set to true.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec says withCredentials should default to false, so that's what people expect

The spec has an annoying default, xhr defaults to true because that's what you want. Also I don't think breaking back compat is a good idea. Note that it should still be optin by setting cors to true, so it DOES default to false

Also, the wildcard is not allowed in Access-Control-Allow-Origin when it's set to true.

The wildcard was always a bad idea, no production service should be using it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I wasn't aware of the best practice there, and I imagine I'm not the only one. Maybe worth a word in the README? Do you have any good resources around this issue to point at?

In the meantime I'll change it to default to true.

xhr.withCredentials = options.withCredentials
}

// Cannot set timeout with sync request
if (!sync) {
xhr.timeout = "timeout" in options ? options.timeout : 5000
Expand Down
15 changes: 15 additions & 0 deletions test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,18 @@ test("can GET current page", function(assert) {
})
})

test("withCredentials option", function(assert) {
var req = xhr({}, function () {})
assert.ok(
!req.withCredentials,
"withCredentials not set when not set in options"
)
req = xhr({
withCredentials: true
}, function () {})
assert.ok(
req.withCredentials,
"withCredentials set to true when true in options"
)
assert.end()
})