0

QUESTION: Are there any security risks or other issues with this implementation? The reason I'm doing this is because I don't want to type f.text_field :field_name for each form input. This seemed like the most DRY approach. Main concern: is that constants aren't enforced and wondering if this implementation poises a security risk? 

class UserController < ApplicationController
  def index
  end

  def new
    @user = User.new
    @fields = USER_FIELDS
  end

  def create
    render text: params.inspect
  end

  private
  USER_FIELDS = ["first_name", "last_name", "email", "password"]
  def require_parmas
    params.require(:user).permit(USER_FIELDS)
  end
end

User#new view

<%= form_for @user, url: {action: :create} do |f| %>
  <% @fields.map do |field| %>
    <li>
      <%= f.label field %>

    <% if field =~ /password/ %>
      <%= f.password_field field %>
    </li>

    <% else %>
      <%= f.text_field field %>
    </li>
    <% end %>
  <% end %>
  <%= f.submit "Create Your Account" %>
<% end %>
MrPizzaFace
  • 7,807
  • 15
  • 79
  • 123

1 Answers1

1

You may want to freeze your constant so that it's impossible for any other code to change the value. This should satisfy your security concerns.

USER_FIELDS = ["first_name", "last_name", "email", "password"].freeze

Otherwise, this is a fair approach. The only thing I'd do different is I'd move the USER_FIELDS constant to the User model since this is more in the "business logic" domain. And then just go ahead and access the constant directly instead of storing it in the @fields instance variable first. (Also, don't mark the constant as private if you take this approach or you won't be able to access it.)

<% User::USER_FIELDS.each do |field| %>
  # ...
<% end %>
pdobb
  • 17,688
  • 5
  • 59
  • 74
  • In the same fashion from the controller: `params.require(:user).permit(User::USER_FIELDS)` – pdobb May 27 '14 at 01:51
  • Yeah thats what I did. Thanks... Just felt iffy about putting the strong params array in the model (felt backwards).. But yeah adjusted to it... Thanks.. Wondering if it just adds complexity unnecessarily.. – MrPizzaFace May 27 '14 at 01:54
  • There's likely a "business" reason why these 4 fields are grouped together. So you may find a reason to reuse this grouping later in the model or another controller or view. In which case, you shouldn't be calling UsersController::USER_FIELDS, because that's a little silly. Maybe that perspective helps. Cheers. – pdobb May 27 '14 at 01:55
  • You think with using `freeze` I should write a rescue block incase something/someone tries to modify the constant? – MrPizzaFace May 27 '14 at 01:56
  • The exception that would be thrown if some code tries to modify a frozen constant would happen at the time of reassignment. So that should be self-discoverable at code time. So don't worry about exception handling :) – pdobb May 27 '14 at 01:57