Skip to content

fix(comments): prevent IDOR in WebDAV comments API#41558

Merged
DeepDiver1975 merged 4 commits into
masterfrom
fix/OC10-53
May 21, 2026
Merged

fix(comments): prevent IDOR in WebDAV comments API#41558
DeepDiver1975 merged 4 commits into
masterfrom
fix/OC10-53

Conversation

@DeepDiver1975
Copy link
Copy Markdown
Member

@DeepDiver1975 DeepDiver1975 commented May 20, 2026

Summary

  • Fixes OC10-53: authenticated users could read, edit, or delete comments on files they have no access to by supplying an arbitrary comment_id paired with any file_id they own
  • EntityCollection::getChild() and childExists() now verify the fetched comment's objectType/objectId match the collection's own entity type and file ID before returning
  • Because Sabre DAV resolves tree paths before dispatching any verb, this single boundary fix covers PROPFIND (read), DELETE, and PROPPATCH (edit) uniformly

Test Plan

  • Run ./lib/composer/phpunit/phpunit/phpunit apps/comments/tests/unit/Dav/EntityCollectionTest.php — all 12 tests pass
  • Manually reproduce POC from OC10-53: user B's PROPFIND /remote.php/dav/comments/files/{userB_file_id}/{userA_comment_id} returns 404

🤖 Generated with Claude Code

…th tests

Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
…t ownership

An authenticated user could PROPFIND/DELETE/PROPPATCH any comment by
supplying an arbitrary comment_id paired with any file_id they own.
EntityCollection::getChild() and childExists() now verify that the
fetched comment's objectType and objectId match the collection's own
entity type and file ID before returning or confirming the node.

Fixes OC10-53

Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
@update-docs
Copy link
Copy Markdown

update-docs Bot commented May 20, 2026

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
Copy link
Copy Markdown
Member

@jvillafanez jvillafanez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code looks good

@DeepDiver1975 DeepDiver1975 merged commit 6ff13d6 into master May 21, 2026
26 of 32 checks passed
@DeepDiver1975 DeepDiver1975 deleted the fix/OC10-53 branch May 21, 2026 08:22
@phil-davis
Copy link
Copy Markdown
Contributor

Note: I have rebased #41552 to master after this PR was merged.
That runs the API Comments acceptance tests in CI.
They are passing, so there was no regression for the ordinary uses of comments tested in those test scenarios.

DeepDiver1975 added a commit that referenced this pull request May 21, 2026
* test(comments): stub objectType/objectId in EntityCollection happy-path tests

Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>

* test(comments): add failing IDOR regression tests for EntityCollection

Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>

* fix(comments): prevent IDOR in WebDAV comments API by checking comment ownership

An authenticated user could PROPFIND/DELETE/PROPPATCH any comment by
supplying an arbitrary comment_id paired with any file_id they own.
EntityCollection::getChild() and childExists() now verify that the
fetched comment's objectType and objectId match the collection's own
entity type and file ID before returning or confirming the node.

Fixes OC10-53

Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>

* docs: add changelog entry for OC10-53 IDOR fix in WebDAV comments API

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>

---------

Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
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