Validate redirect uris are parsable uris #1762
Conversation
**Why**: We need to parse the URIs in the CORS middleware to determine which origins are allowed. With this PR we can be confident that the URIs are properly formed and not need to rescue from URI parsing errors.
jmhooper
requested review from
monfresh,
blacktm,
mugizico,
annalee and
tbaxter-18f
|
Looking at this now and thinking I may want to add a test to make sure the service that updates service providers raises when there is a validation error. |
spec/models/service_provider_spec.rb
Outdated
| valid_sp = build(:service_provider, redirect_uris: ['http://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']) |
There was a problem hiding this comment.
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'?
There was a problem hiding this comment.
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.
app/models/service_provider.rb
Outdated
| errors.add(:redirect_uris, :invalid) | ||
| break | ||
| end | ||
| end |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| expect(bad_uri_sp).to_not be_valid | ||
| end | ||
|
|
||
| it 'allows redirect_uris to be blank' do |
There was a problem hiding this comment.
Can we also add a test to make sure redirect_uris cannot be an empty string?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
I pushed some more commits to make sure we are raising an error when there is a service provider validation error in the seeds or the dashboard response. I think we want both of those to raise so we'll know about problems there, either from the 500 response or the app failing to launch. |
|
@monfresh: I have merged this validation on the dashboard (ref: 18F/identity-dashboard#145) and cleaned the bad sp redirect uris out of the db. With that done, would you mind giving this PR another look and a 👍 or 👎 |
Why: We need to parse the URIs in the CORS middleware to determine
which origins are allowed. With this PR we can be confident that the
URIs are properly formed and not need to rescue from URI parsing errors.
Note that after this PR is merged, we will not be able to update service providers if the dashboard is rendering empty or malformed url strings. This means working with @blacktm to make a corresponding change to the dashboard to prevent it from doing that.