Fixing #385 - Keep defaults local to instance #1395

codeclown · 2018-03-02T10:31:42Z

Alternative solution to some other PR's addressing this issue, such as #1391, addressing the same issues (#385, #812, #1170, #1117, #1387, ...).

Issue

When creating Axios instances, some references are kept to global defaults object.

Proposed solution

I propose that Axios gets rid of the legacy requirement (existing test) of changes to axios.defaults propagating to instance defaults even when that instance was created before the change to axios.defaults. I think that when creating a new Axios instance, it should freeze its own defaults in time and not be affected by changes to global defaults.

I don't know why that behaviour has been intended originally, but IMO it doesn't make sense.

Not to mention that it is a security hazard in server-side environments. Someone might be setting authentication keys in the defaults and that would affect requests made by different end users because other instances would possibly/probably share those keys.

Implementation

I implemented a specific utils.mergeConfig(defaults, instanceConfig) function which very explicitly creates the configuration-object for a new Axios instance. This function is used when a new instance is created, and it leaves no reference to any objects in the base defaults.

At the time of writing this implementation is quite messy because I just focused on getting the tests to pass. I will try to clean it up a bit, suggestions also welcome.

As this changes behaviour, I assume it would be a breaking change.

I hope for comments on this proposal/implementation. Also thanks to everyone who's been working on this issue in various threads already.

cc @nickuraltsev @heisian @emilyemorehouse

emilyemorehouse · 2018-03-02T17:43:10Z

