search

digitalocean / nginxconfig.io

⚙️ NGINX config generator on steroids 💉
https://do.co/nginxconfig
MIT License
27.66k stars 2.04k forks source link

feat: Block common exploits in Security #388

Closed treboryx closed 2 years ago

treboryx commented 2 years ago

Type of Change

What issue does this relate to?

Related to #353

What should this PR do?

This PR adds an option in the Security tab for blocking common exploits. Config can be found here

What are the acceptance criteria?

-

note: I had trouble linting this even with prettier disabled on my side. I'll be happy to fix it with the right instructions

treboryx commented 2 years ago

👋 Would it be possible for you to revert all the unrelated formatting changes in here -- it makes the diff very difficult to parse.

hey Matt! I had trouble linting as I mentioned above. Is the linting rules + command not supposed to work properly?

MattIPv4 commented 2 years ago

👀 The linting should definitely be working properly, the CI is able to run it just fine. It's just regular eslint + stylelint.

MattIPv4 commented 2 years ago

The diff here certainly looks like you've run prettier against it, which isn't tooling this project uses.

treboryx commented 2 years ago

I have manually reverted styling changes made to previously existing code.

Linting simply does not work for me, I run the command and although no errors are printed, it doesn't change the formatting. It doesn't work on my windows machine or mac, even with prettier completely disabled, or even on Codespaces

MattIPv4 commented 2 years ago

What do you mean by "it doesn't change the formatting"? Are you running test:eslint, or test:eslint:fix? Eslint will only fix things that it thinks are violations -- it's not like prettier where it formats your entire project into one very specific style (unless you configure all the rules to do that)

treboryx commented 2 years ago

What do you mean by "it doesn't change the formatting"? Are you running test:eslint, or test:eslint:fix? Eslint will only fix things that it thinks are violations -- it's not like prettier where it formats your entire project into one very specific style (unless you configure all the rules to do that)

