Project

General

Profile

Actions

Task #52668

closed

Install Tool: Remove permission checking and fixing code from "folder structure"

Added by Ernesto Baschny over 10 years ago. Updated over 6 years ago.

Status:
Closed
Priority:
Should have
Category:
Install Tool
Target version:
Start date:
2014-03-26
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
TYPO3 Version:
6.2
PHP Version:
Tags:
Complexity:
medium
Sprint Focus:

Description

We've got several complaints and issues with the new "folder permission" checking and fixing - In the Install Tool "Step Installer" and later in the "Folder Structure" menu entry.

So we decided to simply remove this functionality from the install tool for now, and leave the responsability of a "secure" and "working" environment with the administrator / hosting provider. There are just too many constelations and exceptional conditions to consider here.

Default permissions (for new folders / files) should be:

Folders: 2775
Files: 0664

In the "Folder Structure" checking section a warning could be displayed if these are the active settings, pointing to a Wiki page describing different methods on how to "secure your installation". The wiki page could be collaborative effort documenting the best practice setup for different PHP vs Webservers vs OS variants.

Streamline what is considered an "Error", "Warning" and Notice":

Errors are:
  • Default permissions allowing world (other) writing (o+w)
  • Directories that do not exist
  • Directories that are not writeable (no files can be created within)
  • Paths that are files but should be directories
  • Paths that are not files but should be files
  • Symlinks that should be present but do not exist
Warnings are:
  • Default permissions allowing world (other) reading (o+r)
  • Symlink that is not a symlink (i.e. typo3_src)
  • Files that should be present but do not exist
Notices are:
  • Default permissions allowing group (other) r/w (g+rw)
  • Files that cannot be written (index.html, .htaccess)
  • Files which not match the configured default permission
  • Files that do not contain what we expect

Files

chmodTest.php (897 Bytes) chmodTest.php Michiel Roos, 2014-01-12 10:38
chmodTest.php (1.5 KB) chmodTest.php Michiel Roos, 2014-01-13 10:51

Subtasks 2 (0 open2 closed)

Bug #57152: First Install lists directory errors: confusingClosedErnesto Baschny

Actions
Task #57354: Default file permissions recommendation schould be 0665 instead of 0660Closed2014-03-26

Actions

Related issues 5 (0 open5 closed)

Related to TYPO3 Core - Bug #52578: Install process removes permissionClosedChristian Kuhn2013-10-07

Actions
Related to TYPO3 Core - Bug #53037: Install Tool: Folder structure is too strictClosed2013-10-22

Actions
Related to TYPO3 Core - Task #57055: Wrong unit tests after Installer permission fixClosedMarkus Klein2014-03-18

Actions
Related to TYPO3 Core - Bug #52016: wrong recommendations for typo3temp permissionClosed2013-09-15

Actions
Related to TYPO3 Core - Bug #58809: Default permissions for new files should be 0664Closed2014-05-14

Actions
Actions #1

Updated by Gerrit Code Review over 10 years ago

  • Status changed from Accepted to Under Review

Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/25416

Actions #2

Updated by Gerrit Code Review over 10 years ago

Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/25416

Actions #3

Updated by Ernesto Baschny over 10 years ago

  • Assignee changed from Christian Kuhn to Ernesto Baschny
  • % Done changed from 0 to 90
  • Complexity set to medium
Actions #4

Updated by Gerrit Code Review over 10 years ago

Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/25416

Actions #5

Updated by Gerrit Code Review about 10 years ago

Patch set 4 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/25416

Actions #6

Updated by Michiel Roos about 10 years ago

Would it be possible to use only a umask instead of separate file and dir permissions? The umask could be set once per script invocation (or just outside of the loop doing the file operations). This would save a lot of octdec() calls and chmod() calls.

Just an idea. Have not done any tests yet. The file creation functions respect the current umask.

The PHP man page says:
Avoid using this function in multithreaded webservers. It is better to change the file permissions with chmod() after creating the file. Using umask() can lead to unexpected behavior of concurrently running scripts and the webserver itself because they all use the same umask.

So I guess this would be a nono for most cases. Implementing it as an option could be done, but would introduce unwanted complexity. So I guess we need to live with chmod for a while longer.

We should cache the octdec() calls in the calling classes though.

Actions #7

Updated by Gerrit Code Review about 10 years ago

Patch set 5 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/25416

Actions #8

Updated by Michiel Roos about 10 years ago

Please see this chmod test. All mode values are stored in a variable.

You can see that in the first case, where the mode is stored as an int, and prefixed with a 0, the permissions are set correctly without a call to octdec.

Actions #9

Updated by Michiel Roos about 10 years ago

Here's a little test to figure out how PHP handles this whole chmod situation. We try both string values and plain int values to set the access controls.

It turns out that mkdir does not respect the sticky bit. An extra chmod is needed to add it. Not even ignoring the umask helps. Is this a PHP bug?

http://pastebin.com/RnHcFmJx

If we decide to keep on using the string values for the mode, the octdec() call should happen only once per class instance, when setting the mode property. The default property would have to be the 'dec' value then: 436 or 1533.

    protected function setTargetPermission($permission) {
        $this->targetPermission = octdec($permission);
    }

If we change the mode property to be a plain integer, we can completely do away with the octdec() calls. But the direcotry mode must be prefixed by a 0 for that to work correctly.

Actions #10

Updated by Gerrit Code Review about 10 years ago

Patch set 6 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/25416

Actions #11

Updated by Gerrit Code Review about 10 years ago

Patch set 7 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/25416

Actions #12

Updated by Gerrit Code Review about 10 years ago

Patch set 8 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/25416

Actions #13

Updated by Gerrit Code Review about 10 years ago

Patch set 9 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/25416

Actions #14

Updated by Ernesto Baschny about 10 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 90 to 100
Actions #15

Updated by Jan Radecker almost 10 years ago

  • Estimated time set to 0.00 h

The fileCreateMask has been set to 0665 in the changeset, which is wrong.
As stated above 0664 should be the default setting.

Actions #16

Updated by Jan Radecker almost 10 years ago

  • Estimated time deleted (0.00 h)
Actions #17

Updated by Riccardo De Contardi over 6 years ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF