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

Replace event-stream with something secure and supported #315

Open
jeremywadsack opened this issue Feb 14, 2023 · 3 comments
Open

Replace event-stream with something secure and supported #315

jeremywadsack opened this issue Feb 14, 2023 · 3 comments
Labels
dependencies Pull requests that update a dependency file question Further information is requested

Comments

@jeremywadsack
Copy link

audit-ci depends on dominictarr/event-stream which was added in #97. The author stopped supporting the module in 2016 and there are suggestions of other packages here: dominictarr/event-stream#102 (comment). In 2018 the author archived the repo, months before it was added to this package as a dependency.

This means that audit-ci (that we use for security scanning) includes a dependency on a library that isn't currently maintained and is likely insecure. In fact, event-stream has had an issue with a supply chain attack (dominictarr/event-stream#115) that while resolved raised concerns (and poor etiquette) from the community.

@quinnturner
Copy link
Member

quinnturner commented Feb 15, 2023

Thanks for the report. I am well aware of the history of the dependency. Accordingly, I understand your sentiment and have been toiling with this for quite some time.

Ultimately, I am concerned about the current state of the application's security more than historical.

At this time, here's what I've gathered about the current state of the dependency. If I list anything inaccurate, please let me know.

  • only npmjs.com themselves have write access to publishing new versions of the dependency
  • we have pinned the version to exactly 4.0.1, the latest
  • it has gone under more scrutiny than probably 99.999999% of dependencies on the registry in its current form
  • security concerns such as prototype pollutions, code injections, directory traversals, or ReDoS have not been identified by the community
  • The underlying requirements of the package are static; the definition of JSON changing is not a concern. Thus, the needs for this type of package are "stable".
  • It is performant enough for the usage of this application.

With all that in mind, I'd argue that most alternative dependencies are less likely to be secure than jsonstream.

However, I would be willing to change the dependency if there are tangible benefits over the current version of jsonstream. These may include measurable performance improvements, bug fixes within the scope of audit-ci's usage, a larger test suite for coverage of security-focused issues (such as the types of issues I've identified above), a low number of reputable transitive dependencies by reputable authors, etc.

With the latest release of audit-ci, there are no known bugs related to the usage of jsonstream.

Changing dependencies brings a new set of risks. Updating for the sake of updating is not always what's best for security. It may be in this case, I'd like to be presented with tangible evidence though.

@quinnturner
Copy link
Member

With all that being said, here are some of the alternatives that I found:

https://github.com/uhop/stream-json

https://github.com/creationix/jsonparse

https://github.com/dscape/clarinet

https://github.com/jgranstrom/zipson

https://github.com/Faleij/json-stream-stringify

In my opinion, stream-json seemed like the most reasonable alternative based on the criteria I set out. I have yet to identify the performance difference.

@quinnturner quinnturner added question Further information is requested dependencies Pull requests that update a dependency file waiting on author Waiting on the author to respond labels Feb 17, 2023
@jeremywadsack
Copy link
Author

Thanks for helping me understand this better. I thought I responded last week but don't see that here in the thread, so apologies for the late reply.

Agreed that the current state of the security is more important than historical. I highlighted the issue with event-stream because it appears to be the last known action on that repo.

I guess I don't have anything better to offer and you've given me something to consider.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants