WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#25418 closed defect (bug) (fixed)

bloginfo for pingback_url doesn't respect SSL

Reported by: technosailor Owned by: nacin
Milestone: 3.7 Priority: normal
Severity: normal Version: 2.6
Component: XML-RPC Keywords: has-patch commit
Focuses: Cc:

Description

Attached patch moves from using get_option(), which is not SSL aware, to using site_url() which is.

Attachments (2)

siteurl2.diff (5.5 KB) - added by technosailor 7 years ago.
Here's a broader patch that ensures we eat our dogfood throughout core, but I don't feel as strongly about it and would be okay pushing to 3.8 for the second patch. Still think original patch needs to be in 3.7
siteurl.diff (481 bytes) - added by technosailor 7 years ago.

Download all attachments as: .zip

Change History (13)

#1 @SergeyBiryukov
7 years ago

  • Component changed from General to XML-RPC
  • Milestone changed from Awaiting Review to 3.7
  • Version changed from trunk to 2.6

#2 @nacin
7 years ago

  • Milestone changed from 3.7 to Awaiting Review
  • Severity changed from major to normal

Existing issue, not major.

#3 follow-up: @technosailor
7 years ago

Kind of a security issue, so I'd say it's major.

#4 in reply to: ↑ 3 ; follow-ups: @nacin
7 years ago

  • Milestone changed from Awaiting Review to 3.7

Replying to technosailor:

Kind of a security issue, so I'd say it's major.

I could see how this could be a problem, sure. Can anyone make a case that this could (or would not) break something?

#5 in reply to: ↑ 4 @technosailor
7 years ago

Replying to nacin:

I could see how this could be a problem, sure. Can anyone make a case that this could (or would not) break something?

All you need is a CSRF attack, now or in the future, to corrupt pingback_url and then encrypted traffic would leak. Theoretically.

#6 in reply to: ↑ 4 @technosailor
7 years ago

Replying to nacin:

I could see how this could be a problem, sure. Can anyone make a case that this could (or would not) break something?

As for breaking things, we're not changing bloginfo()/get_bloginfo() and site_url() already wraps, intelligently, around the siteurl option so... I can't foresee how this would possibly break anything. Eating our own dogfood and whatnot. If site_url() broke something, we'd already see it elsewhere... Plus, we have time for this to be baked in during the beta period

#7 @jeremyfelt
7 years ago

Related: #9008

Last edited 7 years ago by jeremyfelt (previous) (diff)

#8 @nacin
7 years ago

  • Keywords commit added

All other references to xmlrpc.php use an SSL-aware URL. #9008 also proposes an SSL URL for the pingback URL in the description, but that is its only mention. This may be because that ticket was focused on home_url(), while xmlrpc.php should be using site_url(). I also see no mention in #7001.

@technosailor
7 years ago

Here's a broader patch that ensures we eat our dogfood throughout core, but I don't feel as strongly about it and would be okay pushing to 3.8 for the second patch. Still think original patch needs to be in 3.7

@technosailor
7 years ago

#9 @nacin
7 years ago

Each instance in siteurl2.diff needs to be reviewed to make sure the current protocol's page is proper for that result. I see a few that could cause problems, a few that are unnecessary, and a few that are definitely correct.

#10 @nacin
7 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 25671:

Ensure that get_bloginfo( 'pingback_url' ) uses site_url(), for SSL awareness.

props technosailor.
fixes #25418.

#11 @nacin
7 years ago

In 25672:

Use site_url() in theme-compat.

props technosailor.
see #25418.

Note: See TracTickets for help on using tickets.