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

MUST verify ownership upon Delete Activity #294

Closed
cjslep opened this issue Jan 30, 2018 · 4 comments
Closed

MUST verify ownership upon Delete Activity #294

cjslep opened this issue Jan 30, 2018 · 4 comments
Labels
pending-closure-in-7-days This issue will be closed in 7 days after review

Comments

@cjslep
Copy link

cjslep commented Jan 30, 2018

In "7.3 Update Activity":

The receiving server MUST take care to be sure that the Update is authorized to modify its object.

However, there is no similar guarantee for "7.4 Delete Activity". This means spec compliant implementations could have all of their data deleted if implementing the SHOULD requirement...

[...] the server receiving the delete activity SHOULD remove its representation of the object with the same id, and MAY replace that representation with a Tombstone object.

...and ignoring the parenthetical non-requirement, which almost seems to suggest that implementations should just go ahead and assume the ownership is valid:

The side effect of receiving this is that (assuming the object is owned by the sending actor / server) [...]

This could be more problematic for library implementations of ActivityPub, since they may provide a callback hook for a Delete ActivityStream without verifying ownership and assume that users of their library will do the verification when called back. The users of the library could then assume vice-versa, and implement deletion without anyone doing verification, and on the whole still be accidentally spec compliant.

Just to make sure to emphasize importance, I'd just suggest making the MUST language in 7.3 match 7.4.

@wilkie
Copy link

wilkie commented Jan 30, 2018

The SHOULD in the delete section says that servers need not implement deletion, which is reasonable. So, to clarify, you are suggesting the the spec include an entirely new MUST that is equivalent to 7.3 such as "The receiving server MUST take care to be sure that the Delete is authorized to modify its object"?

Seems certainly pedantic but not something that, having looked at them, would break existing implementations if made explicit. That said, implementors are gonna break the rules around authorization whenever they want to do something creative, so it's also not that important to me. I'd also agree with removing the MUST from 7.3 and moving authorization to its own recommendation :)

@cjslep
Copy link
Author

cjslep commented Jan 30, 2018

You're correct, if an implementation implements the SHOULD part of deletion then they MUST verify ownership, to bring it in alignment with 7.3. Conceptually, Delete could be thought of as a special case of Update (complete replacement of the existing object with nothing, or a Tombstone). But that may not be the line of thinking in the spec.

I am not picky on the MUST part, just that they are in sync. So if 7.3 and 7.4 change to "SHOULD look at authorization recommendation X", I have no concerns.

@evanp
Copy link
Collaborator

evanp commented Jun 14, 2023

I wrote a bit of an update for the Delete activity in the Primer that should cover a lot of the necessary work. It's hard to define very well in a specification how the processor should handle its internal storage and authorization; I tried to externalize this into the Primer instead.

https://www.w3.org/wiki/ActivityPub/Primer/Delete_activity

Given that, and that there is ambiguous but existing language in the spec about checking for the owner, I'm going to mark this issue pending closure.

@evanp evanp added the pending-closure-in-7-days This issue will be closed in 7 days after review label Jun 14, 2023
@evanp
Copy link
Collaborator

evanp commented Jul 12, 2023

This has been pending for over a month, and so I will close it for now as resolved.

@evanp evanp closed this as completed Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending-closure-in-7-days This issue will be closed in 7 days after review
Projects
None yet
Development

No branches or pull requests

4 participants