Properly fix "Trying to access array offset on value of type bool"

[PhilZ-cwm6]

Since php74, we must properly ensure that array is not null/false before checking any of its column ranges

This properly fixes the "Trying to access array offset on value of type bool" when no time range is specified to filter our calendar events. It also preserves the accurate filtering query when time range is specified

[PhilZ-cwm6]

This should properly fix the error caused by more strict php code checks on null/false arrays
It properly fixes the issues described here:

• Evolution error "HTTP error 500 (Internal Server Error): 4.1.0" Baikal#929
• Avoid invalid time-range #1275
• CalDAV/PropFilter: set empty array as default value for time-range #1300

It fixes the root cause of the error: ensure $timeRange != NULL before checking !$timeRange['start'] and !$timeRange['end']

I tested it and sync is now properly working when no time range was specified to filter calendar events
The fix is really simple. The later !$timeRange['start'] and !$timeRange['end'] in code are properly checked so this is just a missing check when code was written

[codecov]

Codecov Report

Merging #1319 (9d29581) into master (d385f9b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master    #1319   +/-   ##
=========================================
  Coverage     97.12%   97.12%           
- Complexity     2775     2784    +9     
=========================================
  Files           174      174           
  Lines          8025     8038   +13     
=========================================
+ Hits           7794     7807   +13     
  Misses          231      231           

Impacted Files	Coverage Δ	Complexity Δ
lib/CalDAV/Backend/PDO.php	99.17% <100.00%> (+<0.01%)	135.00 <0.00> (+5.00)
lib/CalDAV/CalendarQueryValidator.php	100.00% <100.00%> (ø)	71.00 <0.00> (+4.00)

---

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 d385f9b...9d29581. Read the comment docs.

[phil-davis]

@PhilZ-cwm6 thanks for this. It is good to have someone who has tested this in "real life".

It would be good to also have unit tests that cover combinations of time-range specifications. See 8eb1f08

Can you cherry-pick that commit into your PR? (or add unit tests yourself)

[PhilZ-cwm6]

@phil-davis
I tested using Windows Outlook CalDAV Synchronizer and Android davx5, on my production calendar, with:

• no time range
• range + start + no end
• range + no start + end

I was confident as these settings worked properly in php prior to 7.4. So I knew, the core code was working on these time ranges.

The patch I committed is a proper php syntax fix that should be merged in anycase. It just ensures $timeRange is not null/false before checking its array columns. This is now enforced since php74 it seems

About test unit: I can try it. However, can you just point me on how to test it in a non production environement/calendar ? Please provide a step by step command line tests you want me to do so that they are reproductible for you and I am sure I properly test the situations you want

Best regards

[phil-davis]

In your local git clone you can get the code in my fork also:

git remote add phil https://github.com/phil-davis/dav.git
git fetch phil

Then you will have local access to my commit that has the unit tests, and can cherry-pick that:

git checkout master_20210109_patch_php74_null_check
git cherry-pick 8eb1f0830cd83431b060f91900dfac3be3245595

And just push up to GitHub, where the full suite of tests will run:

git push

I fully expect that those will pass with your code. In my PR I made a slightly different approach to the logic, but your fix is fine and has the same effect.

[PhilZ-cwm6]

I fully expect that those will pass with your code. In my PR I made a slightly different approach to the logic, but your fix is fine and has the same effect.

As I said, the change I did should be merged whatever is the test unit result. It is a code syntax error fix. It restores the original behavior of advanced filters when time ranges are specified as it was before php74. It was just a missing null check on the array.

I will push to the test unit in 1-2 days hopefully

best regards

[phil-davis]

I agree that the code change is "a good thing". It is nice to get the related unit tests merged in the same PR. I added the unit tests and they had some fails. I added some more code to check if array_key_exists.

Now the tests are failing further down the code in CalendarQueryValidator. It also needs checks to see if the array key exists, otherwise https://travis-ci.org/github/sabre-io/dav/jobs/753874559 "Undefined array key "start"" ... happens.

I can keep chasing those in a while.

[PhilZ-cwm6]

Thank you, I see better now your patch
I think the most proper way would be to grep in code for all those 'end' and 'start' then add a check if the key value exists, else pass null as argument

Exp:
/home/travis/build/sabre-io/dav/lib/CalDAV/CalendarQueryValidator.php:72
Add a check before that line for both end and start values and if they are not defined, set it to null like you did to $timeRange[$value] = null. Further in code, validateTimeRange() checks for null argument

Wired because it works with php74, so I guess php8 enforces further null checks or just my real life tests di not reach that part of the code

In anycase, it looks easier to fix with your test unit as I cannot reproduce in real life

[phil-davis]

@staabm @ByteHamster in this PR we are in the rabbit-hole finding all the places that make reference to the time-range start and end array keys, and adding appropriate "if exists" checks in each place.

The other approach was in PR #1275 - If start or end key does not exist, then specifically set it to false. Then the key will always existing when the code uses it. The code does various checks on the value, many of which check if it is false or not false. In older versions of PHP, an array key that was not set would get a "default value" (null, empty string, whatever it did, I don't care) that looked false. So my little "trick" should be equivalent to the old behaviour.

Which approach do you want to take - one of these, or something else?

[PhilZ-cwm6]

I understand. Sorry, pushed a commit that was just a sample to explain my thinking, not a mature code
But I understand the situation better with your last comment. Yes, false is better + fixing further occurrences and ensure they check for null/false and existing array key before it...

I did not write php since years, so I have to rebuild a local debug environment before starting to submit more complex changes to test them before pushing. Not sure I have time for it right now.

[phil-davis]

The latest change looks good - it clearly handles processing "start" and "end" in CalendarQueryValidator.

The unit tests might cover cases that "ordinary" calendar clients do not actually send in real life, but are theoretically possible to receive. It is a bit hard to test every case in real-life!

CI is passing 🚀

I requested a few reviewers. It would be good to get some fix merged and a patch release "real soon" (tm)

[PhilZ-cwm6]

Oh, nice, I really pushed it on a hurry and couldn't compile it locally
Hope it is ok, else yes, it should be rather easy to follow the issues with your test unit and fix them with these null checks
Thank you and best regards

[PhilZ-cwm6]

I committed another check for the prop-filters

[PhilZ-cwm6]

Looking at the sources using search in code for all time-range, there are still a few occurrences that do not cause issues now, but they could in the future
They can be easily fixed

However, what do you think about using isset() instead of array_key_exists() function ?
https://www.geeksforgeeks.org/difference-between-isset-and-array_key_exists-function-in-php/
https://www.php.net/manual/en/function.array-key-exists.php

The main difference between isset() and array_key_exists() function is that the array_key_exists() function will definitely tells if a key exists in an array, whereas isset() will only return true if the key/variable exists and is not null. Also isset() doesn’t render error when array/variable does not exist, while array_key_exists does.

Edited:
Note: if the key is defined as a "false" boolean somewhere, this could be fail I guess since it is not defined in the man, so maybe it is better to keep the two checks: isset() && key ?

[staabm]

I have no relevant opinion on this change, as I don't know the details involved.

Phil, please decide at the best of your knowledge.

[PhilZ-cwm6]

About my last suggestion: Since the tests pass, the code doesn't cause issue now, so maybe it can be merged if reviewed ok.
I will then later commit another future proof patch with isset() + key ? test on time-range arrays where needed.

[phil-davis]

Since the tests pass, the code doesn't cause issue now, so maybe it can be merged if reviewed ok.

For this fix we just want to avoid PHP 7.4 and 8.0 complaining about trying to access an array key that does not exist.

If someone sends the empty string, or null, or some other weird value as the specified start or end of time-range then that should be caught by validation code - it should complain that the start or end is not a proper timestamp. Maybe there are already tests for that sort of edge case, "someone" could look and see - but not me tonight :)

So I agree that we can merge this to fix the actual problem with some real-life clients.

[ByteHamster]

Sorry, I just missed the merge by a few seconds :) Looks good to me (and more clean than #1275). Thanks for fixing!