WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 4 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 (6)

12456.diff (1.2 KB) - added by wonderboymusic 15 months ago.
12456.2.diff (1.3 KB) - added by wonderboymusic 14 months ago.
tests.14773.1.diff (1.7 KB) - added by atimmer 8 months ago.
12456.3.diff (1.3 KB) - added by atimmer 7 months ago.
tests.14773.2.diff (970 bytes) - added by atimmer 7 months ago.
12456.4.diff (2.2 KB) - added by atimmer 6 months ago.

Download all attachments as: .zip

Change History (25)

comment:1 thermoman4 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 kawauso4 years ago

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

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

comment:3 scribu3 years ago

  • Component changed from Permalinks to Canonical

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

comment:4 dd323 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.

wonderboymusic15 months ago

comment:5 wonderboymusic15 months 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 markjaquith14 months 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.

wonderboymusic14 months ago

comment:7 wonderboymusic14 months ago

Now wrapped in set_url_scheme

comment:8 dd3214 months ago

  • Owner dd32 deleted
  • Status changed from accepted to assigned

comment:9 ericlewis12 months ago

  • Keywords has-patch added

comment:10 nacin9 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 wonderboymusic9 months ago

  • Milestone changed from Future Release to 3.7

atimmer8 months ago

comment:12 follow-up: atimmer8 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 8 months ago by atimmer (previous) (diff)

comment:13 atimmer8 months ago

  • Cc atimmermans@… added

comment:14 in reply to: ↑ 12 duck_8 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. (Edit: #25021)

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

atimmer7 months ago

comment:15 atimmer7 months ago

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

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

atimmer7 months ago

comment:16 atimmer7 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 nacin7 months ago

  • Milestone changed from 3.7 to 3.8

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

atimmer6 months ago

comment:18 atimmer6 months ago

12456.4.diff combines the unit tests and the fix.

comment:19 nacin4 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
Note: See TracTickets for help on using tickets.