WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 3 years ago

#13839 closed defect (bug) (fixed)

Configuration script does not allow apostrophes, blank prefixes

Reported by: incaren Owned by: dd32
Milestone: 3.4 Priority: low
Severity: normal Version: 2.9.2
Component: Upgrade/Install Keywords: has-patch commit
Focuses: Cc:

Description

When using the wp-admin/setup-config.php to create a configuration file automatically, the following do not work properly:

  1. Using a MySQL username with an apostrophe.
  2. Using a MySQL password with an apostrophe.
  3. Using a blank prefix.

The expected outcomes is that Wordpress:

  1. Runs
  2. 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)

patch.txt (847 bytes) - added by anthonycole 5 years ago.
13839.diff (1.3 KB) - added by anthonycole 5 years ago.
Submitted first patch incorrectly
13839.2.diff (2.4 KB) - added by dd32 4 years ago.
13839.3.diff (2.6 KB) - added by SergeyBiryukov 4 years ago.
13839.4.diff (2.7 KB) - added by SergeyBiryukov 3 years ago.
13839.5.diff (3.0 KB) - added by SergeyBiryukov 3 years ago.
13839.6.diff (3.2 KB) - added by SergeyBiryukov 3 years ago.
13839.7.diff (2.7 KB) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (34)

comment:1 follow-up: @nacin5 years ago

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

comment:2 in reply to: ↑ 1 @incaren5 years ago

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.

comment:3 @nacin5 years ago

It doesn't need a prefix, but it doesn't hurt either.

@anthonycole5 years ago

comment:4 @rmccue5 years ago

  • Cc me@… added

@anthonycole5 years ago

Submitted first patch incorrectly

comment:5 @anthonycole5 years ago

  • Keywords has-patch added

comment:6 @nacin5 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Owner set to dd32
  • Status changed from new to assigned

comment:7 follow-up: @dd324 years ago

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

@dd324 years ago

@SergeyBiryukov4 years ago

comment:8 in reply to: ↑ 7 @SergeyBiryukov4 years 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.

comment:9 @dd324 years ago

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 @nacin4 years 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 @dd324 years 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..

comment:12 @SergeyBiryukov3 years ago

Closed #19547 as a duplicate.

comment:13 @ryan3 years 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 @nacin3 years ago

  • Milestone changed from Future Release to 3.4

@SergeyBiryukov3 years ago

@SergeyBiryukov3 years ago

comment:15 @SergeyBiryukov3 years 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: @ryan3 years ago

Let's toss in wp_unregister_GLOBALS() as well.

@SergeyBiryukov3 years ago

comment:17 in reply to: ↑ 16 @SergeyBiryukov3 years ago

Replying to ryan:

Let's toss in wp_unregister_GLOBALS() as well.

Done in 13839.6.diff.

comment:18 @SergeyBiryukov3 years ago

  • Severity changed from trivial to normal

comment:19 @nacin3 years ago

In [20482]:

Do sanity checks for register_globals and magic quotes in setup-config.php. see #13839.

comment:20 @nacin3 years ago

In [20483]:

Escape special characters when outputting DB failures. see #13839.

comment:21 @nacin3 years ago

[20482] and [20483] are both good candidates to be backported to the 3.3 branch.

comment:22 @nacin3 years ago

In [20484]:

Do sanity checks for register_globals and magic quotes in setup-config.php. see #13839 for the 3.3 branch.

comment:23 @nacin3 years ago

In [20485]:

Escape special characters when outputting DB failures. see #13839 for the 3.3 branch.

@SergeyBiryukov3 years ago

comment:24 @nacin3 years ago

  • Keywords commit added

comment:25 @nacin3 years ago

  • Component changed from Administration to Upgrade/Install

comment:26 @ryan3 years ago

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

In [20661]:

setup-config.php cleanups

  • Don't allow an empty prefix
  • Make slashing consistent and sane

Props SergeyBiryukov
Fixes #13839

Note: See TracTickets for help on using tickets.