-
Notifications
You must be signed in to change notification settings - Fork 993
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
Fixes #36919 - Add URL validation to HTTP Proxy URL field. #9903
Conversation
e629d12
to
19cb9bb
Compare
Could we have a clear error message, something like: Please enter a valid proxy URL. |
19cb9bb
to
9c5a13a
Compare
@lfu PR updated, here is the new screenshot |
9c5a13a
to
8c94d22
Compare
app/models/http_proxy.rb
Outdated
@@ -34,6 +34,7 @@ class HttpProxy < ApplicationRecord | |||
|
|||
def full_url | |||
uri = URI(url) | |||
raise URI::InvalidURIError, _("Please enter a valid proxy URL.") unless uri.is_a?(URI::HTTP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raise URI::InvalidURIError, _("Please enter a valid proxy URL. ") if !uri.is_a?(URI::HTTP) || uri.host.nil?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the catch @lfu, updated it
3297c33
to
a18bf52
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you raise an exception, you can't properly see it in the form. Normally we use valiations to display errors inline. In fact, we already have that:
foreman/app/models/http_proxy.rb
Line 21 in 7493355
validates :url, :format => { :with => /\Ahttps?:\/\// }, :presence => true |
So why is this not working? Is the validation somehow skipped?
To a bit more guiding: I suspect that you can change this bit: foreman/app/controllers/http_proxies_controller.rb Lines 26 to 33 in 7493355
To def test_connection
http_proxy = HttpProxy.new(http_proxy_params)
if http_proxy.invalid?
# TODO: convert http_proxy.errors to some exception somehow
end
http_proxy.test_connection(params[:test_url])
render :json => {:status => 'success', :message => _("HTTP Proxy connection successful.")}, :status => :ok
rescue => e
render :json => {:status => 'failure', :message => e.message}, :status => :unprocessable_entity
end See https://guides.rubyonrails.org/active_record_validations.html#valid-questionmark-and-invalid-questionmark & https://guides.rubyonrails.org/active_record_validations.html#validations-overview-errors as well The main benefit is that you reuse the already existing validations. It may error out if the name is not specified though, which not be what you want. |
Ok will go with this approach |
a18bf52
to
7e98f22
Compare
7e98f22
to
3f18448
Compare
@ekohl when you get a chance can this one get a review, I have added tests. |
@ekohl can this one get back on your radar to review? It should be a quick one and I did tests as you asked. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working well for me! I see the right error when I pass in an invalid proxy URL. The proxy test still works for valid proxies as well, and fails for invalid proxies with valid URLs.
@ekohl I'd like to merge this, but your previous change request is blocking it. |
Before PR:
After PR:
To Test: