Select correct self link when parsing Webfinger response (#31110)

.callstack.yaml

@@ -0,0 +1,31 @@
+# yaml-language-server: $schema=https://storage.googleapis.com/config-schema/schema.v2.json
+
+pr_review:
+  modules:
+    bug_hunter:
+      enabled: true
+      suggestions: true
+
+    code_suggestions:
+      enabled: false
+
+    code_conventions:
+      enabled: false
+      conventions: |
+        Exported code should be defined before internal code.
+
+    description:
+      enabled: true
+      diagram: true
+
+    grammar:
+      enabled: true
+
+    performance:
+      enabled: true
+
+    security:
+      enabled: true
+
+  exclude:
+    - "**/go.*"

.github/workflows/reviewer.yml

@@ -0,0 +1,33 @@
+name: Cal PR Review
+
+on:
+  workflow_dispatch:
+    inputs:
+      config:
+        type: string
+        description: "config for reviewer"
+        required: true
+      head:
+        type: string
+        description: "head commit sha"
+        required: true
+      base:
+        type: string
+        description: "base commit sha"
+        required: false
+jobs:
+  review:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v4
+        with:
+          ref: ${{ inputs.head }}
+
+      - name: Run reviewer
+        uses: addnab/docker-run-action@v3
+        with:
+          image: callstackai/cal:latest
+          options: -v ${{ github.workspace }}:/code
+          run: |
+            /reviewer --dir /code  --id ${{ github.run_id }} --token ${{ github.token }} --config "${{ inputs.config }}"

app/lib/webfinger.rb

@@ -6,6 +6,8 @@ class GoneError < Error; end
   class RedirectError < Error; end
 
   class Response
+    ACTIVITYPUB_READY_TYPE = ['application/activity+json', 'application/ld+json; profile="https://www.w3.org/ns/activitystreams"'].freeze
+
     attr_reader :uri
 
     def initialize(uri, body)
@@ -20,17 +22,28 @@ def subject
     end
 
     def link(rel, attribute)
-      links.dig(rel, attribute)
+      links.dig(rel, 0, attribute)
+    end
+
+    def self_link_href
+      self_link.fetch('href')
     end
 
     private
 
     def links
-      @links ||= @json.fetch('links', []).index_by { |link| link['rel'] }
+      @links ||= @json.fetch('links', []).group_by { |link| link['rel'] }
+    end
+
+    def self_link
+      links.fetch('self', []).find do |link|
+        ACTIVITYPUB_READY_TYPE.include?(link['type'])
+      end
     end
 
     def validate_response!
       raise Webfinger::Error, "Missing subject in response for #{@uri}" if subject.blank?
+      raise Webfinger::Error, "Missing self link in response for #{@uri}" if self_link.blank?
     end
   end
 

app/services/activitypub/fetch_remote_actor_service.rb

@@ -49,7 +49,7 @@ def check_webfinger!
     confirmed_username, confirmed_domain = split_acct(webfinger.subject)
 
     if @username.casecmp(confirmed_username).zero? && @domain.casecmp(confirmed_domain).zero?
-      raise Error, "Webfinger response for #{@username}@#{@domain} does not loop back to #{@uri}" if webfinger.link('self', 'href') != @uri
+      raise Error, "Webfinger response for #{@username}@#{@domain} does not loop back to #{@uri}" if webfinger.self_link_href != @uri
 
       return
     end
@@ -58,8 +58,7 @@ def check_webfinger!
     @username, @domain                   = split_acct(webfinger.subject)
 
     raise Webfinger::RedirectError, "Too many webfinger redirects for URI #{@uri} (stopped at #{@username}@#{@domain})" unless confirmed_username.casecmp(@username).zero? && confirmed_domain.casecmp(@domain).zero?
-
-    raise Error, "Webfinger response for #{@username}@#{@domain} does not loop back to #{@uri}" if webfinger.link('self', 'href') != @uri
+    raise Error, "Webfinger response for #{@username}@#{@domain} does not loop back to #{@uri}" if webfinger.self_link_href != @uri
   rescue Webfinger::RedirectError => e
     raise Error, e.message
   rescue Webfinger::Error => e

app/services/resolve_account_service.rb

@@ -106,8 +106,6 @@ def split_acct(acct)
   end
 
   def fetch_account!
-    return unless activitypub_ready?
-
     with_redis_lock("resolve:#{@username}@#{@domain}") do
       @account = ActivityPub::FetchRemoteAccountService.new.call(actor_url, suppress_errors: @options[:suppress_errors])
     end
@@ -122,12 +120,8 @@ def webfinger_update_due?
     @options[:skip_cache] || @account.nil? || @account.possibly_stale?
   end
 
-  def activitypub_ready?
-    ['application/activity+json', 'application/ld+json; profile="https://www.w3.org/ns/activitystreams"'].include?(@webfinger.link('self', 'type'))
-  end
-
   def actor_url
-    @actor_url ||= @webfinger.link('self', 'href')
+    @actor_url ||= @webfinger.self_link_href
   end
 
   def gone_from_origin?

spec/fixtures/requests/activitypub-webfinger.txt

@@ -4,4 +4,4 @@ Content-Type: application/jrd+json; charset=utf-8
 X-Content-Type-Options: nosniff
 Date: Sun, 17 Sep 2017 06:22:50 GMT
 
-{"subject":"acct:foo@ap.example.com","aliases":["https://ap.example.com/@foo","https://ap.example.com/users/foo"],"links":[{"rel":"http://webfinger.net/rel/profile-page","type":"text/html","href":"https://ap.example.com/@foo"},{"rel":"http://schemas.google.com/g/2010#updates-from","type":"application/atom+xml","href":"https://ap.example.com/users/foo.atom"},{"rel":"self","type":"application/activity+json","href":"https://ap.example.com/users/foo"},{"rel":"salmon","href":"https://ap.example.com/api/salmon/1"},{"rel":"magic-public-key","href":"data:application/magic-public-key,RSA.u3L4vnpNLzVH31MeWI394F0wKeJFsLDAsNXGeOu0QF2x-h1zLWZw_agqD2R3JPU9_kaDJGPIV2Sn5zLyUA9S6swCCMOtn7BBR9g9sucgXJmUFB0tACH2QSgHywMAybGfmSb3LsEMNKsGJ9VsvYoh8lDET6X4Pyw-ZJU0_OLo_41q9w-OrGtlsTm_PuPIeXnxa6BLqnDaxC-4IcjG_FiPahNCTINl_1F_TgSSDZ4Taf4U9XFEIFw8wmgploELozzIzKq-t8nhQYkgAkt64euWpva3qL5KD1mTIZQEP-LZvh3s2WHrLi3fhbdRuwQ2c0KkJA2oSTFPDpqqbPGZ3QvuHQ==.AQAB"},{"rel":"http://ostatus.org/schema/1.0/subscribe","template":"https://ap.example.com/authorize_follow?acct={uri}"}]}
+{"subject":"acct:foo@ap.example.com","aliases":["https://ap.example.com/@foo","https://ap.example.com/users/foo"],"links":[{"rel":"http://webfinger.net/rel/profile-page","type":"text/html","href":"https://ap.example.com/@foo"},{"rel":"http://schemas.google.com/g/2010#updates-from","type":"application/atom+xml","href":"https://ap.example.com/users/foo.atom"},{"rel":"self","type":"application/html","href":"https://ap.example.com/users/foo.html"},{"rel":"self","type":"application/activity+json","href":"https://ap.example.com/users/foo"},{"rel":"self","type":"application/json","href":"https://ap.example.com/users/foo.json"},{"rel":"salmon","href":"https://ap.example.com/api/salmon/1"},{"rel":"magic-public-key","href":"data:application/magic-public-key,RSA.u3L4vnpNLzVH31MeWI394F0wKeJFsLDAsNXGeOu0QF2x-h1zLWZw_agqD2R3JPU9_kaDJGPIV2Sn5zLyUA9S6swCCMOtn7BBR9g9sucgXJmUFB0tACH2QSgHywMAybGfmSb3LsEMNKsGJ9VsvYoh8lDET6X4Pyw-ZJU0_OLo_41q9w-OrGtlsTm_PuPIeXnxa6BLqnDaxC-4IcjG_FiPahNCTINl_1F_TgSSDZ4Taf4U9XFEIFw8wmgploELozzIzKq-t8nhQYkgAkt64euWpva3qL5KD1mTIZQEP-LZvh3s2WHrLi3fhbdRuwQ2c0KkJA2oSTFPDpqqbPGZ3QvuHQ==.AQAB"},{"rel":"http://ostatus.org/schema/1.0/subscribe","template":"https://ap.example.com/authorize_follow?acct={uri}"}]}

spec/fixtures/requests/webfinger.txt

@@ -8,4 +8,4 @@ Access-Control-Allow-Origin: *
 Vary: Accept-Encoding,Cookie
 Strict-Transport-Security: max-age=31536000; includeSubdomains;
 
-{"subject":"acct:gargron@quitter.no","aliases":["https:\/\/quitter.no\/user\/7477","https:\/\/quitter.no\/gargron","https:\/\/quitter.no\/index.php\/user\/7477","https:\/\/quitter.no\/index.php\/gargron"],"links":[{"rel":"http:\/\/webfinger.net\/rel\/profile-page","type":"text\/html","href":"https:\/\/quitter.no\/gargron"},{"rel":"http:\/\/gmpg.org\/xfn\/11","type":"text\/html","href":"https:\/\/quitter.no\/gargron"},{"rel":"describedby","type":"application\/rdf+xml","href":"https:\/\/quitter.no\/gargron\/foaf"},{"rel":"http:\/\/apinamespace.org\/atom","type":"application\/atomsvc+xml","href":"https:\/\/quitter.no\/api\/statusnet\/app\/service\/gargron.xml"},{"rel":"http:\/\/apinamespace.org\/twitter","href":"https:\/\/quitter.no\/api\/"},{"rel":"http:\/\/specs.openid.net\/auth\/2.0\/provider","href":"https:\/\/quitter.no\/gargron"},{"rel":"http:\/\/schemas.google.com\/g\/2010#updates-from","type":"application\/atom+xml","href":"https:\/\/quitter.no\/api\/statuses\/user_timeline\/7477.atom"},{"rel":"magic-public-key","href":"data:application\/magic-public-key,RSA.1ZBkHTavLvxH3FzlKv4O6WtlILKRFfNami3_Rcu8EuogtXSYiS-bB6hElZfUCSHbC4uLemOA34PEhz__CDMozax1iI_t8dzjDnh1x0iFSup7pSfW9iXk_WU3Dm74yWWW2jildY41vWgrEstuQ1dJ8vVFfSJ9T_tO4c-T9y8vDI8=.AQAB"},{"rel":"salmon","href":"https:\/\/quitter.no\/main\/salmon\/user\/7477"},{"rel":"http:\/\/salmon-protocol.org\/ns\/salmon-replies","href":"https:\/\/quitter.no\/main\/salmon\/user\/7477"},{"rel":"http:\/\/salmon-protocol.org\/ns\/salmon-mention","href":"https:\/\/quitter.no\/main\/salmon\/user\/7477"},{"rel":"http:\/\/ostatus.org\/schema\/1.0\/subscribe","template":"https:\/\/quitter.no\/main\/ostatussub?profile={uri}"}]}
+{"subject":"acct:gargron@quitter.no","aliases":["https:\/\/quitter.no\/user\/7477","https:\/\/quitter.no\/gargron","https:\/\/quitter.no\/index.php\/user\/7477","https:\/\/quitter.no\/index.php\/gargron"],"links":[{"rel":"http:\/\/webfinger.net\/rel\/profile-page","type":"text\/html","href":"https:\/\/quitter.no\/gargron"},{"rel":"http:\/\/gmpg.org\/xfn\/11","type":"text\/html","href":"https:\/\/quitter.no\/gargron"},{"rel":"describedby","type":"application\/rdf+xml","href":"https:\/\/quitter.no\/gargron\/foaf"},{"rel":"self","type":"application/activity+json","href":"https://ap.example.com/users/foo"},{"rel":"http:\/\/apinamespace.org\/atom","type":"application\/atomsvc+xml","href":"https:\/\/quitter.no\/api\/statusnet\/app\/service\/gargron.xml"},{"rel":"http:\/\/apinamespace.org\/twitter","href":"https:\/\/quitter.no\/api\/"},{"rel":"http:\/\/specs.openid.net\/auth\/2.0\/provider","href":"https:\/\/quitter.no\/gargron"},{"rel":"http:\/\/schemas.google.com\/g\/2010#updates-from","type":"application\/atom+xml","href":"https:\/\/quitter.no\/api\/statuses\/user_timeline\/7477.atom"},{"rel":"magic-public-key","href":"data:application\/magic-public-key,RSA.1ZBkHTavLvxH3FzlKv4O6WtlILKRFfNami3_Rcu8EuogtXSYiS-bB6hElZfUCSHbC4uLemOA34PEhz__CDMozax1iI_t8dzjDnh1x0iFSup7pSfW9iXk_WU3Dm74yWWW2jildY41vWgrEstuQ1dJ8vVFfSJ9T_tO4c-T9y8vDI8=.AQAB"},{"rel":"salmon","href":"https:\/\/quitter.no\/main\/salmon\/user\/7477"},{"rel":"http:\/\/salmon-protocol.org\/ns\/salmon-replies","href":"https:\/\/quitter.no\/main\/salmon\/user\/7477"},{"rel":"http:\/\/salmon-protocol.org\/ns\/salmon-mention","href":"https:\/\/quitter.no\/main\/salmon\/user\/7477"},{"rel":"http:\/\/ostatus.org\/schema\/1.0\/subscribe","template":"https:\/\/quitter.no\/main\/ostatussub?profile={uri}"}]}

spec/lib/webfinger_spec.rb

@@ -0,0 +1,41 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe Webfinger do
+  describe 'self link' do
+    context 'when self link is specified with type application/activity+json' do
+      let!(:webfinger) { { subject: 'acct:alice@example.com', links: [{ rel: 'self', href: 'https://example.com/alice', type: 'application/activity+json' }] } }
+
+      it 'correctly parses the response' do
+        stub_request(:get, 'https://example.com/.well-known/webfinger?resource=acct:alice@example.com').to_return(body: Oj.dump(webfinger), headers: { 'Content-Type': 'application/jrd+json' })
+
+        response = described_class.new('acct:alice@example.com').perform
+
+        expect(response.self_link_href).to eq 'https://example.com/alice'
+      end
+    end
+
+    context 'when self link is specified with type application/ld+json' do
+      let!(:webfinger) { { subject: 'acct:alice@example.com', links: [{ rel: 'self', href: 'https://example.com/alice', type: 'application/ld+json; profile="https://www.w3.org/ns/activitystreams"' }] } }
+
+      it 'correctly parses the response' do
+        stub_request(:get, 'https://example.com/.well-known/webfinger?resource=acct:alice@example.com').to_return(body: Oj.dump(webfinger), headers: { 'Content-Type': 'application/jrd+json' })
+
+        response = described_class.new('acct:alice@example.com').perform
+
+        expect(response.self_link_href).to eq 'https://example.com/alice'
+      end
+    end
+
+    context 'when self link is specified with incorrect type' do
+      let!(:webfinger) { { subject: 'acct:alice@example.com', links: [{ rel: 'self', href: 'https://example.com/alice', type: 'application/json"' }] } }
+
+      it 'raises an error' do
+        stub_request(:get, 'https://example.com/.well-known/webfinger?resource=acct:alice@example.com').to_return(body: Oj.dump(webfinger), headers: { 'Content-Type': 'application/jrd+json' })
+
+        expect { described_class.new('acct:alice@example.com').perform }.to raise_error(Webfinger::Error)
+      end
+    end
+  end
+end

spec/services/activitypub/fetch_remote_account_service_spec.rb

@@ -33,7 +33,7 @@
     end
 
     context 'when the account does not have a inbox' do
-      let!(:webfinger) { { subject: 'acct:alice@example.com', links: [{ rel: 'self', href: 'https://example.com/alice' }] } }
+      let!(:webfinger) { { subject: 'acct:alice@example.com', links: [{ rel: 'self', href: 'https://example.com/alice', type: 'application/activity+json' }] } }
 
       before do
         actor[:inbox] = nil
@@ -51,7 +51,7 @@
     end
 
     context 'when URI and WebFinger share the same host' do
-      let!(:webfinger) { { subject: 'acct:alice@example.com', links: [{ rel: 'self', href: 'https://example.com/alice' }] } }
+      let!(:webfinger) { { subject: 'acct:alice@example.com', links: [{ rel: 'self', href: 'https://example.com/alice', type: 'application/activity+json' }] } }
 
       before do
         stub_request(:get, 'https://example.com/alice').to_return(body: Oj.dump(actor), headers: { 'Content-Type': 'application/activity+json' })
@@ -72,7 +72,7 @@
     end
 
     context 'when WebFinger presents different domain than URI' do
-      let!(:webfinger) { { subject: 'acct:alice@iscool.af', links: [{ rel: 'self', href: 'https://example.com/alice' }] } }
+      let!(:webfinger) { { subject: 'acct:alice@iscool.af', links: [{ rel: 'self', href: 'https://example.com/alice', type: 'application/activity+json' }] } }
 
       before do
         stub_request(:get, 'https://example.com/alice').to_return(body: Oj.dump(actor), headers: { 'Content-Type': 'application/activity+json' })
@@ -95,7 +95,7 @@
     end
 
     context 'when WebFinger returns a different URI' do
-      let!(:webfinger) { { subject: 'acct:alice@example.com', links: [{ rel: 'self', href: 'https://example.com/bob' }] } }
+      let!(:webfinger) { { subject: 'acct:alice@example.com', links: [{ rel: 'self', href: 'https://example.com/bob', type: 'application/activity+json' }] } }
 
       before do
         stub_request(:get, 'https://example.com/alice').to_return(body: Oj.dump(actor), headers: { 'Content-Type': 'application/activity+json' })
@@ -111,7 +111,7 @@
     end
 
     context 'when WebFinger returns a different URI after a redirection' do
-      let!(:webfinger) { { subject: 'acct:alice@iscool.af', links: [{ rel: 'self', href: 'https://example.com/bob' }] } }
+      let!(:webfinger) { { subject: 'acct:alice@iscool.af', links: [{ rel: 'self', href: 'https://example.com/bob', type: 'application/activity+json' }] } }
 
       before do
         stub_request(:get, 'https://example.com/alice').to_return(body: Oj.dump(actor), headers: { 'Content-Type': 'application/activity+json' })

spec/services/activitypub/fetch_remote_actor_service_spec.rb

@@ -33,7 +33,7 @@
     end
 
     context 'when the account does not have a inbox' do
-      let!(:webfinger) { { subject: 'acct:alice@example.com', links: [{ rel: 'self', href: 'https://example.com/alice' }] } }
+      let!(:webfinger) { { subject: 'acct:alice@example.com', links: [{ rel: 'self', href: 'https://example.com/alice', type: 'application/activity+json' }] } }
 
       before do
         actor[:inbox] = nil
@@ -51,7 +51,7 @@
     end
 
     context 'when URI and WebFinger share the same host' do
-      let!(:webfinger) { { subject: 'acct:alice@example.com', links: [{ rel: 'self', href: 'https://example.com/alice' }] } }
+      let!(:webfinger) { { subject: 'acct:alice@example.com', links: [{ rel: 'self', href: 'https://example.com/alice', type: 'application/activity+json' }] } }
 
       before do
         stub_request(:get, 'https://example.com/alice').to_return(body: Oj.dump(actor), headers: { 'Content-Type': 'application/activity+json' })
@@ -72,7 +72,7 @@
     end
 
     context 'when WebFinger presents different domain than URI' do
-      let!(:webfinger) { { subject: 'acct:alice@iscool.af', links: [{ rel: 'self', href: 'https://example.com/alice' }] } }
+      let!(:webfinger) { { subject: 'acct:alice@iscool.af', links: [{ rel: 'self', href: 'https://example.com/alice', type: 'application/activity+json' }] } }
 
       before do
         stub_request(:get, 'https://example.com/alice').to_return(body: Oj.dump(actor), headers: { 'Content-Type': 'application/activity+json' })
@@ -95,7 +95,7 @@
     end
 
     context 'when WebFinger returns a different URI' do
-      let!(:webfinger) { { subject: 'acct:alice@example.com', links: [{ rel: 'self', href: 'https://example.com/bob' }] } }
+      let!(:webfinger) { { subject: 'acct:alice@example.com', links: [{ rel: 'self', href: 'https://example.com/bob', type: 'application/activity+json' }] } }
 
       before do
         stub_request(:get, 'https://example.com/alice').to_return(body: Oj.dump(actor), headers: { 'Content-Type': 'application/activity+json' })
@@ -111,7 +111,7 @@
     end
 
     context 'when WebFinger returns a different URI after a redirection' do
-      let!(:webfinger) { { subject: 'acct:alice@iscool.af', links: [{ rel: 'self', href: 'https://example.com/bob' }] } }
+      let!(:webfinger) { { subject: 'acct:alice@iscool.af', links: [{ rel: 'self', href: 'https://example.com/bob', type: 'application/activity+json' }] } }
 
       before do
         stub_request(:get, 'https://example.com/alice').to_return(body: Oj.dump(actor), headers: { 'Content-Type': 'application/activity+json' })

spec/services/activitypub/fetch_remote_key_service_spec.rb

@@ -5,7 +5,7 @@
 RSpec.describe ActivityPub::FetchRemoteKeyService do
   subject { described_class.new }
 
-  let(:webfinger) { { subject: 'acct:alice@example.com', links: [{ rel: 'self', href: 'https://example.com/alice' }] } }
+  let(:webfinger) { { subject: 'acct:alice@example.com', links: [{ rel: 'self', href: 'https://example.com/alice', type: 'application/activity+json' }] } }
 
   let(:public_key_pem) do
     <<~TEXT

spec/services/activitypub/process_account_service_spec.rb

@@ -215,7 +215,7 @@
         }.with_indifferent_access
         webfinger = {
           subject: "acct:user#{i}@foo.test",
-          links: [{ rel: 'self', href: "https://foo.test/users/#{i}" }],
+          links: [{ rel: 'self', href: "https://foo.test/users/#{i}", type: 'application/activity+json' }],
         }.with_indifferent_access
         stub_request(:get, "https://foo.test/users/#{i}").to_return(status: 200, body: actor_json.to_json, headers: { 'Content-Type': 'application/activity+json' })
         stub_request(:get, "https://foo.test/users/#{i}/featured").to_return(status: 200, body: featured_json.to_json, headers: { 'Content-Type': 'application/activity+json' })