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

Don't raise unpermitted parameters error when try to awesome print Ac… #335

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Don't raise unpermitted parameters error when try to awesome print Ac… #335

wants to merge 2 commits into from

Conversation

boatrite
Copy link

…tionController::Parameters object

@imajes
Copy link
Collaborator

imajes commented Jan 9, 2019

this is a meaningful change, but rather than using an unsafe hash we should figure out a way to safely dump this format. (although this is a dev tool, people may forget to not include it in prod, and debug lines always get dropped in...)

@imajes
Copy link
Collaborator

imajes commented Jan 9, 2019

@Boatride would you be willing to work on this to help get it ready for release, please? :)

@boatrite
Copy link
Author

boatrite commented Jan 9, 2019

I don't think this causes any issues even if left in production. As far as I know, the whole unsafe hash thing is really only relevant when being passed in to a model -- to prevent arbitrarily set params from setting attributes on the model that shouldn't allowed to be set. It's not possible to pass the result of ap params into a model's update (or similar) method because the ap method returns nil anyways and not the hash.

As far as I know, unsafe refers to the fact that it's all of the key-value pairs with no filtering applied, and that is exactly the behavior we want when aping an ActionController::Parameters object.

I would be happy to work on this, but I don't see that anything needs to be changed about it. (aside from the test failures)

@boatrite
Copy link
Author

boatrite commented Jan 25, 2019

Heyo @imajes , I added a commit to fix the test failure in Rails 3. There is one failure left, but it seems to be occurring in active record, using ruby-head with frozen string literals enabled. I haven't looked into it.

Do you have any thoughts re: unsafe hash? I might be misunderstanding, could you explain more about the changes you want me to make? :)

@imajes
Copy link
Collaborator

imajes commented Jan 25, 2019

@boatrite thx for the update -- i'm gonna take this work and merge it into the v2 i've been working on :)

Copy link
Collaborator

@BryanH BryanH left a comment

Choose a reason for hiding this comment

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

Ahh, I see another issue with frozen strings....

@matthewhively
Copy link

Since it doesn't appear that version 2.0 PR has changed much recently, would it be possible to get this merged into a release prior than 2.0?

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

Successfully merging this pull request may close these issues.

4 participants