WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 21 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 3 years ago.
12456.2.diff (1.3 KB) - added by wonderboymusic 3 years ago.
tests.14773.1.diff (1.7 KB) - added by atimmer 2 years ago.
12456.3.diff (1.3 KB) - added by atimmer 2 years ago.
tests.14773.2.diff (970 bytes) - added by atimmer 2 years ago.
12456.4.diff (2.2 KB) - added by atimmer 2 years ago.
12456.5.diff (2.3 KB) - added by boonebgorges 10 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 @dd325 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.

@wonderboymusic3 years ago

comment:5 @wonderboymusic3 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 @markjaquith3 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.

@wonderboymusic3 years ago

comment:7 @wonderboymusic3 years ago

Now wrapped in set_url_scheme

comment:8 @dd323 years ago

  • Owner dd32 deleted
  • Status changed from accepted to assigned

comment:9 @ericlewis2 years ago

  • Keywords has-patch added

comment:10 @nacin2 years 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 @wonderboymusic2 years ago

  • Milestone changed from Future Release to 3.7

@atimmer2 years ago

comment:12 follow-up: @atimmer2 years 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 2 years ago by atimmer (previous) (diff)

comment:13 @atimmer2 years ago

  • Cc atimmermans@… added

comment:14 in reply to: ↑ 12 @duck_2 years 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 2 years ago by duck_ (next)

@atimmer2 years ago

comment:15 @atimmer2 years ago

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

Last edited 2 years ago by atimmer (previous) (diff)

@atimmer2 years ago

comment:16 @atimmer2 years 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 @nacin2 years ago

  • Milestone changed from 3.7 to 3.8

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

@atimmer2 years ago

comment:18 @atimmer2 years ago

12456.4.diff combines the unit tests and the fix.

comment:19 @nacin21 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

@boonebgorges10 months ago

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

Note: See TracTickets for help on using tickets.