-
Notifications
You must be signed in to change notification settings - Fork 83
Improve Undo handler object validation and defaults #2284
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
Conversation
Adds a default type of 'Announce' if not set in the activity object and refines object attribute validation to only check arrays. Updates tests to cover cases where the object is a URI, ensuring such cases pass validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the Undo handler by improving object validation to handle URI objects and adding defensive programming for missing activity types. The changes make the handler more robust when processing ActivityPub Undo activities that reference objects by URI rather than embedded objects.
- Adds default fallback to 'Announce' type when activity object type is missing
- Updates validation to only check object attributes when the object is an array (not a URI string)
- Adds test coverage for URI object validation scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| includes/handler/class-undo.php | Updates object validation and adds default type fallback logic |
| tests/includes/handler/class-test-undo.php | Adds test case for URI object validation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Replaces an incorrect if statement with elseif to ensure proper handling of 'Undo' requests for 'Like', 'Create', and 'Announce' activities. This prevents both code blocks from executing when only one should.
| */ | ||
| public static function handle_undo( $activity, $user_id ) { | ||
| $type = $activity['object']['type']; | ||
| $type = $activity['object']['type'] ?? 'Announce'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If $activity['object'] is a URL, should we not resolve it and grab the underlying activity? What if it refers to a Follow activity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
atm. we don't have to. this only works for Announce and Like, so we could have that as quick win.
On the other hand, I can't check the URL any more, because it was deleted already. I contacted Brid.gy to see if we can get an example. Or if this is on purpose and they simply do not provide any queryable objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, but then why not reject non-array objects if it's what we need to act on them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it is an Undo of an Activity we might have to handle!?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if the remote object is no longer accessible, we might still have the Activity in our db. For the lookup we only need the id, so we should at least check for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, not for Follows it looks like? I'm a bit confused. I would have expected a check in the Inbox to see if we find it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right... even the current code is broken! The ID is the Activity ID, not the Object ID... the problem is, that we do not store Likes and Announces in the Inbox (yet)...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we'd have to check both, the Inbox and the saved comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I am totally confused! We store the Activity-ID in a comment meta
| $comment_data['comment_meta']['source_id'] = \esc_url_raw( $activity['id'] ); |
So the current PR works fine for Like and Announce but for Follow we need some more rework and we maybe need to store the Follow request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't know. Maybe it's enough to check the Inbox and be done with it. object as a string seems to be an edge case, given that we've only encountered one so far, so maybe that'll be good enough.
|
Close in favor of #2295 |
Adds a default type of 'Announce' if not set in the activity object and refines object attribute validation to only check arrays. Updates tests to cover cases where the object is a URI, ensuring such cases pass validation.
Proposed changes:
Other information:
Testing instructions:
Undofor aLikeor anAnnouncewith only theIDasObject.Changelog entry
Changelog Entry Details
Significance
Type
Message
Added a default
Announcetype when missing and improved validation to correctly handle URI objects.