Skip to content

Commit

Permalink
Remove wildcard_redirect_uri option
Browse files Browse the repository at this point in the history
This possibility combined with open redirectors (common in omniauth
clients, to redirect back to where user were) can leak information, with
URLs like:

http://lh:3000/oauth/authorize?client_id=123&redirect_uri=http%3A%2F%2FCLIENT.com/callback/../open_redirect?to=http://evil.com&response_type=token

This changeset undos work merged in
#337

We can keep state between client and provider via the `state` OAuth
parameter, so no functionality should be lost.

Thanks @homakov for your help on this.
  • Loading branch information
tute committed Jan 13, 2015
1 parent 6ab4dc8 commit fd57c47
Show file tree
Hide file tree
Showing 7 changed files with 17 additions and 58 deletions.
6 changes: 5 additions & 1 deletion CHANGELOG.md
@@ -1,6 +1,8 @@
# Changelog

## master
## 2.2.0 (unreleased)

- Remove `wildcard_redirect_url` option


## 2.1.0
Expand All @@ -22,10 +24,12 @@
Disables implicit and password grant flows by default.
- [#510, #544, 722113f] Revoked refresh token response bugfix.


## 2.0.1

- [#525, #526, #527] Fix `ActiveRecord::NoDatabaseError` on gem load.


## 2.0.0

### Backward incompatible changes
Expand Down
1 change: 0 additions & 1 deletion lib/doorkeeper/config.rb
Expand Up @@ -191,7 +191,6 @@ def extended(base)
option :native_redirect_uri, default: 'urn:ietf:wg:oauth:2.0:oob'
option :active_record_options, default: {}
option :realm, default: 'Doorkeeper'
option :wildcard_redirect_uri, default: false
option :force_ssl_in_redirect_uri, default: !Rails.env.development?
option :grant_flows, default: %w(authorization_code client_credentials)

Expand Down
9 changes: 2 additions & 7 deletions lib/doorkeeper/oauth/helpers/uri_checker.rb
Expand Up @@ -11,13 +11,8 @@ def self.valid?(url)

def self.matches?(url, client_url)
url, client_url = as_uri(url), as_uri(client_url)
if Doorkeeper.configuration.wildcard_redirect_uri
return true if url.to_s =~ /^#{Regexp.escape(client_url.to_s)}/
false
else
url.query = nil
url == client_url
end
url.query = nil
url == client_url
end

def self.valid_for_authorization?(url, client_url)
Expand Down
5 changes: 0 additions & 5 deletions lib/generators/doorkeeper/templates/initializer.rb
Expand Up @@ -97,9 +97,4 @@

# WWW-Authenticate Realm (default "Doorkeeper").
# realm "Doorkeeper"

# Allow dynamic query parameters (disabled by default)
# Some applications require dynamic query parameters on their request_uri
# set to true if you want this to be allowed
# wildcard_redirect_uri false
end
5 changes: 0 additions & 5 deletions spec/dummy/config/initializers/doorkeeper.rb
Expand Up @@ -95,9 +95,4 @@

# WWW-Authenticate Realm (default "Doorkeeper").
realm "Doorkeeper"

# Allow dynamic query parameters (disabled by default)
# Some applications require dynamic query parameters on their request_uri
# set to true if you want this to be allowed
# wildcard_redirect_uri false
end
6 changes: 0 additions & 6 deletions spec/lib/config_spec.rb
Expand Up @@ -199,12 +199,6 @@
end
end

describe 'wildcard_redirect_uri' do
it 'is disabled by default' do
Doorkeeper.configuration.wildcard_redirect_uri.should be_falsey
end
end

describe 'realm' do
it 'is \'Doorkeeper\' by default' do
expect(Doorkeeper.configuration.realm).to eq('Doorkeeper')
Expand Down
43 changes: 10 additions & 33 deletions spec/lib/oauth/helpers/uri_checker_spec.rb
Expand Up @@ -53,28 +53,16 @@ module Doorkeeper::OAuth::Helpers
expect(URIChecker.matches?(uri, client_uri)).to be_truthy
end

context 'allows wildcard redirect_uri' do
before do
Doorkeeper.configuration.stub(wildcard_redirect_uri: true)
end

it 'ignores query parameter on comparison' do
uri = 'http://app.co/?query=hello'
client_uri = 'http://app.co'
expect(URIChecker.matches?(uri, client_uri)).to be true
end

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 false
end

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 false
end
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
uri = 'http://app.co/?query=hello'
client_uri = 'http://example.com?app.co=test'
expect(URIChecker.matches?(uri, client_uri)).to be_falsey
end
end

Expand Down Expand Up @@ -111,17 +99,6 @@ module Doorkeeper::OAuth::Helpers
uri = client_uri = 'http://app.co/aaa?waffles=abc'
expect(URIChecker.valid_for_authorization?(uri, client_uri)).to be false
end

context 'allows wildcard redirect_uri' do
before do
Doorkeeper.configuration.stub(wildcard_redirect_uri: true)
end

it 'is true if valid, matches and contains a query parameter' do
uri = client_uri = 'http://app.co/aaa?waffles=abc'
expect(URIChecker.valid_for_authorization?(uri, client_uri)).to be true
end
end
end
end
end

0 comments on commit fd57c47

Please sign in to comment.