get_calendar generates query with invalid date formats

[andy]

Given a parameter like ?w=1400, which is obviously not a week number, get_calendar will try to compute the date 9799 days into the year. It would make sense to check that $w is in a valid range, such as not greater than 53.

At the same time, get_calendar does not check that $m is a valid date, resulting in $thisyear == '0' and a query of SELECT DATE_FORMAT((DATE_ADD('00101', INTERVAL 9799 DAY) ), '%m').

[andy]

The overarching problem with this code block is that it relies on MySQL for date computation that can and should be done in PHP. MySQL's date computation may have been more convenient when get_calendar was written 14 years ago in [508]. PHP's date capabilities have improved since then, including support for ISO-8601 date formats.

Along with validating the inputs mentioned in the ticket description, the code should be rewritten to stop using MySQL to find the calendar month for week queries.

[markparnell]

Thanks @andy. Some sanity checks of the provided week and month values would definitely be worthwhile here. I'll leave the bigger question of whether to rewrite the process to use PHP date calculations instead of MySQL to others.

Introduced comprehensive PHPUnit tests to cover various scenarios for the get_calendar() function. Enhanced input validation in get_calendar() to handle invalid week numbers and months, defaulting them to safe values when out of range.

[pbearne]

Hi All

I have added some sanitation code and a load of tests

Paul

Thanks for the PR @pbearne! Would you mind updating it to resolve the conflicts? Thanks!

It appears some tests are failing @pbearne

[audrasjb]

Moving to 6.9.

[rollybueno]

Re: Unit test - ​https://github.com/WordPress/wordpress-develop/pull/8064/files#diff-754277583aa19c1f49a5f5cdf9b6f9cb25d217261e6e7c31bb59eef8fd123d11

Hey @pbearne, while the current tests cover general validation for get_calendar(), they don’t seem to hit the exact scenario described in this ticket.

The issue here is very specific: passing something like ?w=1400 or an invalid $m value triggers odd behavior such as computing a date 9799 days into the year or using DATE_ADD('00101', INTERVAL 9799 DAY) in the query.

It might be worth adding a unit test that explicitly sets these out-of-range parameters to make sure the function handles them. That would both document the edge case and protect against regressions in the future.

[pbearne]

Replying to rollybueno:

Re: Unit test - ​https://github.com/WordPress/wordpress-develop/pull/8064/files#diff-754277583aa19c1f49a5f5cdf9b6f9cb25d217261e6e7c31bb59eef8fd123d11

Hey @pbearne, while the current tests cover general validation for get_calendar(), they don’t seem to hit the exact scenario described in this ticket.

The issue here is very specific: passing something like ?w=1400 or an invalid $m value triggers odd behavior such as computing a date 9799 days into the year or using DATE_ADD('00101', INTERVAL 9799 DAY) in the query.

It might be worth adding a unit test that explicitly sets these out-of-range parameters to make sure the function handles them. That would both document the edge case and protect against regressions in the future.

I think this is covered in the test_get_calendar_out_of_range test
I didn't use 1400, but rather just 60

[rollybueno]

Alright, I'll try to stress test this tomorrow.

Replying to pbearne:

Replying to rollybueno:

Re: Unit test - ​https://github.com/WordPress/wordpress-develop/pull/8064/files#diff-754277583aa19c1f49a5f5cdf9b6f9cb25d217261e6e7c31bb59eef8fd123d11

Hey @pbearne, while the current tests cover general validation for get_calendar(), they don’t seem to hit the exact scenario described in this ticket.

The issue here is very specific: passing something like ?w=1400 or an invalid $m value triggers odd behavior such as computing a date 9799 days into the year or using DATE_ADD('00101', INTERVAL 9799 DAY) in the query.

It might be worth adding a unit test that explicitly sets these out-of-range parameters to make sure the function handles them. That would both document the edge case and protect against regressions in the future.

I think this is covered in the test_get_calendar_out_of_range test
I didn't use 1400, but rather just 60

[rollybueno]

Merged latest trunk, run test and it's a failure:

Replying to pbearne:

Replying to rollybueno:

Re: Unit test - ​https://github.com/WordPress/wordpress-develop/pull/8064/files#diff-754277583aa19c1f49a5f5cdf9b6f9cb25d217261e6e7c31bb59eef8fd123d11

Hey @pbearne, while the current tests cover general validation for get_calendar(), they don’t seem to hit the exact scenario described in this ticket.

The issue here is very specific: passing something like ?w=1400 or an invalid $m value triggers odd behavior such as computing a date 9799 days into the year or using DATE_ADD('00101', INTERVAL 9799 DAY) in the query.

It might be worth adding a unit test that explicitly sets these out-of-range parameters to make sure the function handles them. That would both document the edge case and protect against regressions in the future.

I think this is covered in the test_get_calendar_out_of_range test
I didn't use 1400, but rather just 60

[welcher]

This was reviewed in the 6.9 bug scrub today. There are failures on the linked PR and as we're 1 week from RC 1, I'm going to punt this to Future Release.