Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate redirect uris are parsable uris #1762

Merged
merged 7 commits into from
Nov 21, 2017
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions app/models/service_provider.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
class ServiceProvider < ApplicationRecord
scope(:active, -> { where(active: true) })

validate :redirect_uris_are_parsable

def self.from_issuer(issuer)
find_by(issuer: issuer) || NullServiceProvider.new(issuer: issuer)
end
Expand Down Expand Up @@ -43,4 +45,23 @@ def encryption_opts
def live?
active? && approved?
end

private

def redirect_uris_are_parsable
return if redirect_uris.blank?

redirect_uris.each do |uri|
next if redirect_uri_valid?(uri)
errors.add(:redirect_uris, :invalid)
break
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the URI contains spaces, it will fail (as expected), but we can easily fix that before we parse the URI. From a UX perspective, I think it's better for us to fix formatting issues for the user. This would mean a before_validation callback that calls .strip on the URI. I'm generally not crazy about callbacks, but I think it's fine in this situation since it only operates on the ServiceProvider object. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user here is the dashboard app which should not be sending us malformed URLs. I agree that we should be removing spaces from the URLs for the user, but we should be doing that on the dashboard before we write them into the db, and not in the IDP.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is validation/enforcement we can move to the dashboard instead to guarantee structure of the API output, happy to do that so we don't have to build some elaborate guards in the IdP.

end

def redirect_uri_valid?(redirect_uri)
parsed_uri = URI.parse(redirect_uri)
parsed_uri.scheme.present? || parsed_uri.host.present?
rescue URI::BadURIError, URI::InvalidURIError
false
end
end
16 changes: 8 additions & 8 deletions config/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,14 @@ class Application < Rails::Application
config.middleware.insert_before 0, Rack::Cors do
allow do
origins do |source, _env|
ServiceProvider.pluck(:redirect_uris).flatten.compact.map do |uri|
begin
URI.join(uri, '/').to_s[0..-2]
rescue URI::BadURIError => err
Rails.logger.warn({ warning: err.class.to_s, source: 'Rack::Cors', uri: uri }.to_json)
''
end
end.include?(source)
next if source == Figaro.env.domain_name

ServiceProvider.pluck(:redirect_uris).flatten.compact.find do |uri|
split_uri = uri.split('//')
protocol = split_uri[0]
domain = split_uri[1].split('/')[0]
source == "#{protocol}//#{domain}"
end.present?
end
resource '/.well-known/openid-configuration', headers: :any, methods: [:get]
resource '/api/openid_connect/certs', headers: :any, methods: [:get]
Expand Down
21 changes: 21 additions & 0 deletions spec/models/service_provider_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,27 @@
require 'rails_helper'

describe ServiceProvider do
describe 'validations' do
it 'validates that all redirect_uris are absolute, parsable uris' do
valid_sp = build(:service_provider, redirect_uris: ['http://foo.com'])
missing_protocol_sp = build(:service_provider, redirect_uris: ['foo.com'])
empty_uri_sp = build(:service_provider, redirect_uris: [''])
relative_uri_sp = build(:service_provider, redirect_uris: ['/asdf/hjkl'])
bad_uri_sp = build(:service_provider, redirect_uris: [' http://foo.com'])
Copy link
Contributor

@tbaxter-18f tbaxter-18f Nov 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is that space before http there for a reason? That's what's making it a bad url? If so, maybe we could think of something that would be more clear. Like maybe 'hXttp'?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ruby's URI module specifies between "Bad" and "Invalid" URIs. An Invalid URI is one that does not match against the URI regex. A bad URI is a URI that matches against the regex, but doesn't work when used in practice (e.g. it raises when passed to URI.parse).

Adding the space at the beginning of the URI was the most obvious way I could think of to raise a URI::BadURIError.


expect(valid_sp).to be_valid
expect(missing_protocol_sp).to_not be_valid
expect(empty_uri_sp).to_not be_valid
expect(relative_uri_sp).to_not be_valid
expect(bad_uri_sp).to_not be_valid
end

it 'allows redirect_uris to be blank' do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add a test to make sure redirect_uris cannot be an empty string?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And also a test to make sure that any extra spaces get trimmed. If somehow the dashboard passes in a URI such as ' http://foo.bar', we want to make sure it gets saved as http://foo.bar.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we want to fail if there are spaces around the uri. The failure would alert us to a bug in how the dashboard is saving redirect uris.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see you already covered those. I couldn't easily see the extra space in GitHub's unified view. I'm fine with treating the extra space as an error.

sp = build(:service_provider, redirect_uris: nil)
expect(sp).to be_valid
end
end

describe '#issuer' do
it 'returns the constructor value' do
sp = ServiceProvider.from_issuer('http://localhost:3000')
Expand Down
22 changes: 12 additions & 10 deletions spec/requests/openid_connect_cors_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,6 @@
end
end
end

context 'with a bad URI in the database' do
before { create(:service_provider, redirect_uris: %w[/foobar]) }

it 'does not blow up' do
get api_openid_connect_certs_path, headers: { 'HTTP_ORIGIN' => 'https://example.com' }

expect(response).to be_ok
end
end
end

describe 'certs endpoint' do
Expand Down Expand Up @@ -156,4 +146,16 @@
end
end
end

describe 'domain name as the origin' do
it 'does not load any ServiceProvider objects' do
stub_const 'ServiceProvider', double
get openid_connect_configuration_path, headers: { 'HTTP_ORIGIN' => Figaro.env.domain_name }

aggregate_failures do
expect(response).to be_ok
expect(response['Access-Control-Allow-Origin']).to be_nil
end
end
end
end