When the object associated with the activity was preloaded
(which happens automatically with Activity.normalize used in the
controller) Object.normalize’s "id_only" option did not actually work.
This option and it’s usage were introduced to fix display of Undo
activities in e88f36f72b.
For "Undo"s (and "Delete"s) there is no object preloaded
(since it is already gone from the database) thus this appeared
to work and for the particular case considered there in fact did.
Create activities use different rendering logic and thus remained
unaffected too.
However, for all other types of Activities (yes, including Update
which really _should_ include a properly sanitised, full object)
this new attempt at including "just the id", lead to it instead
including the full, unsanitised data of the referenced object.
This is obviously bad and can get worse due to access restrictions
on the activity being solely performed based on the addressing
of the activity itself, not of the (unintentionally) embedded
object.
Starting with the obvious, this leaks all "internal" fields
but as already mentioned in 8243fc0ef4
all current "internal" fields from Constants.object_internal_fields
are already publicised via MastoAPI etc anyway. Assuming matching
addressing of the referenced object and activity this isn't problematic
with regard to confidentiality.
Except, the internal "voters" field recording who voted for a poll
is currently just omitted from Constants.object_internal_fields
and indeed confidential information (fix in subsequent commit).
Fortunately this list is for the poll as a whole and there are no
inlined lists for individual choices. While this thus leaks _who_
voted for a poll, it at least doesn't directly expose _what_ each voter
chose if there are multiple voters.
As alluded to before, the access restriction not being aware
of the misplaced object data into account makes the issue worse.
If the activity addressing is not a subset of the referenced object’s
addressing, this will leak private objects to unauthorised users.
This begs the question whether such mismatched addressing can occur.
For remote activities the answer is ofc a resounding YES,
but we only serve local ActivityPub objects and for the latter
it currently(!) seems like a "no".
For all intended interactions, the user interacting must already have
access to the object of interest and our ActivityPub Builder
already uses a subset of the original posts addressing for
posts not publicly accessible. This addressing creation logic
was last touched six years ago predating the introduction of this
exposure blunder.
The rather big caveat her being, until it was fixed just yesterday in
dff532ac72 it was indeed possible to
interact with posts one is not allowed to actually see. Combined, this
allowed unauthorised access to private posts. (The API ID of such
private posts can be obtained e.g. from replies one _is_ allowed to see)
During the time when ActivityPub C2S was supported there might have been
more ways to create activities with mismatched addressing and sneak a
peek on private posts. (The AP id can be obtained in an analogous way)
Replaces and fixes e88f36f72b.
Since there never were any users of the
bugged "id_only" option it is removed.
This was reported by silverpill <silverpill@firemail.cc> as an
ActivityPub interop issue, since this blunder of course also
leads to invalid AP documents by adding an additional layer
in form of the "data" key and directly exposing the internal
Pleroma representation which is not always identical to valid AP.
Fixes: https://akkoma.dev/AkkomaGang/akkoma/issues/1017
Until now only a limited number of self-replies were inlined as an
anonymous, unordered ActivityPub collection. Notably the advertised
replies might be private posts.
However, providing all (non-private) replies allows for better thread
consistency across instances if the remote server cooperates.
The collection existing as a stndalone object has two advantages
for this. For one, if it was still anonymous, _all_ replies would need
to be inlined, which might be too bloated in pathological cases.
Secondly, it allows remote servers to update the thread by traversing
the reply collection independent of the original post. (If the remote
part knows about chronological ordering, it can in theory
even efficiently resume from where it previously stopped)
The object lookup is later repeated in the validator, but due to
caching shouldn't incur any noticeable performance impact.
It’s actually preferable to check here, since it avoids the otherwise
occuring user lookup and overhead from starting and aborting a
transaction
Non-Create/Listen activities had their associated object field
normalized and fetched, but only to use their `id` field, which is both
slow and redundant. This also failed on Undo activities, which delete
the associated object/activity in database.
Undo activities will now render properly and database loads should
improve ever so slightly.
