Report High severity vulnerability in react-scripts 3.4.3 dependencies

[mariannekhanfour1]

After auditing my app a high vulnerability is detected in the package object-path dependency of react-scripts.
I tried to run an audit fix however I still got the issue 1 vulnerability requires manual review. See the full report for details..
I tried to fix it manually but react-scripts is forcing the use of version 0.11.4 and I need to update it to version 0.11.5 to fix the vulnerability.

React version:
npm version: 6.14.8
current version of react-scripts: 3.4.3

[eps1lon]

Thanks for the report.

The issue should be filed against facebook/create-react-app.

facebook/react is concerned with packages listed in https://github.com/facebook/react/tree/HEAD/packages

[gaearon]

Note there is no actual vulnerability here for Create React App users. This is a false positive.

[s2t2]

Shouldn't the fix be to update the dependency object-path to version 0.11.5 ?

https://www.npmjs.com/advisories/1573

[s2t2]

#9841

[rickbergfalk]

@gaearon While a false positive for create react app users, there is not always a means to flag it as such in various security tooling. Sometimes security scans are done by an entirely different team.

The reality is devs are going to be required to do something about this vuln.

I don't know about others, but I would very much appreciate frequent patch releases for create-react-app.

[gaearon]

As explained in the other issue, we can’t just easily cut patches for something that’s resolved upstream with a major version. Ideally what should happen is that the “vulnerable” project itself cuts a patch. Then everyone gets it automatically. It’s not sustainable for the projects deep in the tree to only fix these problems with major versions, putting the pressure on the projects above in the stack — which are neither truly affected nor have the means to do the update safely (because it is transitive).

[gaearon]

In other words. Say we have react-scripts > A > B > C@1.0.0 that is vulnerable. What tends to happen today is that C@2.0.0 gets released with the fix. Then everybody comes to the CRA repo asking us for an upgrade. This is in some cases possible but time-consuming (because we have to update the A > B chain while making sure we don’t pull in any breaking changes). In other cases it’s not possible at all — by definition C@2.0.0 means there was a breaking change, so it might also be a breaking change for B (e.g. dropping old Node support), thus it could be breaking for A, and so we can’t make a patch in CRA either!

What should happen instead is that you should go to the C package repository and ask them to cut C@1.0.1 with the fix. Not C@2.0.0. If there are multiple actively used majors, all of them need to be released with a patch. That’s responsible maintenance. Publishing a vulnerability fix as a major release is not responsible because it doesn’t help any of the users who are stuck on the previous major because they can’t afford the breaking change.

Now, of course, if the vulnerability is real, it deserves our energy to go to the C package maintainers ourselves or to work around them at the B level. But these are not real vulnerabilities so if your company picked a policy that they will treat false positives as real issues, I think it is fair that the work is on you to talk to the C and B and A maintainers. Once there is an acceptable version of A that is a patch change from what we have, we are happy to cut a patch on our side.

[rickbergfalk]

Yeah I understand that those situations are not ideal and difficult to deal with, but I don't know if that's the case for this one. Doesn't bumping resolve-url-loader in the linked PR resolve the issue?

In this situation, the A in react-scripts > A > B > C has a patch update meaning it shouldn't be introducing breaking changes.

I completely empathize with the difficulty of keeping up with all the transitive dependency updates in node.js project and not introducing breaking changes because maintainers don't update old versions. I'm kind of exhausted by it myself.

Do realize though that telling users of create-react-app its their problem to get a 4-levels-deep dependency updated isn't going to be ideal either.

At some point the effort required to work around the resistance to keep react-scripts updated will be larger than forking react-scripts, ejecting, or moving off to some other solution.

[gaearon]

Doesn't bumping resolve-url-loader in the linked PR resolve the issue?

I haven’t looked at that yet. My question is — is the patch to it really necessary? Why can’t it be solved at the lower adjust-sourcemap-loader level and be picked up automatically for everyone?

[gaearon]

In other words the patch should always be done at the deepest level possible. Then as long as the next layer above it is permissive (accepts patch differences) there is no extra churn for everyone above. So patching react-scripts is the last resort and makes sense only if the vulnerability is in its direct dependency. When it’s several layers down, it’s at the deepest level that needs to be patched.

[rickbergfalk]

Ideally, yes the fix would be deep in the tree. Realistically though, the immediate dependency is something the create-react-app project has control over.

If there's a patch update to a library react-scripts directly depends on, why hesitate to pull it in?

[gaearon]

Each release is an extra fixed amount of work and each patch risks breakages. I’m not strongly opposed to cutting another one but this sets a bad precedent because each next time we’re going to refer to a previous decision (“we cut a patch that time, why not this time?”). Since the tree is very deep this centralization is unsustainable. And I want to bring attention one more time that we’re talking about false positives rather than real issues. We are literally wasting each other’s time because we don’t have a strong ecosystem convention about how to correctly resolve this issue, and because tools like Snyk mislead users about their severity.

[gaearon]

Have you tried moving react-scripts to devDependencies? Does this fix the npm audit warning? This was suggested by a person who works at Snyk in another thread.

[gaearon]

Seems like that only helps with Snyk but not with npm audit.

[gaearon]

OK, it looks like resolve-url-loader was using an exact dependency (bholloway/resolve-url-loader@4702ae9) so cutting a patch at a lower level wouldn't have helped anyway in this case. I'll make a patch of react-scripts later today. I'm also going to unpin the react-scripts dependencies for 3.x so that we can get the latest fixes going forward as long as they're on the same minors.

[gaearon]

That said, I think we also need to introduce an explicit policy going forward that we're not going to take patches for false positives until all the means upstream have been exhausted.

[rickbergfalk]

Thanks Dan! I know node dependency management is never ending and frustrating, and I really appreciate it!

[Gfast2]

In Berlin, we can find painted berlin wall with paintings by spray-dose gang or some kinda offical place like the "east side gallery". The prev one are normally much much more creative & vivid then those officals'. Thing around node are those spray-dose gangs'.

[johannespfeiffer]

@gaearon good point. I've updated #9841 to unpin the dependency, so the PR now is 2 characters big 😄

Beside from that I've created a PR at bholloway/adjust-sourcemap-loader#18 which does nothing beside updating the npm audit problems and asked for a patch as well. We would still need to unpin resolve-url-loader for that of course.

[gaearon]

I bumped just resolve-url-loader alone in react-scripts@3.4.4.