[fix] Do not create .htaccess with Nginx #226

magikcypress · 2017-04-28T15:47:56Z

I use editorConfig but I'm not sure for indentation ?

added by @rugk:

replaces PR #223
fixes #222

rugk · 2017-04-28T18:50:45Z

It should do that automatically. That's the whole purpose of this tool.

rugk

BTW: Now you did not revert any changes from master… 👍

rugk · 2017-04-28T19:59:48Z

lib/Persistence/AbstractPersistence.php

-            if (!@mkdir(self::$_path, 0700)) {
-                throw new Exception('unable to create directory ' . self::$_path, 10);
+        if (property_exists($data->meta, 'webserver') && $data->meta->webserver && $this->_conf->getKey('webserver') == "Apache") {
+            // Create storage directory if it does not exist.


AFAIK the storage dir (data dir) is not connected to the webserver you use. It should also be done when using nginx…

I moved the condition

rugk · 2017-04-28T20:00:36Z

lib/Configuration.php

@@ -54,6 +54,7 @@ class Configuration
            'icon'                     => 'identicon',
            'cspheader'                => 'default-src \'none\'; manifest-src \'self\'; connect-src *; script-src \'self\'; style-src \'self\'; font-src \'self\'; img-src \'self\' data:; referrer no-referrer; sandbox allow-same-origin allow-scripts allow-forms allow-popups',
            'zerobincompatibility'     => false,
+            'webserver'     => 'Apache',


We can fix the indentation afterwards (PHP-CS does it for us), but if you want, you can of course bring it in a nicely, order "table style"…

Thanks for changing it.

rugk · 2017-04-28T20:02:32Z

cfg/conf.ini.sample

@@ -75,6 +75,10 @@ languageselection = false
 ; sha256 in HMAC for the deletion token
 zerobincompatibility = false

+; allows you to specify the name of the web server you are using to use ParseBin.


Typo: "ParseBin"

Also a funny name, but was not our favourite… 😉

Hum ... Sorry again !

He, this was actually a funny one… 😄

rugk · 2017-04-28T20:03:05Z

cfg/conf.ini.sample

@@ -75,6 +75,10 @@ languageselection = false
 ; sha256 in HMAC for the deletion token
 zerobincompatibility = false

+; allows you to specify the name of the web server you are using to use ParseBin.
+; if you use Nginx, decommente and add nginx.


"decommente" does not exist, please use "uncomment" instead.

My french !

rugk · 2017-04-28T20:08:21Z

lib/Persistence/AbstractPersistence.php

-        if (!is_dir(self::$_path)) {
-            if (!@mkdir(self::$_path, 0700)) {
-                throw new Exception('unable to create directory ' . self::$_path, 10);
+        if (property_exists($data->meta, 'webserver') && $data->meta->webserver && $this->_conf->getKey('webserver') == "Apache") {


This now does not check the web server anymore as far as I see… It just assumes "Apache by default" and only if the admin changes the settings in the options it assumes "any other web server".

However, I'd rather like to see this behaviour:

By default the config is either empty or set to "auto" or so, which auto-selects/detects the correct server based on env. variables e.g.

The server admin can override this behaviour by changing the option in the settings, so can e.g. force Apache or disallow it. This could be useful when the automatic detection does not work…

I understand how you want this to happen, i will see what I can do.
But I do not know if it's a best way ? I try.

If you have a better idea, just explain it…
Comments are there to discuss things. IMHO "autodetection" as it was (partly) implemented in your first PR is a good idea.

rugk · 2017-04-28T20:11:26Z

lib/Persistence/AbstractPersistence.php

-                throw new Exception('unable to write to file ' . $file, 11);
+            $file = self::$_path . DIRECTORY_SEPARATOR . '.htaccess';
+            if (!is_file($file)) {
+                $writtenBytes = @file_put_contents(


It would be awesome if you could also detect the server version of Apache as Apache <2.4 needed a different format. So if you want to solve #194 (comment).

Hint: ce92bfa#diff-bb97f7ec0a7c6ca7a5f672f80703358cL307

rugk · 2017-04-30T20:09:54Z

Thanks for the fixes. Still TODO:

detect server version [fix] Do not create .htaccess with Nginx #226 (comment) (if you want)
autodetection of server [fix] Do not create .htaccess with Nginx #226 (comment)

see PrivateBin#227

elrido · 2017-05-22T19:21:55Z

Thanks for working on this!

The logic of the WebServer class looks quite clean, with my only suggestion being to maybe change the fixed version regex compare to the version_compare one, so it works with future apache versions, too.

Note that .htaccess files may need to be created in various places, not just in the web root, so there should be a mechanism to pass the absolute path into the $file in canHtaccess().

The name canHtaccess() is also a bit misleading. The "can" prefix would indicate that it is a test and returns a boolean, while it actually just writes a file, if needed. How about a name that more clearly describes its function? In the end it abstracts a mechanism to restrict access across various webserver types, so something like restrictAccessTo($path) would semantically make more sense.

That aside, I can write the unit tests for this before merging it, if you want.

magikcypress · 2017-05-22T19:29:31Z

Yes, is a good idea for change name for this function. restrictAccessTo() is a good name for me.

I also think that I have written badly name of my WebServer function. Would it be more logical to write it ServerWeb as for ServerSalt? if that's okay, I'll rename the file too.

I would like to write the tests, but I can't get them to work properly. I want you to write them;)

elrido · 2017-05-22T20:50:24Z

Okaydoky on the tests, WebServer as class name is fine by me, as we usually do call these objects "web servers". ;-)

PS: I am now starting to reconsider renaming ServerSalt to simply Salt, since that class is now used to generate the paste salts, too, and not only for the instance wide salt.

magikcypress · 2017-05-22T21:55:47Z

i will add $path for a restrictAccessTo() function, but for the moment, i don't know how to added neatly into lib/Model/Paste.php

rugk · 2017-08-10T20:12:18Z

@magikcypress Any chance you can have a look at finishing the PR?

magikcypress · 2017-08-15T16:56:30Z

I don't know, i haven't a lot anytime for patch this PR, sorry.

In fact, i don't know how get a variable $_path in my code for to be used a WebServer class anywhere ?

rugk · 2022-07-29T09:08:48Z

@magikcypress Are you still interested in completing this? If not, that's no problem, just add a notice, so I know what the current status of this PR is.

rugk

Hmm this only creates the htaccess file is you explicitly configure PB and state in the config file that you use Apache,

This PR is abanded anyway, but I also would not merge it as per today I guess, because:

that automatic .htaccess is a security feature, mostly intended for sysadmins that are quite new to webservers or just forget stuff like doing any hardening steps.
as such, it is an "automatic breach prevention feature", because if you do not properly secure the server otherwise, it will do it by itself

This is not only theoretical. Back in the days we actually had a vulnerability similar to that and decided to publish a "fixed" version that essentially improved our security procedures by switching to .php files there.
Well anyway… you can read that there…, but the point is: This is one layer of defense against insecure misconfigured servers. And if we can do sth. against that (within constrains), IMHO, we should.

So what?

So given this all applies, I think this change here would decrease the security of the whole system, because this system here is especially aimed at users, who may not even take a look into the config file.
Note that most other stuff of PB may work more or less out of the box (correct me if I am wrong), so they may actually forget or (deliberately) just not see any reason to adjust the config or that single server name thing. I mean, why should they?

If that is the case, they will not adjust it, anyway run it on Apache and have a potentially unprotected or less protected instance.

Advantage

So to way that against the advantage of this PR that is just…

to have a clean file system without that file?

I mean that file is just a file, it may disturb, but really, if that is the only disadvantage of not merging this…

rugk

While writing (rubber duck says hi) it just came to mind that one single change that I do not really see here would probably fix all my concerns:

Make the config (/our defaults) for that value default to "Apache", i.e./and thus to creating that file.

Because then we have the out-of-the-box (OOTB) security while for nginx or so the file does not need to be generated if people do not want that and e.g. if it may cause deployment issues.

So that is why I leave that PR open.

[fix] Do not create .htaccess with Nginx

9008712

rugk requested changes Apr 28, 2017

View reviewed changes

[fix] a little bit error

e64b0bb

rugk and others added 3 commits May 20, 2017 18:59

Add note about PostgreSQL

7fa4e74

see PrivateBin#227

Corrections and missing translations

8994a35

[enh] Create .htaccess into data PrivateBin#222

f23b122

[fix] compare version apache && ServerSalt namespace

9eea316

[fix] rename function canHtaccess by restrictAccessTo

fe24371

Merge branch 'master' into master

861dd34

PrivateBin deleted a comment Oct 27, 2017

elrido added this to In progress in Release 1.3 - Review & refactor paste format via automation Jun 15, 2019

elrido added this to the Release 1.3 - Review & refactor paste format milestone Jun 15, 2019

elrido removed this from In progress in Release 1.3 - Review & refactor paste format Jul 9, 2019

elrido removed this from the Release 1.3 - Review & refactor paste format milestone Jul 9, 2019

elrido added the help wanted label Feb 2, 2020

rugk reviewed Aug 7, 2023

View reviewed changes

rugk requested changes Aug 7, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] Do not create .htaccess with Nginx #226

[fix] Do not create .htaccess with Nginx #226

magikcypress commented Apr 28, 2017 •

edited by rugk

rugk commented Apr 28, 2017 •

edited

rugk left a comment

rugk Apr 28, 2017

magikcypress Apr 29, 2017

rugk Apr 28, 2017

magikcypress Apr 29, 2017

rugk Apr 30, 2017

rugk Apr 28, 2017

magikcypress Apr 29, 2017

rugk Apr 30, 2017

rugk Apr 28, 2017

magikcypress Apr 29, 2017

rugk Apr 28, 2017 •

edited

magikcypress Apr 29, 2017 •

edited

rugk Apr 30, 2017

rugk Apr 28, 2017

rugk commented Apr 30, 2017

elrido commented May 22, 2017

magikcypress commented May 22, 2017

elrido commented May 22, 2017

magikcypress commented May 22, 2017 •

edited

rugk commented Aug 10, 2017

magikcypress commented Aug 15, 2017

rugk commented Jul 29, 2022

rugk left a comment •

edited

rugk left a comment

[fix] Do not create .htaccess with Nginx #226

Are you sure you want to change the base?

[fix] Do not create .htaccess with Nginx #226

Conversation

magikcypress commented Apr 28, 2017 • edited by rugk

rugk commented Apr 28, 2017 • edited

rugk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rugk Apr 28, 2017 • edited

Choose a reason for hiding this comment

magikcypress Apr 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rugk commented Apr 30, 2017

elrido commented May 22, 2017

magikcypress commented May 22, 2017

elrido commented May 22, 2017

magikcypress commented May 22, 2017 • edited

rugk commented Aug 10, 2017

magikcypress commented Aug 15, 2017

rugk commented Jul 29, 2022

rugk left a comment • edited

Choose a reason for hiding this comment

So what?

Advantage

rugk left a comment

Choose a reason for hiding this comment

magikcypress commented Apr 28, 2017 •

edited by rugk

rugk commented Apr 28, 2017 •

edited

rugk Apr 28, 2017 •

edited

magikcypress Apr 29, 2017 •

edited

magikcypress commented May 22, 2017 •

edited

rugk left a comment •

edited