WordPress.org

Make WordPress Core

Opened 13 years ago

Closed 2 months ago

Last modified 7 weeks ago

#5272 closed defect (bug) (fixed)

WordPress allows anonymous user to see slug for private post by guessing post number

Reported by: tzafrir Owned by: SergeyBiryukov
Milestone: 5.7 Priority: normal
Severity: normal Version: 2.3.1
Component: Security Keywords: has-patch has-unit-tests has-dev-note
Focuses: Cc:

Description

I have pretty permalinks enabled, and I set a post as private.

Entering http://blog.url/?p=(postid) will redirect the user, any user, to http://blog.url/perma/link/, and only then give him a 404 error.

Depending on permalink structure, this shows the private post's title to anyone who figures out its post number.

Attachments (3)

5272.patch (1.1 KB) - added by pishmishy 13 years ago.
Basic fix for this issue
5272.diff (48.7 KB) - added by peterwilsoncc 3 months ago.
5272.2.diff (38.7 KB) - added by peterwilsoncc 2 months ago.

Download all attachments as: .zip

Change History (41)

#1 @tzafrir
13 years ago

Oh, and on decent hours I even know how to spell WordPress :(

#2 @Viper007Bond
13 years ago

  • Keywords needs-patch added
  • Summary changed from Wodpress allows anonymous user to see slug for private post by guessing post number to WordPress allows anonymous user to see slug for private post by guessing post number

Luckily, you can fix it. :)

#3 @pishmishy
13 years ago

  • Owner changed from anonymous to pishmishy
  • Status changed from new to assigned

Confirming that the bug exists in trunk. Worth fixing as the slug may have been automatically derived from post data that the user didn't want to disclose.

@pishmishy
13 years ago

Basic fix for this issue

#4 @pishmishy
13 years ago

  • Keywords has-patch canonical redirection private added; needs-patch removed

Fix is to add an additional condition to be satisfied before canonical redirection can take place.

Not entirely happy with my patch but it does fix the problem. Someone more familiar with query.php can probably come up with an is_private() function. is_empty works just as well though.

#5 @pishmishy
13 years ago

Tempted to close this one too. I think it's important to disclose as little information as possible but the fix has been on offer and there doesn't appear to be much demand for it.

#6 @tzafrir
13 years ago

Too bad - I can't see how this is not a major privacy issue.

If we ever want to see WordPress used as a CMS for a major corporation, we can't have their secret keynote surprise open for the world to see.

#7 @Denis-de-Bernardy
12 years ago

  • Keywords needs-patch added; has-patch canonical redirection private removed

The patch is invalid. There is some reprocessing of 404 code further down, that ends up not getting processed. Imo, what WP should do here is return a 403 error.

#8 @Denis-de-Bernardy
12 years ago

  • Milestone 2.9 deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

there's a dup of this one somewhere, and it shoud get wontfixed too.

#9 @peterwilsoncc
14 months ago

  • Component changed from General to Security
  • Keywords needs-unit-tests added
  • Milestone set to Future Release
  • Resolution wontfix deleted
  • Severity changed from major to normal
  • Status changed from closed to reopened

Reopening following discussion with @whyisjake.

This ticket was mentioned in PR #433 on WordPress/wordpress-develop by donmhico.


9 months ago

  • Keywords has-patch added; needs-patch removed

Prevent canonical redirect for private post.

Trac ticket: https://core.trac.wordpress.org/ticket/5272

TODO:

  1. Unit test.
  2. Allow the redirect if user is logged and has permission to read the post.

#11 @donmhico
9 months ago

Added a patch above however it still need some work. Will try to update the PR soon.

#12 @donmhico
9 months ago

  • Keywords needs-testing added; needs-unit-tests removed

Updated the PR with unit tests. Need more people to test :)

#13 @SergeyBiryukov
9 months ago

  • Milestone changed from Future Release to 5.6
  • Owner changed from pishmishy to SergeyBiryukov
  • Status changed from reopened to reviewing

#14 @TimothyBlynJacobs
9 months ago

Should this be looking at get_post_status_object()->private?

This ticket was mentioned in PR #479 on WordPress/wordpress-develop by peterwilsoncc.


8 months ago

  • Keywords has-unit-tests added

#16 @prbot
8 months ago

peterwilsoncc commented on PR #433:

@donmhico

In parallel, I'd written some code elsewhere that I have added as PR #479 .

The code we'd written is similar so I'd make sure you get props.

There are few places in your patch that redirects are determined that had been missed, so I've expanded upon that to catch other places.

For the unit tests I expanded them to include both positive and negative use cases for all types of users, including the original author, you'll find them in the other PR.

If you think I have missed anything, please add comments.

Peter

#17 follow-up: @peterwilsoncc
8 months ago

As the friendly bot points out, I've pushed a PR to https://github.com/WordPress/wordpress-develop/pull/479

@TimothyBlynJacobs I've included a check for get_post_status_object( $redirect_obj->post_status )->public, do you think I should logic check for private or internal too?

@SergeyBiryukov I included some tests for comment feed requests but was getting some unexpected results with ?name=slug&feed=rss2 style requests that I think are unrelated to this ticket. Do you know if they work?

#18 in reply to: ↑ 17 @TimothyBlynJacobs
8 months ago

Replying to peterwilsoncc:

@TimothyBlynJacobs I've included a check for get_post_status_object( $redirect_obj->post_status )->public, do you think I should logic check for private or internal too?

I really don't know. I don't think it's valid for more than one of public, protected or private to be set to true. But since the function doesn't enforce that perhaps some developers have accidentally done that?

I guess the publicly_queryable post status arg might also be the thing to check? At a first glance, I can't find it being used anywhere in Core though.

#19 @whyisjake
5 months ago

In conversation with @dd32:

I’d personally lean towards get_post_status_object( $status )->public as the filterable way, but private isn’t a public post status.

#20 @peterwilsoncc
5 months ago

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

In 49563:

Canonical: Prevent ID enumeration of private post slugs.

Add check to redirect_canonical() to ensure the destination post is not using a private post status.

Props dd32, Denis-de-Bernardy, donmhico, helen, nacin, peterwilsoncc, pishmishy, TimothyBlynJacobs, tzafrir, Viper007Bond, whyisjake.
Fixes #5272.

This ticket was mentioned in PR #745 on WordPress/wordpress-develop by peterwilsoncc.


5 months ago

https://core.trac.wordpress.org/ticket/5272

Starting with some unit tests -- they fail.

#22 @peterwilsoncc
5 months ago

In 49622:

Permalinks: Prevent attachment pages 404ing following [49563].

This largely reverts [49563] due to attachment pages returning 404: File not found errors when they use the inherit status.

Permalink changes to attachment pages are retained when they are descendants of trashed or deleted posts.

Props Toro_Unit, helen, johnbillion, peterwilsoncc.
Fixes #51776.
See #5272.

#23 @peterwilsoncc
5 months ago

  • Milestone changed from 5.6 to Future Release
  • Resolution fixed deleted
  • Status changed from closed to reopened

#25 @peterwilsoncc
3 months ago

  • Keywords needs-testing removed
  • Milestone changed from Future Release to 5.7

Moving this on to the 5.7 milestone for another attempt. At time of writing tests are failing but hope to get it down soon.

#26 @peterwilsoncc
3 months ago

In 5272.diff

  • Actually fixes issue reported in #51776 (this required half a fix of #52373 but it returns ugly links rather than pretty permalinks)
  • redirect_canonical() changes pretty much the same as last time but I think the logic is clearer.
  • New function wp_force_ugly_post_permalink() in link-template.php to unify the check in each of the three permalink generating functions: pages, posts and attachments
  • New functions is_post_status_viewable() and is_post_publicly_viewable() to assist with cleaning up logic (fixes #49380)

Props very much due to @TimothyBlynJacobs for assisting with review on the PR.

@peterwilsoncc
3 months ago

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


2 months ago

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


2 months ago

#29 @peterwilsoncc
2 months ago

  • Keywords needs-dev-note added

In 5272.2.diff:

As above with:

  • A couple of minor comment and docblock fixes
  • Removes fix for #49380 committed in [50130].

Requires dev note for new function. It won't need a full one as it's mainly intended for internal use but may end up been used by plugins using either the pre_post_link or post_link filters.

#30 @peterwilsoncc
2 months ago

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

In 50132:

Canonical: Prevent ID enumeration of private post slugs.

Add check to redirect_canonical() to ensure private posts only redirect for logged in users.

Modifies the read_post mata capability to user get_post_status() rather than the post's post_status property to allow attachments to redirect based on the inherited post status.

Introduces wp_force_ugly_post_permalink() to unify the check to determine if an ugly link should be displayed in each of the functions used for determining permalinks: get_permalink(), get_post_permalink(), _get_page_link() and get_attachment_link().

Improves logic of get_attachment_link() to validate parent post and resolution of inherited post status. This is an incomplete fix of #52373 to prevent the function returning links resulting in a file not found error. Required to unblock this ticket.

Props peterwilsoncc, TimothyBlynJacobs.
See #52373.
Fixes #5272.

#31 @SergeyBiryukov
2 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[50132] looks good to me, but I'm not quite a fan of using "ugly" in a function name :)

Since these are called "Plain" in Permalink Settings UI, could we rename it to wp_force_plain_post_permalink()?

#32 follow-up: @peterwilsoncc
2 months ago

@SergeyBiryukov I went with ugly because that was how it was referred to in a comment (not acode) of get_sample_permalink() but mainly because it was the polar opposite of pretty.

I'm fine with it be renamed but what is the hesitancy about the name?

#33 in reply to: ↑ 32 ; follow-up: @SergeyBiryukov
2 months ago

Replying to peterwilsoncc:

I'm fine with it be renamed but what is the hesitancy about the name?

It just seemed like a better idea to align with the existing wording used in the UI rather than a single inline comment, see [35604] / #34509.

I think the current wording might also have unnecessary negative connotations in some contexts, which I'd like to avoid in favor of a more neutral one, pretty much like [48121] / #50413.

Maybe it's just me though, I'd like to know others' thoughts too :)

#34 in reply to: ↑ 33 @peterwilsoncc
2 months ago

Replying to SergeyBiryukov:

It just seemed like a better idea to align with the existing wording used in the UI rather than a single inline comment, see [35604] / #34509.

I think the current wording might also have unnecessary negative connotations in some contexts, which I'd like to avoid in favor of a more neutral one, pretty much like [48121] / #50413.

Thanks, that makes sense and the negative connotations didn't occur to me. I'll update prior to the next beta.

This ticket was mentioned in PR #988 on WordPress/wordpress-develop by peterwilsoncc.


2 months ago

#36 @peterwilsoncc
2 months ago

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

In 50282:

Canonical: Rename wp_force_plain_ugly_permalink() to match UI terminology.

Rename wp_force_plain_ugly_permalink() to wp_force_plain_post_permalink() to match terminology used in the WordPress dashboard.

Follow up to [50132].
Props SergeyBiryukov.
Fixes #5272.

Note: See TracTickets for help on using tickets.