Open viroulep opened 8 years ago
Actually in your case it's not undefined but equal to "g"
.
We only ensure that it's "general"
by default if nothing else is specified (here).
It could be easily fixed by:
unless %w(general email preferences password avatar).include? params[:section]
params[:section] = "general"
end
But this requires us to remember that we need to add here every new section. Is it worth doing? Manually setting other section param seems unlikely, anyway it's probably right to avoid such side effects.
It is this line of javascript that hides everything except for the magical tab? It looks like we're allowing arbitrary user input to show up as javascript. This feels like an XSS vector...
I just played around with this a bit, and fortunately, the call to j
seems to be protecting us from someone trying to craft a malicious section
parameter, but I still don't feel comfortable with this.
I support sanitizing the value of params[:section]
. Fortunately, we won't have to worry about forgetting to do this, as it will be impossible to add a new section without updating the sanitization code. We already do a similar thing in: https://github.com/cubing/worldcubeassociation.org/blob/d8d9e728e8a570b21ec1836a670e79d832634387/WcaOnRails/app/controllers/competitions_controller.rb#L60 with params[:display]
.
Depending on how involved the sections are you could also avoid duplication by extracting the common pieces:
def form_sections = [
Section.new(title: 'Foo', param: 'foo', partial: 'bar/_foo'),
# ...
]
unless form_sections.map(&:param).include? params[:section]
params[:section] = form_sections.first.param
end
That's a neat idea for a library, @timhabermaas. We've definitely suffered from that "no type coercion until the ActiveRecord layer" problem. For example, we have a lot of calls to ActiveRecord::Type::Boolean.new.type_cast_from_database.
@jfly
I support sanitizing the value of params[:section]. Fortunately, we won't have to worry about forgetting to do this, as it will be impossible to add a new section without updating the sanitization code.
Actually it's not true, you'd be able to add a new tab and access it. The only thing that wouldn't work is the url, so if you'd refresh the page (with the new tab open) then you'd be 'redirected' to the general section. Don't have better solution though.
When an undefined section name is specified, the profile is all on one page, see the screenshot:
I think we want to display only "General" in this case, like when no section is provided.