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.

Sign up for GitHub

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

Jump to bottom

Upgrade qs dependency #61

Merged
merged 1 commit into from
Dec 3, 2018
Merged

Upgrade qs dependency #61

merged 1 commit into from
Dec 3, 2018

Conversation

thornjad
Copy link
Contributor

@thornjad thornjad commented Sep 3, 2018

Previously required version contained a security vulnerability. See #59 and https://snyk.io/vuln/npm:qs:20170213

@thornjad
upgrade qs dependency
2d1149a 
Previously required version contained a security vulnerability. See flatiron#59

Fixes flatiron#59
@thornjad thornjad mentioned this pull request Sep 4, 2018
Closed
@BigBlueHat
Copy link
Collaborator

Same tests fail with or without this patch...so there's that...

Also, the devDependencies are rather old also, and npm audit reports 53 vulnerabilities with or without that patch (as the devDependencies also include the old qs among other old libraries):

[!] 53 vulnerabilities found - Packages audited: 173 (172 dev, 23 optional)
    Severity: 9 Low | 25 Moderate | 19 High

https://david-dm.org/flatiron/union?type=dev

@thornjad
Copy link
Contributor Author

I definitely noticed that with npm audit, and I was thinking of throwing in some other PRs upgrading them.

Speaking of tests, on the other hand, the union/header test hangs for me, are you getting the same?

@BigBlueHat
Copy link
Collaborator

Speaking of tests, on the other hand, the union/header test hangs for me, are you getting the same?

I did see that when running in bash (on Windows via mingw), but that (and the others) completed fine (though some failed) when running in cmd (also, obviously, on Windows 😄).

@thornjad thornjad mentioned this pull request Sep 18, 2018
Open
@thornjad
Copy link
Contributor Author

Yeah it hangs for me on zsh on Linux. Sounds like it's own issue, and like magic it's now #63

@BigBlueHat
Copy link
Collaborator

@indexzero can you take a look at this upgrade? It fixes a vulnerability that effects http-server also:
http-party/http-server#461

I'd like to avoid forking union (even in the short run), so having this handled at this layer would be best. However, we do need to handle this for the good of http-server (if no one else).

Cheers!
🎩

@hiddensanctum
Copy link

Are there any updates on this PR?

@shakeelmohamed
Copy link

@hiddensanctum looks like this is blocked on #63

@thornjad
Copy link
Contributor Author

thornjad commented Dec 1, 2018 •
edited
Loading

@hiddensanctum @shakeelmohamed we need @indexzero to take a look at this, I don't have the power to merge.

@shakeelmohamed
Copy link

@thornjad thanks, I've sent him a kindly worded email. Let's see if that helps make some progress 😃

@indexzero
Copy link
Member

@BigBlueHat made you an admin of this to support the work you do in http-server. From an http-server perspective union is an out-dated architecture that should be killed in favor of other HTTP libraries.

@indexzero indexzero merged commit 8c735a2 into flatiron:master Dec 3, 2018
@indexzero
Copy link
Member

Published in [email protected]. From a future perspectives @BigBlueHat – this should be removed from http-server. It is a pre-streams2 streaming buffered stream implementation (i.e. implementation that handles back-pressure) for HTTP.

In a streams3 world, it serves no purpose.

@BigBlueHat BigBlueHat mentioned this pull request Dec 4, 2018
Open
@thornjad thornjad deleted the prototype-override-vuln branch December 13, 2018 14:25
cpetrov added a commit to eclipsesource/tabris-js-cli that referenced this pull request Jan 29, 2020
@cpetrov
Migrate from union to connect
dd5692d 
Union is not actively developed anymore, see [1].

Union provides a middleware capability similar to connect [2] supporting
buffered streams and providing some convenience APIs on top of the
middleware capability. However, the stream functionality offered by
union is already available since Node.js v0.12 [3] and Tabris CLI does
not use most of the APIs union offered.

Migrate middleware handling to connect. Opt for connect instead of
express, since Tabris CLI does not use most of the extra features
express offers and express comes with significantly more external
dependencies (currently connect 4 vs express 30).

Error handling notes:
  * connect errors do not have a status field. Only log the error
    message.
  * connect does not handle 404 as an error as union did. Restore this
    behavior by using an extra middleware to catch all unhandled
    requests and treat them as errors.

[1]: http-party/http-server#138 (comment)
[2]: https://www.npmjs.com/package/connect
[3]: flatiron/union#61 (comment)

Fix #71

Change-Id: If502571e61f53098afd6cbfbfa66e8053691eeda
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thornjad @BigBlueHat @hiddensanctum @shakeelmohamed @indexzero