WordPress.org

Make WordPress Core

Opened 4 weeks ago

Last modified 2 weeks ago

#48605 reviewing defect (bug)

add_magic_quotes() inappropriately recasts data types

Reported by: Veraxus Owned by: SergeyBiryukov
Milestone: 5.4 Priority: normal
Severity: major Version: trunk
Component: General Keywords: has-patch
Focuses: Cc:
PR Number:

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 3 weeks 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 3 weeks ago.
Fix type change for REQUEST_TIME and REQUEST_TIME_FLOAT

Download all attachments as: .zip

Change History (12)

#1 @SergeyBiryukov
4 weeks ago

  • Description modified (diff)

Related: #18322, #21767, #47663.

@jrf
3 weeks ago

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

#2 @jrf
3 weeks 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
3 weeks ago

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

#4 @SergeyBiryukov
3 weeks ago

#47663 was marked as a duplicate.

#5 @SergeyBiryukov
3 weeks 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
3 weeks 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 3 weeks ago by jrf (previous) (diff)

@jrf
3 weeks ago

Fix type change for REQUEST_TIME and REQUEST_TIME_FLOAT

#7 @jrf
3 weeks 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
3 weeks 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
2 weeks 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
2 weeks 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 2 weeks ago by jrf (previous) (diff)
Note: See TracTickets for help on using tickets.