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

Ensure $_SERVER keys are read as strings. #169

Merged
merged 2 commits into from
Nov 2, 2021

Conversation

fredrik-eriksson
Copy link
Contributor

When looping over an array with foreach php will automatically convert
numeric values to integers. Since the key here is passed to substr it
has to be of string type. This commit explicitly convert the key value
to string.

I found this by accident by setting "env[LC_MESSAGES] = C.UTF-8" in my php-fpm pool configuration. For whatever reason this translated to '"6" => "C.UTF-8' in the $_SERVER array, which caused tons of headache while troubleshooting my nextcloud installation. I havn't really verified if this is the only bug of this kind (I fixed my problem by updating the php-configuration), but since I found this bug I thought I could at least report it.

When looping over an array with foreach php will automatically convert
numeric values to integers. Since the key here is passed to substr it
has to be of string type. This commit explicitly convert the key value
to string.
@phil-davis
Copy link
Contributor

phil-davis commented Nov 2, 2021

I rebased this. @staabm please review.

Hold that - I didn't look at all, the code is not valid.

@codecov
Copy link

codecov bot commented Nov 2, 2021

Codecov Report

Merging #169 (753313f) into master (0af2454) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #169      +/-   ##
============================================
+ Coverage     89.67%   89.72%   +0.04%     
- Complexity      256      260       +4     
============================================
  Files            15       15              
  Lines           891      895       +4     
============================================
+ Hits            799      803       +4     
  Misses           92       92              
Impacted Files Coverage Δ
lib/Sapi.php 61.46% <100.00%> (+0.35%) ⬆️
lib/functions.php 95.71% <0.00%> (+0.03%) ⬆️
lib/Client.php 84.95% <0.00%> (+0.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0af2454...753313f. Read the comment docs.

@fredrik-eriksson
Copy link
Contributor Author

Oops, apparently I can mess up even a simple pull request like this one... Good to know I guess.

Thank you for the fix, and sorry for my php-dyslexia 😓

@phil-davis phil-davis merged commit 07e9918 into sabre-io:master Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants