The setting to bypass access check on update.php (currently located at the top of update.php itself) is easy to overlook in the mass of work with upgrading a site, and problem is only reported at the end of update process (i.e. only just once).

I believe that this is a problem with similar severity as (if not higher than) protection of settings.php and the like, and so it should be reported in the System Status report.

I'm attaching a patch, including changes:

--- Due to impossibility (I believe) to include update.php only just to check setting, but not execute it, THIS PATCH INTRODUCES A CHANGE TO SETTINGS.PHP: The setting to bypass access check on update.php is moved into settings.php, which I believe is consistent, and allows files other than update.php to see it for check.

--- The check is added to System Status report.

--- The changed location of the setting is reflected in comments and descriptions at various places around update.php, and UPGRADE.txt (also fixing previously broken step-numbering in step 2 by the way).

Needs review!

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Fantastic idea, and generally fine patch. The only problem I noticed is that for whatever reason, your use of $t() and t() is mixed up in the patch. This should be fixed, and I think this is close getting into Drupal 6 soon.

JirkaRybka’s picture

Status: Needs work » Needs review
FileSize
8.63 KB

Thanks for comment - I was just pasting code-snippets seen around that place, didn't realize at all that there's an extra $. Now corrected - and by the way, I'm correcting the very same also on one already-existing line now.

chx’s picture

Status: Needs review » Needs work

JirkaRybka , thanks for the great work. However, I would negate the checking: if there is nothing in settings.php that's a disable. You need to specifically add "allow update for anyone" and remove afterwards. This way if settings.php get mangled you can't open up accidentally. This was different with update.php which changes much less than settings.

JirkaRybka’s picture

Status: Needs work » Needs review
FileSize
8.63 KB

I was a bit unsure about this one, but finally decided to keep the already existing setting only just moved. With the previous patch, if the setting is missing from settings.php, update is unprotected indeed, but the problem IS REPORTED in Status report.

But I can see your point - some users are likely to just copy their old settings.php, leaving update unprotected and never bother about red lines in some hidden Status report.

I'm attaching a modified patch with the reversed logic, which ensures update protection by default, even with old settings.php. Otherwise it's the same as previous patch, so I leave it to others to choose which one to use.

But in the other hand, usage of outdated settings.php is now NOT REPORTED as problem in the Status report. I think this is OK, but still might confuse users, being unable to find the setting in their settings.php. This is purely user's fault, though.

chx’s picture

FileSize
8.63 KB

I changed a few == TRUE to !empty to avoid notices.

webchick’s picture

No time test atm, but subscribing and wanted to say this is a fantastic idea. It's so easy to miss, and it's a very, very bad thing to miss. Accidentally giving Anonymous users having access to perform raw database queries (even if they are pre-determined) is never fun. ;)

JirkaRybka’s picture

Re: #5 - I didn't have any problems with the original version, but tested the new patch and works OK for me. So keep the change if you want.

Re: #6 - Yes, and I was especially thinking of people re-running old contrib updates, which might be buggy/unsafe. ;)

chx’s picture

My patch only avoids notice if update_free_access is missing from settings.php -- you probably had that so you saw no notices.

JirkaRybka’s picture

I tested also with setting completely commented-out, and my log was clean. I'm not sure if that's because the variable is mentioned in bootstrap.inc as global, but probably there's no need to discuss this further. Your version seems more clean, and works fine.

Gábor Hojtsy’s picture

Status: Needs review » Fixed

Looks good to me too. Committed!

Anonymous’s picture

Status: Fixed » Closed (fixed)