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

instance.defaults changes global object #385

Closed
isterin opened this issue Jul 22, 2016 · 32 comments

Comments

@isterin
Copy link

commented Jul 22, 2016

If I have two instances of axios created with axios.create and I set a default header on one, it also changes it on all the other ones in existence.

var instance1 = this.oauthService = axios.create({});
var instance2 = this.oauthService = axios.create({});

instance2.defaults.headers.common['Authorization'] = 'askdjfaksdjf';

Both instance1 and 2 now have the Authorization header changed. I believe the problem is that the util.merge operation doesn't do deep merging, though as you see below, only if the result[key] and val are both objects, does it do a recursive merge, otherwise it assigns the value by reference.

function merge(/\* obj1, obj2, obj3, ... */) {
  var result = {};
  function assignValue(val, key) {
    if (typeof result[key] === 'object' && typeof val === 'object') {
      result[key] = merge(result[key], val);
    } else {
      result[key] = val;
    }
  }

  for (var i = 0, l = arguments.length; i < l; i++) {
    forEach(arguments[i], assignValue);
  }
  return result;
}
@isterin

This comment has been minimized.

Copy link
Author

commented Jul 22, 2016

BTW, easily fixed with lodash.cloneDeep when merging. I'm not sure though you want to bring in lodash dependency or implement deep cloning outside of it?

@nickuraltsev

This comment has been minimized.

Copy link
Member

commented Aug 3, 2016

Thank you! This definitely needs to be fixed.

@isterin

This comment has been minimized.

Copy link
Author

commented Aug 11, 2016

Is bringing in outside deps like lodash acceptable?

@mzabriskie

This comment has been minimized.

Copy link
Member

commented Aug 15, 2016

I played around with using _.cloneDeep inside the utils.merge and it certainly fixed this problem. However it also somehow broke stuff for node. I'm looking into what's going on there.

@nickuraltsev

This comment has been minimized.

Copy link
Member

commented Aug 15, 2016

@mzabriskie Please take a look at the discussion in #370

On Monday, August 15, 2016, Matt Zabriskie notifications@github.com wrote:

I played around with using _.cloneDeep inside the utils.merge and it
certainly fixed this problem. However it also somehow broke stuff for node.
I'm looking into what's going on there.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#385 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGBhkM7uqsZxyo0RuqnRlwCUwxK1Cor7ks5qgIoQgaJpZM4JSa2R
.

@jonjaques

This comment has been minimized.

Copy link

commented Oct 6, 2016

I was able to work around this by adding a request interceptor. I have a custom class wrapping axios, so something like this:

    this.client.interceptors.request.use(request => {
      request.headers['Authorization'] = `Bearer ${this.token}`
      return request
    })
@codeclown

This comment has been minimized.

Copy link
Contributor

commented Oct 27, 2016

Note that instead of _.cloneDeep or other external dependency, JSON.parse(JSON.stringify(defaults)) may work nicely, assuming there are no circular references.

@matiishyn

This comment has been minimized.

Copy link

commented Apr 12, 2017

looks like the issue is still present in the newest version

@matiishyn

This comment has been minimized.

Copy link

commented Apr 12, 2017

so how can we fix this with cloneDeep?

here's what I tried, but no luck:

// BASE URL
axios.defaults.baseURL = CONFIG.API_SERVER;

// SEPARATE INSTANCES
const authAxios = axios.create();
authAxios.defaults = _.cloneDeep(authAxios.defaults);

const apiAxios = axios.create();
apiAxios.defaults = _.cloneDeep(apiAxios.defaults);

// SETTING JWT HEADER
const savedToken = LS.getJwt();
if (savedToken) {
  apiAxios.defaults.headers.common.Authorization = `JWT ${savedToken}`;    
  // ^=== this will set headers to both instances :( 
}

console.log(authAxios.defaults);
console.log(apiAxios.defaults);

But both authAxios and apiAxios instances have the Authorization headers.

Did anyone fix it somehow?

@lukaszgrolik

This comment has been minimized.

Copy link

commented May 12, 2017

@matiishyn try this:

if (savedToken) {
  const headers = _.cloneDeep(apiAxios.defaults.headers);
  headers.common.Authorization = `JWT ${savedToken}`;
  apiAxios.defaults.headers = headers;
}

Cloning and setting whole headers object works for me. Took hours of debugging and finding this thread though, so I look forward to see this fixed in the future releases.

@prakashgp

This comment has been minimized.

Copy link

commented Aug 1, 2017

This worked for me
request.defaults.headers = Object.assign({}, request.defaults.headers, {'X-Customer-Id': '123'})

@foundryspatial-duncan

This comment has been minimized.

Copy link

commented Aug 30, 2017

Using Object.assign won't always work because it doesn't do a "deep clone."
A cheap alternative without using lodash might be to JSON encode and decode it:

// save the default headers before changing any instances
const defaultHeaders = JSON.parse(JSON.stringify(axios.defaults.headers));

// change something on your instance(s)
fooInstance.defaults.headers.common.bar = "baz";

// re-set the default settings in the global axios
axios.defaults.headers = defaultHeaders;

This workflow should suffice until the bug is fixed, I hope!

@marbemac

This comment has been minimized.

Copy link

commented Oct 25, 2017

Using Axios in a server side rendering situation (next.js, meteor, any SSR focused library) leads to a huge security issue when using defaults.headers to set the currently logged in user token... this should be an urgent and critical bug IMHO. We only just discovered this, crazy its been open for over a year :/.

@MartinGian

This comment has been minimized.

Copy link

commented Jan 13, 2018

I use the @jonjaques solution and works fine!

@codeclown

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2018

There is a PR open to fix this issue but it's stuck currently due to no interaction from project owners. #533

@robaxelsen

This comment has been minimized.

Copy link
Collaborator

commented Jan 24, 2018

@codeclown if you look at the issue you linked to, you will see that @nickuraltsev came with PR review feedback that was never acted on by @yanivefraim.

While this project currently has a lot of old issues, there was a recent popular "help wanted for axios" by the creator or Axios. Let's hope that will help clear old issues and increase response time for new ones.

Also, if you feel you can contribute, please let us know.

@yanivefraim

This comment has been minimized.

Copy link

commented Jan 24, 2018

@robaxelsen - as I said, I would really love to help here!

@codeclown

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2018

@robaxelsen I understand, and I sincerely apologize for seeming hostile towards the project owners. It appeared to me that the comment on the issue did not involve direct review/instructions for the PR maintainer to act upon, but rather a wider context which can also be found on the issue #812.

I just looked into the problem myself in an attempt to provide a PR and discovered that the issue #812 really captures the tricky situation we are in due to wanting to get rid of merging all the options.

@heisian

This comment has been minimized.

Copy link

commented Feb 28, 2018

@nickuraltsev

Thank you! This definitely needs to be fixed.

How's that comin, eh?

@wlingke

This comment has been minimized.

Copy link

commented Feb 28, 2018

This is a serious bug. Can we prioritize fixing it? It's been over a year and half

@codeclown

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2018

@heisian @wlingke This is not a simple issue to fix. Currently it is intended behaviour that Axios keeps a reference to the global defaults, as seen in this test:

it('should be used by custom instance if set after instance created', function (done) {
  var instance = axios.create();
  axios.defaults.baseURL = 'http://example.org/';

  instance.get('/foo');

  getAjaxRequest().then(function (request) {
    expect(request.url).toBe('http://example.org/foo');
    done();
  });
});

This makes it impossible to have the following test passing:

it('should not be affected by changes to global defaults', function (done) {
  var instance = axios.create();
  axios.defaults.headers['X-Foobar'] = 'test';
  expect(instance.defaults.headers['X-Foobar']).toNotBe('test');
});

Well, it would be possible with some specific logic (e.g. if instance-specific baseURL === null, grab from global defaults), but I would rather see less specific logic and get rid of this restriction posed by keeping reference to global defaults.

@heisian

This comment has been minimized.

Copy link

commented Feb 28, 2018

@codeclown The current behaviour doesn't make any sense. Why is the reference kept?

One would think that calling .create creates a new instance, and any configuration defaults applied to that instance are localized to it, not arbitrarily setting a mutable global instance. This is flawed logic.

