WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 6 days ago

#48605 reviewing defect (bug)

add_magic_quotes() inappropriately recasts data types

Reported by: Veraxus Owned by: SergeyBiryukov
Milestone: 5.5 Priority: normal
Severity: major Version: 5.4
Component: Bootstrap/Load Keywords: has-patch early has-unit-tests
Focuses: Cc:

Description (last modified by SergeyBiryukov)

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.

Attachments (2)

48605-fix-add-magic-quotes.patch (854 bytes) - added by jrf 6 months ago.
Fix add_magic_quotes() Magic quotes are only relevant for string values. Anything non-string should be left untouched.
48605.patch (1.2 KB) - added by jrf 6 months ago.
Fix type change for REQUEST_TIME and REQUEST_TIME_FLOAT

Download all attachments as: .zip

Change History (25)

#1 @SergeyBiryukov
6 months ago

  • Description modified (diff)

Related: #18322, #21767, #47663.

@jrf
6 months ago

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

#2 @jrf
6 months ago

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

#3 @SergeyBiryukov
6 months ago

  • Milestone changed from Awaiting Review to 5.4
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#4 @SergeyBiryukov
6 months ago

#47663 was marked as a duplicate.

#5 @SergeyBiryukov
6 months ago

  • Keywords has-patch added; needs-patch removed

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

#6 @jrf
6 months ago

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.

Last edited 6 months ago by jrf (previous) (diff)

@jrf
6 months ago

Fix type change for REQUEST_TIME and REQUEST_TIME_FLOAT

#7 @jrf
6 months ago

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

#8 follow-up: @Rarst
6 months ago

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.

#9 in reply to: ↑ 8 @jrf
6 months ago

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.

#10 @jrf
6 months ago

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.

Last edited 6 months ago by jrf (previous) (diff)

#11 @audrasjb
3 months ago

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.

#12 @jrf
3 months ago

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​.

#13 @audrasjb
3 months ago

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.

#14 @audrasjb
3 months ago

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

Last edited 3 months ago by audrasjb (previous) (diff)

#15 @SergeyBiryukov
3 months ago

  • Component changed from General to Bootstrap/Load

#16 @SergeyBiryukov
3 months ago

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.

#17 @SergeyBiryukov
3 months ago

  • Keywords early added
  • Milestone changed from 5.4 to 5.5

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

#18 @SergeyBiryukov
7 weeks ago

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.

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


6 weeks ago

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


3 weeks ago

#21 @davidbaumwald
3 weeks ago

  • Keywords needs-unit-tests added

Adding needs-unit-tests per @SergeyBiryukov suggestion.

#23 @donmhico
6 days ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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
    ],
];
Note: See TracTickets for help on using tickets.