Fix #1170: add check for loopback IP URIs

NEWS.md

@@ -7,7 +7,8 @@ User-visible changes worth mentioning.
 
 ## master
 
-- [#1177] Limiting `scopes` for certain `grant_types`
+- [#1182] Fix loopback IP redirect URIs to conform with RFC8252, p. 7.3 (fixes #1170).
+- [#1177] Allow to limit `scopes` for certain `grant_types`
 - [#1162] Fix `enforce_content_type` for requests without body.
 - [#1164] Fix error when `root_path` is not defined.
 - [#1175] Internal refactor: use `scopes_string` inside `scopes`.

lib/doorkeeper/oauth/helpers/uri_checker.rb

@@ -1,6 +1,25 @@
 # frozen_string_literal: true
+require 'ipaddr'
 
 module Doorkeeper
+  module IPAddrLoopback
+    def loopback?
+      case @family
+      when Socket::AF_INET
+        @addr & 0xff000000 == 0x7f000000
+      when Socket::AF_INET6
+        @addr == 1
+      else
+        raise AddressFamilyError, "unsupported address family"
+      end
+    end
+  end
+
+  # For backward compatibility with old rubies
+  if Gem::Version.new(RUBY_VERSION) < Gem::Version.new("2.5.0")
+    IPAddr.send(:include, Doorkeeper::IPAddrLoopback)
+  end
+
   module OAuth
     module Helpers
       module URIChecker
@@ -23,10 +42,23 @@ def self.matches?(url, client_url)
             client_url.query = nil
           end
 
+          # RFC8252, Paragraph 7.3
+          # @see https://tools.ietf.org/html/rfc8252#section-7.3
+          if loopback_uri?(url) && loopback_uri?(client_url)
+            url.port = nil
+            client_url.port = nil
+          end
+
           url.query = nil
           url == client_url
         end
 
+        def self.loopback_uri?(uri)
+          IPAddr.new(uri.host).loopback?
+        rescue IPAddr::Error
+          false
+        end
+
         def self.valid_for_authorization?(url, client_url)
           valid?(url) && client_url.split.any? { |other_url| matches?(url, other_url) }
         end

spec/lib/oauth/helpers/uri_checker_spec.rb

@@ -61,18 +61,36 @@ module Doorkeeper::OAuth::Helpers
         expect(URIChecker.matches?(uri, client_uri)).to be_truthy
       end
 
-      it 'doesn\'t allow non-matching domains through' do
+      it "doesn't allow non-matching domains through" do
         uri = 'http://app.abc/?query=hello'
         client_uri = 'http://app.co'
         expect(URIChecker.matches?(uri, client_uri)).to be_falsey
       end
 
-      it 'doesn\'t allow non-matching domains that don\'t start at the beginning' do
+      it "doesn't allow non-matching domains that don't start at the beginning" do
         uri = 'http://app.co/?query=hello'
         client_uri = 'http://example.com?app.co=test'
         expect(URIChecker.matches?(uri, client_uri)).to be_falsey
       end
 
+      context 'loopback IP redirect URIs' do
+        it 'ignores port for same URIs' do
+          uri = 'http://127.0.0.1:5555/auth/callback'
+          client_uri = 'http://127.0.0.1:48599/auth/callback'
+          expect(URIChecker.matches?(uri, client_uri)).to be_truthy
+
+          uri = 'http://[::1]:5555/auth/callback'
+          client_uri = 'http://[::1]:5555/auth/callback'
+          expect(URIChecker.matches?(uri, client_uri)).to be_truthy
+        end
+
+        it "doesn't ignore port for URIs with different queries" do
+          uri = 'http://127.0.0.1:5555/auth/callback'
+          client_uri = 'http://127.0.0.1:48599/auth/callback2'
+          expect(URIChecker.matches?(uri, client_uri)).to be_falsey
+        end
+      end
+
       context "client registered query params" do
         it "doesn't allow query being absent" do
           uri = 'http://app.co'