WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 7 years ago

#12868 closed defect (bug) (fixed)

General Settings Page Needs Error Checking

Reported by: Josh Jones Owned by: technosailor
Milestone: 3.0 Priority: high
Severity: major Version: 2.9.2
Component: Administration Keywords: has-patch ux-feedback
Focuses: Cc:

Description

When adjusting the default "general" settings in the dashboard, if one enters a blank value in the "WordPress address (URL)" input area and saves changes there is no error checking to prevent the blank value from being saved in the database.

This is turn messes up the entire dashboard and returns the WordPress error page with a message that reads:

"One or more database tables are unavailable. The database may need to be repaired."

Attachments (6)

12868.diff (869 bytes) - added by blepoxp 10 years ago.
Partial fix adds notification. Still need to kill update.
12868-new.diff (1.0 KB) - added by technosailor 10 years ago.
This should do the trick.
12868-3.diff (1.9 KB) - added by technosailor 10 years ago.
New patch makes sure the value _looks_ like a valid URL using a new function is_url() which provides minimal validation. Much more could be done with it but domains like 'localhost' fail traditional URL regex's
12868-4.diff (2.0 KB) - added by technosailor 10 years ago.
Ok, try this
12868-5.diff (2.5 KB) - added by blepoxp 10 years ago.
Hey Aaron, That worked on my system... though I realized we were having the same problem with the admin email as well. An error was being reported, but the option was getting updated anyway. Here is your diff updated to prevent emails from being updated if its not formatted correctly.
12868-6.diff (2.7 KB) - added by technosailor 10 years ago.
-1 on admin_email. I think the scope of this ticket is basically about the site breaking if siteurl is blank. However, attaching patch to handle admin email as well and making it consistent with the rest of the code.

Download all attachments as: .zip

Change History (18)

#1 @nacin
10 years ago

  • Milestone changed from 2.9.3 to 3.0

#2 @blepoxp
10 years ago

  • Cc glenn@… added
  • Keywords has-patch dev-feedback added

Looking into this ticket if found what I think is a bug in the sanitize_options() function (wp-includes/formatting.php).

My first solution was to just call the add_settings_error on line 2440 (taking my cue from line 2361 for the admin_email).

This didn't work though because update_option here doesn't validate what's been returned... which means it still gets updated down on line 532.

This fact can be confirmed by using the admin email as an example. You can leave it blank, submit the form, and get the error - but it still updates the DB with an empty string. As noted in this ticket, the bug is more destructive if the Site URL is left empty.

I've attached a half updated patch (it includes the error for the empty Site URL) but we need to decide how to prevent the option from being updated.

I would have just included a check on line 502 but wasn't sure how that would effect other options.

Can someone give me the preferred way to proceed?

@blepoxp
10 years ago

Partial fix adds notification. Still need to kill update.

#3 @dd32
10 years ago

In sanitize_option(), we can do $value = get_option($option) to prevent the option update going through.

@technosailor
10 years ago

This should do the trick.

@technosailor
10 years ago

New patch makes sure the value _looks_ like a valid URL using a new function is_url() which provides minimal validation. Much more could be done with it but domains like 'localhost' fail traditional URL regex's

#4 @dd32
10 years ago

Perhaps this could be changed to:

 case 'siteurl': 
   $value = esc_url_raw($value); 
   if ( !is_url( $value ) ) { 
 	$value = get_option( $option ); // Resets option to stored val in case of empty 
 	if ( function_exists('add_settings_error') 
 	    add_settings_error('siteurl', 'invalid_siteurl', __('The WordPress address you submitted was not in the right format. Please enter a valid URL.')); 
   } 
   break; 

that way it'll prevent against acidental changes elsewhere too perhaps?

I'll review it more in the morning

@technosailor
10 years ago

Ok, try this

@blepoxp
10 years ago

Hey Aaron, That worked on my system... though I realized we were having the same problem with the admin email as well. An error was being reported, but the option was getting updated anyway. Here is your diff updated to prevent emails from being updated if its not formatted correctly.

#5 @technosailor
10 years ago

  • Owner set to technosailor
  • Status changed from new to accepted

@technosailor
10 years ago

-1 on admin_email. I think the scope of this ticket is basically about the site breaking if siteurl is blank. However, attaching patch to handle admin email as well and making it consistent with the rest of the code.

#6 follow-ups: @dd32
10 years ago

  • Keywords ux-feedback added; error checking blank value dev-feedback removed

Had to change the patch slightly.

  • Changed the strings, Can I get some ux-feedback on the strings?
  • Had to rework the if branching, The patch did not protect against invalid url's at all, 'dubya dubya dubya' would result in a url of 'http://dubyadubyadubya' thanks to esc_url() forcing it into a URL looking string
  • Removed is_url(), I feel that people might mistake its purpose, I'd also like to expand on the invalid url prevention in 3.1 to prevent URL's which do not load the Current WordPress instance. (Theres another ticket somewhere for this)

#7 in reply to: ↑ 6 ; follow-up: @technosailor
10 years ago

dd32: have a patch to attach? :)

#8 in reply to: ↑ 7 @dd32
10 years ago

Replying to technosailor:

dd32: have a patch to attach? :)

Did 1 better: (It seems the SVN notice didnt flow through to this ticket)

(In [14231]) Add basic email/url validation to General options page. Prevents users entering a invalid Admin email or WordPress/Site Address which is not in URL form. Props technosailor for initial patch, slightly reworked. See #12868

#9 @technosailor
10 years ago

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

Ah cool. Thanks

#10 @raymor
10 years ago

It's quite unfortunate that the WP devs see fit to break
everything but the simplest little site in order to pander to
the clueless. This kind of improper use of fully qualified
URLs breaks authentication, load balancing, cookies, and
creates lots of other subtle little bugs.

I had hoped fully qualified URL abuse was going to stop around
the time of HTML 3.2, when designers and engineers realized how
many problems are caused by this kind of improper use of FQ URLs,
but thirteen years later the WordPress team is still making the
same mistakes that we all thought we had learned from decades ago.

I guess we can look forward to user agent based browser sniffing
in WP next, and perhaps some ActiveX controls.

Perhaps what's needed is a Wordpress Enterprise fork, and let the
mainline Wordpress pander to the lowest common denominator, which
seems to be the trend, and introduce "cool" new features without
regard for established standards and how many professional sites
are broken by these spontaneous changes. The Enterprise version
could be aimed at true professionals and follow standards, and
avoid breaking compatibility with established protocols.

#11 in reply to: ↑ 6 @nofearinc
7 years ago

Replying to dd32:

  • Removed is_url(), I feel that people might mistake its purpose, I'd also like to expand on the invalid url prevention in 3.1 to prevent URL's which do not load the Current WordPress instance. (Theres another ticket somewhere for this)

Is that still a valid point? There are different cases of user input for website fields and some get in as relative URLs instead due to the lack of a standardized is_url function. Any thoughts of adding one as helper?

Note: See TracTickets for help on using tickets.