The ban on redirects was based on a misreading of FEP-2c59’s
requirements. It is only meant to forbid addresses other than
the canonical ActivityPub ID being advertised as such in the
returned WebFinger data.
This does not meaningfully lessen security and verification still
remains stricter than without FEP-2c59.
Notably this allows Mastodon with its backwards WebFinger redirect
(redirecting from the canonical WebFinger domain to the AP domain)
to adopt FEP-2c59 without causing issues or extra effort to existing
deplyoments which already adopted the Mastodon-recommended setup.
One asserted the response format of finger_actor on a finger_mention
call as a previous iteration of the implementation mistakenly returned.
The other didn’t actually test anything WebFinger but fundamental id
containment and verification for generic AP fetches. Now it does.
It is used in security checks and only due to an abudance of
existing tests lacking it, _only_ the test env is allowed to
fallback to the query URL for theses tests as a temporary
(well, ... it’s been a while now) measure.
We really shouldn’t be adding more deficient tests like that.
Notably user follow* collections faked a zero totalItem count
rather than simply omitting the field and included a link to a first
page even when knowing this page cannot be fetched while most others
omitted it. Omitting it will spare us receiving requests doomed to
failure for this page and matches the format used by GtS and Mastodon.
Such requests occur e.g. when other *oma servers try to determine
whether follow* relationships should be publicly shown. Other
implementations like Mastodon¹ simply treat the presence of a (link to)
a first page aready as an indicator for a public collection. By
omitting the link Mastodon servers will now too honour our users
request to hide follow* details.
The "featured" user collection are typically quite small and thus
the sole occurence of the alternative form where all items are directly
inlined into the root without an intermediate page. Thus it is not
converted but also no helper for this format created.
1: eb848d082a/app/services/activitypub/process_account_service.rb (L303)
The count is precomputed as a user property.
Masto API already relies on this cached value.
This let’s us skip actually querying follow* details unless
follow(ing/ed) users are publicly exposed and thus will be served.
In fact this could now be optimised further by using keyset pagination
to only fetch what’s actually needed for the current page. This would
also completely obsolete the need for the _offset collection page
helpers. However, for this pagination to be efficient it needs to happen
o the follow relation table, not users. This is left to a future commit.
Due to an ambiguity with PhoenixHtmlHelpers the Ecto.Query
select import was unusable without extra qualification,
therfore it is converted to a require expression.
While "is_active" is a property of users, it is not a recognised keyword
for the User.Query helper; instead "deactivated" with negated value must
be used.
This arose because originally the user property was also called
"deactivated" until it was renamed adn negated five years ago
in 860b5c7804. This included renaiming
the parameter in most but not all User.Query usages.
Notably the parameter in User.get_followers_query was renamed
but not in User.get_friends_query (friends == following).
The accepted query parameter in User.Query however was not changed.
This lead to the former mistakenly including deleted users causing
too large values to be reported in the ActivityPub follower, but not
following collection as reported in #1078.
In Masto API responses filtering by `User.is_visible` weeded out
the extra accounts before they got displayed to API users.
On the surface it might seem logical to align the name of the User.Query
parameter with the actual property name. However, User.Query already
accepts an "active" parameter which is an alias for limiting to accounts
which are neither deleted nor deactivated by moderators (both indicated
by is_active) and also are not newly created account requests still
pending an admin approval (is_approved) or necessary email confirmation
(is_confirmed); in short as the alias suggests whether the account is
active. Two highly similar parameter names like this would be much too
confusing.
The renamed "is_active" on the other hand does not actually suffice to
say whether an account is actually active, only whether it has (not yet)
ceased to be active (by its own volition or moderator action).
Meaning its "new" name is actively misleading. Arguably the rename
made things worse for no reason whatsoever and should not ever have
happened.
For now, we’ll just revert the incorrect query helper parameter renames.
Fixes: https://akkoma.dev/AkkomaGang/akkoma/ issues/ 1078
With the follow info update now actually running after being fixed
a bunch of errors about :not_found and :forbidden collection fetches
started to be logged
Even when the "always force revalidation" option is not enabled
while avoiding unnecessary revalidations if nothing changed.
With this heuristic we should be able to change the default to "false"
soon, but for now keep it enabled to help amend recent bugs.
The real intent behind the commit introducing this seemed to have been
avoiding running this when the actor does not expose follow collection ids
ec24c70db8.
This is already taken care of with the :collections_available check.
Some Implementations use other actor type like Group etc for visible,
followable actors making skipping undesirable.
Notably though, this actually has _always_ skipped counter updates
as even when this check was introduced, the user changeset data and
struct used the :actor_type key not :type.
In some situations fetch_follow_information_for_user is called directly
from other modules thus occasionally counters still got updated
for accounts with closer federation relationships masking the issue.
Resolves all security issues discussed in 5a120417fd86bbd8d1dd1ab720b24ba02c879f09
and thus reactivates skipped tests.
Since the necessary logic significantly differs for WebFinger handle dicovery/validation
and fetching of actors from just the webfinger handle the relevant public function was split
necessitating also a partial rewrite of the user fetch logic.
This works with all of the following:
- ActivityPub domain is identical to WebFinger handle domain
- AP domain set up host-meta LRDD link to WebFinger domain
- AP domain set up a HTTP redirect on /.well-known/webfinger
to the WebFinger domain
- Mastodon style: WebFinger domain set up a HTTP redirect
on its well-known path to AP backend (only for discovery
from nickname though until Mastodon supports FEP-2c59)
This intentionally does not work for setups where FEP-2c59 is not
supported and the initially queried domain simply directly responds with
data containing a nickname from another domain’s authority without any
redirecty. (This includes the setup currently recommended by Mastodon,
when enriching an AP actor. Once Mastodon supports FEP-2c59 though its
setup will start to work again too automatically).
While technically possible to cross-verify the data with the nickname
domain, the existing validation logic is already complex enough and
such cross-validation needs extra safety measures to not get trapped
into infinite loops. Such setups are considered broken.
It doesn’t make sense in general (many implementations use ids not nicks in ap_id)
and just wastes time by making additional, unnecessary, failing network requests.
This arguably should have never been committed.
The ActivityPub module is already overloaded and way too large.
Logic for fetching users and user information is isolated from
all other parts of the ActivityPub module, so let’s split it out.
It makes discovery and validation of the desired webfinger address
much easier. Future commits will actually use it for validation
and nick change discovery.
Proper validation of nicknames must consider both the domain
the nickname is associated with _and_ the actor to be assigned this
nickname consent to this.
Prior attempts at securing this wer emade in
a953b1d927 and
0d66237205 but they do not suffice.
The existing code attempted to validate webfinger responses independent
of the actual ActivityPub actor data and only ever consider the former
(yet failed to properly validate even this).
When resolving a user via a user-provided nickname the assignment
done by the provided URL was simply trusted regardless of the actors
AP host or data. When the nresolving the AP id, the nickname from this
original WebFinger query was passed along as pre-trusted data overriding
any discovery or validation from the actual actors side.
This allowed malicious actors to serve rogue WebFinger data associating
arbitrary actors with any nicknames they wished. Prompting servers to
resolve this rogue nickname then creates this nonconsensual assignment.
Notably, the existing code’s attempt at verification (only for domain
consent) used the originally requested URL for comparison against the
domain in the nickname handle. This effectively disabled custom
WebFinger domains for honest servers unless using an host-meta LRDD link.
(While LRDD links were recommend in the past by both *oma nad Mastodon,
today most implementations other than *oma instead
recommend setups emplyoing HTTP redirects.)
Still, this strictness did not prevent spoofing by malicious server.
It did however mean that rogue nickname assignments from an initial
nickname-based discovery were at least undone on the next user refresh
provided :pleroma, Pleroma.Web.WebFinger, :update_nickname_on_user_fetch
was not changed from its default truthy value.
(A renewed fetch via the rogue nickname would re-establish it though)
When enriching an already resolved ActivityPub actor to discover its
nickname the WebFinger query was not done with the unique AP id as a
resource, but a guessed nickname handle.
Furthermore, the received WebFinger response was not validated
to ensure the ActivityPub ID the WebFinger server pointed to
for the final nickname matched the actual ID in the considered
AP actor data.
While the faulty request URI check described above provides some
friction for malicious actors, it is still possible for mischiveous
AP instances to setup a rogue LRDD link poitning to a third-party
domain’s WebFInger and using the freedom provided by the LRDD link
to overwrite the resource value we provide in the lookup. Thus usurping
existing nicknames in another domains authority.
Proposed tweaks to the existing, faulty checks to work with
HTTP-redirect-based custom WebFInger domains would have made it
even easier to usurp nicknames from foreign domains.
For now simply disable custom WebFinger domains as a quick hotfix.
Subsequent commits will partially de-spaghettify the relevant code
and completely overhaul webfinger and nickname handling and validation.
Deleting a whole pack at once didn’t remove its emoji from memory
and ewly updated or added emoji used a wrong path. While the pack
also contains a path, this is the filesystem path while in-memory
Emoji need the URL path constructed from a constant prefix,
pack name and just the filename component of the filesystem file.
Test used to not check for this at all.
Fixes oversight in 318ee6ee17
