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

Allow Hash 'compact' & 'to_json' in safe mode #8892

Closed

Conversation

stejskalleos
Copy link
Contributor

No description provided.

@theforeman-bot
Copy link
Member

Issues: #33825

config/initializers/safe_mode_jail.rb Outdated Show resolved Hide resolved
@@ -1,3 +1,7 @@
class URI::Generic::Jail < Safemode::Jail
allow :host, :path, :port, :query, :scheme
end

class Hash::Jail < Safemode::Jail
allow :to_json, :compact
Copy link
Member

@ofedoren ofedoren Nov 2, 2021

Choose a reason for hiding this comment

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

AFAIK to_json is quite recursive, what if we have { "key": [1, 2, 3] } and Array has not to_json allowed?

UPD: it works...

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 wondering if this can also expose way more data than you'd expect and leaking internal details that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm wondering if this can also expose way more data than you'd expect and leaking internal details that way

That's interesting thought, mind to share any example?

Copy link
Member

Choose a reason for hiding this comment

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

Let's say you have a hash where the values are ActiveRecord objects, will the full object be serialized? Could that include private data such as the bmc password on hosts?

Copy link
Member

Choose a reason for hiding this comment

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

Well, I've tried that and I didn't see BMC password, but other password was returned as crypted. Although I agree that to_json can be dangerous since it ignores other Jail methods: if there is a Host object in a hash then all its attributes will be in the json string.

This reminds me of one of concerns why we didn't allow this method for webhooks: user must create a hash with attributes they can collect via Jailed methods and then pass this hash to a special method which will convert this to json and send as a payload...

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @ofedoren for being more verbose in the explanation. That is indeed what I was driving at. The BMC password is just one example. And if we don't have it today, it could become a problem later on.

@stejskalleos
Copy link
Contributor Author

Rebased & updated: added doc to and safe_mode_jail.rb moved to lib/foreman/renderer/

@ares ares self-assigned this Nov 3, 2021
Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

Few nitpicks:

module Hash
extend ApipieDSL::Module

apipie_class 'Array' do
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
apipie_class 'Array' do
apipie_class 'Hash' do


apipie_method :to_json, 'Returns a JSON string representing the hash.' do
returns ::String
example '{ id: 1, name: "test" }.to_json #=> {"id":"1","name":"test"}'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
example '{ id: 1, name: "test" }.to_json #=> {"id":"1","name":"test"}'
example '{ id: 1, name: "test" }.to_json #=> {"id":1,"name":"test"}'

end

apipie_method :compact, 'Returns a new hash with the nil values/key pairs removed.' do
returns ::String
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
returns ::String
returns ::Hash

@stejskalleos
Copy link
Contributor Author

Closing the issue, to_json can cause some issues (#8892 (comment))

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