fix: refuse an avatar_url that points at the runner's own network (#20)

What comes back from an avatar fetch is base64-embedded into a file the run then
commits, so where it came from is a policy question rather than a transport
detail. `safe_redirect` already treated it as one — but it only ever saw the
second request onward. The first URL, the one written in the config, was fetched
with nothing looking at it: a loopback address, a private range, a cloud metadata
endpoint. Whatever answered was labelled `image/png` regardless of its real
content type and embedded. On a workflow that builds a mural from a pull request,
that address is the contributor's to choose.

Verified end to end before the fix: a config naming a local server had that
server's JSON response base64-embedded in the committed SVG, exit code 0. After,
the fetch is refused, nothing is written, and the run fails with the reason.

Two holes in the existing redirect rule went with it:

- A host was judged by how it was spelled rather than by where it points, so a
  name aimed at an internal address walked past — as did the decimal form of one,
  since `2130706433` is not a literal to `Socket::IPAddress` but is 127.0.0.1 to
  the resolver. Names are resolved now, and every address they answer with is
  judged. Answers are cached per host, so a wall of a few thousand faces asks the
  resolver once per host rather than once per face.
- `URI#host` keeps the brackets around an IPv6 literal, so `[::1]` was read as an
  unrecognised hostname and allowed. `#hostname` is the one that strips them.

This raises the bar rather than sealing it, and the comment says so: the
connection resolves the name again for itself, so a name that answers differently
each time can still get through. Closing that means connecting to an
already-vetted address and carrying the `Host` header by hand, which is a
different piece of work.

The reason a face is missing now reaches the log. Every failure — a 404, a
timeout, a refused address — was reported as "avatar could not be fetched", which
does not tell anyone whether to fix a typo, a permission or a URL. The fetcher
had already worked it out; `Embedder` was dropping it on the floor. This is what
makes the new rule diagnosable rather than mysterious for anyone whose avatar
host really is internal.

Note for review: that last case is a behaviour change for a self-hosted runner
pulling avatars from an internal host — GitHub Enterprise Server, say. It fails
now, with a message naming the address. If that is worth supporting, an opt-out
input is the shape, and I would rather it be your call than my guess.

Author: hahwul

CHANGELOG.md

@@ -26,6 +26,21 @@ and the generated files.
   mural. The specs now parse and validate every config the README shows, render
   every committed example config, and check that a run emits exactly the
   outputs `action.yml` declares.
+- An `avatar_url` pointing at the runner's own network is refused instead of
+  fetched. Redirects were already held to that rule, but the first request was
+  never checked, so an address written straight into the config — a cloud
+  metadata endpoint, a service on the host — was fetched, labelled `image/png`
+  whatever came back, and base64-embedded into a file the run then commits. On a
+  workflow that builds a mural from a pull request, that address is the
+  contributor's to choose. The check now also judges a host by the addresses it
+  resolves to rather than by how it is spelled, which is what a name pointed at
+  an internal address, or the decimal form of one, used to walk past — and
+  `[::1]` no longer slips through the redirect rule either, where the brackets
+  were being read as part of a hostname.
+- A skipped avatar says why. Every failure — a 404, a timeout, a refused address
+  — was reported as "avatar could not be fetched", which does not tell anyone
+  whether to fix a typo, a permission, or a URL. The fetcher already worked out
+  the reason; now it survives as far as the log.
 - A control character in a name, role or section title no longer costs the whole
   file. XML carries three of them and no parser will read a document holding any
   of the others, but `HTML.escape` leaves them alone because in HTML they are

README.md

@@ -799,7 +799,11 @@ bin/contributor-mural -c my.yml --commit               # opt in to commit/push l
 
 `--config` is resolved against the current directory, while output paths and local
 `avatar_url` files are resolved against `--workspace` (the current directory by default;
-`GITHUB_WORKSPACE` inside the action). Committing happens automatically when
+`GITHUB_WORKSPACE` inside the action). A local `avatar_url` has to stay inside the
+workspace, and a remote one has to be a public address: whatever comes back is embedded
+in a file the run commits, so an `avatar_url` pointing at the runner's own network —
+loopback, a private range, or a metadata endpoint — is refused rather than published.
+Committing happens automatically when
 `GITHUB_ACTIONS=true` — including on runners that emulate it, such as act or Forgejo —
 and otherwise only with `--commit`.
 

spec/embedder_spec.cr

@@ -74,7 +74,11 @@ describe ContributorMural::Embedder do
     embedded, skipped = embedder.embed(users, grid_renderer, fail_on_missing: false)
 
     embedded.map(&.login).should eq(["alpha"])
-    skipped.should eq(["bravo"])
+    skipped.map(&.login).should eq(["bravo"])
+    # The reason travels with the login. Reported as "could not be fetched" on
+    # its own, a refused address, a 404 and a timeout all read the same, and none
+    # of them tells the reader which one they are looking at.
+    skipped.first.reason.should contain("404")
   end
 
   it "raises on failure when fail_on_missing is set" do

spec/github/avatar_fetcher_spec.cr

@@ -31,6 +31,11 @@ private def with_test_server(&)
     when "/redirect-internal"
       context.response.status_code = 302
       context.response.headers["Location"] = "https://127.0.0.1/secret.png"
+    when "/redirect-internal-v6"
+      # The bracketed form: `URI#host` keeps the brackets, so this used to be
+      # read as a hostname nobody recognised and allowed straight through.
+      context.response.status_code = 302
+      context.response.headers["Location"] = "https://[::1]/secret.png"
     when "/missing"
       context.response.status_code = 404
     when "/flaky"
@@ -165,7 +170,7 @@ describe ContributorMural::HTTPAvatarSource do
   describe "#fetch" do
     it "returns bytes and content type" do
       with_test_server do |base, _requests|
-        source = ContributorMural::HTTPAvatarSource.new(backoff_base: 0.seconds)
+        source = ContributorMural::HTTPAvatarSource.new(backoff_base: 0.seconds, allow_local_targets: true)
         bytes, content_type = source.fetch(user_with("#{base}/ok.png"), 64)
         String.new(bytes).should eq("JPEGDATA")
         content_type.should eq("image/jpeg")
@@ -194,7 +199,7 @@ describe ContributorMural::HTTPAvatarSource do
 
     it "does not retry a 404" do
       with_test_server do |base, requests|
-        source = ContributorMural::HTTPAvatarSource.new(backoff_base: 0.seconds)
+        source = ContributorMural::HTTPAvatarSource.new(backoff_base: 0.seconds, allow_local_targets: true)
         error = expect_raises(ContributorMural::AvatarError, /404/) do
           source.fetch(user_with("#{base}/missing"), 64)
         end
@@ -205,7 +210,7 @@ describe ContributorMural::HTTPAvatarSource do
 
     it "falls back to image/png for malformed or non-image content types" do
       with_test_server do |base, _requests|
-        source = ContributorMural::HTTPAvatarSource.new(backoff_base: 0.seconds)
+        source = ContributorMural::HTTPAvatarSource.new(backoff_base: 0.seconds, allow_local_targets: true)
         # `@@@` makes MIME parsing raise; `text/html` is simply not an image.
         _, content_type = source.fetch(user_with("#{base}/bad-mime"), 64)
         content_type.should eq("image/png")
@@ -216,7 +221,7 @@ describe ContributorMural::HTTPAvatarSource do
 
     it "refuses redirects that leave https or target internal addresses" do
       with_test_server do |base, _requests|
-        source = ContributorMural::HTTPAvatarSource.new(backoff_base: 0.seconds)
+        source = ContributorMural::HTTPAvatarSource.new(backoff_base: 0.seconds, allow_local_targets: true)
         expect_raises(ContributorMural::AvatarError, /non-https avatar redirect/) do
           source.fetch(user_with("#{base}/redirect"), 64)
         end
@@ -228,7 +233,7 @@ describe ContributorMural::HTTPAvatarSource do
 
     it "retries server errors and succeeds" do
       with_test_server do |base, requests|
-        source = ContributorMural::HTTPAvatarSource.new(backoff_base: 0.seconds)
+        source = ContributorMural::HTTPAvatarSource.new(backoff_base: 0.seconds, allow_local_targets: true)
         bytes, _ = source.fetch(user_with("#{base}/flaky"), 64)
         String.new(bytes).should eq("RECOVERED")
         requests.count("/flaky").should eq(3)
@@ -240,7 +245,7 @@ describe ContributorMural::HTTPAvatarSource do
       # the status is under 500, so the fetch failed for good and that person
       # quietly went missing from the mural.
       with_scripted_server([429, 429, 200], retry_after: "0") do |url, attempts|
-        source = ContributorMural::HTTPAvatarSource.new(backoff_base: 0.seconds)
+        source = ContributorMural::HTTPAvatarSource.new(backoff_base: 0.seconds, allow_local_targets: true)
         bytes, _ = source.fetch(user_with(url), 64)
         String.new(bytes).should eq("SCRIPTED")
         attempts.call.should eq(3)
@@ -249,7 +254,7 @@ describe ContributorMural::HTTPAvatarSource do
 
     it "gives up on sustained throttling with the status intact" do
       with_scripted_server([429], retry_after: "0") do |url, attempts|
-        source = ContributorMural::HTTPAvatarSource.new(backoff_base: 0.seconds)
+        source = ContributorMural::HTTPAvatarSource.new(backoff_base: 0.seconds, allow_local_targets: true)
         error = expect_raises(ContributorMural::AvatarError, /429/) do
           source.fetch(user_with(url), 64)
         end
@@ -260,14 +265,82 @@ describe ContributorMural::HTTPAvatarSource do
 
     it "does not retry a status that will not heal" do
       with_scripted_server([418]) do |url, attempts|
-        source = ContributorMural::HTTPAvatarSource.new(backoff_base: 0.seconds)
+        source = ContributorMural::HTTPAvatarSource.new(backoff_base: 0.seconds, allow_local_targets: true)
         expect_raises(ContributorMural::AvatarError, /418/) do
           source.fetch(user_with(url), 64)
         end
         attempts.call.should eq(1)
       end
     end
   end
