Problem/Motivation

The htaccess lines that are added to the error log are incorrectly escaped and therefore tell the user to do the wrong line and doesn't make their site safe.

Proposed resolution

We should not escape the lines before logging because this means that if escaping is meaningless for the logger it won't be done (e.g. syslog)

Remaining tasks

Review
Commit

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#7 htaccess_message_patch.png138.35 KBxjm
#7 htaccess_message_head.png82.52 KBxjm
#2 2564321.2.patch2.46 KBalexpott
#2 2564321.2.test-only.patch1.48 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 2564321.2.test-only.patch1.48 KB
new 2564321.2.patch2.46 KB

Using the pre tags means that we don't have to use the nl2br().

The last submitted patch, 2: 2564321.2.test-only.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Much better.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

I tested this also manually and this seemed to fix the problem.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +SafeMarkup
xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes
StatusFileSize
new htaccess_message_head.png82.52 KB
new htaccess_message_patch.png138.35 KB

Nice, the updated code seems cleaner anyway.

Note that when testing manually, it's always valuable to document exactly what was tested and what the specific results were. "I tested manually and it fixes the problem" doesn't convey sufficient information for reviewers.

I tested manually with these steps:

  1. Install the Standard profile.
  2. Uncomment and edit this line in settings.php to point to a non-existent directory:
    $settings['file_private_path'] = '/private';

  3. drush cr
  4. Visit /admin/reports/status. (Triggers the writing of the log message.)
  5. Visit /admin/reports/dblog and click on the "Security warning" log message.

Attached screenshots show the double-escaping (HEAD) and the expected single escaping (with the patch):

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

And, committed and pushed to 8.0.x. Thanks!

  • xjm committed 74d6632 on 8.0.x 
    Issue #2564321 by alexpott: file_save_htaccess() generates error logs...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.