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

Properly implement the libravatar federation API #2334

Merged
merged 2 commits into from Apr 9, 2024

Conversation

LukeShu
Copy link
Contributor

@LukeShu LukeShu commented Apr 4, 2024

I have no strong opinions about whether WebFinger should be supported instead of Libravatar (as suggested in #1043), but as long as there's a thing in Liberapay that says "libravatar", it should do what the libravatar API docs say. It is surprising and confusing that Liberapay will tell users "We were unable to get an avatar for you from libravatar" while https://www.libravatar.org/tools/check/ is telling them that their avatar is A-OK.

Unlike the old #1043, this implements the libravatar API in Liberapay, instead of using the unmaintained (since 2016) pyLibravatar library (which in turn uses the py3dns library, instead of the Dnspython library that Liberapay prefers).

I have done minimal testing of this locally on my laptop.

Closes #1367

@LukeShu
Copy link
Contributor Author

LukeShu commented Apr 5, 2024

v2: Fixed flake8 complaints

I have no strong opinions about whether WebFinger should be supported
instead of Libravatar (as suggested in
#1043), but as long as
there's a thing in Liberapay that says "libravatar", it should do what
the libravatar API docs say.  It is surprising and confusing that
Liberapay will tell users "We were unable to get an avatar for you
from libravatar" while https://www.libravatar.org/tools/check/ is
telling them that their avatar is A-OK.

Unlike the old #1043,
this implements the libravatar API in Liberapay, instead of using the
unmaintained (since 2016) pyLibravatar library (which in turn uses the
py3dns library, instead of the Dnspython library that Liberapay
prefers).
@LukeShu
Copy link
Contributor Author

LukeShu commented Apr 8, 2024

v3:

  • Add more comments
  • add srv_records.sort(key=lambda rec: rec.weight) to ensure that .weight=0 recs are first in the list, per RFC 2782 ("in any order, except that all those with weight 0 are placed at the beginning of the list."); without this, .weight=0 records are unselectable by the algorithm.
diff --git a/liberapay/models/participant.py b/liberapay/models/participant.py
index 9995e0775..4d42b01db 100644
--- a/liberapay/models/participant.py
+++ b/liberapay/models/participant.py
@@ -2183,6 +2183,10 @@ class Participant(Model, MixinTeam):
             if not email:
                 return
             # https://wiki.libravatar.org/api/
+            #
+            # We only use the first SRV record that we choose; if there is an
+            # error talking to that server, we give up, instead of retrying with
+            # another record.  pyLibravatar does the same.
             normalized_email = email.strip().lower()
             try:
                 # Look up the SRV record to use.
@@ -2194,10 +2198,20 @@ class Participant(Model, MixinTeam):
                     srv_records = DNS.query('_avatars._tcp.'+email_domain, 'SRV')
                     scheme = 'http'
                 # Filter down to just the records with the "highest" `.priority`
-                # (lower number = higher priority).
+                # (lower number = higher priority); for the libravatar API tells us:
+                #
+                # > Libravatar clients MUST only consider servers listed in the
+                # > highest SRV priority.
                 top_priority = min(rec.priority for rec in srv_records)
                 srv_records = [rec for rec in srv_records if rec.priority == top_priority]
-                # Of those, choose randomly based on their relative `.weight`s.
+                # Of those, choose randomly based on their relative `.weight`s;
+                # for the libravatar API tells us:
+                #
+                # > They MUST honour relative weights.
+                #
+                # RFC 2782 (at the top of page 4) gives us this algorithm for
+                # randomly selecting a record based on the weights:
+                srv_records.sort(key=lambda rec: rec.weight)  # ensure that .weight=0 recs are first in the list
                 weight_choice = randint(0, sum(rec.weight for rec in srv_records))
                 weight_sum = 0
                 for rec in srv_records:

@Changaco
Copy link
Member

Changaco commented Apr 9, 2024

Privacy impact assessment: as it was already possible to inject arbitrary avatar URLs through the Mastodon integration, implementing the Libravatar federation protocol doesn't significantly increase the possibility for third parties to observe who is visiting the Liberapay website.

Security considerations: instructing browsers to download images from untrusted domains is a risk that should be mitigated by proxying (as initially envisaged in #202), but since this risk already exists and isn't being significantly increased by the implementation of the Libravatar federation protocol, it doesn't justify holding this pull request.

@Changaco Changaco merged commit a4deb11 into liberapay:master Apr 9, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Federated libravatar should be supported
2 participants