feat($http): add option for unsafe xhr requests in FirefoxOS #7903

amccausl · 2014-06-19T03:05:02Z

Previously, it was not possible to pass settings to the constructor of the XMLHttpRequest. This
adds an optional config property xhrConfig to the $http constructor, allowing us to do so. This
is required for use of the mozAnon and mozSystem options for FirefoxOS.
See https://developer.mozilla.org/en-US/docs/DOM/XMLHttpRequest#XMLHttpRequest() for details.

Closes #2318

caitp · 2014-06-19T03:09:27Z

src/ng/httpBackend.js

@@ -8,7 +8,7 @@ function createXhr(method) {
      !window.XMLHttpRequest)) {
      return new window.ActiveXObject("Microsoft.XMLHTTP");
    } else if (window.XMLHttpRequest) {
-      return new window.XMLHttpRequest();
+      return new window.XMLHttpRequest(config);


this will possibly cause IDL arity errors in other browsers...

not chromium at least =) a lot of the blink bindings will throw when the arity does not match though.... Guess that doesn't apply to constructors

I didn't know that was a thing :(. Better safe than sorry, so have updated. Thanks for the feedback :)

nah I wouldn't worry about it if tests are passing

Previously, it was not possible to pass settings to the constructor of the XMLHttpRequest. This adds an optional config property `xhrConfig` to the $http constructor, allowing us to do so. This is required for use of the mozAnon and mozSystem options for FirefoxOS. See https://developer.mozilla.org/en-US/docs/DOM/XMLHttpRequest#XMLHttpRequest() for details. Closes angular#2318

caitp · 2014-06-19T16:58:13Z

Related jQuery bug http://bugs.jquery.com/ticket/13394 (their resolution doesn't really apply here since we don't have a similar mechanism yet)

caitp · 2014-06-19T19:07:50Z

Anyways, we probably should have something like xhrFields in angular, instead of solving this this way. I'll see what people think about that, I guess.

sjurba · 2014-06-19T19:16:43Z

Oh.. Damit.. Didn't see this before I just submitted my own pull request #7906. Sorry for the duplicate. We have actually made quite identical solutions. Only difference I can see is that I called it xhrFields like jQuery and that I added support for setting it on the $http.defaults object. I also added some tests and updated the documentation.

caitp · 2014-06-19T19:18:55Z

The jQuery xhrFields stuff is a bit different, though --- they don't pass the object to the constructor, but instead this: https://github.com/jquery/jquery/blob/01c360f96390ff16edfe65ef3b34e167087ef645/src/ajax/xhr.js#L51-L55

caitp · 2014-06-19T19:20:25Z

I think we need something like that, as well as a way to have defaults which are applied to all XHR objects, as well as per-request xhrFields, similar to our headers strategy. That would make more sense IMO.

sjurba · 2014-06-19T19:22:47Z

The problem with that approach is that it doesn't actually work since the mozSystem property is read-only and cannot be set except through the constructor. (On Firefox OS)

caitp · 2014-06-19T19:39:21Z

That sounds like a bug you should file on b2g, because the problem with this is that the XHR constructor isn't normally supposed to take any parameters, but if one extension to it wants to add custom parameters, what if another one does too which behaves differently? The whole thing is a mess.

Anyways, this is the solution suggested by the jQuery team, so it was probably valid and tested at some point.

sjurba · 2014-06-19T19:45:48Z

Yes, you I right. And I agree that it would be a better solution. I'm double-checking my statement and will check if making a jQuery ajax request actually works on Firefox OS. I'll post back with my findings.

amccausl · 2014-06-19T20:05:12Z

I'm happy with changes from #7906. Not sure what the protocol is now that discussion is here, but will happily close this PR or give @sjurba access to update the branch behind this PR with his change.

Great work @sjurba :)

sjurba · 2014-06-19T20:06:32Z

