Make WordPress Core

Opened 6 months ago

Closed 4 months ago

#59866 closed defect (bug) (fixed)

Attachment pages are only disabled for users that are logged in

Reported by: joppuyo's profile joppuyo Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.4.3 Priority: normal
Severity: normal Version: 6.4
Component: Media Keywords: has-patch has-unit-tests fixed-major dev-reviewed
Focuses: Cc:

Description

This is related to the previous ticket #57913 regarding "disable attachment page functionality".

I'm an author of a plugin Disable Media Pages which has provided this functionality in previous WordPress versions. I'm trying to figure out what's the best way forward for the plugin since WordPress 6.4 as this functionality is now built into the core.

While testing 6.4, I noticed that even if attachment_pages_enabled is set to 0 the redirection only kicks in for logged-in users. For anonymous users, the attachment page is still displayed. It seems this is because read_post capability is checked before the redirection is performed. Anonymous users don't have any capabilities so the redirection work in this case.

Is this feature intended to work this way or is this an oversight in the implementation?

Attachments (3)

59866.diff (843 bytes) - added by afercia 6 months ago.
59866-cp.diff (1.2 KB) - added by chesio 6 months ago.
Screenshot 2023-11-09 at 16.44.24.png (1.6 MB) - added by joppuyo 6 months ago.

Download all attachments as: .zip

Change History (48)

@afercia
6 months ago

#1 @afercia
6 months ago

  • Keywords has-patch needs-testing added
  • Milestone changed from Awaiting Review to 6.4.2

@aristath @SergeyBiryukov @poena and myself had a look at it this morning and we'd like to propose a simple patch.
59866.diff changes the capability check to be performed against the attachment post parent instead of the attachment post itself.

Proposing for 6.4.2 consideration.

#2 @afercia
6 months ago

To Test instructions:

Before the patch:

  • Test with a fresh install of WordPress 6.4 with a fresh database.
  • Alternatively test with WordPress 6.4 and make sure your database has a wp_attachment_pages_enabled option set to 0 in the wp_options table.
  • I guess you can also use this filter somewhere: add_filter( 'pre_option_wp_attachment_pages_enabled', '__return_false' );
  • Create a post, add an image block and link the image to its attachment page.
  • Publish the post.
  • View the front end as a logged in user.
  • Click the image and observe you are redirected to the media file.
  • View the front end as a logged out user.
  • Click the image and observe you are redirected to the attachment page.

After the patch:

  • View the front end as a logged in user.
  • Click the image and observe you are redirected to the media file.
  • View the front end as a logged out user.
  • Click the image and observe you are redirected to the media file.

#3 follow-up: @joppuyo
6 months ago

I tested @afercia :s patch by applying it to WordPress 6.4 and it actually makes things worse. This is with wp_attachment_pages_enabled set to 0

  1. Upload image foo.jpeg to the media gallery
  2. Visit https://example.com/foo as a logged-in user
  3. Attachment page is displayed
  1. Upload image foo.jpeg to the media gallery
  2. Visit https://example.com/foo as a logged-out user
  3. Attachment page is displayed
  1. Create page called Bar
  2. Add image foo.jpeg to the post as an image block
  3. Visit https://example.com/bar/foo as a logged-out user
  4. Attachment page is displayed
  1. Create page called Bar
  2. Add image foo.jpeg to the post as an image block
  3. Visit https://example.com/bar/foo as a logged-in user
  4. You are redirected to the media file

I don't think the issue is that the capability check is performed against the attachment or the parent page. The issue is the capability check itself. Because anonymous users do not have any capabilities they are never redirected. The capability check should be removed for non-private posts so that the functionality works for users that are not logged in.

Last edited 6 months ago by joppuyo (previous) (diff)

@chesio
6 months ago

#4 @chesio
6 months ago

I concur, the original patch does not solve the problem - there is still no redirect for anonymous users due to the read_post capability check.

I propose a slightly different solution (see 59866-cp.diff):

  1. Attachments not attached to any post cannot be private, so they should be always redirected.
  2. Attachments attached to a post should use existing check for private status. This check is triggered when $redirect_obj is set. See: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/canonical.php#L776

#5 follow-ups: @afercia
6 months ago

@joppuyo thanks for testing.

To my understanding, your reproduction steps try to access the attachment page url directly? I'm not sure that's a case that has been covered by the current implementation neither I'm sure whether it should be covered. I'm not saying it should not, just that I'm not sure as it appears there is some historical inconsistency in the way WordPress handles attachments for public / private / password protected posts.

The goal of #57913 was to prevent attachment pages from being indexed by search engine crawlers. Is there any way crawlers can access the attachment pages by using directly the attachment page url? I'd think a crawler would first crawl the parent post, and then follow the links to the attachment pages. Unless I'm missing something.

