Opened 3 years ago
Closed 13 months ago
#13839 closed defect (bug) (fixed)
Configuration script does not allow apostrophes, blank prefixes
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Priority: | low | Milestone: | 3.4 |
| Component: | Upgrade/Install | Version: | 2.9.2 |
| Severity: | normal | Keywords: | has-patch commit |
| Cc: | me@… |
Description
When using the wp-admin/setup-config.php to create a configuration file automatically, the following do not work properly:
- Using a MySQL username with an apostrophe.
- Using a MySQL password with an apostrophe.
- Using a blank prefix.
The expected outcomes is that Wordpress:
- Runs
- Uses tables with no prefix (not swp)
The actual outcome is that the configuration file has errors due to unclosed apostrophes, and the tables created start with wp_. Adding addslashes() to certain statements and removing
Line 152:
if (empty($prefix)) $prefix = 'wp_';
would correct this.
Attachments (8)
Change History (34)
Replying to nacin:
1 and 2 look fine, but I don't think we should simply create tables without a prefix if the field gets cleared.
Please correct me if I'm wrong, but if the Wordpress instance is getting its own database, there often is no need for a prefix. This applied for our use case at least.
anthonycole — 3 years ago
comment:5
anthonycole — 3 years ago
- Keywords has-patch added
- Milestone changed from Awaiting Review to Future Release
- Owner set to dd32
- Status changed from new to assigned
Related: #16229
Like nacin I don't believe we should support a blank prefix in the wp-config.php wizard, it should be throwing an error in that case instead. Being able to set a blank/empty prefix via a manual file creation is a bit more deliberate
Also note, Table prefixes are used in more than just table names, they're also used for per-site capabilities(Multisite and shared user tables, etc).
The patches here fix ' being in a username or password, but not the database name, but also break " being in a password. As an addition \ would also break it (as it's not escaped properly either).
Looking at this, it looks like addcslashes($.., "\\'"); is what should be used, which escapes both \ and ' whilst leaving " alone. An example password would be: 1'2\3"4 which should end up like this: define('DB_PASSWORD', '1\'2\\3"4');
Attached is a patch for testing based on addcslashes and erroring out on an empty prefix
SergeyBiryukov — 21 months ago
comment:8
in reply to:
↑ 7
SergeyBiryukov — 21 months ago
Replying to dd32:
Attached is a patch for testing based on addcslashes and erroring out on an empty prefix
13839.2.diff still doesn't allow apostrophes in a password, resulting in "Error establishing a database connection".
In 13839.3.diff I've added stripslashes() before trying to connect to a database, and it seems to works properly with the example password of 1'2\3"4.
13839.2.diff still doesn't allow apostrophes in a password, resulting in "Error establishing a database connection".
I had to edit the patch to remove some other changes, so I must've removed too much in the process.
I'm not really happy with the patch to start with, the changes to wp_die() are half-baked in this patch, the refactoring in #18180 is really needed IMO.
I'm going to rework the stripslashes()/addcslashes() changes specifically from the patch and go from there I think, That'll at least fix this tickets main issue. For consistancy, We should probably apply this to the Hostname and the Prefix as well (as noted in #18180) even although Apostrophes should never be in a hostname.
comment:10
nacin — 21 months ago
Would a prefix that requires slashes even work? I mean, that seems like most queries would then break. We don't quote tables in most of our SQL.
comment:11
dd32 — 21 months ago
Would a prefix that requires slashes even work?
Nope. And it shouldn't be able to be set either, as there's a check for a-z0-9, I was purely going for code consistency there, but we can probably just play it safe and ignore the slashing there..
Closed #19547 as a duplicate.
comment:13
ryan — 16 months ago
Don't we need to run wp_magic_quotes() before doing unconditional adds and strips since setup-config.php is not loading wp-settings.php?
comment:14
nacin — 15 months ago
- Milestone changed from Future Release to 3.4
SergeyBiryukov — 15 months ago
SergeyBiryukov — 15 months ago
13839.4.diff is the refreshed the patch after the changes in #18180. Added wp_magic_quotes() (requires formatting.php).
13839.5.diff adds/strips slashes for all the five variables for consistency, as suggested by dd32.
See also #19970 for empty prefix warning with a manually created wp-config.php.
comment:16
follow-up:
↓ 17
ryan — 14 months ago
Let's toss in wp_unregister_GLOBALS() as well.
SergeyBiryukov — 14 months ago
comment:17
in reply to:
↑ 16
SergeyBiryukov — 14 months ago
- Severity changed from trivial to normal
comment:19
nacin — 13 months ago
In [20482]:
comment:20
nacin — 13 months ago
In [20483]:
comment:21
nacin — 13 months ago
comment:22
nacin — 13 months ago
In [20484]:
comment:23
nacin — 13 months ago
In [20485]:
SergeyBiryukov — 13 months ago
comment:24
nacin — 13 months ago
- Keywords commit added
comment:25
nacin — 13 months ago
- Component changed from Administration to Upgrade/Install
comment:26
ryan — 13 months ago
- Resolution set to fixed
- Status changed from assigned to closed
In [20661]:

1 and 2 look fine, but I don't think we should simply create tables without a prefix if the field gets cleared.