I have verified my findings. It doesn't work with the current implementation by jQuery either. This is also backed up by this bug report: http://bugs.jquery.com/ticket/13862.
Here they actually suggest several different workarounds, all of which rely on the xhr config param (http://api.jquery.com/jquery.ajax/) which basically is an option to override the xhr constructor function:
https://github.com/jquery/jquery/blob/01c360f96390ff16edfe65ef3b34e167087ef645/src/ajax/xhr.js#L22

Don't know if that is something you guys would consider as well. It would sure make mocking the XMLHttpRequest object a lot simpler..

sjurba · 2014-07-10T09:09:07Z

@caitp: So it seems this one stalled since passing arguments to a constructor that is not supposed to take one is a bad idea and should really be fixed by Mozilla B2G. But maybe you would be willing to accept a workaround that lets you pass in your own createXhr method instead. In other words the same thing jQuery offers with its xhr parameter: http://api.jquery.com/jquery.ajax/

If so I could probably make a PR for it.

caitp · 2014-07-10T11:07:23Z

I don't see a problem with allowing custom createXhr()

ripper2hl · 2014-07-24T02:56:01Z

hello, change jquery to angular in my application, but do not know how this property is added to the service $http, was something in jquery:
var xhr = new XMLHttpRequest({mozSystem: true});

my english is bad.

caitp · 2014-07-24T03:08:07Z

@ripper2hl there's no way in angular to do this currently... I guess I could submit a patch for it... but frankly the B2G people should just get their act together and not use a different function signature from the interoperable XHR api

ripper2hl · 2014-07-24T04:34:48Z

It is sad to know that the standard is not followed, if you send the patch would be a great help, thanks for the information.

sjurba · 2014-07-24T09:53:51Z

I started on a fix here: https://github.com/sjurba/angular.js/tree/createXhr

The implementation is simple enough, but it kinda cries for some refactoring.. But I'm not quite sure how to approach that. What's best for you? Should I just create a pull request for what I got so far and start a discussion there?

Narretz · 2014-09-28T11:40:50Z

@sjurba Sorry for the late response, but yes, sending a PR is a good idea

sjurba · 2014-09-28T12:37:31Z

No worries. I've been busy elsewhere.. I'll dig back into it later this evening/afternoon.. —
Sjur

On Sun, Sep 28, 2014 at 1:42 PM, Narretz notifications@github.com wrote:

@sjurba Sorry for the late response, but yes, sending a PR is a good idea

Reply to this email directly or view it on GitHub:
#7903 (comment)

sjurba · 2014-09-28T19:04:53Z

Ok, PR #9319 is sent..

pkozlowski-opensource · 2014-12-14T12:30:25Z

This one looks a lot like #7906 and that one was closed in favour of the more generic approach. Currently we've got a PR for a more flexible approach (#9319) but I'm not super-happy with it either...

Anyway, closing this one as almost identical #7906 was closed. Let's focus on solving it in a more flexible way.

amccausl added cla: yes and removed cla: no labels Jun 19, 2014

caitp reviewed Jun 19, 2014
View reviewed changes

caitp mentioned this pull request Jun 19, 2014

feat($http): adds xhrFields to $http config object #7906

Closed

Narretz added type: feature labels Jul 17, 2014

Narretz added this to the Ice Box milestone Jul 17, 2014

Narretz added the needs: work label Jul 17, 2014

caitp force-pushed the master branch from 591bac5 to d713ad1 Compare August 22, 2014 13:51

tbosch force-pushed the master branch from 6c27b89 to 271572c Compare August 30, 2014 00:57

tbosch force-pushed the master branch from a3d7934 to 1418383 Compare September 10, 2014 18:14

petebacondarwin force-pushed the master branch from 02dc2aa to fd2d6c0 Compare September 16, 2014 11:26

jeffbcross force-pushed the master branch from dbbabb9 to 27d1234 Compare September 27, 2014 00:05

sjurba mentioned this pull request Sep 28, 2014

feat ($http): Add custom createXhr factory to config object #9319

Closed

jeffbcross force-pushed the master branch from cad9560 to f294244 Compare October 2, 2014 22:09

btford force-pushed the master branch from 9011a65 to 46db47b Compare October 6, 2014 22:54

jeffbcross force-pushed the master branch 4 times, most recently from e8dc429 to e83fab9 Compare October 10, 2014 17:37

petebacondarwin force-pushed the master branch from 4dd5a20 to 998c61c Compare October 19, 2014 10:57

pkozlowski-opensource closed this Dec 14, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat($http): add option for unsafe xhr requests in FirefoxOS #7903

feat($http): add option for unsafe xhr requests in FirefoxOS #7903

amccausl commented Jun 19, 2014

caitp Jun 19, 2014

caitp Jun 19, 2014

amccausl Jun 19, 2014

caitp Jun 19, 2014

caitp commented Jun 19, 2014

caitp commented Jun 19, 2014

sjurba commented Jun 19, 2014

caitp commented Jun 19, 2014

caitp commented Jun 19, 2014

sjurba commented Jun 19, 2014

caitp commented Jun 19, 2014

sjurba commented Jun 19, 2014

amccausl commented Jun 19, 2014

sjurba commented Jun 19, 2014

sjurba commented Jul 10, 2014

caitp commented Jul 10, 2014

ripper2hl commented Jul 24, 2014

caitp commented Jul 24, 2014

ripper2hl commented Jul 24, 2014

sjurba commented Jul 24, 2014

Narretz commented Sep 28, 2014

sjurba commented Sep 28, 2014

@sjurba Sorry for the late response, but yes, sending a PR is a good idea

sjurba commented Sep 28, 2014

pkozlowski-opensource commented Dec 14, 2014

feat($http): add option for unsafe xhr requests in FirefoxOS #7903

feat($http): add option for unsafe xhr requests in FirefoxOS #7903

Conversation

amccausl commented Jun 19, 2014

caitp Jun 19, 2014

Choose a reason for hiding this comment

caitp Jun 19, 2014

Choose a reason for hiding this comment

amccausl Jun 19, 2014

Choose a reason for hiding this comment

caitp Jun 19, 2014

Choose a reason for hiding this comment

caitp commented Jun 19, 2014

caitp commented Jun 19, 2014

sjurba commented Jun 19, 2014

caitp commented Jun 19, 2014

caitp commented Jun 19, 2014

sjurba commented Jun 19, 2014

caitp commented Jun 19, 2014

sjurba commented Jun 19, 2014

amccausl commented Jun 19, 2014

sjurba commented Jun 19, 2014

sjurba commented Jul 10, 2014

caitp commented Jul 10, 2014

ripper2hl commented Jul 24, 2014

caitp commented Jul 24, 2014

ripper2hl commented Jul 24, 2014

sjurba commented Jul 24, 2014

Narretz commented Sep 28, 2014

sjurba commented Sep 28, 2014

@sjurba Sorry for the late response, but yes, sending a PR is a good idea

sjurba commented Sep 28, 2014

pkozlowski-opensource commented Dec 14, 2014