AkkomaGang/akkoma
AkkomaGang/akkoma
16
67
Fork 101
Code Issues 170 Pull requests 16 Projects 2 Releases 19 Packages Wiki Activity

MRF.ObjectAgePolicy: Check objects alongside Create activities #845

Closed
norm wants to merge 1 commit from norm/akkoma:fix-object-age-policy into develop
pull from: norm/akkoma:fix-object-age-policy
merge into: AkkomaGang:develop
Conversation 5 Commits 1 Files changed 3 +71 -24
norm commented 2024-10-25 19:58:10 +00:00
Contributor
Copy link

ObjectAgePolicy only checked Create activites, which still resulted in
some old posts making it to user timelines, even when it was enabled.

This fixes that issue by also checking objects of an updatable type
as defined in Pleroma.Constants.updatable_object_types.

ObjectAgePolicy only checked Create activites, which still resulted in some old posts making it to user timelines, even when it was enabled. This fixes that issue by also checking objects of an updatable type as defined in `Pleroma.Constants.updatable_object_types`.
norm commented 2024-10-25 19:59:34 +00:00
Author
Contributor
Copy link

There might be a better way of refactoring the new stuff, so tips would be appreciated for doing that.

There might be a better way of refactoring the new stuff, so tips would be appreciated for doing that.
norm force-pushed fix-object-age-policy from fc2ba27d80 to 17e3655886 2024-10-25 20:04:39 +00:00 Compare
Oneric reviewed 2024-11-01 18:02:56 +00:00
Oneric left a comment
Owner
Copy link

reminder to run mix test; the function clause mismatches show up there and speaking of mix test it’d be good to add a test for objects not being fully rejected but still stripped

Btw, I know I suggested the base idea with object types and it still seems sensible, but did you confirm it works in practice when e.g. searching an URL of an old, not-yet-known post of a followed user?

reminder to run `mix test`; the function clause mismatches show up there and speaking of `mix test` it’d be good to add a test for objects not being fully rejected but still stripped Btw, I know I suggested the base idea with object types and it still seems sensible, but did you confirm it works in practice when e.g. searching an URL of an old, not-yet-known post of a followed user?
lib/pleroma/web/activity_pub/mrf/object_age_policy.ex Outdated
@ -15,0 +27,4 @@
end
defp check_date(%{"published" => published} = message) do
Oneric commented 2024-11-01 17:57:23 +00:00
Owner
Copy link

This now checks for published in the top-level object, but the code path for activities still passes the top-level activity instead of the child object to check_date. The activity might not have a published field and thus crash the process

This now checks for `published` in the top-level object, but the code path for activities still passes the top-level activity instead of the child object to `check_date`. The activity might not have a `published` field and thus crash the process
norm marked this conversation as resolved
lib/pleroma/web/activity_pub/mrf/object_age_policy.ex Outdated
@ -100,2 +110,4 @@
end
@impl true
def filter(%{"type" => type} = message) when type in Pleroma.Constants.updatable_object_types() do
Oneric commented 2024-11-01 17:54:01 +00:00
Owner
Copy link

Two things:

  • the pattern match must also include a "published" => _ parameter like for child objects above, else there won't be a function clause match later in check_date and processing crashes (effectively rejecting the post with added perf overhead from retries)
  • instead of updatable_object_types a new fetchable_object_types or perhaps just a complete object_types constant would be clearer imho.
    Checking some other object type matches in the codebase, the only type missing from updatable_object_types appears to be Answer (transmogrifier maps Notes in response to a Question to a separate Answer type)
Two things: - the pattern match must also include a `"published" => _` parameter like for child objects above, else there won't be a function clause match later in `check_date` and processing crashes *(effectively rejecting the post with added perf overhead from retries)* - instead of updatable_object_types a new `fetchable_object_types` or perhaps just a complete `object_types` constant would be clearer imho. Checking some other object type matches in the codebase, the only type missing from `updatable_object_types` appears to be `Answer` *(transmogrifier maps `Note`s in response to a `Question` to a separate `Answer` type)*
lib/pleroma/web/activity_pub/mrf/object_age_policy.ex Outdated
@ -101,1 +111,4 @@
@impl true
def filter(%{"type" => type} = message) when type in Pleroma.Constants.updatable_object_types() do
# Don't reject fetched objects, only delist/strip followers
Oneric commented 2024-11-01 17:59:19 +00:00
Owner
Copy link

Instead of duplicating the base function, i’d prefer to lift the logic out into a shared function and pass a flag to it indicating whether or not a full reject is allowed

Instead of duplicating the base function, i’d prefer to lift the logic out into a shared function and pass a flag to it indicating whether or not a full reject is allowed
norm marked this conversation as resolved
norm force-pushed fix-object-age-policy from 17e3655886 to 3c0d98dd7f 2024-11-02 05:27:06 +00:00 Compare
norm force-pushed fix-object-age-policy from 3c0d98dd7f to 84ef99daac 2024-11-02 05:29:01 +00:00 Compare
norm force-pushed fix-object-age-policy from 84ef99daac to d78927113e 2024-11-02 05:46:16 +00:00 Compare
norm force-pushed fix-object-age-policy from d78927113e to 7507984e07 2024-11-02 06:15:02 +00:00 Compare
norm force-pushed fix-object-age-policy from 7507984e07 to a1ba6357e8 2024-11-02 06:25:14 +00:00 Compare
norm force-pushed fix-object-age-policy from a1ba6357e8 to 50f7b6367f 2024-11-02 07:11:09 +00:00 Compare
norm commented 2024-11-02 07:12:53 +00:00
Author
Contributor
Copy link

Tests should pass now, will test the functionality a bit later

Tests should pass now, will test the functionality a bit later
👍 1
Oneric commented 2024-11-15 23:11:27 +00:00
Owner
Copy link

Checking this out with debug logs reveals current unpatched develop:

  • generates a fake "Create" for manually fetched objects — and passes only the Create to MRFs, breaking the assumption underlying this fix
  • this activity doesn’t have its own published field
  • meaning fetches already get rejected atm if :reject is set making it a pretty destructive option
  • calling into question why some MRFs check raw objects at all
  • unfortunately meaning there’s no ready-to-use way to distinguish fetches

After the Create activity passed the actual fetched object also passes through MRFs.

Well, how come the MRF obviously doesn’t prevent fetched posts from entering TLs?

  • it actually successfully :delists ”public” posts (only stripping the generated activity though, not the object recipients)*

  • it also removes follower collection from the public post’s activity

  • this also works to prevent those psots from showing up in the timeline

  • but... note how i specified “public” post above; the issue actually lies in the delisting logic. Taking a look at debug logs for an “unlisted” post:

    --- Checking Create: nil []  for https://fri/objects/87b92a04-f535-4b7d-b508-c0e9159854b3 [2024-04-12T18:24:27.686336Z]
--- --- [DELIST] Initial to: ["https://fri/users/e/followers"]
--- --- [DELIST] Initial cc: ["https://www.w3.org/ns/activitystreams#Public"]
--- --- [DELIST] Fixed to: ["https://fri/users/e/followers", "https://fri/users/e/followers"]
--- --- [DELIST] Fixed cc: ["https://www.w3.org/ns/activitystreams#Public", "https://www.w3.org/ns/activitystreams#Public"]
--- --- [STRIPFW] Initial to: ["https://fri/users/e/followers", "https://fri/users/e/followers"]
--- --- [STRIPFW] Initial cc: ["https://www.w3.org/ns/activitystreams#Public", "https://www.w3.org/ns/activitystreams#Public"]
--- --- [STRIPFW] Fixed to: ["https://fri/users/e/followers"]
--- --- [STRIPFW] Fixed cc: ["https://www.w3.org/ns/activitystreams#Public", "https://www.w3.org/ns/activitystreams#Public"]
--- Let pass unchecked: Note  https://fri/objects/87b92a04-f535-4b7d-b508-c0e9159854b3

    Whoops, the existing follower address and public scope scope indicator get duplicated, and in accordance with List.delete’s documentation only the first instance dropped later.
    Actually, the logic would even add public indicators to any received DMs if they arrive late enough... ouch
    ... and it converting DMs to "unlisted" is even checked in tests, marvellous (this seems like an accident from mindlessly adjusting matches; it got added when Pleroma fixed crashes on nil adressing)

Checking this out with debug logs reveals current unpatched develop: - generates a fake `"Create"` for manually fetched objects — and passes only the Create to MRFs, breaking the assumption underlying this fix - this activity doesn’t have its own `published` field - meaning fetches already get rejected atm if `:reject` is set making it a pretty destructive option - ~~calling into question why some MRFs check raw objects at all~~ - unfortunately meaning there’s no ready-to-use way to distinguish fetches **After** the Create activity passed the actual fetched object _also_ passes through MRFs. Well, how come the MRF obviously doesn’t prevent fetched posts from entering TLs? - it actually successfully `:delists` ”public” posts (only stripping the generated activity though, not the object recipients)* - it _also_ removes follower collection from the public post’s activity - this also _works_ to prevent those psots from showing up in the timeline - **but...** note how i specified “public” post above; the issue actually lies in the delisting logic. Taking a look at debug logs for an “unlisted” post: ``` --- Checking Create: nil [] for https://fri/objects/87b92a04-f535-4b7d-b508-c0e9159854b3 [2024-04-12T18:24:27.686336Z] --- --- [DELIST] Initial to: ["https://fri/users/e/followers"] --- --- [DELIST] Initial cc: ["https://www.w3.org/ns/activitystreams#Public"] --- --- [DELIST] Fixed to: ["https://fri/users/e/followers", "https://fri/users/e/followers"] --- --- [DELIST] Fixed cc: ["https://www.w3.org/ns/activitystreams#Public", "https://www.w3.org/ns/activitystreams#Public"] --- --- [STRIPFW] Initial to: ["https://fri/users/e/followers", "https://fri/users/e/followers"] --- --- [STRIPFW] Initial cc: ["https://www.w3.org/ns/activitystreams#Public", "https://www.w3.org/ns/activitystreams#Public"] --- --- [STRIPFW] Fixed to: ["https://fri/users/e/followers"] --- --- [STRIPFW] Fixed cc: ["https://www.w3.org/ns/activitystreams#Public", "https://www.w3.org/ns/activitystreams#Public"] --- Let pass unchecked: Note https://fri/objects/87b92a04-f535-4b7d-b508-c0e9159854b3 ``` Whoops, the existing follower address and public scope scope indicator get duplicated, and in accordance with `List.delete`’s documentation only the first instance dropped later. Actually, the logic would even add public indicators to any received DMs if they arrive late enough... ouch ... and it converting DMs to "unlisted" is even checked in tests, marvellous (this seems like an accident from mindlessly adjusting matches; it got added when Pleroma [fixed _crashes_ on `nil` adressing](https://akkoma.dev/AkkomaGang/akkoma/commit/10c792110e6ea8ed21f739ef8f4f0eff4659ebf9))
Oneric commented 2024-11-16 00:24:35 +00:00
Owner
Copy link

Tested fix in #851

Tested fix in #851
norm commented 2024-11-16 16:26:00 +00:00
Author
Contributor
Copy link

looks like this PR isn't needed then as the problem lays elsewhere.

looks like this PR isn't needed then as the problem lays elsewhere.
norm closed this pull request 2024-11-16 16:26:02 +00:00
Some checks are pending
ci/woodpecker/pull_request_closed/build-amd64 Pipeline is pending approval
Details
ci/woodpecker/pull_request_closed/build-arm64 Pipeline is pending approval
Details
ci/woodpecker/pull_request_closed/docs Pipeline is pending approval
Details
ci/woodpecker/pull_request_closed/lint Pipeline is pending approval
Details
ci/woodpecker/pull_request_closed/test Pipeline is pending approval
Details
ci/woodpecker/pr/build-amd64 Pipeline is pending approval
Details
ci/woodpecker/pr/docs Pipeline is pending approval
Details
ci/woodpecker/pr/build-arm64 Pipeline is pending approval
Details
ci/woodpecker/pr/lint Pipeline is pending approval
Details
ci/woodpecker/pr/test Pipeline is pending approval
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
approved, awaiting change

   
bug

Something is not working

  
configuration

This requires config changes

  
documentation

This is about human-readable docs

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
extremely low priority

   
feature request

Something new?

  
Fix it yourself

Support has been attempted

  
help wanted

Need some help

  
invalid

Something is wrong

  
mastodon_api

   
needs change/feedback

Issue or PR is stalled on response from the author or a third party

  
needs docs

   
needs tests

   
not a bug

   
planned

   
pleroma_api

   
privacy

   
question

More information is needed

  
static_fe

   
triage

   
wontfix

This won't be fixed

No labels
approved, awaiting change
bug
configuration
documentation
duplicate
enhancement
extremely low priority
feature request
Fix it yourself
help wanted
invalid
mastodon_api
needs change/feedback
needs docs
needs tests
not a bug
planned
pleroma_api
privacy
question
static_fe
triage
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: AkkomaGang/akkoma#845
No description provided.
Delete branch "norm/akkoma:fix-object-age-policy"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?