Make WordPress Core

Opened 9 years ago

Last modified 4 days ago

#36803 reopened defect (bug)

ms-files.php: inconsistent behaviour for upload visibility on archived sites

Reported by: antwortzeit's profile antwortzeit Owned by: audrasjb's profile audrasjb
Milestone: 6.9 Priority: normal
Severity: normal Version: 4.5.2
Component: Media Keywords: has-patch close
Focuses: multisite Cc:

Description

Hey,

i just ran into an odd issue on one of our clients Multisites. I archived the said site and – as you know – it remains accessible for network admins. The matching files however don't. I followed this down to ms-files.php::21ff.

<?php

if ( $current_blog->archived == '1' || $current_blog->spam == '1' || $current_blog->deleted == '1' ) {
        status_header( 404 );
        die( '404 &#8212; File not found.' );
}

You see that ms-files.php checks, if the blog is archived (or spam or deleted) and than throws out a 404. Shouldn't this include a check for network admin users to see the files? Or, if that's not desirable, couldn't we make this check accessible for filters?

Thanks for the good work!

Christian

Change History (15)

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


9 years ago

#2 @SergeyBiryukov
9 years ago

  • Component changed from General to Media
  • Keywords needs-patch added

#3 @jeremyfelt
9 years ago

  • Milestone changed from Awaiting Review to Future Release

Hi @antwortzeit, thanks for taking the time to open a ticket. We should be able to provide some more consistency here.

It looks like the safest way would be to add an is_super_admin() check rather than rely on any other capabilities. Because ms-files.php uses SHORTINIT, some of our other options are limited.

This ticket was mentioned in PR #7696 on WordPress/wordpress-develop by @debarghyabanerjee.


6 months ago
#4

  • Keywords has-patch added; needs-patch removed

Trac Ticket: Core-36803

## Description:

  • An issue was identified in the multisite installations where archived sites remain accessible to network administrators, but the associated files do not. This behavior originates from the logic in ms-files.php, particularly around line 21:
if ( $current_blog->archived == '1' || $current_blog->spam == '1' || $current_blog->deleted == '1' ) {
    status_header( 404 );
    die( '404 &#8212; File not found.' );
}
  • The current implementation checks if the blog is archived, marked as spam, or deleted, and subsequently returns a 404 error for file requests. However, this does not account for network administrators who should retain access to these files.

## Proposed Solution

  • An additional check using is_super_admin() has been implemented. This adjustment allows network administrators to access files even if the site is archived, spam, or deleted. The modified code snippet is as follows:
if ( ( $current_blog->archived == '1' || $current_blog->spam == '1' || $current_blog->deleted == '1' ) && ! is_super_admin() ) {
    status_header( 404 );
    die( '404 &#8212; File not found.' );
}

## Benefits

  • Enhanced Access for Network Administrators: This change ensures that network admins can access necessary files for archived sites, improving usability and functionality.
  • Preservation of Current Logic: The existing restrictions remain in place for regular users, maintaining intended access controls.

#5 @SergeyBiryukov
6 months ago

  • Milestone set to 6.8

@audrasjb commented on PR #7696:


3 months ago
#6

I tested this patch and I can confirm the resource doesn't return a 404 when logged in as a super-admin like role 👍

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


3 months ago

@audrasjb commented on PR #7696:


3 months ago
#8

I committed the change proposed by Clorith.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


6 weeks ago

#10 @audrasjb
6 weeks ago

  • Owner set to audrasjb
  • Status changed from new to reviewing

#11 @audrasjb
6 weeks ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 59967:

Media: Allow super-admin to access files of archived sites.

This changeset fixes an issue in multisite installations where archived sites remain accessible to network administrators, but the associated files do not. The previous implementation was checking if the blog is archived, marked as spam, or deleted, to subsequently return a 404 error for file requests. However, this did not account for network administrators who should retain access to these files.

Props antwortzeit, jeremyfelt, debarghyabanerjee, audrasjb.
Fixes #36803.

#13 @peterwilsoncc
5 days ago

In 60170:

Media: Prevent Multisite fatal error in legacy file rewrites.

Removes a call to the is_super_admin() function in ms-files.php as the function isn't available when using the SHORTINIT feature, causing fatal errors on systems using file rewrites.

Reverts [59967].

Props audrasjb, desrosj, jeremyfelt, johnjamesjacoby, jorbin, presskopp, verygoode, dd32, joemcgill.
See #36803.
Fixes #63285.

#14 @peterwilsoncc
5 days ago

  • Keywords close added
  • Milestone changed from 6.8 to 6.9
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening as the commit for this ticket was reverted r60170 as it was triggering a fatal error in systems using the legacy rewrites.

Adding the lose keyword as my inclination is to close the ticket as unplanned/wontfix as the legacy rewrites are part of a long deprecated system.

#15 @peterwilsoncc
4 days ago

In 60171:

Media: Prevent Multisite fatal error in legacy file rewrites.

Removes a call to the is_super_admin() function in ms-files.php as the function isn't available when using the SHORTINIT feature, causing fatal errors on systems using file rewrites.

Reverts [59967].

Reviewed by jorbin.
Merges [60170] to the 6.8 branch.

Props audrasjb, desrosj, jeremyfelt, johnjamesjacoby, jorbin, presskopp, verygoode, dd32, joemcgill.
See #36803.
Fixes #63285.

Note: See TracTickets for help on using tickets.