I would recommend to please test the patch following the steps in comment:2. That would help clarify the nature of the issue and better understand what is best to do next.

#6 in reply to: ↑ 5 @chesio
6 months ago

Replying to afercia:

@joppuyo thanks for testing.

The goal of #57913 was to prevent attachment pages from being indexed by search engine crawlers. Is there any way crawlers can access the attachment pages by using directly the attachment page url? I'd think a crawler would first crawl the parent post, and then follow the links to the attachment pages. Unless I'm missing something.

I see the point, though I don't think it's safe to assume that attachment page URLs can be only discovered for attachments attached to a post. Therefore I would prefer this feature to cover all attachments (regardless whether they are attached to a post or not).

#7 in reply to: ↑ 5 @joppuyo
6 months ago

In the blog post announcing this feature there's the following quote:

As of WordPress 6.4, attachment pages for new WordPress installations are fully disabled.

Until WordPress 6.4 was released, WordPress created attachment pages by default for every attachment uploaded. On the vast majority of sites, these attachment pages don’t add any meaningful information. They do, however, exist, get indexed by search engines, and sometimes even rank in search results, leading to bad results for users and site owners.

I thought "fully disabling" attachment pages would mean that if I visit the page it would show a 404 status code. The current redirection behaviour feels like a workaround and it still reserves slugs from pages based on the file name (something that my plugin addresses). Also the redirect does not work if you are an anonymous user. The attachments still pollute the permalink structure too.

So instead of "fully disabling" the attachment pages current functionality could be described as "redirects attachment pages to the file if you are logged in" which feels a bit half baked.

Even Yoast SEO does a better job by doing the redirection both for logged-in and logged out users.

#8 @swissspidy
6 months ago

  • Keywords needs-unit-tests added

#9 @joppuyo
6 months ago

This might be off-topic but shouldn't the option to link an image block to the attachment page be disabled if wp_attachment_pages_enabled is set to 0 ? (see attachment)

Last edited 6 months ago by joppuyo (previous) (diff)

#10 follow-up: @afercia
6 months ago

Turns out that it appears a big source of confusion while working on 59866.diff is the fact modern browsers tend to cache redirects in some way.

Strongly recommended for development and testing: keep your browser's dev tools open and disable caching in the Network tab.

That said, the logic in 59866.diff seems wrong as a check for read_post is only necessary for private posts. A new fix will need to take into account private posts because we don't want users to see media attached to private posts. See soem similar logic at https://github.com/WordPress/wordpress-develop/blob/7287ff52633264b4e16fdaed5697307d4b8ceac1/src/wp-includes/canonical.php#L776-L796

However, that's not trivial as a media attachmend can have one parent but actually be used in other posts as well.

#11 in reply to: ↑ 10 @chesio
6 months ago

Replying to afercia:

That said, the logic in 59866.diff seems wrong as a check for read_post is only necessary for private posts. A new fix will need to take into account private posts because we don't want users to see media attached to private posts. See soem similar logic at https://github.com/WordPress/wordpress-develop/blob/7287ff52633264b4e16fdaed5697307d4b8ceac1/src/wp-includes/canonical.php#L776-L796

Please, see my patch: 59866-cp.diff.

However, that's not trivial as a media attachmend can have one parent but actually be used in other posts as well.

I think this is irrelevant here. An attachment that is also used in a private post still has its page publicly accessible if it is attached to a public post.

#12 in reply to: ↑ 3 ; follow-up: @chesio
6 months ago

Replying to joppuyo:

I don't think the issue is that the capability check is performed against the attachment or the parent page. The issue is the capability check itself. Because anonymous users do not have any capabilities they are never redirected. The capability check should be removed for non-private posts so that the functionality works for users that are not logged in.

@joppuyo, could you test my patch: 59866-cp.diff It works for me, but you seem to have more experience in this area.

Note that it only fixes the redirect feature and does not "fully disable" the attachment pages as you suggested in #comment:7.

#13 @joppuyo
6 months ago

I can confirm that @chesio 's patch fixes the issue in my testing, here are the steps to reproduce:

This is stock WordPress 6.4.1 with wp_attachment_pages_enabled set to 1

  1. Upload image foo.jpeg to the media gallery
  2. Visit https://example.com/foo as a logged-in user
  3. Attachment page is displayed
  1. Upload image foo.jpeg to the media gallery
  2. Visit https://example.com/foo as an anonymous user
  3. Attachment page is displayed
  1. Create page called Bar
  2. Add image foo.jpeg to the post as an image block
  3. Visit https://example.com/bar/foo as a logged-in user
  4. Attachment page is displayed
  1. Create page called Bar
  2. Add image foo.jpeg to the post as an image block
  3. Visit https://example.com/bar/foo as an anonymous user
  4. Attachment page is displayed