If the defaults property was truly meant to be a global reference, then it should be immutable from the child instance. There should also be a separate property, like config, in the child instance that holds defaults for the child instance.

Currently I cannot do:

myClient = axios.create()
myClient.baseURL = 'https://google.com' // Doesn't work.

If we wanted to retain the reference to the global defaults, then this should be viable:

axios.defaults.baseURL = 'https://asdf.com'

myClient = axios.create()
myClient.config.baseURL = 'https://google.com'
myClient.get('/') // should GET 'https://google.com/'

console.log(myClient.defaults.baseURL) // should output 'https://asdf.com'

I'm not sure this is not a "simple" issue, in fact it is a pretty basic issue for pretty basic functionality that many other languages simply leverage curl for, alas, everyone in the node ecosystem wants to reinvent the wheel. /rant

@codeclown

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2018

@heisian

The current behaviour doesn't make any sense. Why is the reference kept?

One would think that calling .create creates a new instance, and any configuration defaults applied to that instance are localized to it, not arbitrarily setting a mutable global instance. This is flawed logic.

I agree completely. I'm not sure where to propose deprecating this "functionality", because there are so many issues regarding this defaults-issue. I do, however, think that it should be deprecated because it is not only illogical but harmful and even dangerous (especially on the server-side).


Anyways, whenever that problem is resolved, the second half of fixing this defaults-issue is noticing that not all the configuration can be just deep merged with the defaults. In a comment to another issue (expand the second section) I categorized the configuration properties based on how they should be defined when a new instance is created. I hope this list is of help to anyone who might be stabbing away at this later.

@heisian

This comment has been minimized.

Copy link

commented Feb 28, 2018

@codeclown
Yeah, I'm not sure.. there seems to be a dangling reference somewhere, because I can set defaults.timeout independently across different axios instances.. maybe I'll keep digging, but at the end of the day we haven't heard from the owner in a year and a half so at best we're stuck with forks only even if there is a definite fix.

@codeclown

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2018

@heisian
Here's how the options for an individual request are set:

config = utils.merge(defaults, this.defaults, config);

So, the problem comes from nested objects, e.g. headers, not primitive values such as timeout.


at the end of the day we haven't heard from the owner in a year and a half so at best we're stuck with forks only even if there is a definite fix.

I'm sure that a PR would be very appreciated by many, considering the amount of times this issue has been reported. I personally got stuck with that one failing test. I would encourage you or anyone to create a PR if you are able to propose a fix.

@heisian

This comment has been minimized.

Copy link

commented Feb 28, 2018

@codeclown Got it. I'll give it a shot.

@marbemac

This comment has been minimized.

Copy link

commented Feb 28, 2018

Regardless of the conclusion on this issue, in the interim there should be a big bold notice on the readme that warns NOT to use defaults for sensitive info when in a server context (SSR w react, for example) - because it can result in bad security issues w shared API keys etc.

@heisian

This comment has been minimized.

Copy link

commented Feb 28, 2018

codeclown added a commit to codeclown/axios that referenced this issue Mar 2, 2018

codeclown added a commit to codeclown/axios that referenced this issue Mar 2, 2018

@throrin19

This comment has been minimized.

Copy link

commented Mar 30, 2018

Same problem here

nickuraltsev added a commit that referenced this issue Apr 10, 2018

Merge pull request #1395 from codeclown/instance-options
Fixing #385 - Keep defaults local to instance
@nickuraltsev

This comment has been minimized.

Copy link
Member

commented Apr 10, 2018

Fixed via #1395

0.19.0 automation moved this from In progress to Done Apr 10, 2018

chihaoyo referenced this issue in watchout-tw/watchout-common-functions May 15, 2018

Use empty headers object when creating coreInstance & coralreefInstan…
…ce; add core.set & unsetAuthHeaders; remove core.setHeaders; unsetAuthHeader at logout

tanoabeleyra added a commit to tanoabeleyra/cryptomkt-bot-pwa that referenced this issue Aug 18, 2018

Fix JWT being sent on every request
Requests to CryptoMKT weren't working in Firefox because of this.

This is a temporary workaround for axios/axios#385
Bug will be fixed in axios v0.19.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
You can’t perform that action at this time.