@geekdenz opened this Pull Request on August 15th 2021 Contributor

fixes #17577

Description:

Some uncertainties in this PR:

  • global.php seems to always set path.tmp, not sure it will be outside docroot and require the check
  • translations are not implemented yet
  • unit tests required and easy and possible?

Review

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@Findus23 commented on August 15th 2021 Member

I'm not sure what the advantage of splitting everything into two classes is. Couldn't you just return DiagnosticResult::STATUS_WARNING for the ones you consider recommended?

Also I am not sure if I agree that the lang/en.json check is less important than the tmp/cache/tracker/matomocache_general.php one: Of course sharing the lang/en.json itself doesn't matter. But it is a very good indicator that the .htaccess files are not working (as they protect lang/en.json) and therefore more important files are also public.
And it is more reliable to check as we know the content of lang/en.json and therefore if the HTTP response contains 12HourClock we know for sure that the file is public (checking the content was my original idea in the check I implemented in https://github.com/Findus23/matomo-DiagnosticsExtended/blob/main/Diagnostic/URLCheck.php).

@tsteur commented on August 15th 2021 Member

@Findus23 we definitely don't want to show a big security warning when lang/en.json is accessible especially because that likely means they are not using apache etc. Are you saying we should move tmp/cache/tracker/matomocache_general.php" to the not required check? If there are more important files, may be good to create an issue for these to add them? (as long as it's not too many as things can be slow). That might be good in a different issue though.

@Findus23 commented on August 15th 2021 Member

Isn't this check not mainly a reminder for people not using Apache (and those using Apache with .htaccess disabled) that they need to manually take care of protecting files?
I'm not sure what is exactly in the cache and if there is something in those files that might help attackers.
Then again it isn't that important what exactly we check as long as it points people that they should check their setup.

@geekdenz commented on August 16th 2021 Contributor

I have implemented translation and added it for de and en. Not sure the wording is perfect but should give an impression.

Also, I noticed that the translations are handled differently in the other code. Is this a legacy issue? Should I change that?

See https://github.com/matomo-org/matomo/blob/m-17577/plugins/Diagnostics/Diagnostic/RequiredPrivateDirectories.php#L40 for example.

@geekdenz commented on August 16th 2021 Contributor

@geekdenz could we refactor this code so that we don't have duplicate code in both classes? Maybe one class could extend the other or so.

Agree. It should be refactored. I was thinking to provide an abstract class that both extend.

Also I think German translations we cannot provide directly in Github as they are managed through https://www.transifex.com/matomo/matomo/ by translators. In Github we only provide default translation in English.

Oh! Noted. Should I revert that change?

@tsteur commented on August 16th 2021 Member

Oh! Noted. Should I revert that change?

That be good.

@justinvelluppillai commented on August 25th 2021 Contributor

@geekdenz you need to fix this PR before requesting review - it's got the same problem we fixed on another one the other day with your merge gone wrong.

@tsteur commented on August 30th 2021 Member

@geekdenz I just started testing and was checking first thing the config file which wasn't working last time. It looks like it's still not working. This is what I'm getting when the config file is not writable. It says the above URLs are having issues but there is no URL. I won't continue testing further for now.

image

@tsteur commented on August 31st 2021 Member

I haven't tested again but I think it looks good to merge if there are no tests failing because of this.

@geekdenz commented on August 31st 2021 Contributor

Not sure if these failing tests are related to this issue's fix, @tsteur :

https://builds-artifacts.matomo.org/matomo-org/matomo/4.x-dev/48437/

@justinvelluppillai commented on August 31st 2021 Contributor

Looks like those are related to the plugin updates last night @geekdenz.

@GermanKiwi commented on August 31st 2021

Hi @tsteur and @geekdenz, I'm still puzzled by the fact that I'm getting this private directories warning for my /lang/en.json file, even though I actually am running on Apache and there is a valid .htaccess file in the /lang directory.

I wrote about this earlier here:
https://github.com/matomo-org/matomo/issues/17577#issuecomment-843613876

As I understand it, your patch will simply mark this check as recommended, rather than required, but it seems to me that the underlying issue itself won't be resolved on my site, and I'd really like to figure out why Matomo says this file is exposed in my site, even though I'm running on Apache and have the .htaccess file there.

Can either of you help with this?

@tsteur commented on August 31st 2021 Member

Hi @GermanKiwi could you maybe paste the content of that .htaccess in the comment below and also let us know what version of Apache you are using?

Cheers

@GermanKiwi commented on August 31st 2021

Hi @GermanKiwi could you maybe paste the content of that .htaccess in the comment below and also let us know what version of Apache you are using?

Cheers

Sure thing. Below is the contents of the .htaccess file inside the /lang directory. My site is running on Apache 2.4.41.

# This file is auto generated by Matomo, do not edit directly
# Please report any issue or improvement directly to the Matomo team.

# First, deny access to all files in this directory
<Files "*">
<IfModule mod_version.c>
    <IfVersion < 2.4>
        Order Deny,Allow
        Deny from All
    </IfVersion>
    <IfVersion >= 2.4>
        Require all denied
    </IfVersion>
</IfModule>
<IfModule !mod_version.c>
    <IfModule !mod_authz_core.c>
        Order Deny,Allow
        Deny from All
    </IfModule>
    <IfModule mod_authz_core.c>
        Require all denied
    </IfModule>
</IfModule>
</Files>
@tsteur commented on August 31st 2021 Member

@GermanKiwi could you try to access https://yourmatomodomain.com/lang/en.json yourself and see if content is returned or maybe even what HTTP response code it returns? (you need to replace the domain with the domain/path for your Matomo install)

From our perspective things look good and should work. I would actually recommend you get in touch with SiteGround as something doesn't look right there. But let's double check first there isn't an application error. Be great to know if you can request that file or not.

@GermanKiwi commented on August 31st 2021

@GermanKiwi could you try to access https://yourmatomodomain.com/lang/en.json yourself and see if content is returned or maybe even what HTTP response code it returns? (you need to replace the domain with the domain/path for your Matomo install)

From our perspective things look good and should work. I would actually recommend you get in touch with SiteGround as something doesn't look right there. But let's double check first there isn't an application error. Be great to know if you can request that file or not.

Yeah, if I paste the URL for /lang/en.json into my browser (corrected for my own domain, of course), then it returns the full contents of that file in the browser. :(

And if I check the HTTP response (using https://www.webconfs.com/http-header-check.php) it returns HTTP/1.1 200 OK

@GermanKiwi commented on August 31st 2021

Incidentally, I only got a warning about this from Matomo after updating to version 4.3.0. Not sure if that's relevant.

@tsteur commented on August 31st 2021 Member

@GermanKiwi before that we didn't have this check this is why you are seeing it only now. I recommend you get in touch with your hoster and ask them. I would assume they should be able to help you there.

@Findus23 commented on September 1st 2021 Member

The most likely thing is that in the main apache configuration file of your server overriding settings via .htaccess files is disallowed and they therefore do nothing. (And if this is the case, this check is working as it notified you of this fact)

This Pull Request was closed on August 31st 2021
Powered by GitHub Issue Mirror