GNU bug report logs - #40354
[PATCH] Fix Libravatar federation handling

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 40354 in the body.
You can then email your comments to 40354 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Philip K <philip <at> warpmail.net>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Fix Libravatar federation handling
Date: Tue, 31 Mar 2020 20:03:24 +0200

Previous implementation didn't correctly handle the result of
dns-query, and didn't implement the necessary record selection
algorithm as specified on the wiki (https://wiki.libravatar.org/api/,
"Federated servers").

* lisp/image/gravatar.el (gravatar--service-libravatar): Fix
libravatar image host resolver algorithm.
---
 lisp/image/gravatar.el | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/lisp/image/gravatar.el b/lisp/image/gravatar.el
index ff59a72ac8..a2207af962 100644
--- a/lisp/image/gravatar.el
+++ b/lisp/image/gravatar.el
@@ -149,13 +149,36 @@ gravatar--service-libravatar
           (dolist (record '(("_avatars-sec" . "https")
                             ("_avatars" . "http")))
             (let* ((query (concat (car record) "._tcp." domain))
-                   (result (dns-query query 'SRV)))
+                   (result (dns-query query 'SRV t)))
               (when result
-                (throw 'found (format "%s://%s/avatar"
-                                      (cdr record)
-                                      result)))))
-          "https://seccdn.libravatar.org/avatar")))))
+                (let* ((res (mapcar (lambda (rec)
+                    	                  (dns-get 'data (cdr rec)))
+                                    (dns-get 'answers result)))
+                       (prio
+                        (mapcar (lambda (r) (dns-get 'priority r)) res))
+                       (max (apply #'max prio))
+                       (sum 0) top)
+                  ;; Attempt to find all records with the same maximal
+                  ;; priority, and calculate the sum of their weights.
+                  (dolist (rec res)
+                    (when (= max (dns-get 'priority rec))
+                      (setq sum (+ sum (dns-get 'weight rec)))
+                      (push rec top)))
+                  ;; In case there is more than one maximal priority
+                  ;; record, choose one at random, while taking the
+                  ;; individual record weights into consideration.
+                  (dolist (rec top)
+                    (when (and (<= (if (= 0 sum) -1 (random sum))
+                                   (dns-get 'weight rec))
+                               (<= 1 (dns-get 'port rec) 65535)
+                               (string-match-p "\\`[-.0-9A-Za-z]+\\'"
+                                               (dns-get 'target rec)))
+                      (throw 'found (format "%s://%s:%s/avatar"
+                                            (cdr record)
+                                            (dns-get 'target rec)
+                                            (dns-get 'port rec))))
+                    (setq sum (- sum (dns-get 'weight rec)))))))
+            "https://seccdn.libravatar.org/avatar"))))))
 
 (defun gravatar-hash (mail-address)
   "Return the Gravatar hash for MAIL-ADDRESS."
-- 
2.20.1

Message #8 received at 40354 <at> debbugs.gnu.org (full text, mbox):

From: "Philip K." <philip <at> warpmail.net>
To: 40354 <at> debbugs.gnu.org
Subject: Re:  [PATCH] Fix Libravatar federation handling
Date: Wed, 01 Jul 2020 16:55:29 +0200

I hope this isn't bad style, but I'd like to ping this issue once
more. The current implementation for libravatar is *not* correct, and
won't properly implement federation. Instead it just always defaults to
"https://seccdn.libravatar.org/avatar", even if a server has explicitly
set up their own avatar server.

-- 
	Philip K.

Message #11 received at 40354 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Philip K <philip <at> warpmail.net>
Cc: 40354 <at> debbugs.gnu.org
Subject: Re: bug#40354: [PATCH] Fix Libravatar federation handling
Date: Sat, 08 Aug 2020 15:10:00 +0200

Philip K <philip <at> warpmail.net> writes:

> Previous implementation didn't correctly handle the result of
> dns-query, and didn't implement the necessary record selection
> algorithm as specified on the wiki (https://wiki.libravatar.org/api/,
> "Federated servers").
>
> * lisp/image/gravatar.el (gravatar--service-libravatar): Fix
> libravatar image host resolver algorithm.

Looks good to me, although this didn't apply either, since the code has
been rewritten to be asynchronous.  Could you re-spin this patch, too,
and I'll get it applied...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #14 received at 40354 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Philip K <philip <at> warpmail.net>
Cc: 40354 <at> debbugs.gnu.org
Subject: Re: bug#40354: [PATCH] Fix Libravatar federation handling
Date: Tue, 18 Aug 2020 16:49:12 +0200

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Philip K <philip <at> warpmail.net> writes:
>
>> Previous implementation didn't correctly handle the result of
>> dns-query, and didn't implement the necessary record selection
>> algorithm as specified on the wiki (https://wiki.libravatar.org/api/,
>> "Federated servers").
>>
>> * lisp/image/gravatar.el (gravatar--service-libravatar): Fix
>> libravatar image host resolver algorithm.
>
> Looks good to me, although this didn't apply either, since the code has
> been rewritten to be asynchronous.  Could you re-spin this patch, too,
> and I'll get it applied...

Philip, have you had time to look at this?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #17 received at 40354 <at> debbugs.gnu.org (full text, mbox):

From: philipk <at> posteo.net (Philip K.)
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 40354 <at> debbugs.gnu.org
Subject: Re: bug#40354: [PATCH] Fix Libravatar federation handling
Date: Tue, 18 Aug 2020 16:57:10 +0200

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Lars Ingebrigtsen <larsi <at> gnus.org> writes:
>
>> Philip K <philip <at> warpmail.net> writes:
>>
>>> Previous implementation didn't correctly handle the result of
>>> dns-query, and didn't implement the necessary record selection
>>> algorithm as specified on the wiki (https://wiki.libravatar.org/api/,
>>> "Federated servers").
>>>
>>> * lisp/image/gravatar.el (gravatar--service-libravatar): Fix
>>> libravatar image host resolver algorithm.
>>
>> Looks good to me, although this didn't apply either, since the code has
>> been rewritten to be asynchronous.  Could you re-spin this patch, too,
>> and I'll get it applied...
>
> Philip, have you had time to look at this?

I'll go over it to check if the images are resolved correctly.

-- 
	Philip K.

Message #20 received at 40354 <at> debbugs.gnu.org (full text, mbox):

From: "Philip K." <philipk <at> posteo.net>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 40354 <at> debbugs.gnu.org
Subject: Re: bug#40354: [PATCH] Fix Libravatar federation handling
Date: Tue, 18 Aug 2020 20:53:37 +0200

[Message part 1 (text/plain, inline)]

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Lars Ingebrigtsen <larsi <at> gnus.org> writes:
>
>> Philip K <philip <at> warpmail.net> writes:
>>
>>> Previous implementation didn't correctly handle the result of
>>> dns-query, and didn't implement the necessary record selection
>>> algorithm as specified on the wiki (https://wiki.libravatar.org/api/,
>>> "Federated servers").
>>>
>>> * lisp/image/gravatar.el (gravatar--service-libravatar): Fix
>>> libravatar image host resolver algorithm.
>>
>> Looks good to me, although this didn't apply either, since the code has
>> been rewritten to be asynchronous.  Could you re-spin this patch, too,
>> and I'll get it applied...
>
> Philip, have you had time to look at this?

The patch below should implement the fixes from bug#40354. I found a
libravatar user here[0], and could test it by evaluating

        (gravatar--service-libravatar "who <at> zerokspot.com" #'message)

or

        (gravatar--service-libravatar "who <at> gnu.org" #'message)

as an example for a domain that doesn't use libravatar. Both work as
expected, and don't lag.

[0]: https://zerokspot.com/weblog/2020/05/26/avatars-without-gravatar/

-- 
	Philip K.

[0001-Fix-Libravatar-federation-handling.patch (text/x-patch, inline)]

From 9b345b965cce8f79aa31b6a8989ff466c84b6798 Mon Sep 17 00:00:00 2001
From: Philip K <philipk <at> posteo.net>
Date: Tue, 18 Aug 2020 20:50:07 +0200
Subject: [PATCH] Fix Libravatar federation handling

Previous implementation didn't correctly handle the result of
dns-query, and didn't implement the necessary record selection
algorithm as specified on the wiki (https://wiki.libravatar.org/api/,
"Federated servers").

* lisp/image/gravatar.el (gravatar--service-libravatar): Implement
  correct algorithm
---
 lisp/image/gravatar.el | 49 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 7 deletions(-)

diff --git a/lisp/image/gravatar.el b/lisp/image/gravatar.el
index e917033562..cca737656c 100644
--- a/lisp/image/gravatar.el
+++ b/lisp/image/gravatar.el
@@ -158,18 +158,53 @@ gravatar--service-libravatar
         (setq func
               (lambda (result)
                 (cond
-                 (result
-                  (funcall callback (format "%s://%s/avatar"
-                                            (cdar records) result)))
-                 ((> (length records) 1)
-                  (pop records)
+                 ((and result           ;there is a result
+                       (let* ((data (mapcar (lambda (record)
+                                              (dns-get 'data (cdr record)))
+                                            (dns-get 'answers result)))
+                              (priorities (mapcar (lambda (r) (dns-get 'priority r))
+                                                  data))
+                              (max-priority (if priorities
+                                                (apply #'max priorities)
+                                              0))
+                              (sum 0) top)
+                         ;; Attempt to find all records with the same maximal
+                         ;; priority, and calculate the sum of their weights.
+                         (dolist (ent data)
+                           (when (= max-priority (dns-get 'priority ent))
+                             (setq sum (+ sum (dns-get 'weight ent)))
+                             (push ent top)))
+                         ;; In case there is more than one maximal priority
+                         ;; record, choose one at random, while taking the
+                         ;; individual record weights into consideration.
+                         (catch 'done
+                           (dolist (ent top)
+                             (when (and (or (= 0 sum)
+                                            (<= 0 (random sum) (dns-get 'weight ent)))
+                                        ;; Ensure that port and domain data are
+                                        ;; valid. In case non of the results
+                                        ;; were valid, `catch' will evaluate to
+                                        ;; nil, and the next cond clause will be
+                                        ;; tested.
+                                        (<= 1 (dns-get 'port ent) 65535)
+                                        (string-match-p "\\`[-.0-9A-Za-z]+\\'"
+                                                        (dns-get 'target ent)))
+                               (funcall callback
+                                        (url-normalize-url (format "%s://%s:%s/avatar"
+                                                                   (cdar records)
+                                                                   (dns-get 'target ent)
+                                                                   (dns-get 'port ent))))
+                               (throw 'done t))
+                             (setq sum (- sum (dns-get 'weight ent))))))))
+                 ((setq records (cdr records)) ;in case there are at least two methods
                   (dns-query-asynchronous
                    (concat (caar records) "._tcp." domain)
                    func 'SRV))
-                 (t
+                 (t                     ;fallback
                   (funcall callback "https://seccdn.libravatar.org/avatar")))))
         (dns-query-asynchronous
-         (concat (caar records) "._tcp." domain) func 'SRV)))))
+         (concat (caar records) "._tcp." domain)
+         func 'SRV t)))))
 
 (defun gravatar-hash (mail-address)
   "Return the Gravatar hash for MAIL-ADDRESS."
-- 
2.26.2

Message #23 received at 40354 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: "Philip K." <philipk <at> posteo.net>
Cc: 40354 <at> debbugs.gnu.org
Subject: Re: bug#40354: [PATCH] Fix Libravatar federation handling
Date: Tue, 18 Aug 2020 21:20:14 +0200

"Philip K." <philipk <at> posteo.net> writes:

> The patch below should implement the fixes from bug#40354. I found a
> libravatar user here[0], and could test it by evaluating
>
>         (gravatar--service-libravatar "who <at> zerokspot.com" #'message)
>
> or
>
>         (gravatar--service-libravatar "who <at> gnu.org" #'message)
>
> as an example for a domain that doesn't use libravatar. Both work as
> expected, and don't lag.

Thanks; I've applied the patch to Emacs 28.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.