Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#27498 closed defect (bug) (fixed)

Canonical redirect is applied upon POST request

Reported by: caxelsson's profile c.axelsson Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 4.0 Priority: normal
Severity: normal Version: 3.1
Component: Canonical Keywords: has-patch 4.0-early
Focuses: Cc:

Description (last modified by SergeyBiryukov)

If $_POST is empty a canonical redirect is still made even though it should be ignored upon all POST requests. As this redirect occurs before user have a chance to handle the request themselves in a template_redirect filter it prevents users to write proper REST APIs that include POST requests with no data.

A patch that uses $_SERVER['REQUEST_METHOD'] instead of relying on the $_POST variable to identify request type is attached.

Attachments (4)

correctly-identify-post-request.patch (712 bytes) - added by c.axelsson 10 years ago.
correctly-identify-post-request-v2.patch (711 bytes) - added by c.axelsson 10 years ago.
27498.patch (575 bytes) - added by SergeyBiryukov 10 years ago.
27498.2.patch (589 bytes) - added by SergeyBiryukov 10 years ago.

Download all attachments as: .zip

Change History (17)

#1 @SergeyBiryukov
10 years ago

  • Description modified (diff)
  • Version changed from trunk to 3.1

Introduced in [5978], modified in [16797].

Last edited 10 years ago by SergeyBiryukov (previous) (diff)

#2 @SergeyBiryukov
10 years ago

  • Keywords has-patch added

#3 @SergeyBiryukov
10 years ago

  • Keywords 4.0-early added
  • Milestone changed from Awaiting Review to Future Release

#4 @c.axelsson
10 years ago

I have attached an updated the patch that makes sure that the request actually is a GET request. It's needed for HTTP OPTIONS, DELETE, PATCH etc.

#5 @SergeyBiryukov
10 years ago

  • Milestone changed from Future Release to 4.0

#6 @SergeyBiryukov
10 years ago

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

In 28958:

Canonical redirects should only be applied for GET requests.

props c.axelsson.
fixes #27498.

#7 @nacin
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I don't see why HEAD requests should be ignored.

#8 @markjaquith
10 years ago

Agree with Nacin. HEAD requests really should have canonical redirects. I think this is a regression otherwise.

#9 @SergeyBiryukov
10 years ago

27498.patch adds HEAD requests to the condition.

#10 @markjaquith
10 years ago

We should probably strtoupper() first. PHP does not guarantee the value to be uppercase. The HTTP specs do, but you never know what a requester will do.

#12 @nacin
10 years ago

template-loader.php is way simpler:

if ( 'HEAD' === $_SERVER['REQUEST_METHOD'] && apply_filters( 'exit_on_http_head', true ) )

This also happens to fail when it isn't set, but you're usually not running template-loader.php via CLI.

I'm fine with adjusting template-loader.php later, just pointing out where else we do this.

#13 @nacin
10 years ago

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

In 29663:

Canonical: Apply redirects to HEAD requests too. Adjusts [28958].

props SergeyBiryukov.
fixes #27498.

Note: See TracTickets for help on using tickets.