I'm a fan of this -- I also think that creating an instance should freeze its defaults. I'm not sure if this will get a push into 0.19 (I'd like to focus on getting the security issues fixed in the least difficult way possible) but I would definitely want this in 1.0.0.

(As a side note -- 1.0.0 has ditched most of the custom utils in favor of lodash, which could make things easier as well)

codeclown · 2018-03-02T18:15:12Z

I'm not sure if this will get a push into 0.19 but I would definitely want this in 1.0.0.

It's a breaking change so that's definitely understandable. If this would only ship in 1.0.0, then a deprecation notice in the next minor version wouldn't make sense either.

What is the leadership on this project at the moment? Who decides what goes into which version?

I'd like to focus on getting the security issues fixed in the least difficult way possible

Is there any other way to fix the security issue other than by losing all references between global defaults and/or separate instances?

(As a side note -- 1.0.0 has ditched most of the custom utils in favor of lodash, which could make things easier as well)

Does it make sense for me to work on cleaning up this PR at all at this time? I don't mind closing it if it would be redundant work. I would still love to help out in some way if possible.

Also another thing just occurred to me. Some config-properties are arrays. Should they always lose reference between instances?

const instance1 = axios.create()
const instance2 = axios.create()

// Should this never affect instance2?
instance1.defaults.transformRequest.push(anotherTransformer)

Obviously that could result in some accidents.

One could also argue that there is a use case for keeping the reference:

const commonTransformers = [...]

const instance1 = axios.create({ transformRequest: commonTransformers })
const instance2 = axios.create({ transformRequest: commonTransformers })

// Add new transformer to all instances, yay!
commonTransformers.push(anotherTransformer)

heisian · 2018-03-02T22:55:10Z

lib/utils.js


+function mergeConfig(defaults, instanceConfig) {
+  var config = {};
+  forEach(['url', 'method', 'params', 'data'], function(prop) {


modify this line to

forEach(['url', 'method', 'params', 'data'], function assignProp(prop) {

to pass eslint warnings in CI tests (or some other function name of your choosing)

codeclown · 2018-03-05T18:58:01Z

@emilyemorehouse When you have time, could you check out the questions I posted above, so I know if I should close this PR or not?

@heisian By freezing the object I did not mean making the configuration-object immutable, which is what Object.freeze is intended for. I would honestly appreciate it if you could collapse (with <detail>) or delete your comment because it takes a lot of vertical space in this discussion and is not relevant to this PR.

heisian · 2018-03-05T19:12:50Z

@codeclown deleted. I think the terminology you are looking for is to clone rather than to freeze.

I think that when creating a new Axios instance, it should freeze its own defaults in time

The above statement strongly suggests that you want to freeze the defaults. When you want one instance to not affect the other instance that is called passing by value - not freezing.

…stance-options

codeclown · 2018-03-09T12:04:45Z

@emilyemorehouse I went ahead and cleaned up this PR. There are a few questions above that I'd appreciate an answer to. I would also like to know if this PR is worth pursuing or if I should close it. Currently Travis fails due to some network error, I can spend time on resolving it if this PR has a future.

The functionality in this PR is similar to what has been agreed to a long time ago in the comments: #370

emilyemorehouse · 2018-03-09T16:02:59Z

@codeclown I'd say keep it open -- I think this approach would be preferred over #1391, since it takes into account many of the issues and proposed solutions from the previous discussions around this. Since this is an incredibly important issue, I'd love to have approval from at least @nickuraltsev and myself since he's been working on the project longer than I have.

Here's answers to your previous questions, let me know if you have more!

What is the leadership on this project at the moment? Who decides what goes into which version?

I believe that Matt and Ruben have stepped back, and Nick is helping out when he can. I stepped up recently and am still getting my footing (hence wanting another owner's approval for larger issues like this).
Otherwise, there's a solid plan for 1.0.0 and I deemed a few things critical for the 0.18 and 0.19 releases. I'm tracking everything in projects.

Is there any other way to fix the security issue other than by losing all references between global defaults and/or separate instances?

We could deem certain attributes of the config "critical", like the headers, and ensure that just those parts aren't shared between global config and instance configs (#1391's original approach). But I feel like that isn't ideal and ignores the bigger picture.

Also another thing just occurred to me. Some config-properties are arrays. Should they always lose reference between instances?

Oo, I do like the use case for references being kept for commonTransformers... but if everything else is solved and we lose this, then I think that's okay.

I've been interested in digging into how AWS handles this in their JS SDK- they have a similar idea of global configs and "instances" for each service, I've just been bogged down the past couple of weeks.

heisian · 2018-03-09T17:18:21Z

@codeclown @emilyemorehouse agreed after reading the threads referenced by @nickuraltsev This PR seems like the best route to go, or at least is most in-line with what was discussed in those threads. If we can get tests to pass we may be in good shape.

Personally I am not a fan of having different behaviours between different properties within the defaults object, but I suppose that's a compromise to make if going for the most backwards-compatibility.

I definitely agree on headers being critical, as it often contains Authorization data that shouldn't be shared between instances.

nickuraltsev · 2018-03-10T01:46:40Z

@codeclown First of all, thank you so much for working on this!

Here are some comments:

I really like the idea of freezing instance defaults. I think we on the right track.
Please move mergeConfig and friends to core. utils is a library of generic helper functions non-specific to axios as this comment suggests.
I have some other comments on the PR, but if you'd like to clean it up first, I'll be happy to review it when you are ready.

@emilyemorehouse We don't have to wait for the 1.0 release. It's ok to introduce breaking changes in 0.19, 0.20, etc. There is a warning here: https://github.com/axios/axios#semver

heisian · 2018-03-20T17:50:41Z

Still alive?

heisian · 2018-03-20T17:50:57Z

@nickuraltsev Could you just implement this going off of what's here?

nickuraltsev · 2018-03-21T16:57:23Z

@codeclown Hey, are you still interested in working on this PR?

codeclown · 2018-03-21T17:35:00Z

Yes, I'll try to address the comments some evening this week. Of course I have no issue if someone has the time and wants to do it before me.

…

On 21 Mar 2018, at 17.57, Nick Uraltsev ***@***.***> wrote: @codeclown Hey, are you still interested in working on this PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

codeclown · 2018-03-23T19:26:28Z

@nickuraltsev @emilyemorehouse Sorry for the delay on this. Changes in latest commit:

Moved mergeConfig from utils.js to core/mergeConfig.js
Implemented new deepMerge alteration of merge, and used it in place of my original JSON.parse(JSON.stringify(...)) work-around
- (I'm surprised there are no unit tests for these internal functions.)

Please let me know if there's any refactoring or other changes you'd like to see.

nickuraltsev · 2018-03-24T04:20:04Z

@codeclown Thank you so much! I'll review it over the weekend.

nickuraltsev · 2018-04-02T15:09:40Z

lib/utils.js

+  var result = {};
+  function assignValue(val, key) {
+    if (typeof result[key] === 'object' && typeof val === 'object') {
+      result[key] = merge(result[key], val);


Show we use merge or deepMerge here?

nickuraltsev · 2018-04-02T15:53:33Z

lib/utils.js

+    if (typeof result[key] === 'object' && typeof val === 'object') {
+      result[key] = merge(result[key], val);
+    } else if (typeof val === 'object') {
+      result[key] = merge({}, val);


deepMerge?

nickuraltsev · 2018-04-02T16:05:43Z

lib/core/mergeConfig.js

+ * @param {Object} instanceConfig Instance-specific config
+ * @returns {Object} New object resulting from merging instanceConfig to defaults
+ */
+module.exports = function mergeConfig(defaults, instanceConfig) {


Nitpicking: I'm not sure about the parameter names (defaults and instanceConfig). mergeConfig is also used to merge the request config with the instance config in the request method so I would suggest to rename them to config1 and config2.

nickuraltsev · 2018-04-03T15:08:13Z

lib/core/mergeConfig.js

+  });
+
+  utils.forEach(['headers', 'auth', 'proxy'], function mergeInstanceConfigWithDefaults(prop) {
+    if (typeof instanceConfig[prop] !== 'undefined') {


As far as I can see, it's safe to pass undefined or null to utils.deepMerge (correct me if I'm wrong), so we can probably simplify this block as follows:

if (defaults[prop] || instanceConfig[prop]) { config[prop] = utils.deepMerge(defaults[prop], instanceConfig[prop]) }

I actually was not able to simplify this at all. If I got rid of explicitly checking for undefined and just did ||, the tests would fail. I assume something somewhere relies on null values and that broke it. If absolutely necessary, I can try to dig deeper into this later. I do agree that the if-statements are nasty.

nickuraltsev · 2018-04-03T15:12:43Z

lib/core/mergeConfig.js

+  var config = {};
+
+  utils.forEach(['url', 'method', 'params', 'data'], function valueFromInstanceConfig(prop) {
+    config[prop] = instanceConfig[prop];


I would suggest to add if (typeof instanceConfig[prop] !== 'undefined') check here so we don't create entries like data => undefined, etc.

Also, please see my comment about parameter names.

nickuraltsev · 2018-04-03T15:58:00Z

lib/utils.js

+ * @param {Object} obj1 Object to merge
+ * @returns {Object} Result of all merge properties
+ */
+function deepMerge(/* obj1, obj2, obj3, ... */) {


It would be great to add some tests for deepMerge.

I didn't notice that unit tests were in specs folder. I've now copied the merge tests and added two additional ones.

nickuraltsev · 2018-04-03T17:00:40Z

lib/core/mergeConfig.js

+    }
+  });
+
+  utils.forEach([


Instead of listing all these properties here, we could apply this function to all properties that don't belong to the first two groups. Or maybe, it's actually a great idea to specify all properties explicitly so that every time a new property is added, we will have to think whether its values should be merged or not. What do you think?

I would keep this explicit listing, that was also partly the idea behind this config-specific merge-function. The idea is that it's no longer a mystery where config-options originate from, and as you said, when adding new properties it forces to consider how they should be merged (or not).

nickuraltsev · 2018-04-03T17:04:31Z

@codeclown Sorry for delay! I've just submitted my review. The PR looks great. There are just a few minor issues / questions.

codeclown · 2018-04-03T20:35:57Z

@nickuraltsev No problem. Thank you for the review, much appreciated. I've addressed your comments. Please let me know if there's anything to change still. I also don't mind squashing these commits into one before merging if that's preferred.

I still don't know why the tests are failing in CI, because I haven't unfortunately had time to dive into the test setup. Apparently some local server instance is not responding as expected. I will save tinkering with that to last, after the PR is otherwise OK to go. If you have an idea about it, would be cool to hear.

nickuraltsev · 2018-04-06T17:33:20Z

lib/core/mergeConfig.js

+
+/**
+ * Config-specific merge-function which creates a new config-object
+ * based on given defaults and instance config.


Could you please update the comment so it doesn't suggest that this function can be used only to merge defaults with an instance config?

nickuraltsev · 2018-04-06T17:42:38Z

lib/core/mergeConfig.js

+  var config = {};
+
+  utils.forEach(['url', 'method', 'params', 'data'], function valueFromInstanceConfig(prop) {
+    if (typeof config2[prop] !== 'undefined') {


I've just realized that we always want to use url, method, params, and data values from the request config (not from defaults or the instance config). It means that we should probably remove the if (typeof config2[prop] !== 'undefined') check. Sorry for the back and forth!

valueFromInstanceConfig name is misleading. valueFromConfig2?

nickuraltsev · 2018-04-06T17:47:44Z

lib/core/mergeConfig.js

+  });
+
+  utils.forEach(['headers', 'auth', 'proxy'], function mergeInstanceConfigWithDefaults(prop) {
+    if (typeof config2[prop] !== 'undefined') {


Can we simplify it like this?

function mergeValues(prop) { if (config1[prop] !== 'undefined' || config2[prop] !== 'undefined') { config[prop] = utils.deepMerge(config1[prop], config2[prop]); } }

It should be ok to pass null or undefined to deepMerge so I would just check if at least one of the config objects contains the prop.

I would also change the name to smth like mergeValues

nickuraltsev · 2018-04-06T17:49:11Z

lib/core/mergeConfig.js

+    'timeout', 'withCredentials', 'adapter', 'responseType', 'xsrfCookieName',
+    'xsrfHeaderName', 'onUploadProgress', 'onDownloadProgress', 'maxContentLength',
+    'validateStatus', 'maxRedirects', 'httpAgent', 'httpsAgent', 'cancelToken'
+  ], function defaultToInstanceConfig(prop) {


Could you please rename it to something like defaultToConfig2?

nickuraltsev · 2018-04-10T22:29:50Z

@codeclown Thank you for this PR!
@emilyemorehouse Thank you for looking into the issue with tests!

soederpop · 2018-04-19T01:00:21Z

This is amazing news. Thanks for fixing this. What is the plan for releasing this to npm?

emilyemorehouse · 2018-05-07T15:51:29Z

ATM merging this caused tests to fail on Travis (see https://travis-ci.org/axios/axios/branches). We'll be able to release as soon as that's fixed.

Dakkers · 2018-08-09T17:33:11Z

has this been shipped? we have to use a ghetto XMLHTTPRequest atm instead 😿

emilyemorehouse · 2018-08-09T17:41:29Z

@Dakkers the current build is still failing (even with cutting older IE versions) so I haven't released it yet 😔

emilyemorehouse · 2018-08-09T18:47:46Z

Released as part of 0.19.0-beta.1.

This can be installed using npm install axios@0.19.0-beta.1 or npm install axios@next

see axios/axios#1395

ps2goat · 2019-12-18T16:42:01Z

It looks like #2216 and #2207 are related to this change, and the fix is supposed to be in 0.19.1, which isn't released, yet.

I'll follow that, thanks.

Fixing axios#385 - Keep defaults local to instance

5bfd2ea

codeclown force-pushed the instance-options branch from ebd36a6 to 5bfd2ea Compare March 2, 2018 10:31

Merge branch 'master' into instance-options

0445d6c

heisian reviewed Mar 2, 2018

View reviewed changes

codeclown added 2 commits March 9, 2018 13:58

Clean up PR

d782047

Merge branch 'instance-options' of github.com:codeclown/axios into in…

d1154ea

…stance-options

Refactor and introduce deepMerge

72c66df

nickuraltsev suggested changes Apr 3, 2018

View reviewed changes

codeclown added 4 commits April 3, 2018 21:53

Use deepMerge in deepMerge

82030ae

Rename mergeConfig arguments

6083d63

Unit tests for deepMerge

506d4e8

Prevent undefined values in mergeConfig

b1c3786

soederpop mentioned this pull request Apr 4, 2018

Add "browser" property to package.json SumoLogic/js-sumo-logger#29

Closed

nickuraltsev suggested changes Apr 6, 2018

View reviewed changes

nickuraltsev merged commit ec97c68 into axios:master Apr 10, 2018

This was referenced Apr 10, 2018

instance.defaults changes global object #385

Closed

Stack overflow error when overwriting a default agent option #370

Closed

This was referenced Apr 11, 2018

Fixing #385 - instance.defaults changes global object #533

Closed

Fix issues with global and instance defaults #812

Closed

emilyemorehouse mentioned this pull request May 7, 2018

Instance defaults rewrite global defaults #1522

Closed

bkaney mentioned this pull request May 8, 2018

Fix axios bug Vermonster/fhir-kit-client#56

Closed

edgesite mentioned this pull request Aug 9, 2018

#1395 breaks custom config #1718

Closed

TomiTakussaari mentioned this pull request Sep 10, 2018

Retry is broken contentful/contentful.js#277

Closed

codeclown mentioned this pull request Oct 22, 2018

Case insensitive handling of headers #1291

Closed

Mechazawa mentioned this pull request Jan 9, 2019

new instance Influence the global instance #1664

Closed

TotomInc added a commit to TotomInc/discord-stream that referenced this pull request Jan 28, 2019

fix(discord-bot): upgrade axios@next to keep defaults local to instance

5bcc644

see axios/axios#1395

esimons mentioned this pull request Apr 25, 2019

Instance defaults example in README #2112

Closed

glebtv mentioned this pull request Jun 3, 2019

Default Authorization header no longer working in 0.19.0 #2199

Closed

codeclown mentioned this pull request Jul 19, 2019

Fixing custom config options #2207

Merged

chinesedfan mentioned this pull request Feb 26, 2020

Posting using an instance ignores setting method with config #2779

Closed

This was referenced Mar 24, 2020

Refactor mergeConfig without utils.deepMerge #2844

Merged

post data being over written with undefined when specified using axios.create #1584

Closed

axios locked and limited conversation to collaborators May 3, 2020

Uh oh!

Fixing #385 - Keep defaults local to instance #1395

Fixing #385 - Keep defaults local to instance #1395

Uh oh!

Conversation

codeclown commented Mar 2, 2018

Issue

Proposed solution

Implementation

Uh oh!

emilyemorehouse commented Mar 2, 2018

Uh oh!

codeclown commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

heisian Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codeclown commented Mar 5, 2018

Uh oh!

heisian commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codeclown commented Mar 9, 2018

Uh oh!

emilyemorehouse commented Mar 9, 2018

Uh oh!

heisian commented Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nickuraltsev commented Mar 10, 2018

Uh oh!

heisian commented Mar 20, 2018

Uh oh!

heisian commented Mar 20, 2018

Uh oh!

nickuraltsev commented Mar 21, 2018

Uh oh!

codeclown commented Mar 21, 2018 via email

Uh oh!

codeclown commented Mar 23, 2018

Uh oh!

nickuraltsev commented Mar 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickuraltsev commented Apr 3, 2018

Uh oh!

codeclown commented Apr 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codeclown commented Mar 2, 2018 •

edited

Loading

heisian Mar 2, 2018 •

edited

Loading

heisian commented Mar 5, 2018 •

edited

Loading

heisian commented Mar 9, 2018 •

edited

Loading

emilyemorehouse commented Aug 9, 2018 •

edited

Loading