Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REQUEST_TIME/REQUEST_TIME_FLOAT missing in INPUT_SERVER #17543

Open
kkmuffme opened this issue Jan 22, 2025 · 13 comments
Open

REQUEST_TIME/REQUEST_TIME_FLOAT missing in INPUT_SERVER #17543

kkmuffme opened this issue Jan 22, 2025 · 13 comments

Comments

@kkmuffme
Copy link

Description

The following code:

<?php

if ( php_sapi_name() === 'cli' ) {
    echo "filter_input does not work in PHP CLI";
    exit;
}

var_dump( filter_input( INPUT_SERVER, 'REQUEST_TIME_FLOAT' ) );
var_dump( $_SERVER['REQUEST_TIME_FLOAT'] );

var_dump( filter_input( INPUT_SERVER, 'REQUEST_TIME' ) );
var_dump( $_SERVER['REQUEST_TIME'] );

Resulted in this output:

NULL
float(1737560728.0001)
NULL
int(1737560728)

But I expected this output instead:

float(1737560728.0001)
float(1737560728.0001)
int(1737560728)
int(1737560728)

These are the only 2 documented in https://www.php.net/manual/en/reserved.variables.server.php with that behavior? Is this a bug?

I only encountered this behavior with other (non-documented) variables HOME and USER

PHP Version

PHP 8.4

Operating System

No response

@Girgias
Copy link
Member

Girgias commented Jan 23, 2025

This is not a bug, the filter_input APIs work directly with the values provided by the SAPI, and not the content of $_SERVER.

There is a note about this in the documentation for filter_input_array

@Girgias Girgias closed this as not planned Won't fix, can't repro, duplicate, stale Jan 23, 2025
@kkmuffme
Copy link
Author

The docs say:

prior to any user modification to the supergloba

REQUEST_TIME/REQUEST_TIME_FLOAT are not set by the user but by PHP.

REQUEST_TIME=123 php -r 'echo $_SERVER['REQUEST_TIME'];' will not show 123

If the time is not set by the SAPI, it also means that the time reported isn't correct, since the start time is only set after the SAPI is started which can be multiple ms after the request started being handled by PHP - which would be a separate bug then.

@cmb69
Copy link
Member

cmb69 commented Jan 26, 2025

See e.g.

php-src/sapi/cli/php_cli.c

Lines 316 to 348 in bd23d3a

static void sapi_cli_register_variables(zval *track_vars_array) /* {{{ */
{
size_t len;
char *docroot = "";
/* In CGI mode, we consider the environment to be a part of the server
* variables
*/
php_import_environment_variables(track_vars_array);
/* Build the special-case PHP_SELF variable for the CLI version */
len = strlen(php_self);
if (sapi_module.input_filter(PARSE_SERVER, "PHP_SELF", &php_self, len, &len)) {
php_register_variable("PHP_SELF", php_self, track_vars_array);
}
if (sapi_module.input_filter(PARSE_SERVER, "SCRIPT_NAME", &php_self, len, &len)) {
php_register_variable("SCRIPT_NAME", php_self, track_vars_array);
}
/* filenames are empty for stdin */
len = strlen(script_filename);
if (sapi_module.input_filter(PARSE_SERVER, "SCRIPT_FILENAME", &script_filename, len, &len)) {
php_register_variable("SCRIPT_FILENAME", script_filename, track_vars_array);
}
if (sapi_module.input_filter(PARSE_SERVER, "PATH_TRANSLATED", &script_filename, len, &len)) {
php_register_variable("PATH_TRANSLATED", script_filename, track_vars_array);
}
/* just make it available */
len = 0U;
if (sapi_module.input_filter(PARSE_SERVER, "DOCUMENT_ROOT", &docroot, len, &len)) {
php_register_variable("DOCUMENT_ROOT", docroot, track_vars_array);
}
}
/* }}} */

We register some variables for ext/filter explicitly. Adding REQUEST_TIME(_FLOAT) makes some sense to me.

@Girgias
Copy link
Member

Girgias commented Jan 29, 2025

I have put the various filter_input functions on the 8.5 deprecation RFC as I find the behaviour to be very odd and confusing, as its existence seems to just be there to "undo" whatever the filter.default INI defined filter does. Because this INI setting has been deprecated, I don't think those functions should also exist.

@cmb69
Copy link
Member

cmb69 commented Jan 29, 2025

Ah, okay, makes sense to me. Thank you!

@kkmuffme
Copy link
Author

kkmuffme commented Feb 2, 2025

@Girgias actually the filter_input are extremely useful, since they guarantee that the data hasn't been tampered with.
A classic example is the use of WordPress and it's slashing (and previously magic quotes).

Therefore obviously also Deprecate FILTER_CALLBACK filter is an issue, since it's useful with filter_input to e.g. sanitize input.

EDIT:

as I find the behaviour to be very odd and confusing, as its existence seems to just be there to "undo" whatever the filter.default

Actually this isn't what these do at all/these are used for. What exactly is odd or confusing?

@cmb69
Copy link
Member

cmb69 commented Feb 3, 2025

A classic example is the use of WordPress and it's slashing (and previously magic quotes).

Well, the question is, do we want to keep a duplicate set of input parameters so users can work around the framework of their choice?

@kkmuffme
Copy link
Author

kkmuffme commented Feb 4, 2025

It's not only about working around the framework but also about ensuring data is untampered with.

With filter_input I just need to verify compliance of the SAPI and PHP core, when using the superglobals I suddenly have to check and verify compliance at all places that use or possibly change any of the superglobals.
In an environment where compliance is important, this suddenly adds tons of extra time and cost.

In fact, I would go a step further and deprecate the superglobals instead, since that was a design mistake to begin with and keep only filter_input. (never going to happen of course)

@cmb69
Copy link
Member

cmb69 commented Feb 4, 2025

Okay, but this is not the best place to discuss this. More appropriate would be the Internals list. Either we deprecate filter_input() and related functionality, or we should fully support it. @Girgias, did you already write to internals about the deprecation (couldn't find anything).

@Girgias
Copy link
Member

Girgias commented Feb 4, 2025

@cmb69 I did not write anything to internals, as we normally start discussion for the mass deprecation RFC around April/May.

If the framework (e.g. WordPress) tampers with the superglobal they must have a good reason for doing this, and thus being able to access the untampered superglobals seems to be defeating the point of the framework. If WP is doing something that it shouldn't be doing, this should be brought to WP IMHO.

The filter extension is extremely clunky and has so many edge cases that if I could do swooping changes just on my own I would be unbundling from php-src rather than removing support for the superglobals.

@cmb69
Copy link
Member

cmb69 commented Feb 4, 2025

Regarding WP, see https://wpartisan.me/tutorials/wordpress-auto-adds-slashes-post-get-request-cookie (not sure if that still applies, but apparently it does).

Anyway, I think @kkmuffme made a good point (emphasis mine):

It's not only about working around the framework but also about ensuring data is untampered with.

Say, if you have a plugin system, a plugin could change the superglobals at will, and besides filter_input() your only chance to get the original input would be to parse it again (if that's even possible).

Anyway, I think we should discuss this topic on internals. If you don't mind, I'll send a mail.

@Girgias
Copy link
Member

Girgias commented Feb 4, 2025

Feel free, but I still doubt the usefulness of this. For POST/GET PSR-7 interfaces/objects seem like the "obvious" fix. Cookies and environment variables I don't really see the point of using the raw SAPI variables as surely any plugin that adds or remove to them does it for a purpose. And the server variables should just be read only, but again if a plugin tampers with them there surely is a point in doing so.

@bukka
Copy link
Member

bukka commented Feb 5, 2025

This should be re-open - REQUEST_TIME/REQUEST_TIME_FLOAT are set by SAPI and they should be probably supported

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants