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 #36786 - Permit Symbol for YAML.safe_load report parser #9844

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

bastian-src
Copy link
Contributor

Take a look at the Foreman ticket for the issue explanation.

I assumed the issue shouldn't appear due to this config:

# Starting 6.1.6.1 Rails forced users to explicitly list all the classes
# that are allowed to be loaded by Psych
# (https://github.com/rails/rails/pull/45584#issuecomment-1183255990)
config.active_record.yaml_column_permitted_classes = [
Symbol,
Time,
ActiveSupport::HashWithIndifferentAccess,
ActiveSupport::TimeZone,
ActiveSupport::TimeWithZone,
]

But, it seems like the exception for HashWithIndifferentAccess has been added to the YAML.safe_load call already (which is extended with this PR) - just the one for Symbol is missing which leads to the issue described in the Foreman ticket.

I might've gotten something wrong here tho 🤞

@ekohl ekohl changed the title Fixes #36786 - Permit colon for YAML.safe_load report parser Fixes #36786 - Permit Symbol for YAML.safe_load report parser Sep 27, 2023
@ekohl
Copy link
Member

ekohl commented Sep 27, 2023

I assumed the issue shouldn't appear due to this config:

The config is just for ActiveRecord. If you use YAML directly it doesn't apply.

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.

Can you modify the commit message to say Symbol instead of colon? Other than that 👍

@bastian-src
Copy link
Contributor Author

Thanks for the comment, I've changed the commit message accordingly 🙌

@bastian-src bastian-src changed the title Fixes #36786 - Permit Symbol for YAML.safe_load report parser [WIP] Fixes #36786 - Permit Symbol for YAML.safe_load report parser Oct 4, 2023
@bastian-src
Copy link
Contributor Author

Marked as WIP to add some tests.

@bastian-src bastian-src marked this pull request as draft October 4, 2023 11:57
@bastian-src bastian-src changed the title [WIP] Fixes #36786 - Permit Symbol for YAML.safe_load report parser Fixes #36786 - Permit Symbol for YAML.safe_load report parser Oct 4, 2023
@bastian-src bastian-src marked this pull request as ready for review October 5, 2023 09:14
Copy link
Contributor Author

@bastian-src bastian-src left a comment

Choose a reason for hiding this comment

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

@ekohl Added some tests here too. 🙌

test/models/report_test.rb Show resolved Hide resolved
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.

Thanks!

@ekohl ekohl merged commit 044c105 into theforeman:develop Oct 5, 2023
9 checks passed
@bastian-src bastian-src deleted the salt_fix_report_symbol branch October 5, 2023 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants