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

Avoid triggering unpermitted params on JS link #1507

Merged
merged 1 commit into from Dec 27, 2019

Conversation

pablobm
Copy link
Collaborator

@pablobm pablobm commented Dec 23, 2019

With #1457, I introduced a bug. Reproduction:

  1. Visit the index page of a model with a has_many relationship (eg: customers).
  2. Click on a table header to trigger some sorting. The current URL will gain query params.
  3. Click on a row to see the show page of a record. Don't click on actual data on the table (eg: customer name), but instead click on a "blank" area of the row. This is so that the JS click handler kicks in and performs the "link" behaviour, as opposed to clicking on an actual link.
  4. You'll get an error ActionController::UnpermittedParameters.

This is triggered by the following:

  1. At the index page, the URL is something like /admin/customers
  2. When sorting the table, the current URL becomes something like /admin/customers?customer%5Bdirection%5D=desc&customer%5Border%5D=name
  3. Clicking on the row, the JS gets us a URL like so /admin/customers/123?customer%5Bdirection%5D=desc&customer%5Border%5D=name
  4. The partial that renders the has_many relationship in the show page can receive query params. It checks that the current ones are permitted. Turns out those aren't (they are in the index page, but not here), so it raises an exception.

I'm not sure about the spec example I have introduced. I feel that someone who finds it in the future is going to wonder where that came from. I guess they can check the git history...? Thoughts?

@nickcharlton
Copy link
Member

I'm happy with relying on the Git history for the explanation of this, so I'm going to merge it. Thanks!

@nickcharlton nickcharlton merged commit b71930f into thoughtbot:master Dec 27, 2019
@pablobm pablobm deleted the remove-query branch June 24, 2021 10:40
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.

None yet

2 participants