[faisalahammad]

Fixes #3883

Proposed changes:

• Some Group implementations, like Mobilizon, send the Update for a Group's object from a member of the Group, not from the Group itself. Remote_Posts::update() only accepted an Update from the actor that owns the stored post, so these edits were rejected and the reader kept the old version.
• Remote_Posts::update() now also accepts the Update when the stored owner of the post is a cached remote actor of type Group and the actor is on the same host as that Group.
• This only applies to posts that are already stored. New content from a Group still comes in through the Group's Announce, so the Group can still moderate it.
• The inbox recipient lookup is not changed. The earlier version of this PR widened the recipients in get_local_recipients(); that approach was removed after review.
• Deletes are already covered: Delete::maybe_delete_post() checks for a tombstone at the origin before it removes a stored post, so it does not depend on who sent the Delete.

Other information:

• Have you written new tests for your changes, if applicable?

The Group check only runs when the actor is not the stored owner, so normal updates have no extra cost. A Person's post can still only be updated by that Person, even from the same host.

Testing instructions:

• Enable "Create posts" for the reader (activitypub_create_posts).
• Follow a Mobilizon group from your site. Do not follow the member account that will edit.
• Wait until one of the group's events is stored as a remote post on your site.
• As a member of that group, edit the event on Mobilizon.
• Before: the Update is rejected with activitypub_update_forbidden and the stored event keeps the old title and content. After: the stored event shows the new title and content.
• Send an Update for that stored event from an account on another server. It must still be rejected.
• Send an Update from a Mastodon account for a stored post of another Person on the same server. It must still be rejected.
• Normal Mastodon posts, edits and deletes work as before.
• Unit tests: npm run env-test -- --filter Test_Remote_Posts

Changelog entry

• Automatically create a changelog entry from the details below.

Changelog Entry Details

Significance

• Patch
• Minor
• Major

Type

• Added - for new features
• Changed - for changes in existing functionality
• Deprecated - for soon-to-be removed features
• Removed - for now removed features
• Fixed - for any bug fixes
• Security - in case of vulnerabilities

Message

Edits that a member of a group you follow makes to the group's posts, like a Mobilizon event, now show up in your reader.

[pfefferle]

Thanks @faisalahammad for picking this up!

I am not sure the inbox recipients are the right place for this. A few things I noticed:

• Remote_Posts::add() only accepts a Create if the actor matches attributedTo, and Remote_Posts::update() checks the actor against the stored owner. So the activities this lets through the inbox will be rejected later anyway, at least for the reader. The tests only check the recipient list, not that something is stored.
• Same host as the only check is quite open. Any account on a Lemmy, PeerTube or Mobilizon instance could send a post attributed to a Group on that instance to all WordPress followers of that Group. Normally the Group announces the content, so it can moderate it. Here nothing checks that the actor is a member.
• A Delete often only has the object URI or a Tombstone without attributedTo, so I think Delete is not covered, but #3883 is mainly about Update and Delete.
• get_attributed_groups() runs for every incoming activity, before the public check, with up to 5 queries plus parsing the actor JSON.

My idea would be to keep it smaller: only Update and Delete, only for objects we already stored, and only if the stored owner is a Group on the same host as the actor. That check would live in the handler (or Remote_Posts), not in the recipient lookup. New content from a Group would still come through the Announce of the Group.

/cc @Menrath, is "same host" enough for Mobilizon, or do you think we need a membership check?

[faisalahammad]

Thanks @pfefferle, good points. I changed it to your idea in c993366.

• The inbox recipient lookup is not changed anymore. get_attributed_groups() is gone, so there is no extra cost per incoming activity.
• Remote_Posts::update() now also accepts an Update when the stored owner of the post is a cached Group, and the actor is on the same host as that Group. It only runs when the actor is not the owner.
• It only works for posts we already stored. New content from a Group still comes through the Announce of the Group.
• Delete: Delete::maybe_delete_post() already checks for a tombstone at the origin before it removes a stored post. It does not check the actor, so a Delete sent by a member already works.
• The inbox still fires activitypub_handled_inbox_update when there are no recipients, so the handler runs even if nobody follows the member.
• New tests check that a member can update a Group post, an actor from another host can not, and a neighbor on the same host can not update a post of a Person.

I kept "same host" as the only check for now, and wait for @Menrath on the membership question. I also updated the PR description.

[Menrath]

@pfefferle

is "same host" enough for Mobilizon, or do you think we need a membership check?

I don't know myself how Mobilizon Group architecture works. I tried to dig into that a while ago and gave up. But I guess

Same host as the only check is quite open. Any account on a Lemmy, PeerTube or Mobilizon instance could send a post attributed to a Group on that instance to all WordPress followers of that Group. Normally the Group announces the content, so it can moderate it. Here nothing checks that the actor is a member.

I think this argument does not fit for Server-to-Server federation. We have to trust an remote origin anyway. If anyone could just freely make their Server send malicious Server-to-Server content, it is a clear bug on their side?