perf: optimize $_SERVER import

AlliBalliBaba commented 1 month ago

This PR replaces #1103 since actions wouldn't trigger when merging from a fork.

I refactored it so the import happens directly in cgi.go, the additional Cgo calls don't seem to matter in the flamegraphs.

Without this PR: flame_fringe_mode

With this PR: flame

AlliBalliBaba commented 1 month ago

Still not sure why checks aren't triggering.

AlliBalliBaba commented 1 month ago

After doing some testing to summarize everything I found here again:

PHP doesn't do -any- validation on the registered values, the correctness of these values is largely left up to the SAPIs that register them.

What does PHP validate then?

sapi_module.input_filter

filter is a PHP extension that allows filtering strings. There is a php.ini setting called filter.default that also allows pre-filtering all globals. The global filter is deprecated , noone really uses it anymore and passing anything but "unsafe_raw" will lead to a deprecation notice. "unsafe raw" does nothing to the value except copy it to a 'filtered globals' array.

php_register_variable_safe

Even php_register_variable_safe does absolutely nothing to the registered value (beside from checking that it is not null). So what does it actually do?

The main sanitation logic happens in php_register_variable_ex and is about the array key.

First it ensures that the variable name does not contain any space or dot (not binary safe).
If it contains a [, the function will register an array (aka when sending a query string like this: ?values[]=1&values[]=2).
Then it will check for illegal variable names like $this, $GLOBALS __Host- or __Secure-.
Then it will check if the array is $_COOKIESand prevent registering values twice.
Then it will finally just write the zval into the array as is

So if we have hard-coded keys (from all known vars) this function is also redundant.

How can we make variable registration safer?

I added some fuzzing tests to the pipeline. These tests are well suited for security checks and will rerun a specific test with 'fuzzed' or randomized strings. If any combination of strings causes a failure or crash it will stop the test, otherwise it will continue (theoretically) forever. Due to these tests I already noticed that go will allow a null byte in the Url.Path, which can lead to a truncated registered path. I'll look into fuzzing more potential inputs, maybe it will uncover other issues.

withinboredom commented 1 month ago

TIL. Thanks @AlliBalliBaba for working on this!

AlliBalliBaba commented 1 month ago

I filed a bug report to github as I'm still not sure why checks have disappeared from all my PRs.

dunglas commented 4 weeks ago

@AlliBalliBaba I rebased and force-pushed your branch, and this triggered the checks. There is something weird however: the ownership of some commits changed. It looks like some commits are made on the behalf of "alliballibaba" but some other on the behalf of "a.stecher". This may be the source of the issue. Maybe are you using an email that is not set in your GitHub account for some commits?

AlliBalliBaba commented 4 weeks ago

Hmm yeah that's possible, I'll try adding that email. It's weird though that it worked before

AlliBalliBaba commented 4 weeks ago

Oh I think it's probably because I hit this limit, my actions will be back next month I guess.

withinboredom commented 3 weeks ago

Oh I think it's probably because I hit this limit, my actions will be back next month I guess.

That's weird that it is counting your commits to this repository against your personal limits. I guess because this is technically a personal repository (i.e., belongs to @dunglas) and not an organization. In other words, "why should @dunglas have to pay for contributors to run actions?" is the use-case? I don't see an option to change this in the settings on one of my personal repositories though.

Might it be time for an organization then?

dunglas commented 3 weeks ago

@withinboredom I don't think it's related. I have never seen that on any of my repo (org or personal).

withinboredom commented 3 weeks ago

Same. It's just the only thing I can think of.

AlliBalliBaba commented 3 weeks ago

Could also be something else, I'll know once someone responds to the github ticket.

dunglas commented 2 weeks ago

@AlliBalliBaba could you try to make your commits verified, this may help fixing the issue.

dunglas / frankenphp