WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 2 years ago

#19693 closed defect (bug) (duplicate)

redirect_guess_404_permalink() can catch the wrong post types

Reported by: nacin Owned by: nacin
Milestone: Priority: normal
Severity: normal Version:
Component: Canonical Keywords:
Focuses: Cc:

Description

When I access /blah, and that 404s, redirect_guess_404_permalink() will try to find a matching post name.

If blah is the start of a post name for a random post type (perhaps one used as internal storage), it'll match it and redirect (and then end up 404ing, potentially). There's no constraint on the post type if the post_type query var isn't set, which is going to be common.

We need to come up with a stricter query. (And while we're at it, we should hook redirect_guess_404_permalink() into redirect_canonical() so it may be unhooked without removing all canonical support.)

Attachments (3)

19693.diff (1000 bytes) - added by jeremyfelt 6 years ago.
Adds stricter post_type query to redirect_guess_404_permalink
19693.2.diff (2.1 KB) - added by SergeyBiryukov 6 years ago.
19693.3.diff (705 bytes) - added by jeremyfelt 6 years ago.

Download all attachments as: .zip

Change History (18)

@jeremyfelt
6 years ago

Adds stricter post_type query to redirect_guess_404_permalink

#1 @jeremyfelt
6 years ago

  • Cc jeremy.felt@… added

This patch addresses part 1 of the ticket by only looking for post types that are registered as publicly queryable if a post type has not already been specified as one of the query vars.

I might need clarification to understand part 2.

#2 @jkudish
6 years ago

  • Keywords has-patch added

#3 @nacin
6 years ago

  • Keywords commit added

I am fine with this. It should be 'public' rather than 'publicly_queryable'.

#4 @jeremyfelt
6 years ago

I think it's safe to be stay specific with 'publicly_queryable' here.

There is a possibility of a post type being registered with 'public' => false and an overriding 'publicly_queryable' => true (or vice versa) that we should allow for.

#5 @nacin
6 years ago

publicly_querable is simply for whether you should be able to do ?post_type=tweet (yes, this post type is publicly queryable) or ?post_type=page (no, this post type is not publicly queryable). It does not affect whether the post type is publicly accessible — that is controlled by the "public" flag. Anyone setting public to false but publicly_queryable to true is asking for it. :-)

#6 @SergeyBiryukov
6 years ago

19693.2.diff is an attempt to address the second point of the ticket as well (the ability to unhook redirect_guess_404_permalink()).

#7 @nacin
6 years ago

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

In [20688]:

Only look for public post types in redirect_guess_404_permalink() when no post type query var is present. props jeremyfelt. fixes #19693.

@jeremyfelt
6 years ago

#8 @jeremyfelt
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

In the current state, if I have a post type registered as ( 'public' => true, 'publicly_queryable' => false ), redirect_guess_404_permalink() will attempt to redirect '/blah' to '/non-publicly-queryable/blah/', resulting in a 404 at the new URL.

19693.3.diff corrects this by adding 'publicly_queryable' => true as a second argument to get_post_types()

#9 @nacin
6 years ago

There is nothing wrong with a public post type not being publicly_queryable. For example, pages.

/non-publicly-queryable/blah is a valid construct. What isn't is /non-publicly-queryable — this is what publicly_queryable prevents.

#10 @jeremyfelt
6 years ago

I understand the coexistence of public and !publicly_queryable. I think that is why redirect_guess_404_permalink() needs both arguments.

When a guess occurs, the resulting redirect initiates a new parse_request(), which will see the guessed query_var, which is directly related to the publicly_queryable argument.

Using my internal post type of 'my-special-post-type', that I have registered as 'public' => true, 'publicly_queryable' => false, 'rewrite' => true , the following happens:

  • mysite/post-name
    • not found, initiates redirect_guess_404_permalink()
  • mysite/my-special-post-type/post-name
    • becomes the redirected URL after guess, which results in 404 because publicly_queryable is false
    • if publicly_queryable is set to true, this URL becomes valid.

With 'public' => true, 'publicly_queryable' => false, 'rewrite' => false , I get:

  • mysite/?my-special-post-type=post-name
    • becomes the redirected URL after guess, which results in 404 because publicly_queryable is false
    • if publicly_queryable is set to true, this URL becomes valid.

19693.3.diff adds the argument 'publicly_queryable' => true to the existing 'public' => true, resulting in a 404 at mysite/post-name - leaving out the incorrect guess.

Pages may need to be included separately, as that is a special case.

#11 @ryan
6 years ago

Per bug scrub, going with 19693.3.diff. Eventually, the confusion here will be eliminated.

#12 @ryan
6 years ago

  • Keywords has-patch commit removed
  • Milestone changed from 3.4 to Future Release

19693.3.diff breaks some page related unit tests because publicly_queryable is false for pages, as mentioned in comment:10.

On second thought, we're going to call [20688] good enough. It's certainly an improvement. We'll clean the remainder up in 3.5.

#13 @SergeyBiryukov
6 years ago

And while we're at it, we should hook redirect_guess_404_permalink() into redirect_canonical() so it may be unhooked without removing all canonical support.

Related: #16557

#14 @MikeSchinkel
4 years ago

What we really need is a hook to allow us to disable redirect_guess_404_permalink() per #16557.

#15 @wonderboymusic
2 years ago

  • Milestone Future Release deleted
  • Resolution set to duplicate
  • Status changed from reopened to closed

I just uploaded a patch to #13041 that has way more nonsense in it https://core.trac.wordpress.org/attachment/ticket/13041/13041.diff

Let's work on it there.

Note: See TracTickets for help on using tickets.