+
+  # What comes back from an avatar fetch is base64-embedded into a file the run
+  # commits, so the address it came from is a policy question. Redirects were
+  # already held to it; the first request was not, and on a workflow that builds
+  # a mural from a pull request that address is the contributor's to choose.
+  describe "where it will fetch from" do
+    it "refuses an internal address given as the avatar_url itself" do
+      source = ContributorMural::HTTPAvatarSource.new(backoff_base: 0.seconds)
+      {"127.0.0.1", "169.254.169.254", "10.0.0.1", "192.168.1.1", "[::1]", "0.0.0.0"}.each do |host|
+        error = expect_raises(ContributorMural::AvatarError, /runner's own network/) do
+          source.fetch(user_with("http://#{host}/secret.png"), 64)
+        end
+        # 400 rather than a retryable status: this will read the same way in a
+        # second, and the message is the whole answer.
+        error.status.should eq(400)
+      end
+    end
+
+    it "refuses localhost by name" do
+      source = ContributorMural::HTTPAvatarSource.new(backoff_base: 0.seconds)
+      expect_raises(ContributorMural::AvatarError, /runner's own network/) do
+        source.fetch(user_with("https://localhost/secret.png"), 64)
+      end
+    end
+
+    # Spelling is not the question — where the name points is. A literal-only
+    # check let the decimal form of a loopback address straight through.
+    it "refuses a name that resolves to an internal address" do
+      decimal = begin
+        Socket::Addrinfo.resolve("2130706433", 80, type: Socket::Type::STREAM)
+          .any?(&.ip_address.loopback?)
+      rescue Socket::Error
+        false
+      end
+
+      if decimal
+        source = ContributorMural::HTTPAvatarSource.new(backoff_base: 0.seconds)
+        expect_raises(ContributorMural::AvatarError, /runner's own network/) do
+          source.fetch(user_with("http://2130706433/secret.png"), 64)
+        end
+      else
+        # Some resolvers (musl) do not accept the decimal form at all, which
+        # closes this door by itself.
+        decimal.should be_false
+      end
+    end
+
+    it "still refuses a redirect that lands on an internal address" do
+      with_test_server do |base, _requests|
+        source = ContributorMural::HTTPAvatarSource.new(backoff_base: 0.seconds, allow_local_targets: true)
+        {"/redirect-internal", "/redirect-internal-v6"}.each do |path|
+          expect_raises(ContributorMural::AvatarError, /internal address/) do
+            source.fetch(user_with("#{base}#{path}"), 64)
+          end
+        end
+      end
+    end
+
+    it "leaves a public host alone" do
+      # Resolves, is not internal, and the fetch fails on the request rather
+      # than on the policy — which is what a real avatar host has to do.
+      source = ContributorMural::HTTPAvatarSource.new(backoff_base: 0.seconds)
+      error = expect_raises(ContributorMural::AvatarError) do
+        source.fetch(user_with("https://example.invalid/avatar.png"), 64)
+      end
+      error.message.to_s.should_not match(/runner's own network/)
+    end
+  end
 end
 
 private def response_with(retry_after : String?) : HTTP::Client::Response

src/contributor_mural/embedder.cr

@@ -27,23 +27,29 @@ module ContributorMural
       fetch_missing(jobs.uniq!(&.url))
     end
 
-    # Returns users with embedded avatars (input order preserved) and the
-    # logins skipped due to fetch failures. Raises the first failure instead
-    # when `fail_on_missing` is set.
+    # One person left out of the mural, and why. The reason travels with the
+    # login because the caller is what reports it, and "could not be fetched"
+    # on its own does not tell anyone whether to fix a typo, a permission, or
+    # an address — the fetcher already worked that out.
+    record Skipped, login : String, reason : String
+
+    # Returns users with embedded avatars (input order preserved) and the people
+    # skipped due to fetch failures. Raises the first failure instead when
+    # `fail_on_missing` is set.
     def embed(users : Array(ResolvedUser), renderer : Renderer,
-              fail_on_missing : Bool) : {Array(EmbeddedUser), Array(String)}
+              fail_on_missing : Bool) : {Array(EmbeddedUser), Array(Skipped)}
       jobs = jobs_for(users, renderer)
       fetch_missing(jobs.uniq(&.url))
 
       embedded = [] of EmbeddedUser
-      skipped = [] of String
+      skipped = [] of Skipped
       jobs.each do |job|
         case result = @cache[job.url]
         in String
           embedded << EmbeddedUser.new(job.user, result)
         in AvatarError
           raise AvatarError.new("#{job.user.login}: #{result.message}", result.status) if fail_on_missing
-          skipped << job.user.login
+          skipped << Skipped.new(job.user.login, result.message || "avatar could not be fetched")
         end
       end
       {embedded, skipped}

src/contributor_mural/github/avatar_fetcher.cr

@@ -59,11 +59,19 @@ module ContributorMural
 
     ALLOWED_CONTENT_TYPES = CONTENT_TYPES.values.to_set
 
-    # `allow_local_redirects` exists for specs, which redirect between
-    # 127.0.0.1 ports; production keeps redirects on public https only.
+    # Both flags exist for specs, which fetch from and redirect between
+    # 127.0.0.1 ports; production keeps redirects on public https only and
+    # refuses to fetch from the runner's own network at all.
+    # `allow_local_redirects` implies `allow_local_targets`, since a spec that
+    # follows a local redirect had to reach the local server first.
     def initialize(@workspace : String = Dir.current, @backoff_base : Time::Span = 1.second,
-                   @allow_local_redirects : Bool = false, pool : HTTPPool? = nil)
+                   @allow_local_redirects : Bool = false, pool : HTTPPool? = nil,
+                   allow_local_targets : Bool? = nil)
+      @allow_local_targets = allow_local_targets.nil? ? @allow_local_redirects : allow_local_targets
       @pool = pool || HTTPPool.new
+      # Answered once per host: a mural of a few thousand faces asks about the
+      # same two or three, and the answer involves the resolver.
+      @internal_hosts = {} of String => Bool
       # Only the UA: an `Accept` narrower than the wild card would let an
       # avatar host we have never heard of answer 406 where it used to work.
       @headers = HTTP::Headers{"User-Agent" => USER_AGENT}
@@ -148,6 +156,7 @@ module ContributorMural
     end
 
     private def get_following_redirects(url : String) : {Bytes, String}
+      refuse_internal_target(url)
       MAX_REDIRECTS.times do
         response = @pool.get(url, @headers)
         case response.status_code
@@ -188,7 +197,7 @@ module ContributorMural
       unless target.scheme == "https"
         raise AvatarError.new("refusing non-https avatar redirect to #{target}", 400)
       end
-      host = target.host
+      host = target.hostname
       raise AvatarError.new("avatar redirect without a host: #{target}", 400) unless host
       if internal_host?(host)
         raise AvatarError.new("refusing avatar redirect to internal address #{host}", 400)
@@ -198,10 +207,62 @@ module ContributorMural
       raise AvatarError.new("invalid avatar redirect target: #{ex.message}", 400)
     end
 
+    # The first URL is the one nobody was vetting. `safe_redirect` only ever saw
+    # the second request onward, so an `avatar_url` written straight at the
+    # runner's own network — a cloud metadata endpoint, a service on the host —
+    # was fetched, labelled `image/png` whatever came back, and base64-embedded
+    # into a file the run then commits. On a workflow that builds a mural from a
+    # pull request, the address is the contributor's to choose.
+    private def refuse_internal_target(url : String) : Nil
+      return if @allow_local_targets
+      # `hostname`, not `host`: the latter keeps the brackets around an IPv6
+      # literal, and `[::1]` is not an address any of this recognises.
+      host = URI.parse(url).hostname
+      # No host means nothing to reach: a workspace-relative avatar never gets
+      # here, it is read off disk.
+      return unless host
+      return unless internal_host?(host)
+      raise AvatarError.new("refusing to fetch an avatar from #{host} — " \
+                            "it is an address on the runner's own network", 400)
+    rescue ex : URI::Error
+      raise AvatarError.new("invalid avatar URL #{url.inspect}: #{ex.message}", 400)
+    end
+
     private def internal_host?(host : String) : Bool
+      cached = @internal_hosts[host]?
+      return cached unless cached.nil?
+      verdict = resolves_internal?(host)
+      @internal_hosts[host] = verdict
+      verdict
+    end
+
+    # Judged on the addresses the name actually answers with, not on how it is
+    # spelled. Testing the literal alone let a hostname pointed at an internal
+    # address walk past — as did the decimal form of one, since `2130706433` is
+    # not a literal to `Socket::IPAddress` but is 127.0.0.1 to the resolver.
+    #
+    # This raises the bar rather than sealing it: the connection resolves the
+    # name again for itself, so a name that answers differently each time can
+    # still get through. Closing that means connecting to an address already
+    # vetted and carrying the `Host` header along by hand.
+    private def resolves_internal?(host : String) : Bool
       return true if host.compare("localhost", case_insensitive: true).zero?
-      address = Socket::IPAddress.new(host, 0) rescue nil
-      return false unless address
+      if literal = (Socket::IPAddress.new(host, 0) rescue nil)
+        return internal_address?(literal)
+      end
+
+      addresses =
+        begin
+          Socket::Addrinfo.resolve(host, 80, type: Socket::Type::STREAM)
+        rescue Socket::Error
+          # A name that does not resolve is not a verdict to make here; let the
+          # request fail where the error can say what actually happened.
+          return false
+        end
+      addresses.any? { |info| internal_address?(info.ip_address) }
+    end
+
+    private def internal_address?(address : Socket::IPAddress) : Bool
       address.loopback? || address.private? || address.link_local? || address.unspecified?
     end
   end

src/contributor_mural/runner.cr

@@ -101,8 +101,8 @@ module ContributorMural
     private def draw(path : String, renderer : Renderer, users : Array(ResolvedUser),
                      embedder : Embedder, warned : Set(String)) : {String, Int32}
       embedded, skipped = embedder.embed(users, renderer, @config.fail_on_missing?)
-      skipped.each do |login|
-        Annotations.warning("skipped #{login}: avatar could not be fetched") if warned.add?(login)
+      skipped.each do |skip|
+        Annotations.warning("skipped #{skip.login}: #{skip.reason}") if warned.add?(skip.login)
       end
       if embedded.empty?
         raise AvatarError.new("no avatars could be fetched — refusing to write an empty #{path}")