add_magic_quotes() inappropriately recasts data types

[Veraxus]

Discovered as a result of: ​https://github.com/sebastianbergmann/php-code-coverage/issues/708

wp_magic_quotes() applies add_magic_quotes() to a wide variety of globals, including $_SERVER. Unfortunately, add_magic_quotes() indiscriminately calls addslashes() on every single non-array value, effectively recasting integers to strings.

For instance, $_SERVER['REQUEST_TIME'], which is a unix timestamp, gets recast to a string of numbers, breaking downstream functionality (like PhpUnit) that expects the timestamp to actually be a timestamp.

add_magic_quotes() needs to be updated to only apply addslashes() when the type of the value is already a string.

[SergeyBiryukov]

Related: #18322, #21767, #47663.

[jrf]

Fix add_magic_quotes() Magic quotes are only relevant for string values. Anything non-string should be left untouched.

[jrf]

I agree with @Veraxus that this is incorrect behaviour on the part of WordPress and should be fixed.

I've just uploaded two alternative patches for this.

The first is a plaster on the wound which would fix just the $_SERVER['REQUEST_TIME'] issue. /cc @rarst

The second is a more structural patch, fixing the add_magic_quotes() method to behave properly and only apply addslashes() to strings, but may have more far reaching consequences.

If the second patch would be considered too "dangerous", the first can be applied without real consequences.

I've done a search for plugins and themes which use the $_SERVER['REQUEST_TIME'] index and reviewed the code in a fair number of these.

Most use it to create timestamped file names for file based caches or export files, to create a unique hash or for display.

None of the code I've seen would be affected by the type change as they either:

• Concatenate the value (which will automatically cast it to a string anyway);
• Use it in a calculation (which would automatically cast it to an int anyway);
• Use md5() (which, again, would automatically create a string anyway)
• Or actually expect an int, as an alternative to time() or for use in a comparison with strtotime() which itself returns an int, though they don't do any type checking, so that type of code will also continue to work without problems.

Plugins: ​https://wpdirectory.net/search/01DT85QH7FM046D4369YHS6X4C
Themes: ​https://wpdirectory.net/search/01DT85QSER5DB2VHG2280TZTF5

[SergeyBiryukov]

#47663 was marked as a duplicate.

[SergeyBiryukov]

Note: On commit, props should also be given to @mickaelperrin for reporting this earlier in #47663.

[jrf]

Oh and I forgot to mention in my earlier comment that I have of course also searched Core for usage of $_SERVER['REQUEST_TIME'] and could find no code using it whatsoever, so this should not impact Core itself either way.

[jrf]

Fix type change for REQUEST_TIME and REQUEST_TIME_FLOAT

[jrf]

I've replaced the original first patch as it only addressed $_SERVER['REQUEST_TIME'] and $_SERVER['REQUEST_TIME_FLOAT'] should also be fixed. Includes adding some inline documentation about the change.

The second patch already handles that correctly.

Similar to before:

• I've confirmed that $_SERVER['REQUEST_TIME_FLOAT'] is not used in WP Core itself.
• I've searched the plugins and themes and looked through the code of a number of usage instances and my findings are the same as for $_SERVER['REQUEST_TIME']: the type change to the expected type should not cause any issues.

Plugins: ​https://wpdirectory.net/search/01DTAEJ2SGSEMWPRQ1S0XK31X3
Themes: ​https://wpdirectory.net/search/01DTAEJ8787G6624VA1VQMFD0S

[Rarst]

I don't know on top of my head, but is there (significant) loss of precision if float is converted to string and back? Unlike integers, floats do not necessarily map 1:1 to string representation.

Not touching them in first place seems to be preferable approach to me, over potentially lossy type juggling.

[jrf]

Replying to Rarst:

I don't know on top of my head, but is there (significant) loss of precision if float is converted to string and back? Unlike integers, floats do not necessarily map 1:1 to string representation.

Not touching them in first place seems to be preferable approach to me, over potentially lossy type juggling.

@Rarst Good point and thanks for asking this. Good to document the answer as part of this ticket.

The short answer is: no, this won't be an issue.

The long answer:

• The loss of precision occurs when doing calculations like multiplication/division, not when doing casts.
• The default precision ini value of PHP is 14 and WP doesn't change that. This affects the value of REQUEST_TIME_FLOAT way before doing the casts.

Evidence: ​https://3v4l.org/3dEnW

And yes, it would always be better not to touch the values in the first place (aka: the second patch), but the first patch should work just fine in this instance without significant loss of precision.

[jrf]

Addition: just checked with @derickr, who happened to be at the same event as me, and the loss of precision starts happening at 53 bits and with precision 14, we won't hit that number, so we should be fine.

[audrasjb]

Hi there,

With Beta 3 approaching, we'll need a decision and a commit action very soon.
For Component maintainers: if you don't feel the current patch is ready to land in WP 5.4 in the next few day, it's could be better to move it to 5.5. We're leaving it in the milestone for now.

[jrf]

IMO and based on the research done and discussed above, patch 48605.patch should have gone in a while back already. Committing it now should be safe and at least solves the more urgent problem.

I agree that 48605-fix-add-magic-quotes.patch needs more discussion and should be punted to a next/future release​.

[audrasjb]

Thanks for the feedback!
No problem, it's up to you as core committer to move this ticket back to milestone 5.4 and commit the patch is it's ready.
In this case, it should be committed as soon as possible (before beta 3, on Tuesday) to give some time for testing to be sure there is no regression.

[audrasjb]

Ah sorry, this one wasn't moved to Future Release or 5.5, so it's still in milestone 5.4 👍

[SergeyBiryukov]

In 47370:

Bootstrap/Load: In wp_magic_quotes(), revert the type change to string for REQUEST_TIME and REQUEST_TIME_FLOAT values, which should retain their proper type.

Among other things, this preserves compatibility of WP with PHPUnit Code Coverage generation.

Props jrf, Veraxus, Rarst.
See #48605.

[SergeyBiryukov]

Since [47370] was intended as a temporary fix, let's get 48605-fix-add-magic-quotes.patch​ in early in 5.5.

[SergeyBiryukov]

48605-fix-add-magic-quotes.patch​ could use some unit tests.

On a related note, the issue of recasting data types also applies to wp_slash(), see #42195.

[davidbaumwald]

Adding needs-unit-tests per @SergeyBiryukov suggestion.

Trac ticket: https://core.trac.wordpress.org/ticket/48605

[donmhico]

In ​https://github.com/WordPress/wordpress-develop/pull/284/files, I added the ! is_string( $v ) check after the is_array( $v ) compared to https://core.trac.wordpress.org/attachment/ticket/48605/48605-fix-add-magic-quotes.patch because otherwise it won't perform addslashes() in the strings inside a subarray.
Example

<?php
$data = [
    1,
    'This is a "string"',
    [
        true,
        null,
        525,
        'This is another "string"', // This won't be passed to `addslashes()` in 48605-fix-add-magic-quotes.patch
    ],
];

[davidbaumwald]

With 5.5 Beta 1 approaching, the time for early tickets has now passed, and this ticket is being moved to Future Release. If any maintainer or committer feels this can be quickly resolved for 5.5 or wishes to assume ownership of this ticket during a specific cycle, feel free to update the milestone accordingly.

[SergeyBiryukov]

In 48205:

Bootstrap/Load: Make sure add_magic_quotes() does not inappropriately recast non-string data types to string.

Props donmhico, jrf, Veraxus, Rarst.
Fixes #48605.

[SergeyBiryukov]

In 48206:

Bootstrap/Load: Remove special handling for REQUEST_TIME and REQUEST_TIME_FLOAT server values in wp_magic_quotes().

This was intended as a temporary fix until add_magic_quotes() is modified to leave non-string values untouched, which has now been done.

Follow-up to [47370], [48205].

See #48605.

[SergeyBiryukov]

In 48440:

Bootstrap/Load: Adjust the logic in add_magic_quotes() for better readability.

Follow-up to [48205].

See #48605.

[desrosj]

This received a call out in the Miscellaneous Developer Changes in 5.5 dev note: ​https://make.wordpress.org/core/2020/07/29/miscellaneous-developer-focused-changes-in-wordpress-5-5/.