Links in the update results page lead to 403s

Subscribing. Nice catches on the other broken links!

Oops, not sure how that happened.

Still a valid issue in Drupal 7.2. Re-rolling patch in #1. Unfortunately, this patch breaks strings, so uploading another version for D7 without the string change.

Reroll.

Tested and reviewed the patches for D8 and D7. Code looks good. Can't see how the code can fail. See the result of the tests below.

Review log in Drupal 7 with all permissions.

Review log in Drupal 7 with only the 'administer software updates' permission.

Review log in Drupal 8 with all permissions.

Review log in Drupal 8 with only the 'administer software updates' permission.

As can be seen, there will still be a possible 403 in the review log for Drupal 7, as the string can't be changed.

D'oh! I just realized strings in update.php are not translated, as Drupal is probably not fully bootstrapped.
So I'll attach a D7 version of the D8 patch with the string change.

There, a new D7 patch, which has the same lines of code as the D8 patch in #6.

Results:

Review log in Drupal 7 with all permissions.

Review log in Drupal 7 with only the 'administer software updates' permission.

Easy patch to review and set to RTBC ;)

#6: update-links-598414-6.patch queued for re-testing.

index a75153f..e865a25 100644
--- a/modules/system/system.test

Needs re-roll for core directory change at least.

-27 days to next Drupal core point release.

That's the D7 version in #9. The D8 version is in #6.

Latest D7 version: update-links-598414-9-D7.patch.
Latest D8 version: update-links-598414-6.patch.

Sorry about my confusion.

Patch look pretty good, but I would suggest one little thing more.

+++ b/core/update.php
@@ -155,7 +157,7 @@ function update_results_page() {
+  if (module_exists('dblog') && user_access('access site reports')) {

It would be great if this line also have some lines of testing, creating a user which has access site reports permission and one who doesn't.

-27 days to next Drupal core point release.

Thanks for the review! In fact, the tests already created by Dave Reid are only for the screen you get when there are no pending updates. The text
Updates were attempted. If you see no failures below, you may proceed happily to ... Otherwise, you may need to update your database manually. All errors have been logged.
is only displayed after updates have been performed successfully. I'm not sure how to test this process and I can't find any automated tests in core that performs a database update and allows me to add a test for the links on the successful outcome page after the update.

Found out how to write a test for the text on the successful update page. Coming up.

This should test all functionality that the original fix has touched upon.

Thanks for the implementation!

I guess now it's ready.

+++ b/core/update.phpundefined
@@ -166,7 +168,12 @@ function update_results_page() {
   if ($_SESSION['update_success']) {
-    $output = '<p>Updates were attempted. If you see no failures below, you may proceed happily to the <a href="' . base_path() . '?q=admin">administration pages</a>. Otherwise, you may need to update your database manually.' . $log_message . '</p>';
+    if (user_access('access administration pages')) {
+      $output = '<p>Updates were attempted. If you see no failures below, you may proceed happily to the <a href="' . base_path() . '?q=admin">administration pages</a>. Otherwise, you may need to update your database manually.' . $log_message . '</p>';
+    }
+    else {
+      $output = '<p>Updates were attempted. If you see no failures below, you may proceed happily back to your <a href="' . base_path() . '">site</a>. Otherwise, you may need to update your database manually.' . $log_message . '</p>';

How about we always print the second one? We already have a linke to the administration pages above. Seems like this could be simplified a bit.

You're right. There is not much use to linking to the administration pages 2 times. Will remove the if else tomorrow. Thanks!

That should do it.

Committed to 7.x and 8.x. Thanks!