Attachment pages are only disabled for users that are logged in

[joppuyo]

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?

[afercia]

@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.

[afercia]

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.

[joppuyo]

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.

[chesio]

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

[afercia]

@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.

[chesio]

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).

[joppuyo]

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.

[joppuyo]

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)

[afercia]

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.

[chesio]

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.

[chesio]

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.

[joppuyo]

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

[joppuyo]

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.

[lakshmananphp]

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.

[afercia]

@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?

[SergeyBiryukov]

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.

Work in progress

Trac ticket: #59866

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

[chesio]

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.

[chesio]

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...

[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. Currently we have ”undefined behavior” for users that are logged out.

[afercia]

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.

Trac ticket: 59866

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

https://core.trac.wordpress.org/ticket/59866

[peterwilsoncc]

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.

[peterwilsoncc]

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.

Merged in r57310 / ​https://github.com/WordPress/wordpress-develop/commit/c5fff20a8fb88b7dcf7af08f6395c663272d17c3

A follow up pull request based on this one was merged in r57310 / ​https://github.com/WordPress/wordpress-develop/commit/c5fff20a8fb88b7dcf7af08f6395c663272d17c3

[peterwilsoncc]

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.

[peterwilsoncc]

In 57318:

Media: Revert [57310].

This commit reintroduced a minor data exposure issue.

Props swissspidy.
See #59866, #57913.

[peterwilsoncc]

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

[jorbin]

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.

[jorbin]

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

[joemcgill]

This looks good to backport. Thanks @jorbin!

[jorbin]

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.