Numeric array key type errors

[elrido]

This PR fixes #1435

Changes

• added a test to trigger the issue
• fixed the issue via explicit cast

ToDo

There are two other areas I found that might have the same issue, but have not been able to trigger the issue from the outside - please review and share your thoughts:

• [x] In Configuration.php we iterate over the default configuration array - all keys happen to be strings, but we would get an issue if we ever introduced a number as a configuration key. Since these defaults are hardcoded, I don't know if we can trigger the potential breakage from a test: https://github.com/PrivateBin/PrivateBin/blob/d23bb748d45bb8cde8f3737b632d7ddd6a383e35/lib/Configuration.php#L147
• [x] In Database.php in the private _exec function, a flat array is used and we expect all keys to be int and never strings. All callers do so and therefore I again didn't find a way to call this from the unit tests: https://github.com/PrivateBin/PrivateBin/blob/d23bb748d45bb8cde8f3737b632d7ddd6a383e35/lib/Data/Database.php#L509-L510

So I think that at this time both of the above cases are un-problematic, but could be potential future foot-guns if used incorrectly?

[rugk]

Thanks a lot! This seems like the proper fix.

About your concerns:

• Configuration.php -> I agree here, nothing than strings can ever land. Especially, if it is really only the keys of the configuration, they really are strings. We are very unlikely to introduce a numeric "key" like 123 = "this-option" or so … :upside_down_face:
• database -> Here I don't have much insight into how that function is used and also it is internal-only.

So yeah hard to trigger from the outside, but IMHO, we could anyway do explicit casts, if you've already identified it as a problem, IMHO. I mean casting strings explicitly to a string won't hurt, does it? I.e. let's fix the problem even if we have no unit test to prevent breakage.

[elrido]

I do not want to add the casts blindly if I can't create a test that trigger a problem.

For the config I tried to write one where I pass a config file that contains a numeric key, but that simply gets ignored by the loop, since we loop over known default keys. In general, if people add things to our config files that are not in the defaults, they will get ignored and are not accessible through the Configuration object. The internal config is based on the defaults, with the config file values replacing the default.

In the database case it is a flat array (i.e. array('first param', 'second param'), so keys are expected to always be int (i.e. 0, 1 for the previous example). The only reason they would not be, is if someone added a call to exec_ passing an incorrect array (with string key) and then that PDO prepared statement call would fail because the bound params would not line up with the ? mark in the query. So it is unlikely to get added and remain undetected, unless we don't write a test for the new code path.

[rugk]

Okay then though our config should be safe.

And okay, though I would have said a cast to the correct type does not hurt and an incorrect case would be detected as it breaks everything(?). So yeah…

[github-actions[bot]]

Test Results

    8 files  ±0    148 suites  ±0   1m 21s ⏱️ ±0s   346 tests +1    330 ✅ +2  0 💤 ±0  0 ❌ ±0  16 🔥  - 1  1 804 runs  +7  1 788 ✅ +8  0 💤 ±0  0 ❌ ±0  16 🔥  - 1 

For more details on these errors, see this check.

Results for commit 8752354d. ± Comparison against base commit d23bb748.

:recycle: This comment has been updated with latest results.

[elrido]

I also restated the database prepared statement argument binding to use a regular incremented position variable. That doesn't require using keys. Possibly more readable, too?

[rugk]

Thanks! LGTM.