791259 - Make it easier to debug TBPL / log php errors locally

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[Ed Morley [:emorley]]

Attachment: Patch v1

At the moment, both logging and display of php errors is disabled.

For display of errors, this makes perfect sense in production, however we should still enabled logging (and protect with .htacesss) to:
 (a) enable debugging of problems that only occur on tbpl-dev/prod, eg bug 790889, and 
 (b) save people working on TBPL from having to continually push/pop mq patches enabling logging in the htaccess when they are testing locally.

In addition, php-cli (obviously) skips over .htaccess, so we need an easy way for people to toggle display_errors etc.

This patch...

.htaccess:
* Enables logging of errors to logs/php_errors.log by default
* Denies access to files of name php_errors.log
* Sets error_reporting to show all errors (equivalent to E_ALL | E_STRICT)
* Switches on HTML markup of errors
* Fixes indentation

getLogExcerpt.php / config-cli.php:
* Extracts php directives for php-cli from getLogExcerpt.php into config-cli.php
* Adds the new directives from .htaccess to config-cli.php
* Cleans up some comments

Only things I'm still on the fence about:
* Name/location of the log (ie the relative path in htaccess isn't ideal)
* Naming of config-cli.php, since it is a bit more than a config, with the |parse_str($argv[1], $_GET);| (but then config.php does a bit more too, so this is probably fine).

Will get someone from IT to check this out from a security POV.

Oh and to deploy, we'll need to get a logs directory created on tbpl-dev and prod (though I guess we could just add an .htaccess or something in the repo, which would do this for us; albeit will double up on the "deny from all" entries in the root .htaccess in this patch).

Comment 1

[Ed Morley [:emorley]]

Attachment: Patch v2

Bah, qrefresh; sorry for spam.

Comment 2

[Ed Morley [:emorley]]

Oops, spotted that in the two places where I've added dirname(__FILE__).'foo', I forgot the forwardslash immediately before |foo|. 

Fixed locally; not going to upload again until I've had feedback on the rest :-)

Comment 3

[Ed Morley [:emorley]]

The cli version is also currently missing the following .htaccess directive, should we add it?
# Parsing logs needs lots of memory, so be generous
php_value memory_limit 2048M

In addition, the cli scripts have a max execution time of 30 seconds, those called from apache 10 mins. Should these be closer together?

Comment 4

[Arpad Borsos [:Swatinem]]

Comment on attachment 661234 [details] [diff] [review]
Patch v2

Review of attachment 661234 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay.
Not sure if we want to rather name this file config-cli.php.example like the normal config.php so we don’t got merge conflicts all the time...

::: php/config-cli.php
@@ +5,5 @@
> + */
> +
> +// Execution time is unlimited for CLI scripts, however we dont want to
> +// hold up the parent process for that long in case something goes wrong
> +ini_set('max_execution_time', 30);

We might want to remove this completely, since python kills the process for us anyway when it is running for too long.

Comment 5

[Ed Morley [:emorley]]

Attachment: Patch v3

Same as version 2, except:
* Removes ini_set('max_execution_time', 30); from config-cli.php
* Adds ini_set('memory_limit', "2048M"); to config-cli.php to match the entry in .htaccess

Regarding config-cli.php, I've not renamed it to config-cli.php.example, since:
* AIUI config.php mainly needs to do this due to the passwords contained within, whereas config-cli.php does not contain anything sensitive.
* I'd like to avoid adding more footguns if possible, by having another untracked (and invisible to us) file on tbpl-dev and production.
* Whilst devs may need to push/pop an mq with changes to |ini_set("display_errors", "On")| in config-cli.php (since the file will be tracked) - they would still need to do the same due to the equivalent value in .htaccess, so we're not making things worse. Also since logging is now enabled by default, devs don't even really need to change display_errors most of the time.

Is that ok with you?

I'll also get someone from IT to confirm the logging behind htaccess is ok from a security POV.

Comment 6

[Ed Morley [:emorley]]

Attachment: Patch v4

Just a few wording tweaks to added comments.

Comment 7

[Ed Morley [:emorley]]

Ben/Shyam, do the log_errors changes in the attached patch look ok to you from an IT POV? PHP error logging will then be enabled by default on tbpl-dev and prod, with the log file stored in logs/ and protected by the root .htaccess. PHP error display in the web output will remain disabled.

The logs directory will need creating on both - or else we could say pop an additional .htaccess (or other placeholder) file in there to avoid that step.

Comment 8

[Shyam Mani [:fox2mike]]

I'll CC opsec for their inputs on this.

Comment 9

[Ed Morley [:emorley]]

Opsec, ping for comments :-)

