Status visibility checks for post interactions, stop leaking internal Activity representation (Akkoma PR 1014 and 1018)
Closes#3383
See merge request pleroma/pleroma!4400
- Adds another Pleroma.ActivityPub.Visibility.visible_for_user?/2 func
- Modifies existing tests to include a local Activity referencing a
remote Object
- Changes Announce Activity test factory to reference Objects instead of
Activities and use a different Actor for the Announce
- Changes ap_id of remote user in Announce test factory to match Objects
- Adds `object_local` option to Note factories that explicitly changes
the domain in the URL to not match the endpoint URL in the test env
to properly work with the new visibility func, since we don't store
locality of Object unlike Activities
Akkoma patches returned 403 and some of my previous commits returned 422.
This unifies the errors returned to 404 "Record not found", gaslighting
user just like we do for other endpoints and how Mastodon does it.
When a user tried to unpin a status not belonging to them, a full
MastoAPI response was sent back even if status was not visible to them.
Ditto with (un)mutting except ownership.
Before a request arrives to update_outbox, it already passed through out
Plug authentication (:authenticate), so at this point all users should
be local.
Also adds Listen Activities to the list of allowed Activities that don't
need an existing normalized object referenced in them.
A local user could previously send Announce/EmojiReact/Like activities
to their outbox referencing objects that aren't visible to them and they
would get processed as if can see them. Only requirement is knowing
the URI of the object and the users instance having C2S enabled (currently
disabled by default).
Port of commit 85171750f17725b71dcda098a5085b7f402cb061 from
Akkoma PR 1018.
Modifications from Akkoma patch:
- Pleroma.Web.ActivityPub.Utils.make_json_ld_header() calls had
activity.data as argument.
- render() had Listen activities in activity_type, Akkoma only has
Create activities there. Needs testing whether transmogrifier can
handle this.
Original commit author: Oneric <oneric@oneric.stub>
Original commit message:
Duped code just means double the chance to mess up. This would have
prevented the leak of confidential info more minimally fixed in
6a8b8a14999f3ed82fdaedf6a53f9a391280df2f and now furthermore
fixes the representation of Update activites which _need_ to have their
object inlined, as well as better interop for follow Accept and Reject
activities and all other special cases already handled in Transmogrifier.
It also means we get more thorough tests for free.
This also already adds JSON-LD context and does not add bogus Note-only
fields as happened before due to this views misuse of prepare_object
for activities. The doc of prepare_object clearly states it is only
intended for creatable objects, i.e. (for us) Notes and Questions.
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 e88f36f72b5317debafcc4209b91eb35ad8f0691.
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 8243fc0ef482a28daf2bcae2c64a9510bdb76489
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
dff532ac723310903e58c5d28f897cc2d116594f 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 e88f36f72b5317debafcc4209b91eb35ad8f0691.
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
Port of Akkoma PR 1014 with a few changes:
- comments regarding akkomafe changed to Pleroma-FE when applicable
- different error message for replying to/interacting with invisible post
in Pleroma.Web.CommonAPI.ActivityDraft.in_reply_to/1
- split "doesn't do funny things to other users favs" test into three:
- can't unfavourite post that isn't favourited
- can't unfavourite other user's favs
- can't unfavourite other user's favs using their activity
- switched order of args for some CommonAPI function since Akkoma hasn't
backported our old change for that
Pleroma.Web.CommonAPI.ActivityDraft.in_reply_to/1 now refactored to use
`with` statement as in Akkoma. Some defp in_reply_to/1 were therefore removed
Original PR author: Oneric
Original commit message:
It doesn't make sense to like, react, reply, etc to something you cannot
see and is unexpected for the author of the interacted with post and
might make them believe the reacting user actually _can_ see the post.
Wrt to fav, reblog, reaction indexes the missing visibility check was
also leaking some (presumably/hopefully) low-severity data.
Add full-API test for all modes of interactions with private posts.
Hackney interferes with out URI encoding and implements older RFC 2396
instead of RFC 3986 which we and Elixir implement. As an example "'"
and "!" will get encoded by it and cause problems with our MediaProxy
making unexpected 302 redirects.
If an admin supplies a different function via *.secret.exs, we
don't override it.
https://github.com/benoitc/hackney/issues/399
