Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid multiple log files in multi-language environments #23722

Merged

Conversation

jensschuppe
Copy link
Contributor

Overview

Fixes dev/core/3491

Before

Multiple log files (one per language) in multi-language environments when using path prefixes.

After

Only one log file per environment.

Technical Details

Removes language path prefixes from user framework base URL when generating log file name hash.

Comments

I dird not have the time to thoroughly test this, but wanted to provide it as a PR as requested by @jaapjansma in the issue.

@civibot
Copy link

civibot bot commented Jun 8, 2022

(Standard links)

@civibot civibot bot added the master label Jun 8, 2022
@mlutfy
Copy link
Member

mlutfy commented Jun 13, 2022

The languageNegotiationURL function is weird (well, the remove args), but this change is otherwise harmless, and I have to admit that the multiple log files are really annoying/confusing. +1 on concept.

I would have recommended to remove the URL from there completely, but then everyone's log files will be renamed, and that will annoy people to. Then again, maybe simpler code is worth it.

@sluc23 @seamuslee001 this might interest you. Opinion on keeping the URL, or drop?

@sluc23
Copy link
Contributor

sluc23 commented Jun 14, 2022

+1 to have only 1 ConfigAndLog/CiviCRM.*.log in multi-languages sites
+1 to remove the url completely.. i never understood why log files had such a complex name.. it does not add any value adding that hash to filename in my POV..

@totten
Copy link
Member

totten commented Jun 14, 2022

Soft +1 for common log file in the multi-language sites (using this patch -- ie using the common base URL).

IMHO, long-term, we should probably try to replace (phase-out) generateLogFileHash() with some kind of constant in civicrm.settings.php... i.e. a value that is (a) truly random, (b) independent of DSN and site-key, (c) able to rotate on its own cycle, and (d) possible to customize.

However.... within the rubric of generateLogFileHash(), there is an argument for including some kind of URL. Recall that generateLogFileHash() is expected to be both stable and hard-to-guess. It's cobbled together from multiple entropy sources, but (individually) each source of entropy has issues:

  1. dsn should have a strong password component in most architectures. But some systems have silly/obvious values (mysql://root:root@localhost:3306/civicrm - which the implementer may justify by using containers/firewalls/sockets/etc).
  2. CIVICRM_SITE_KEY should be a long secret. But it can be silly/obvious value (eg if it wasn't set -- NO_SITE_KEY), and sometimes it can be readily discovered (eg if you have mobile or desktop integrations).
  3. userFrameworkBaseURL is never secret, but it is always unique.

In the worst case, dsn and CIVICRM_SITE_KEY are both weak/common values, and the checksum of 1+2 would also be weak/common. You could simply search Google for a couple constants -- and find logs for poorly-configured sites. Adding the URL makes the checksum unique - which frustrates Google searches.

So... 1+2+3 is more secure than just 1+2, but it's still not particularly secure (overall). IMHO, it would be more robust (secure+simple) to have this random value pre-generated and stored in civicrm.settings.php.

@mlutfy
Copy link
Member

mlutfy commented Jun 21, 2022

OK, so I guess +1 from Tim on keeping the URL in there.

Which makes this PR merge-ready? Any objections?

@mlutfy mlutfy added the merge ready PR will be merged after a few days if there are no objections label Jun 21, 2022
@demeritcowboy demeritcowboy merged commit 6da670d into civicrm:master Jun 25, 2022
@jensschuppe jensschuppe deleted the fix/multipleLogfilesMultilanguage branch July 18, 2022 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants