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.

Sign up for GitHub

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

Jump to bottom

Fixes #36644 - Validate root_url before using it #9795

Merged
merged 1 commit into from
Aug 8, 2023
Merged

Fixes #36644 - Validate root_url before using it #9795

merged 1 commit into from
Aug 8, 2023

Conversation

evgeni
Copy link
Member

@evgeni evgeni commented Aug 7, 2023

No description provided.

@theforeman-bot
Copy link
Member

Issues: #36644

evgeni
evgeni commented Aug 7, 2023
View reviewed changes
app/controllers/links_controller.rb Outdated
@@ -1,5 +1,8 @@
TRUSTED_DOMAINS = ['theforeman.org', 'redhat.com', 'orcharhino.com'].freeze
Copy link
Member Author

Choose a reason for hiding this comment

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

cc @timogoebel @sbernhard

are there any other domains that you'd need allowed in the redirection code (which is not hard coded in external_url)?

Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on making it an item in SETTINGS?

Copy link
Member Author

Choose a reason for hiding this comment

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

WWTD - what would Tomer do? ;-)

(no idea if this needs to be a setting, as I don't anticipate it to be changed by users, but can certainly make it one if that makes handling easier?)

Copy link
Member

Choose a reason for hiding this comment

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

I think Tomer mostly had a problem with user visible settings, but I thought about something you can override in settings.yaml.

Copy link
Member Author

Choose a reason for hiding this comment

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

done that now

ekohl
ekohl reviewed Aug 7, 2023
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

A note from a private conversation: once #9756 is merged we should consider logging a deprecation warning when root_url is passed.

app/controllers/links_controller.rb Outdated Show resolved Hide resolved
@evgeni
Copy link
Member Author

evgeni commented Aug 8, 2023

now, why did that not fire off jenkins?

[test unit]

evgeni
evgeni commented Aug 8, 2023
View reviewed changes
config/settings.rb
@@ -28,6 +28,8 @@

SETTINGS[:hosts] ||= []

SETTINGS[:trusted_redirect_domains] ||= ['theforeman.org', 'redhat.com', 'orcharhino.com'].freeze
Copy link
Member Author

Choose a reason for hiding this comment

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

should this even be frozen? idk.

@evgeni
Copy link
Member Author

evgeni commented Aug 8, 2023

the only code I could find that could be affected by that is:
https://github.com/theforeman/foreman_datacenter/blob/466e2d95d6a0fc8db8f28cdcd650d1602b3c51e1/app/helpers/foreman_datacenter/application_helper.rb#L12-L23

but it seems to override the root_url after the request has been accepted, so it should not really be a problem?

@ekohl ekohl merged commit 57bbbd1 into theforeman:develop Aug 8, 2023
4 checks passed
@evgeni evgeni deleted the validate_root_url branch August 8, 2023 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekohl ekohl ekohl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@evgeni @theforeman-bot @ekohl