Initially test:eslint:fix but I spend some time trying to figure out why this is happening (cause I just wanted to meet the project's styling) and tried all of them too.

treboryx commented 2 years ago

Here's a screenshot of what happens when I run test:eslint to detect violations and then test:eslint:fix

image
MattIPv4 commented 2 years ago

Yah, looks to be running just fine then, eslint sees no violations.

PDowney commented 2 years ago

I'm not sure that any of these rules are appropriate for this tool. They don't appear to have any immediate or long-term value. These rules appear to be a copy/paste of stuff that has been floating around for a decade or more and could introduce many more issues than they solve. A quick google search shows these same rules in a thread from 2013 found here: https://serverfault.com/questions/563248/nginx-security-vs-conditionals?rq=1. Who knows exactly how old these rules are, that's just what I found on the front page of the search results. To say they are outdated would likely be a tremendous understatement.

Some of the blocked query strings in the "Block spam" section include gems such as huronriveracres, troyhamby, and sandyauer. Google returns absolutely nothing of value for any of those strings that would suggest the need to block them for all users. My best guess is that a decade or so ago, a single server admin was trying to stop some very specific text from his website and shared these rules someplace where it got passed down as sacred lore that all users should employ, which it is not. It's possible that these specific words are actually just placeholders too, who knows. All I know is that blocking them is useless and makes zero sense. Many of the other strings include common words that not all users would wish to block and may not skim through the code enough to realize what they've done. These words include medical terms such as ejaculation, impotence, libido, and erectile. While these words may be common in spammers trying to leave comments on blogs, they are also medical terms that can have legitimate usage on a variety of websites.

The rules to "Block SQL Injections", "Block file injections", and "Block common exploits" are so dangerous to just blindly enable everything, as they most definitely will break things on modern web applications. Totally inappropriate for inclusion here.

The "Block user agents" rules are no better, as they are wildly out of date. This is useless stuff.

It is my opinion that this tool is not the correct place for such generic rules to be employed, as there are existing software and service solutions that will do a much better job at keeping things safe, secure, and most importantly up-to-date without breaking stuff.

My suggestion is that this entire pull request be denied.

PDowney commented 2 years ago

@MattIPv4 Please refer to this comment. This PR should not be implemented IMO.

https://github.com/digitalocean/nginxconfig.io/pull/388#issuecomment-1264958298

treboryx commented 2 years ago

@MattIPv4 Please refer to this comment. This PR should not be implemented IMO.

#388 (comment)

Hi there! Matt wouldn't be able to miss the wall of text you've sent here even if he wanted to.

Also on a personal note, I would very much appreciate your input during the "issue" phase (issue #353) before anyone allocates personal time to implement it. Thanks!

PDowney commented 2 years ago

@MattIPv4 Please refer to this comment. This PR should not be implemented IMO. #388 (comment)

Hi there! Matt wouldn't be able to miss the wall of text you've sent here even if he wanted to.

Also on a personal note, I would very much appreciate your input during the "issue" phase (issue #353) before anyone allocates personal time to implement it. Thanks!

Copy and pasting rules that you found on the web when that you seemingly haven't even bothered to review is not ideal or appropriate for a widely used application such as this. I'd love to hear your justification for the text in the query string blocks. Why did you include terms such as ypxaieo, unicauca, huronriveracres, sandyauer, and troyhamby? We both know why, you didn't look at them for even a second. The rules in this entire PR are not vetted at all, which is why I had to write a wall of text to try to prevent them from being blindly included in a very good application.

treboryx commented 2 years ago

@MattIPv4 Please refer to this comment. This PR should not be implemented IMO. #388 (comment)

Hi there! Matt wouldn't be able to miss the wall of text you've sent here even if he wanted to. Also on a personal note, I would very much appreciate your input during the "issue" phase (issue #353) before anyone allocates personal time to implement it. Thanks!

Copy and pasting rules that you found on the web when that you seemingly haven't even bothered to review is not ideal or appropriate for a widely used application such as this. I'd love to hear your justification for the text in the query string blocks. Why did you include terms such as ypxaieo, unicauca, huronriveracres, sandyauer, and troyhamby? We both know why, you didn't look at them for even a second. The rules in this entire PR are not vetted at all, which is why I had to write a wall of text to try to prevent them from being blindly included in a very good application.

Peter, there's absolutely no reason to attack me like that.

Issue #353 has been open for months and you can clearly see it's labeled properly for someone to just pick it and allocate time to implement it. This is why I kindly asked for your input during the "issue" phase next time

This config was not copy pasted randomly from the web, the source is listed, and if you ask me Nginx Proxy Manager is a popular tool.

PDowney commented 2 years ago

@MattIPv4 Please refer to this comment. This PR should not be implemented IMO. #388 (comment)

Hi there! Matt wouldn't be able to miss the wall of text you've sent here even if he wanted to. Also on a personal note, I would very much appreciate your input during the "issue" phase (issue #353) before anyone allocates personal time to implement it. Thanks!

Copy and pasting rules that you found on the web when that you seemingly haven't even bothered to review is not ideal or appropriate for a widely used application such as this. I'd love to hear your justification for the text in the query string blocks. Why did you include terms such as ypxaieo, unicauca, huronriveracres, sandyauer, and troyhamby? We both know why, you didn't look at them for even a second. The rules in this entire PR are not vetted at all, which is why I had to write a wall of text to try to prevent them from being blindly included in a very good application.

Peter, there's absolutely no reason to attack me like that.

Issue #353 has been open for months and you can clearly see it's labeled properly for someone to just pick it and allocate time to implement it. This is why I kindly asked for your input during the "issue" phase next time

This config was not copy pasted randomly from the web, the source is listed, and if you ask me Nginx Proxy Manager is a popular tool.

And it's totally inappropriate for them to have pushed that .conf live without actually vetting the code. My apologies for not commenting on the issue thread, I just happened to see the PR and was curious as to what was being added and was horrified to see this stuff that's been floating around the internet for a decade.

MattIPv4 commented 2 years ago

@PDowney Thank you for your input on this and for voicing your concerns, though as Robert noted, in future, I'd ask that you word things in a way that is less focused on personally attacking someone who was just implementing an ask from an issue with a clear source for what was being implemented.

When this issue was originally filed in the project I'd taken a quick glance at the ask, saw that it was a file already being distributed by the proxy manager project, and so green-lit the issue.

I pulled down this PR last night to start testing the generated config locally and see how aggressive the blocking was, and I am inclined to agree with Peter here that the SQL/Injection blocks are likely to be very dangerous and break things they shouldn't, and some of the other blocks in here are weirdly specific and outdated.

@treboryx thank you for the work you put into this, and sorry that this wasn't caught in the original issue by myself or anyone else. I am going to close this PR out based on the discussion here, but I hope this was at least a useful learning experience for you with contributing to the project, working with eslint, etc. 💙