Make WordPress Core

Opened 2 years ago

Last modified 33 hours ago

#53362 accepted defect (bug)

Invalid paginated requests not treated as such

Reported by: daleharrison's profile daleharrison Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.3 Priority: normal
Severity: normal Version: 5.7.2
Component: Canonical Keywords: has-unit-tests needs-refresh
Focuses: Cc:

Description

This is a follow-up to #40773.

The security department that scans our WordPress websites identified the issue resolved in #40773 as cross-site scripting, as our themes add the webpage's precise URL to Open Graph data (og:url) in the <head>.

So, we were happy to see the resolutions applied in that ticket. Unfortunately, while testing, we found that adding /0/ to any URL is still possible, e.g. https://example.com/about-us/0/ does not redirect back to the canonical https://example.com/about-us/.

Additionally, we have found that it is possible to append /page/ followed by a number to the URL of a page that does not support pagination, e.g. https://example.com/about-us/page/0/, https://example.com/about-us/page/12345/, etc.

In the latter example, the <title> also changes from "About Us" to "About Us – Page 12345" as WordPress seems to think this is a valid paginated page. The paged-12345 and page-paged-12345 classes are also added to the <body>. These specific tests were done using the Twenty Twenty-One theme.

Furthermore, appending /page/0/ to the page that displays blog posts does not trigger a 404 or a redirect, e.g. https://example.com/blog/ is "identical" to https://example.com/blog/page/0/.

Attachments (1)

53362.diff (6.4 KB) - added by SergeyBiryukov 10 months ago.

Download all attachments as: .zip

Change History (15)

#1 @henry.wright
2 years ago

Last edited 2 years ago by henry.wright (previous) (diff)

#2 @SergeyBiryukov
2 years ago

Hi there, welcome back to WordPress Trac! Thanks for the report.

Additionally, we have found that it is possible to append /page/ followed by a number to the URL of a page that does not support pagination, e.g. https://example.com/about-us/page/0/, https://example.com/about-us/page/12345/, etc.

Just noting that this part of the ticket is already being tracked in #28081.

#3 @SergeyBiryukov
10 months ago

#56298 was marked as a duplicate.

#4 @SergeyBiryukov
10 months ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 6.1
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

#5 follow-up: @mukesh27
8 months ago

@SergeyBiryukov Is this ticket on your radar for 6.1?

As we change test_redirect_canonical_with_nextpage_pagination test is it good to use ticket number for the this test?

Last edited 8 months ago by mukesh27 (previous) (diff)

This ticket was mentioned in PR #3388 on WordPress/wordpress-develop by SergeyBiryukov.


8 months ago
#6

  • Keywords has-unit-tests added

#7 in reply to: ↑ 5 @SergeyBiryukov
8 months ago

  • Milestone changed from 6.1 to 6.2

Replying to mukesh27:

As we change test_redirect_canonical_with_nextpage_pagination test is it good to use ticket number for the this test?

Thanks! As these tests cover multiple tickets, the ticket number is included in the data provider and passed to assertCanonical() as the third parameter.

It looks like this needs some more work, as the new test cases pass with this patch, but some older rewrite tests fail. Moving to 6.2 for now.

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


4 months ago

@costdev commented on PR #3388:


4 months ago
#9

Hi @SergeyBiryukov, could you merge/rebase on trunk and push to the PR so we can look into the test failures? 🙂

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


3 months ago

#11 @costdev
3 months ago

  • Milestone changed from 6.2 to 6.3

This ticket was discussed in the bug scrub and it was agreed to move this ticket to 6.3.

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


33 hours ago

@mukesh27 commented on PR #3388:


33 hours ago
#13

@SergeyBiryukov Can you please update PR and look into test failures?

#14 @mukesh27
33 hours ago

  • Keywords needs-refresh added; has-patch removed

This ticket was discussed in the recent bug scrub.

The PR needs some work as some unit tests are failing.

Prop to @oglekler

Note: See TracTickets for help on using tickets.