#12868 closed defect (bug) (fixed)
General Settings Page Needs Error Checking
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (18)
#3
@
15 years ago
In sanitize_option(), we can do $value = get_option($option) to prevent the option update going through.
@
15 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
@
15 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
@
15 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.
@
15 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:
↓ 7
↓ 11
@
15 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)
#8
in reply to:
↑ 7
@
15 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
#10
@
15 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
@
11 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?
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?