This is stock WordPress 6.4.1 with wp_attachment_pages_enabled set to 0

  1. Upload image foo.jpeg to the media gallery
  2. Visit https://example.com/foo as a logged-in user
  3. You are redirected to the media file
  1. Upload image foo.jpeg to the media gallery
  2. Visit https://example.com/foo as an anonymous user
  3. Attachment page is displayed <- This is incorrect behaviour
  1. Create page called Bar
  2. Add image foo.jpeg to the post as an image block
  3. Visit https://example.com/bar/foo as a logged-in user
  4. You are redirected to the media file
  1. Create page called Bar
  2. Add image foo.jpeg to the post as an image block
  3. Visit https://example.com/bar/foo as an anonymous user
  4. Attachment page is displayed <- This is incorrect behaviour

This is WordPress 6.4.1 with @chesio 's patch and wp_attachment_pages_enabled set to 1

  1. Upload image foo.jpeg to the media gallery
  2. Visit https://example.com/foo as a logged-in user
  3. Attachment page is displayed
  1. Upload image foo.jpeg to the media gallery
  2. Visit https://example.com/foo as an anonymous user
  3. Attachment page is displayed
  1. Create page called Bar
  2. Add image foo.jpeg to the post as an image block
  3. Visit https://example.com/bar/foo as a logged-in user
  4. Attachment page is displayed
  1. Create page called Bar
  2. Add image foo.jpeg to the post as an image block
  3. Visit https://example.com/bar/foo as an anonymous user
  4. Attachment page is displayed

This is WordPress 6.4.1 with @chesio 's patch and wp_attachment_pages_enabled set to 0

  1. Upload image foo.jpeg to the media gallery
  2. Visit https://example.com/foo as a logged-in user
  3. You are redirected to the media file
  1. Upload image foo.jpeg to the media gallery
  2. Visit https://example.com/foo as an anonymous user
  3. You are redirected to the media file
  1. Create page called Bar
  2. Add image foo.jpeg to the post as an image block
  3. Visit https://example.com/bar/foo as a logged-in user
  4. You are redirected to the media file
  1. Create page called Bar
  2. Add image foo.jpeg to the post as an image block
  3. Visit https://example.com/bar/foo as an anonymous user
  4. You are redirected to the media file

#14 in reply to: ↑ 12 @joppuyo
6 months ago

Replying to chesio:

Note that it only fixes the redirect feature and does not "fully disable" the attachment pages as you suggested in #comment:7.

I think redirect/404 are two different ways of disabling media pages. While I think 404 would be more effective, at least making the redirect work also for logged out users would be a good first step because right now the feature works only half the time.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


6 months ago

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


6 months ago

#17 follow-up: @lakshmananphp
6 months ago

I've tested @chesio's patch, and it works.

I have a suggestion: can we add one more condition to this check?

