Skip to content

Update brace-expansion dependency to version 2.0.2 in yarn.lock #537

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

Closed
wants to merge 1 commit into from

Conversation

DmitryNekrasov
Copy link
Contributor

No description provided.

@DmitryNekrasov DmitryNekrasov self-assigned this Jun 16, 2025
Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb left a comment

Choose a reason for hiding this comment

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

I don't understand the point. yarn.lock, the way I understand this, is what we will download when working with the library, but it's not what our users will see. Am I mistaken?

Also, are our users actually somehow affected by the problem solved in this update? How can one trigger this? If this doesn't affect anyone, why not just keep the default?

@DmitryNekrasov
Copy link
Contributor Author

I don't understand the point. yarn.lock, the way I understand this, is what we will download when working with the library, but it's not what our users will see. Am I mistaken?

Also, are our users actually somehow affected by the problem solved in this update? How can one trigger this? If this doesn't affect anyone, why not just keep the default?

yarn.lock is the place where the Kotlin/JS tool‑chain pins every NPM package that is pulled while we build - both in CI and on every contributor’s workstation. As long as the lock file says brace‑expansion 2.0.1, that is what Node will download, even though the semver range in someone’s package.json would allow a newer version. In other words, the lock file is our single‑source‑of‑truth, so patching it is the only way to guarantee that all builds pick up the fixed 2.0.2 artifact.

The CVE behind Dependabot’s alert (CVE‑2025‑5889) is a regular‑expression DoS in the library’s expand() routine; a crafted brace pattern that looks like {a},,,,,…\0 can drive the regexp into backtracking and block the event loop. The maintainer fixed it with a one‑line negative‑look‑ahead and an accompanying regression test in this commit, and the advisory explicitly lists 2.0.2 as the patched build .

Do our end‑users call expand() directly? Probably not. But the vulnerable code is still delivered.

Keeping the lock file on a safe version serves three goals: it removes an avoidable availability risk in our own build pipeline, it silences automated vulnerability scanners that consumers run on the published artefacts, and it ensures that future refactors that do expose brace expansion cannot silently re‑introduce the CVE. The change is non‑functional and backward‑compatible, so the safest default is to update now and carry on.

@dkhalanskyjb
Copy link
Collaborator

Do our end‑users call expand() directly? Probably not. But the vulnerable code is still delivered.

If the vulnerable code is delivered (how?), just updating yarn.lock is not sufficient, we need to research the problem, warn our users to upgrade, and explain how exactly the user code is affected.

it removes an avoidable availability risk in our own build pipeline

What risk do you mean? We don't accept untrusted inputs during our builds. The build either passes everywhere or fails everywhere, there should be no variability between builds depending on the environment.

silences automated vulnerability scanners that consumers run on the published artefacts

Do published artifacts actually expose this internal yarn.lock? Do the published artifacts trigger a warning?

future refactors that do expose brace expansion cannot silently re‑introduce the CVE

We do not use this library in our Kotlin/JS implementation directly, so I struggle to imagine what refactorings could expose this issue.

Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb left a comment

Choose a reason for hiding this comment

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

The reason I'm opposed to making this change without a good reason is the first line in this file. It's autogenerated, and if there isn't a valid concern that makes that problematic, it's more straightforward to keep allowing the compiler to manage it. If there is a valid concern, then it is also a problem that probably affects others, not just us, and then we should report it.

@DmitryNekrasov
Copy link
Contributor Author

My main motivation for making this change was to avoid introducing any potential vulnerabilities, even the most minimal ones. I thought it was possible that some bank would decide to use our library, but then their security team would come along, look at our yarn.lock, see a library with a vulnerability, and simply refuse to use it, without understanding how it is used inside our library.

I think it is safe to make such an update, since the regexp change is the only change in 2.0.2 compared to 2.0.1.

Yes, you are right about the comment about automatically changing yarn.lock. But I did it not quite manually, but as Dependabot suggested:

Or, manually upgrade brace-expansion to version 2.0.2 or later. For example:
brace-expansion@^2.0.2:
  version "2.0.2"

In the kotlin-js-store/yarn.lock file, I replaced the line brace-expansion@^2.0.1: to brace-expansion@^2.0.2:, and ran:

./gradlew kotlinUpgradeYarnLock

Also, as I see, yarn.lock usually updates with Kotlin version update, so we can wait for that moment and close the issue for now as not having serious impact. What do you think @dkhalanskyjb?

@dkhalanskyjb
Copy link
Collaborator

@DmitryNekrasov, the thing is, I don't know if we should close the issue, I haven't looked into it. If there truly is a potential security vulnerability, we should fix it instead of closing it, of course. If there is no security vulnerability, then there is not much point to taking on the burden of maintaining yarn.lock on our own.

@DmitryNekrasov
Copy link
Contributor Author

@DmitryNekrasov, the thing is, I don't know if we should close the issue, I haven't looked into it. If there truly is a potential security vulnerability, we should fix it instead of closing it, of course. If there is no security vulnerability, then there is not much point to taking on the burden of maintaining yarn.lock on our own.

IIUC, the connection between kotlinx-datetime and brace-expansion is entirely through the build process, not runtime functionality. The dependency chain works as follows:
Kotlin/JS target of kotlinx-datetime uses the @js-joda/core and js-joda/timezone libraries for JavaScript date operations. During compilation, the Kotlin/JS Gradle plugin employs webpack for JavaScript bundling. Webpack relies on minimatch for file pattern matching operations, which in turn depends on brace-expansion for pattern expansion.
The risk profile for this vulnerability is low for runtime security. Since brace-expansion is not included in final application artifacts, end-user security remains unaffected. kotlinx-datetime itself contains no vulnerable code. So my suggestion is to treat this as a low-priority build toolchain issue rather than a security vulnerability in kotlinx-datetime itself.

@dkhalanskyjb
Copy link
Collaborator

Thanks for the research! In that case, this does not look like a kotlinx-datetime problem at all. At most, it's a Kotlin/JS Gradle plugin issue that should be solved exactly by upgrading its version. If this were an actual vulnerability, we could introduce a workaround for it, but as is, I propose to close the issue without changing our code and silence the false dependabot warning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants