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

Feature Request: Drop (certain) Delete requests? #554

Closed
janboddez opened this issue Nov 9, 2023 · 8 comments
Closed

Feature Request: Drop (certain) Delete requests? #554

janboddez opened this issue Nov 9, 2023 · 8 comments

Comments

@janboddez
Copy link
Contributor

janboddez commented Nov 9, 2023

What

Seeing a lot of Delete activities in debug.log, followed by a 401 response ("Remote profile not accessible" or something).

But I'm (i.e., my WordPress user is) not following anyone, and less than 0.01% of these Deletes concern followers of mine.

Don't think it makes sense to look up remote profiles for all of these Delete requests. If they concern a follower, we should probably verify the delete actually took place. If not, can't we just drop the request and be done with it? (Or maybe that's already happening and the debug is simply ambiguous?)

Why

Don't think it makes sense to look up remote profiles for Delete requests if they don't concern a profile that we actually know.

Hoping this frees up some server resources.

How

No response

@janboddez
Copy link
Contributor Author

janboddez commented Nov 9, 2023

Seems it merely tries to fetch the remote public key for sig verification (which then fails because the remote profile probably responds with a 410, although I get why you'd want to verify these requests :-D), but I'm wondering if we can't simply skip this step for "Deletes" of unknown users, and return a "444" or similar, or even a 202, but return early or terminate the request.

@pfefferle
Copy link
Member

I am currently working on activity handlers to better support delete and update requests. I will ping you in the PR if I have something to show!

@janboddez
Copy link
Contributor Author

Had another look and decided to try and write a activitypub_defer_signature_verification callback.

In it, I mainly just check for the Delete type. If the request is for a Delete activity, I check if the actor is in our list of followers. If so, I won't defer. If not, I do just respond with a 202 and exit.

This is probably overkill, still; I think I could, right now at least, also just choose to defer verification. (I don't think Deletes are acted upon at all?) But simply terminating the request seemed safer.

In fact, if Deletes really are silently discarded by the plugin, we could probably just skip verification for all of them. Or I could create my own rest_request_before_callbacks callback and simply return a 202 or whatever and be done with it there. (I could, in fact, also use rest_request_before_callbacks; it'd give me access to $request, and that's really all I need.) :-D

Also, as I type this, I realize I am was doing this for all Deletes, which is far from ideal. If I'm really going to check that an actor is unkown to WordPress, I should also check that it's really an actor that is being deleted, i.e. that actor === object.

@mediaformat
Copy link
Collaborator

mediaformat commented Nov 12, 2023

😅 I actually started a follow up to #552 yesterday which checks for a delete actor request, compares to known commenters, and either skips the delete, or go ahead and delete all the comments if the actor is known.

@pfefferle
Copy link
Member

pfefferle commented Nov 13, 2023

@janboddez I currently work on the delete handler and experiment with late signature verification. something like if user is in follower list, verify request and delete it, otherwise ignore it!

https://github.com/Automattic/wordpress-activitypub/pull/551/files#diff-896366377392300e7606f7d50f26d18b24086b826cd1923e5a6e197e7d51f8bf

@pfefferle
Copy link
Member

Or rather we must not check the signature at all on delete, it could be a simple check for http code 404 or 410!

@mediaformat
Copy link
Collaborator

@pfefferle This is what I've done in #561

Copy link

This issue is stale because it has been open 120 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Mar 13, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants