WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 18 months ago

#14773 reviewing defect (bug)

Error in slug parsing leads to unlimited URLs for the same article = duplicate content

Reported by: thermoman Owned by: dd32
Milestone: Future Release Priority: normal
Severity: normal Version: 2.5
Component: Canonical Keywords: 3.2-early has-patch
Focuses: Cc:

Description

an example says more than 1000 words:

right url:

http://ahmongwoman.wordpress.com/2010/09/02/i-am-not-hmong-and-i-dont-speak-spanish/

wrong urls:

http://ahmongwoman.wordpress.com/2010/09/02/i-am-not------hmong-and-i-dont-speak-spanish/

http://ahmongwoman.wordpress.com/2010/09/02/i----am-not-hmong-and-i-dont-speak-spanish/

http://ahmongwoman.wordpress.com/2010/09/02/i-am-not-----hmong-and-i-------dont-speak---------spanish/

Problem:

Wordpress returns the article with HTTP status code 200 for the wrong URLs.

This is a serious issue for people regarding search engine optimization (duplicate content).

Expected results:

Wordpress returns HTTP Status code 301 with Location-Header and right URL to the client.

Attachments (7)

12456.diff (1.2 KB) - added by wonderboymusic 2 years ago.
12456.2.diff (1.3 KB) - added by wonderboymusic 2 years ago.
tests.14773.1.diff (1.7 KB) - added by atimmer 22 months ago.
12456.3.diff (1.3 KB) - added by atimmer 21 months ago.
tests.14773.2.diff (970 bytes) - added by atimmer 21 months ago.
12456.4.diff (2.2 KB) - added by atimmer 20 months ago.
12456.5.diff (2.3 KB) - added by boonebgorges 7 months ago.
Contains .4.patch plus the extracted existing test. See #30284.

Download all attachments as: .zip

Change History (26)

comment:1 @thermoman5 years ago

Tested with several wordpress blogs - not only with wordpress.com hosted blogs.

Bug is confirmed to be an issue in 3.0 and 2.5

comment:2 @kawauso5 years ago

Discussed in #wordpress and explained <meta rel="canonical" />.

Behaviour with multiple dashes is inconsistent with behaviour for incomplete URLs.

comment:3 @scribu5 years ago

  • Component changed from Permalinks to Canonical

Shouldn't redirect_canonical() be taking care of this?

comment:4 @dd324 years ago

  • Keywords 3.2-early added; dash slug duplicate content removed
  • Milestone changed from Awaiting Review to Future Release
  • Owner set to dd32
  • Status changed from new to accepted

Shouldn't redirect_canonical() be taking care of this?

Thats where it should be handled, However, Canonical at present mainly covers redirecting query vars to rewritten url's, along with common duplicate content url's (such as those using %category%).

My patch on #12456 should be able to deal with this case.

@wonderboymusic2 years ago

comment:5 @wonderboymusic2 years ago

  • Milestone changed from Future Release to 3.6

Kudos to all of us for letting these bugs sit so long - the dd32 patch fixes this as well

comment:6 @markjaquith2 years ago

What about HTTP vs HTTPS? Wouldn't this change redirect from HTTPS to HTTP? That would be a breaking change. I think the check has to be more conservative and just make sure the post slug exists in the URL, or do a direct slug comparison.

@wonderboymusic2 years ago

comment:7 @wonderboymusic2 years ago

Now wrapped in set_url_scheme

comment:8 @dd322 years ago

  • Owner dd32 deleted
  • Status changed from accepted to assigned

comment:9 @ericlewis2 years ago

  • Keywords has-patch added

comment:10 @nacin23 months ago

  • Keywords needs-unit-tests added
  • Milestone changed from 3.6 to Future Release
  • Version set to 2.5

Needs unit tests like whoa.

comment:11 @wonderboymusic22 months ago

  • Milestone changed from Future Release to 3.7

@atimmer22 months ago

comment:12 follow-up: @atimmer22 months ago

I have added tests.14773.1.diff which tests common wordpress canonical functionality and adds this ticket to it as well, for which the tests fail.

According to the tests it only happens if all the words in the URL are already present, when only 1 word of the page_name is present the URL will be right because the page_name is not complete.

Edit:
Adding to this, the patch 12456.2.diff fixes only 1 of the testcases.
It fixes: "/this--should-be-resolved-" -> "/this-should-be-resolved/"
It does not fix: "/this----should---be---resolved-" -> "/this-should-be-resolved/"

Last edited 22 months ago by atimmer (previous) (diff)

comment:13 @atimmer22 months ago

  • Cc atimmermans@… added

comment:14 in reply to: ↑ 12 @duck_22 months ago

Replying to atimmer:

I have added tests.14773.1.diff which tests common wordpress canonical functionality and adds this ticket to it as well, for which the tests fail.

Thanks for the tests! Not a big deal, but I don't really like the name of the class. Maybe we just need to increase the number of tests in Tests_Canonical which actually already has a test for this ticket.

Edit:
Adding to this, the patch 12456.2.diff fixes only 1 of the testcases.
It fixes: "/this--should-be-resolved-" -> "/this-should-be-resolved/"
It does not fix: "/this----should---be---resolved-" -> "/this-should-be-resolved/"

This isn't a problem with the patch since these URLs 404 on trunk. It looks like the problem is that the multiple -s are being picked up as octet placeholders by sanitize_title_with_dashes(), i.e. ---be--- becomes %be. Maybe we should open a ticket to make a better placeholder.

Version 0, edited 22 months ago by duck_ (next)

@atimmer21 months ago

comment:15 @atimmer21 months ago

12456.3.diff refreshes the patch against current trunk and the new repository structure.

Last edited 21 months ago by atimmer (previous) (diff)

@atimmer21 months ago

comment:16 @atimmer21 months ago

Added tests.14773.2.diff

It seemed like the canonical test class already has a test for this ticket, I have added tests to test the dashes at the front and the back of the url. I also refreshed the patch and the patch fixes all 3 test cases.

comment:17 @nacin20 months ago

  • Milestone changed from 3.7 to 3.8

Hi atimmer, could you combine your tests and the patch together?

@atimmer20 months ago

comment:18 @atimmer20 months ago

12456.4.diff combines the unit tests and the fix.

comment:19 @nacin18 months ago

  • Keywords needs-unit-tests removed
  • Milestone changed from 3.8 to Future Release
  • Owner set to dd32
  • Status changed from assigned to reviewing

@boonebgorges7 months ago

Contains .4.patch plus the extracted existing test. See #30284.

Note: See TracTickets for help on using tickets.