Motivation.

After installing and running the security_review module 8.x-1.x I found the language for the details of the File system permissions to be a bit confusing.

I propose language like this:

In addition to inspecting existing directories,
this test attempts to create and write to your file system. Look in
your security_review module directory on the server for:

  • A file named: file_write_test.YYYYMMDDHHMMSS
    • If this file exists the web server can write files to the
      security_review module directory and perhaps to other directories.
      You should correct the file permissions on all code directories of
      your Drupal installation.
  • Open the file IGNOREME.txt.
    • If a timestamp is appended at the end of
      it. That means the web server has permission to write to your files.
      This is insecure and the permissions should be corrected.

The intent is to make it clear to the user what the tests are doing and what they should check for to see if there file permissions are secure. For example it was not clear to me that the test on the IGNOREME.txt was trying to write to the contents of that file as opposed to renaming it.

patch forthcoming.

CommentFileSizeAuthor
#12 2535944-12.patch3.13 KBsandeepsingh199
#6 security_review-add-translation-2535944-6-drupal8.patch3.14 KBserundeputy
#5 security_review-add-translation-2535944-5-drupal8.patch209.03 KBserundeputy
#2 security-review-file-permissions-test-2535944-2.patch2.44 KBserundeputy

Issue fork security_review-2535944

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

serundeputy’s picture

serundeputy commented
Issue summary: View changes
serundeputy’s picture

serundeputy commented
Title: Details for file permissions security review report » patch Details for file permissions security review report
Issue tags: +patch
StatusFileSize
new security-review-file-permissions-test-2535944-2.patch2.44 KB

Here is a patch with the proposed language.

serundeputy’s picture

serundeputy commented
Status: Active » Needs review
c-logemann’s picture

c-logemann
he/him
Primary language German
Location Frankfurt/M, Germany
commented
Status: Needs review » Needs work

I didn't made a review of your text. But I see in your patch that you add some text without t()-function. Especially security information should be translated in my opinion.

serundeputy’s picture

serundeputy commented
Status: Needs work » Needs review
StatusFileSize
new security_review-add-translation-2535944-5-drupal8.patch209.03 KB

@C_Logemann thanks for your review!

I've added `t()` to the markup in this patch.

~Geoff

serundeputy’s picture

serundeputy commented
StatusFileSize
new security_review-add-translation-2535944-6-drupal8.patch3.14 KB

sorry last patch was no good; re-roll.

vuil’s picture

vuil
Location Bulgaria 🇧🇬 🇪🇺 🌍
commented
Status: Needs review » Closed (outdated)

I close the issue as Closed (outdated) for non-activity since 19 Jul 2015, it can not be apply and it's totally outdated.

c-logemann’s picture

c-logemann
he/him
Primary language German
Location Frankfurt/M, Germany
commented
Status: Closed (outdated) » Needs review

Closing an issue because nobody helps doesn't make it "oudated". This issue is about current Drupal 8 Dev version and just need a review help.
Especially file system checks are very important. So I think a feature request about explaining users what's going on in this security area is also very important.

vuil’s picture

vuil
Location Bulgaria 🇧🇬 🇪🇺 🌍
commented

OK, I agree @c_logemann. But then set the issue to Needs work because the patch can not be applied on the current 8.x-1.x-dev branch, it needs to be updated and/or re-rolled. Thank you!

c-logemann’s picture

c-logemann
he/him
Primary language German
Location Frankfurt/M, Germany
commented
Status: Needs review » Needs work

When the patch can't apply anymore "needs work" is correct. I try to find some time going forward with this feature and other file check improvements.

smustgrave’s picture

smustgrave commented
Issue tags: - +Needs reroll
sandeepsingh199’s picture

sandeepsingh199 commented
StatusFileSize
new 2535944-12.patch3.13 KB

hello team, I have re-rolled the #6 patch for current 8.x-1.x-dev branch with D9.5.x. kindly check & review. Thanks.

smustgrave’s picture

smustgrave commented
Version: 8.x-1.x-dev » 2.0.x-dev
smustgrave’s picture

smustgrave commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll

Tests are now passing thank you!

smustgrave opened merge request !14

  • smustgrave committed 31b00e4 on 2.0.x 
    Issue #2535944: patch Details for file permissions security review...
smustgrave’s picture

smustgrave commented
Status: Reviewed & tested by the community » Fixed
c-logemann’s picture

c-logemann
he/him
Primary language German
Location Frankfurt/M, Germany
commented

When I read the (new) description of this test I think there is lots of space for file checking improvement. But when the test currently is working in this way the text should reflect this. When I find some time I will open a new issue.

Thanks @smustgrave for cleaning up some issues of this module.

tobiasb’s picture

tobiasb
Primary language German
Location Berlin
commented

fyi: Potx will skip the new string, because it finds/use only plain strings. (l.d.o.)

So the t can be removed or the string needs to wrapped with t.

Status: Fixed » Closed (fixed)

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