if ($attachment_parent_id && 0 !== $attachment_parent_id ) {  
// just to make sure that the attachment's parent id is not zero.

This ticket was mentioned in Slack in #core-test by lakshmananphp. View the logs.


6 months ago

#19 @afercia
6 months ago

@asif2bd @peterwilsoncc @TimothyBlynJacobs
pinging you since you're either maintainers of the Permalinks core component or you did work on canonical and redirects. If I'm wrong please forgive me for the unnecessary ping.
Could you please have a look at 59866-cp.diff, when you have a chance?

#20 follow-up: @SergeyBiryukov
6 months ago

Looking at 59866-cp.diff, it makes sense to me at a glance, and leveraging the existing post status check from [50132] / #5272 seems like a good idea.

Would be great to have some unit tests for this.

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


6 months ago
#21

  • Keywords has-unit-tests added; needs-unit-tests removed

Work in progress

Trac ticket: #59866

I am writing unit tests for redirecting attachment pages to attachment URLs.

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


6 months ago

#23 in reply to: ↑ 17 @chesio
5 months ago

Replying to lakshmananphp:

I have a suggestion: can we add one more condition to this check?

if ($attachment_parent_id && 0 !== $attachment_parent_id ) {  
// just to make sure that the attachment's parent id is not zero.

This extra condition would be superfluous. If $attachment_parent_id is 0, the first condition always fails and the extra condition is never executed.

#24 in reply to: ↑ 20 @chesio
5 months ago

  • Keywords dev-feedback added

Replying to SergeyBiryukov:

Would be great to have some unit tests for this.

I had a look and there is already a test for this feature. It currently passes, because the whole Tests_Canonical suite, which the test is part of, sets up a backend user before start. So the redirect feature is being tested for logged in user.

I'm not sure how to proceed here: if the consensus is that the redirect should work for anonymous users in the same way as logged ones, then I believe the existing test should be fixed.

Also I'm not sure why the Tests_Canonical suite is run with backend user set. I tend to think this is just a relic of some past refactoring. If I run the suite without call to wp_set_current_user( self::$author_id ); in its setup method, all tests pass just fine - except for the test_canonical_attachment_page_redirect_with_option_disabled test...

#25 @chesio
5 months ago

  • Keywords dev-feedback removed

#26 follow-up: @joppuyo
5 months ago

It wouldn’t really hurt to have another set of tests that test the feature for logged-out users in addition to current logged-in behavior, regardless of what’s the desired behavour. Currently we have ”undefined behavior” for users that are logged out.

Last edited 5 months ago by joppuyo (previous) (diff)

#27 in reply to: ↑ 26 @afercia
5 months ago

Replying to joppuyo:

It wouldn’t really hurt to have another set of tests that test the feature for logged-out users in addition to current logged-in behavior, regardless of what’s the desired behavour.

It totally makes sense and this ticket is a clear evidence the whole feature needs to have tests for both logged-in and logged-out users.

#28 @SergeyBiryukov
5 months ago

  • Milestone changed from 6.4.2 to 6.4.3

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


5 months ago
#29

Trac ticket: 59866

This PR adds a test for checking the attachment page redirection to the attachment file for the visitors.

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


5 months ago

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


5 months ago

#33 @peterwilsoncc
4 months ago

I think the approach in 59866-cp.diff makes sense.

In the linked pull request I've added tests for the following situations:

  • no post parent, logged out user
  • no post parent, logged in user
  • private post parent, logged out user
  • private post parent, logged in user
  • public post parent, logged out user
  • public post parent, logged in user

I've switched to a data provider as there's enough of an increase in the tests that I didn't want to add any more near repeats.

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


4 months ago

#35 @peterwilsoncc
4 months ago

  • Owner set to peterwilsoncc
  • Resolution set to fixed
  • Status changed from new to closed

In 57310:

Media: Redirect inactive attachement pages for logged-out users.

Ensure logged out users are redirected to the media file when attachment pages are inactive. This removes the read_post capability check from the canonical redirects as anonymous users lack the permission.

Follow-up to [56657], [56658], [56711].

Props afercia, aristath, chesio, joppuyo, jorbin, lakshmananphp, poena, sergeybiryukov.
Fixes #59866.
See #57913.

#38 @peterwilsoncc
4 months ago

  • Keywords dev-feedback added; needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport to the 6.4 branch following another committers sign-off.

@mukesh27 I have added your props manually as I initially missed your review on one of the linked pull requests.

#39 @peterwilsoncc
4 months ago

In 57318:

Media: Revert [57310].

This commit reintroduced a minor data exposure issue.

Props swissspidy.
See #59866, #57913.

#40 @peterwilsoncc
4 months ago

  • Keywords dev-feedback removed

Per revert above, [57310] can not be backported to the 6.4 branch. Committers, please refrain from doing so.

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


4 months ago

#42 @jorbin
4 months ago

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

In 57357:

Media: Redirect inactive attachment pages for logged-out users.

Ensure logged out users are redirected to the media file when attachment pages are inactive. This removes the read_post capability check from the canonical redirects as anonymous users lack the permission.

This was previously committed in [57310] before being reverted in [57318]. This update includes a fix to cover instances where revealing a URL could be considered a data leak and greatly expands the unit tests to ensure that this is covered along with many other instances.

Follow-up to [56657], [56658], [56711], [57310], [57318].

Props peterwilsoncc, jorbin, afercia, aristath, chesio, joppuyo, jorbin, lakshmananphp, poena, sergeybiryukov, swissspidy, johnbillion.
Fixes #59866.
See #57913.

#43 @jorbin
4 months ago

  • Keywords dev-feedback fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening so that [57357] can be considered for backport to 6.4.3

#44 @joemcgill
4 months ago

  • Keywords dev-reviewed added; dev-feedback removed

This looks good to backport. Thanks @jorbin!

#45 @jorbin
4 months ago

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

In 57358:

Media: Redirect inactive attachment pages for logged-out users.

Ensure logged out users are redirected to the media file when attachment pages are inactive. This removes the read_post capability check from the canonical redirects as anonymous users lack the permission.

This was previously committed in [57310] before being reverted in [57318]. This update includes a fix to cover instances where revealing a URL could be considered a data leak and greatly expands the unit tests to ensure that this is covered along with many other instances.

Follow-up to [56657], [56658], [56711], [57310], [57318].

Reviewed by joemcgill.
Merges [57357] to 6.4 branch.

Props peterwilsoncc, jorbin, afercia, aristath, chesio, joppuyo, jorbin, lakshmananphp, poena, sergeybiryukov, swissspidy, johnbillion, mukesh27.
Fixes #59866.
See #57913.

Note: See TracTickets for help on using tickets.