Comment 10

[Ed Morley [:emorley]]

Attachment: Patch v5

As before except logging is off by default, so I can land this now & enable the logging after opsec reply. (Several other patches in by queue need rebasing without this).

I've also moved the .htaccess protect php_errors.log part from the root htaccess to the one in the logs directory, to:
a) Save having to get someone to create the directory on tbpl-dev/prod.
b) Make it more obvious when looking at the logs folder that the files are protected

In order to hg add 'logs/.htaccess', I've also had to tweak .hgignore.

Comment 11

[Ed Morley [:emorley]]

Attachment: Patch v6

sigh... qref; sorry :-/

Comment 12

[Arpad Borsos [:Swatinem]]

Comment on attachment 663389 [details] [diff] [review]
Patch v5

Review of attachment 663389 [details] [diff] [review]:
-----------------------------------------------------------------

::: logs/.htaccess
@@ +1,4 @@
> +# Protect error log
> +<Files php_errors.log>
> +  order deny,allow
> +  deny from all 

trailing whitespace

Comment 13

[Ed Morley [:emorley]]

Comment on attachment 663392 [details] [diff] [review]
Patch v6

Trailing whitespace removed; good spot :-)

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/3e16da54b185

Comment 14

[Ed Morley [:emorley]]

Attachment: Enable php error logging

Patch pending opsec feedback

Comment 15

[Guillaume Destuynder [:kang] [this account is no longer active]]

a few opsec questions:

- why do we need a relative path for the log? what is the absolute path, and what is the DocumentRoot value? (and would it be an issue if logs were in /var/log/...host-php.log or a similar path?)

- is it possible to set this outside htaccess? (ie in the main config), if not why?

thanks!

Comment 16

[Ed Morley [:emorley]]

(In reply to Guillaume Destuynder [:kang] from comment #15)
> a few opsec questions:
> 
> - why do we need a relative path for the log? what is the absolute path, and
> what is the DocumentRoot value? (and would it be an issue if logs were in
> /var/log/...host-php.log or a similar path?)
> 
> - is it possible to set this outside htaccess? (ie in the main config), if
> not why?
> 
> thanks!

The answer to both is that I would like logging to be enabled both on production and when testing locally. For locally, I won't know the paths (or in the case of our Vagrant project file I will, but they'll be different from production).

Happy to do it another way if we can get {prod,tbpl-dev,local-vagrant,local-set-up-manually} all easily logging by default?

Comment 17

[Guillaume Destuynder [:kang] [this account is no longer active]]

Since we dont know the full path, I fear that sometimes we may end up with the php log in the DocumentRoot, unless you have an idea to make sure this cannot happen. 

I'd rather have default logging properly set (ie to /var/log/...host-php.log or a similar path with read rights for whoever needs it).

Looping in webops to see if that's possible

Comment 18

[Jake Maul [:jakem]]

Yes, it should be possible to set this info in the main Apache vhost for tbpl (dev/stage/prod) rather than in a .htaccess file... and give it an absolute path at the same time.

The obvious downside is that this hurts the autonomy of the app (that is, you can't just check out the code into a local dev box and have it work in exactly the same way as dev/stage/prod, because the git repo doesn't have the vhost config). IMO that's not too surprising... a lot of system-level settings generally affect how an app works. Very very few apps try to go the distance and be *completely* portable regardless of system-level settings.

Translation: yes, we can set this at the system level for the official dev/stage/prod instances... but it won't help local test installations as comment 16 alludes to. It won't *hurt* them necessarily, they just won't have the same log settings.

Comment 19

[Ed Morley [:emorley]]

Unassigning since we'll need a different solution for TBPL2 (which won't be based on php) anyway.

Comment 20

[Ed Morley [:emorley]]

Since the local dev part landed, lets morph this to just be about that and call it fixed.