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

Fixes #37341 - catch exception if host parameter can not be rendered #10122

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

sbernhard
Copy link
Contributor

@sbernhard sbernhard commented Apr 10, 2024

See https://projects.theforeman.org/issues/37341#note-1 for the reproducer
rescue blocks catches exception if safemode is 'on' and if 'off'

@sbernhard
Copy link
Contributor Author

[test katello]

@sbernhard
Copy link
Contributor Author

[test unit]

@sbernhard
Copy link
Contributor Author

Looks like, the bug was introduced with c8236d2 @nadjaheitmann - maybe you want tor review this?

Copy link
Contributor

@dosas dosas left a comment

Choose a reason for hiding this comment

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

@sbernhard Is it possible to adapt/add tests to catch this error

@sbernhard sbernhard force-pushed the fix_37341 branch 13 times, most recently from 4e12efe to 7633793 Compare April 29, 2024 15:23
if host.params.has_key? "#{last_report.origin.downcase}_interval"
interval = host.params["#{last_report.origin.downcase}_interval"]
end
rescue NameError, Safemode::SecurityError, Safemode::NoMethodError => exception
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I like catching these exceptions here in the model layer. A user doesn't have access to the logs so they don't know their parameter is silently ignored.

Implementation wise I'd really want it to be:

def reported_origin_interval
  return unless last_report&.origin

  name = "#{last_report.origin.downcase}_interval"
  host.params.fetch(name) { Setting[name.to_sym] }
end

Looking at the code I think host.params will render all the parameters, so just calling that should be enough to trigger it an exception.

Thinking about solving it at a deeper level: should parameters be rendered before saving? Can that reliably work? It may still not be enough.

Perhaps HostParams needs a safe_params that ignores any invalid ERB. The reason I suggest this is that we use params in more places so I suspect there are more ways this bug could surface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants