php
perf: Optimize $_SERVER import
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:
With this PR:
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
spaceordot(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.
I filed a bug report to github as I'm still not sure why checks have disappeared from all my PRs.
@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?
Hmm yeah that's possible, I'll try adding that email. It's weird though that it worked before
Oh I think it's probably because I hit this limit, my actions will be back next month I guess.
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?
@withinboredom I don't think it's related. I have never seen that on any of my repo (org or personal).
Could also be something else, I'll know once someone responds to the github ticket.