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

fix(caldav): harden null handling of iMip scheduling method #36935

Merged
merged 1 commit into from
Mar 2, 2023

Conversation

st3iny
Copy link
Member

@st3iny st3iny commented Mar 1, 2023

Summary

The schedule() method of the iMip plugin poorly handles various null values. I hardened it a bit to prevent further breakage in the future.

This PR can be tested using the following patch:

diff --git a/apps/dav/lib/CalDAV/Schedule/IMipPlugin.php b/apps/dav/lib/CalDAV/Schedule/IMipPlugin.php
index ca79205c09f..8c4507b0870 100644
--- a/apps/dav/lib/CalDAV/Schedule/IMipPlugin.php
+++ b/apps/dav/lib/CalDAV/Schedule/IMipPlugin.php
@@ -209,6 +209,9 @@ class IMipPlugin extends SabreIMipPlugin {
 			$senderName = $senderName->getValue() ?? null;
 		}
 
+		$senderName = null;
+		$this->userId = null;
+
 		// Try to get the sender name from the current user id if available.
 		if ($this->userId !== null && ($senderName === null || empty(trim($senderName)))) {
 			$senderName = $this->userManager->getDisplayName($this->userId);

TODO

Checklist

@st3iny st3iny added bug 3. to review Waiting for reviews feature: caldav Related to CalDAV internals labels Mar 1, 2023
@st3iny st3iny added this to the Nextcloud 26 milestone Mar 1, 2023
@st3iny st3iny self-assigned this Mar 1, 2023
@st3iny st3iny force-pushed the fix/noid/imip-plugin-null-hardening branch 2 times, most recently from 26a906b to 5a47ed6 Compare March 1, 2023 12:42
@nickvergessen
Copy link
Member

./autotest.sh sqlite apps/dav/tests/unit/CalDAV/Schedule/
Using PHP executable /usr/bin/php
Using database oc_autotest
Setup environment for sqlite testing on local storage ...
0 Pfade vom Index aktualisiert
Installing ....
Nextcloud was successfully installed
Testing with sqlite ...
No coverage
/home/nickv/Tools/vendor/bin/phpunit --configuration phpunit-autotest.xml --log-junit autotest-results-sqlite.xml ../apps/dav/tests/unit/CalDAV/Schedule/ 
PHPUnit 9.6.0 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.0.28
Configuration: phpunit-autotest.xml

..............................                                    30 / 30 (100%)

Time: 00:00.085, Memory: 34.00 MB

OK (30 tests, 128 assertions)

Could we extend the tests?

@st3iny
Copy link
Member Author

st3iny commented Mar 2, 2023

/rebase

Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
@nextcloud-command nextcloud-command force-pushed the fix/noid/imip-plugin-null-hardening branch from 5a47ed6 to a35b960 Compare March 2, 2023 09:42
@st3iny st3iny enabled auto-merge March 2, 2023 10:22
@st3iny st3iny merged commit 7a020b1 into master Mar 2, 2023
@st3iny st3iny deleted the fix/noid/imip-plugin-null-hardening branch March 2, 2023 11:28
@kesselb
Copy link
Contributor

kesselb commented Jul 27, 2023

/backport to stable25

@backportbot-nextcloud
Copy link

The backport to stable25 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable25
git pull origin stable25

# Create the new backport branch
git checkout -b fix/foo-stable25

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable25

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

@kesselb
Copy link
Contributor

kesselb commented Jul 27, 2023

Backport for stable25: #39591

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug feature: caldav Related to CalDAV internals
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants