Make WordPress Core

Opened 4 years ago

Last modified 4 years ago

#52373 new defect (bug)

URL returned by get_attachment_link() can 404.

Reported by: peterwilsoncc's profile peterwilsoncc Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.4
Component: Media Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

The permalink returned by get_attachment_link() and get_permalink( /* attachment */ ) can result in a file not found page in the following circumstances:

  • The attachment's post parent has been deleted but post_parent is set (ie, post parent is pointing to an invalid ID).
  • The attachment's post parent is an unregistered post type.

In both cases get_permalink() will return a URL in the format w.org/attachment-name that will 404 when visited on the front end of the website.

If pretty URLs are enabled, the URL should return w.org/attachment/attachment-name as the permalink.

Adding the following unit test will demonstrate the bug:

<?php
public function test_attachment_attached_to_non_existent_post_type_does_not_404() {
        global $wp_post_types;

        $this->set_permalink_structure( '/%year%/%monthnum%/%day%/%postname%/' );
        flush_rewrite_rules();

        // Create temporay post type.
        register_post_type( 'not_a_post_type', array( 'public' => true ) );
        $post_id = self::factory()->post->create( array( 'post_type' => 'not_a_post_type' ) );

        // Attach media to post of post type.
        $attachment_id = self::factory()->attachment->create_object(
                'image.jpg',
                $post_id,
                array(
                        'post_mime_type' => 'image/jpeg',
                        'post_type'      => 'attachment',
                        'post_title'     => 'An Attachment!',
                        'post_status'    => 'inherit',
                )
        );

        // Unregister post type.
        foreach ( $wp_post_types as $id => $pt ) {
                if ( 'not_a_post_type' === $pt->name ) {
                        unset( $wp_post_types[ $id ] );
                        break;
                }
        }

        // Visit permalink.
        $this->go_to( get_permalink( $attachment_id ) );
        $this->assertQueryTrue( 'is_attachment' );
}

This looks to have been introduced in #1914

Change History (4)

This ticket was mentioned in PR #926 on WordPress/wordpress-develop by peterwilsoncc.


4 years ago
#1

  • Keywords has-patch has-unit-tests added

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

Failing unit tests to start with.

#2 @peterwilsoncc
4 years ago

If pretty URLs are enabled, the URL should return w.org/attachment/attachment-name as the permalink.

This is only true for the %postname% permalink structure.

If pretty URLs are enabled, the URL should return a URL in the same format as the post post type, but with attachment/attachment-name as the %postname% replacement.

#3 @peterwilsoncc
4 years ago

In 50132:

Canonical: Prevent ID enumeration of private post slugs.

Add check to redirect_canonical() to ensure private posts only redirect for logged in users.

Modifies the read_post mata capability to user get_post_status() rather than the post's post_status property to allow attachments to redirect based on the inherited post status.

Introduces wp_force_ugly_post_permalink() to unify the check to determine if an ugly link should be displayed in each of the functions used for determining permalinks: get_permalink(), get_post_permalink(), _get_page_link() and get_attachment_link().

Improves logic of get_attachment_link() to validate parent post and resolution of inherited post status. This is an incomplete fix of #52373 to prevent the function returning links resulting in a file not found error. Required to unblock this ticket.

Props peterwilsoncc, TimothyBlynJacobs.
See #52373.
Fixes #5272.

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


4 years ago

Note: See TracTickets for help on using tickets.