Repository navigation
New fork resolving vulnerabilities and incorporating most current PRs #224
Copy link
Copy link
Closed
Labels
Description
Hi,
As issues had not received feedback here and the latest commit 3 years ago, I went ahead to make a fork and publish it as @brettz9/node-static.
Besides making a few of my own changes:
- (Breaking change) npm: Set
enginesto 10.11.0+ (allowing native URL to fix an issue and better flexibility in language features) - Security Update/fix: Use
URLconstructor over deprecatedurl.parse;
should fix Open Redirect issue https://www.npmjs.com/advisories/1207 - Optimization: 'use strict' directive
- Refactoring: Use safer non-prototype version of
colors - (Also some plain, dev-facing changes; see our
CHANGES.md)
...the fork also incorporates the following, indicating also the PR numbers here that they close:
User-facing
- Security: Fix dependency vulnerabilities by switching from
optimistto
neodoc(@fidian); Fix vulnerabilities found with npm audit #222 - Security update:
mimeandcolors(@fidian); Fix vulnerabilities found with npm audit #222 - Security update/fix: Protect
fs.statcalls from bad path arguments; fixes
Denial of Service issue https://www.npmjs.com/advisories/1208
(@brpvieira); Protect fs.stat calls from invalid path arguments #223; also avoids need for Prevent DoS attack #213 - Fix: Support
bytes=0-0Range header (@prajwalkman); Properly handle "bytes=0-0" range header #167 - Fix: Avoid octal (@bgao / @Ilrilan); Fix issue of 'Octal literals are not allowed in strict mode' #187, Fix: octal literals are not allowed in strict mode #215 ; also avoids need for Octal literals are not allowed in strict mode, passed string to parseInt method #155
- Fix: For
spa, allow dots after path (@gjuchault); fix(spa): parse URL before matching files for 404 #204 - Enhancement: Allow access with local ip (@flyingsky); fix bug, cannot access with local ip #139 #140
- Enhancement: Allow
serverInfoto benull(@martindale); Allow Removal ofServerHeader #150 - Enhancement: Time display logging with leading 0 (@mauris); added zero leading time display for console log #154
- Enhancement: Respect static
--cache 0(@matthew-andrews); Respect static --cache 0 #138 - Enhancement: New option:
defaultExtension(@fmalk); New option: defaultExtension #173 - Enhancement: Added glob matching for setting cache headers (@lightswitch05); Added glob matching feature for setting cache headers. #183
- Docs: Fix header example (@emmanouil); fixed "headers" example in README #156
- Docs: Sp. (@EdwardBetts); correct spelling mistake #194
Dev-facing
- Testing: Allow tests to end (@fmalk); New option: defaultExtension #173
I also made some updates/improvements to the PRs:
- Expanded the
fs.statchecking, adding one beyond that covered in the originalfs.statPR (Protect fs.stat calls from invalid path arguments #223), and covering the newly-added one in thedefaultExtensionPR (New option: defaultExtension #173). - Updated
minimatch(Added glob matching feature for setting cache headers. #183) - I avoided the Travis addition, as figured might use GitHub Actions if someone wants to do so.
These remaining prexisting PRs were not fully incorporated:
- [#188] Allow disabling of cache #189 - the PR for Respect static --cache 0 #138 allowed for disabling of cache already; if you still want the
fandfalsealiases, feel free to file an issue - read content-type from response #184 - Looked like there were concerns
- Improvements for README. #177 on README improvements; I figure some would be good, but would like to continue showing output and keeping headings (useful in navigation for users of HeadingsMap type browser add-ons, as well as for accessibility in general)
- Renaming
statictonodeStatic#172 - There is no longer a need for avoiding the reservedstatickeyword, as I renamed the examples (to usestatik). - [writeHead] survive if http.serverResponse.writeHead() re-defined #166 - I guess we could protect overwrites to
writeHead, but what's to prevent someone from rewritingsetHeader? If it's a common enough use case to overwritewriteHead, I could add the preventative measure, esp. with a test.
Remaining steps:
- The Unauthorized File Access issue https://www.npmjs.com/advisories/1206 does not appear to be an issue per testing (if it ever was); if you can provide a test case where it fails, please report
- I've added
nycfor coverage, but I'm not sure that withvows, we can do binary file testing. I'm thinking whether we should switch tomochafor this (I prefer that to jest for the ecosystem). Ideally we'd get to full coverage, including the binary.
Reactions are currently unavailable
Activity
tchakabam commentedon May 20, 2021
not sure what @cloudhead s view on it is. (i have done stuff on this code, but am not a maintainer)
maybe he should officialise the lack of maintainership on this project?
if not hand over the repo here - I guess making clear this repo is not maintained anymore - so that other people who want to continue based on the work of this fork for example can gather crowd around them with a clear direction?
i guess anyone can just use the published fork dist packages, but some clear direction where this is going and who to follow or who not is the point @bacloud14 ?
cloudhead commentedon May 20, 2021
Hey, what would be everyone's preference here? The problem is I don't have the capacity to review such a big change. I'd be happy to pass on maintainership to someone. Does this fork have any breaking API changes? (besides the "engines" change)
brettz9 commentedon May 20, 2021
Thanks for your reply, @cloudhead .
For anyone wishing to review, most of the changes were a series of merges for PRs already here and some non-API-changing refactoring. But I understand there were a number of merges, and I created however inevitable diff noise with the refactoring.
If no one else is interested, I can accept, but knowing that:
nycto give us an idea of areas where we need coverage, we don't have full testing coverage, and I can't claim deep familiarity already with all the internals.No. It uses ES6+ features, so for someone ignoring the stated
enginesrequirement, it might not work for that reason as well as due to the dependency updates and additions having different Node requirements, but there should be no breaking API changes affecting users except those earlier Node users.Of course with all the changes, e.g., the CLI parsing engine switch, it's always possible there could be some subtle differences, but it's been working for me in my projects, and if there really was any breakage accidentally introduced, my intent would be to fix it.
cloudhead commentedon May 21, 2021
Ok, this sounds good. I would then propose that I give you write access to merge this fork into master, and make any fixes that may be needed in the future, given you have the time/capacity. We can then release this as version 0.8.
brettz9 commentedon May 21, 2021
Thanks for that, @cloudhead
I've made the tweaks I think I'm ready to make at this point, so if you want to add the 0.8.0 release, I think we'd be ready with these steps (which I mention given that they involve files which weren't there before):
package.jsonversionpackage-lock.jsonversionCHANGES.mdNote that there still is an advisory per https://www.npmjs.com/advisories/1206/versions which I wasn't able to replicate (on the contrary, it seemed already fixed), so barring any test case, I'm not sure what we can do there, but the release should at least address the other reported vulnerabilities.
brettz9 commentedon May 21, 2021
If you want, we could also bump
enginesto Node 12 now though as it has reached end-of-life.[-]New fork resolving vulernabilities and incorporating most current PRs[/-][+]New fork resolving vulnerabilities and incorporating most current PRs[/+]tchakabam commentedon May 24, 2021
Ok nice @cloudhead @brettz9 :)
I don't want to be the devils advocate here, but
made me remind that story 2-3 years ago (how fast we forget these things as well right ...?)
where some guy took over maintainership of a package from an overworked open-source dev
and eventually used it to inject a Trojan if I remember well, into some crypto-currency platforms backend
I am still not sure what the FLOSS & NPM communitys take on this is tbh.
tchakabam commentedon May 24, 2021
see
brettz9 commentedon May 24, 2021
There are no guarantees that even the regular owner of a project won't suddenly turn into a malicious person. One either has to:
eslint-plugin-jsdoc) is so likely to do so.)cloudhead commentedon May 24, 2021
I'm well aware of supply chain attacks.. but basically a big part of that responsibility is on users auditing their dependencies. I did my duty of making sure @brettz9 is a legitimate contributor to the open source community (which he clearly is), and so I think he will do a great job. I wouldn't have given write access to someone with no credentials or github history!
tchakabam commentedon May 25, 2021
Indeed @cloudhead you are right about all that, and @brettz9 is probably very good maintainer for this project! Happy you guys found each other :)
My remark was actually unspecific to you guys as a person or this situation, and I felt similarly it being my duty in such a case to remind all participants about needed trust concerns of such a handover. Which is done, and I agree everything seems all fine here 👍
brettz9 commentedon May 30, 2021
@cloudhead : I bumped to Node 12. Anything else you want for a release?
12 remaining items
brettz9 commentedon Jul 25, 2021
I've also added a GitHub Action file, if you wanted to enable workflows in the repo Settings, @cloudhead .
cloudhead commentedon Jul 25, 2021
@brettz9 please pay attention to your commit messages, eg. this doesn't work as a commit message: c8a003a
I'm trying to go through the history to fix everything but somehow it's causing issues..
brettz9 commentedon Jul 25, 2021
Ah, ok, apologies, I'm used to using my own format a lot of times. I think maybe
testshould betestingif you plan to usesemantic-release. I might also have left some line breaks but I don't thinksemantic-releasehas a problem with those even though some commit checkers do.cloudhead commentedon Jul 25, 2021
I'm ok if we don't follow semantic-release to the letter, but at least the commits should be formatted according to the way git works, ie. it shouldn't break
git log --onelinefor example, and the commit title and description should be clearly separated, otherwise it's hard to read for humans too.brettz9 commentedon Jul 25, 2021
I barely use the command line for Git, but will try to remember to adhere to it. Alternatively, it should be doable as per https://github.com/semantic-release/semantic-release#commit-message-format , to add commitizen or commitlint to enforce valid commit messages before they are committed (including possibly via husky).
cloudhead commentedon Aug 9, 2021
I've tried to clean up the history but unfortunately because of all the merges it's very very difficult to rebase (even with the help of some pretty advanced git commands). So I'm left with two choices:
Neither is ideal for me, but (1) would at least mark a clear separation between the old and the new node-static and it would be easy to follow the change. We would of course lose the history, but since it's the history that is not particularly usable as-is, I don't see that as a major downside.
Zarel commentedon Aug 9, 2021
@cloudhead I should be able to do it for you! Please just hold on a moment.
Zarel commentedon Aug 10, 2021
@cloudhead I did it! I converted all history since 2018 (i.e. everything @brettz9 pushed) into a linear tree. You can take my cleaned history with:
You can then go ahead and do an interactive rebase with
git rebase -i 83aac2eto clean the history to your liking.brettz9 commentedon Aug 22, 2024
@cloudhead : Hi... At this point, I'm looking to update my earlier fork (@brettz9/node-static) and give up on
node-staticproper—unless I hear back from you. Any chance of reviewing and publishing a new version?chore: some quality of life fixes (#5)
Zarel commentedon Jul 24, 2025
@brettz9 In my experience, it's not too hard to get NPM to give you access to NPM listings that belong to you. I got access this way once. Just e-mail them.
https://www.npmjs.com/support
I think the combination of "you are a collaborator" + "original owner is MIA" means they'd probably give it to you.
brettz9 commentedon Aug 15, 2025
@Zarel : I've sent a message since we haven't heard anything recent from @cloudhead .
Merge commit '9c9389b30caa43c3e2c6f64d5adcad47780b2cde'
brettz9 commentedon Oct 15, 2025
I'm closing this as #231 is dedicated to the need for an npm release. We can track there. Unfortunately, I wasn't given a wait time by GitHub (who is now handling npm ownership issues).