Showing posts with label rails. Show all posts
Showing posts with label rails. Show all posts

Friday, February 12, 2010

Authorization And You

I have been noticing a dangerous anti-pattern recently. It comes in the form of not enough authorization checking. When you have a function in your website that should only be available for particular users (administrators, moderators, whatever), you might be tempted to implement the code to restrict access once, in the view. Consider the following view to delete from a list of users (I snuck in the %: scriptlet tag which I've discussed before):


<ul>
<% @users.each do |user| %>
<li>
<%: user.name %>
<% if @me.can_delete_users? %>
<a href="/users/delete?id=<%: user.id %>">delete</a>
<% end %>
</li>
<% end %>
</ul>


This list might be accessible to everyone, but the deletion should be allowed only to privileged users. This could be a good argument to put all administrative features like this in their own actions/controllers, but there's really no need. Just be sure to check authorization again on the controller side:


class UsersController < ApplicationController
before_filter :construct_me # Set up @me from session

def delete
raise "Nice try!" unless @me.can_delete_users?
User.delete params["id"].to_i
redirect_to "/users/list"
end

def list
@users = User.find :all
end
end


Did you see the extra check that the user is allowed to delete users from within the delete action? That is extra critical. It's easy to consider relying on security through obscurity. Who will try that URL to delete the user if they have no idea what the URL is and what parameters are needed? If the URL were less obvious, you may be even more strongly considering just hiding the URL. While the thought to reduce code clutter and duplicated work may be tempting, it leaves a gaping hole to anyone malicious who decides to start playing around with URLs, searching for administrative functions they shouldn't have access to. You may even get disgruntled former administrators/moderators/whatever who knew the URLs to decide to have some nasty fun with those functions they've been since blocked from.

Security is a hard thing to get right, but there's no excuse to succumb to laziness and knowingly leave dangerous holes in your system. The more powerful the function, the more on edge you need to be for possible ways to break your security.

Note that my snippets include an XSRF hole, but that's a problem for another post.

Friday, December 4, 2009

A Few Worthy Bytes

Bandwidth is a lot cheaper now than it was 10 years ago. Dialup is a dying breed. So please, please, please don't craft your HTML to save every last byte. It is a lot more worthwhile to make your code readable and immediately intuitive than to prevent an extra 10 bytes from going to the user. If you find that you are causing huge downloads of extra HTML, and it is becoming an issue based on real statistics... then start looking into addressing the issue.

For example, consider the following snippet:

<a href="/path/elsewhere"><%
if @condition
%>True text<%
else
%>False text<%
end
%></a>


It may not look that bad alone, but when you try to save every byte, the result months from now will be a garbled mess that is difficult to comprehend. Instead, ignore those extra bytes and produce the following:

<a href="/path/elsewhere">
<% if @condition %>
True text
<% else %>
False text
<% end %>
</a>


If outputting all that extra useless whitespace makes you feel too icky, Rails has an option for you. If you close your scriptlet tag with a dash, as "-%>", Rails will strip some of the whitespace.

<a href="/path/elsewhere">
<% if @condition -%>
True text
<% else -%>
False text
<% end -%>
</a>


Just remember that someone has to maintain the code, and making code more difficult to comprehend will end up costing you more than the bandwidth you saved.

Monday, November 9, 2009

View Sanitizing and Micro-Optimizing

Maurício Linhares posted an intriguing response to my recent post about auto-sanitizing Rails views. I was just gearing up to respond via a comment when I realized I could probably turn it into a full post, so here goes my response!

So, first of all, let me applaud Maurício for actually writing some code and sharing it, rather than keeping the discussion academic and merely flinging arguments around the interwebs. I can't say I always do it, but I have the most fun writing a blog post where I show some code to achieve a solution to a problem I am having. He even went the extra mile to create a plugin for his idea.

That said, I must respectfully disagree with his approach. The gist of how he tackles the problem is to sanitize data as it comes in rather than when you are displaying it. He even argues in his blog post that it is "more adherent to the MVC":

Now, as the data is cleanly stored in your database, you don’t have to waste CPU cycles cleaning up data in your view layer (and you can even say that you’re more adherent to the MVC, as cleaning up user input was never one of it’s jobs).


He makes a convincing argument that the view layer should not sanitize input. My big problem with this is that you have actually taken very specific view details and moved it into the controller and model layers, contrary to what he is claiming. Namely, you have introduced into the controller/model layers the idea that your data is going to be displayed via HTML. However, what if you want to expose a JSON API later on via the same controllers, but with new views? Now, you will need to unsanitize the data, and resanitize it for JavaScript output! You have inadvertently snuck view information into the database! Your data is pigeonholed as HTML data, and it now takes double effort to use the data in another matter (such as JSON data).

This last point deserves some extra attention. Consider if our transform on the data was a lossy transform. This case isn't, because you can easily unsanitize sanitized HTML, but forget that for a second. For example, let's say we wanted all data to be sanitized and censored, such that words like "ass" and "crap" got changed to "***". If we had a bug that caused "crass" to be changed to "cr***", we have just lost information that is irretrievable. If we saved the sanitizing and censoring for the view, where it belongs, we could always fix the censoring code and our "high fidelity" representation will allow us to now correctly show "crass." Let me quote a Stack Overflow podcast, where Joel explains this same position:

Spolsky: Here's my point. Uhh, in general, my design philosophy, which I have learned over many years, is to try and keep the highest fidelity and most original document in the database, and anything that can be generated from that, just regenerate it from that. Every time I've tried to build some kind of content management system or anything that has to generate HTML or anything like that. Or, for example, I try not to have any kind of encoding in the database because the database should be the most fidelitous, (fidelitous?) highest fidelity representation of the thingamajiggy, and if it needs to be encoded, so that it can be safely put in a web page then you run that encoding later, rather than earlier because if you run it before you put the thing in the database, now you've got data that is tied to HTML. Does that make sense? So for example, if you just have a field that's just their name, and you're storing it in the database, they can type HTML in the name field, right? They could put a < in there. So, the question is what do you store in the database, if they put a < as their name. It should probably just be a < character, and it's somebody else's job, whoever tries to render an HTML page, it's their job to make sure that that HTML page is safe, and so they take that string, and that's when you convert it to HTML. And the reason I say that is because, if you try to convert the name to HTML by changing the less than to &lt; before you even put it in the database. If you ever need to generate any other format with that name, other than HTML - for example you get to dump it in HTML to an Excel file, or convert it to Access, or send it to a telephone using SMS, or anything else you might have to do with that, or send them an email, for example, where you're putting their name on the "to" line, and it's not HTML - in all those cases, you'd rather have the true name. You don't want to have to unconvert it from HTML.


Yes, it is tedious and error prone to use "h" everywhere, but that is the exact same problem I was trying to address in my post. However, I feel training myself to use <%: foo %> over <%= h foo %> is a better muscle memory than marking all input as sanitizing. Let's consider the consequences if you forget to apply the new scriptlet versus if you forget to apply the sanitizing of inputs. If you forget the new scriptlet, you have a new XSS hole that needs to be closed by simply changing "=" to ":" (or alternatively adding a call to "h"). If you forget to use the sanitizing of inputs, you have 2 major problems. You have an unknown amount of XSS exploits (everywhere you display that data, which could be in many places). You also have a bunch of data that is now invalid. You now need to either add sanitizing to all the view locations you output the information (which would be tedious and contrary to the whole point of Maurício's approach), or you need to update all existing records to be sanatized, just before enabling sanitizing of input.

There is another issue with this approach that a new scriptlet tag avoids. By making the sanitize decision in the view layer, you have the option of exactly what you will sanitize. Let's consider a site like a blog or Stack Overflow. In such applications, you want some amount of HTML displayed, though not necessarily on all fields of the model. You might want to whitelist sanitize the blog post, question text, or answer text, yet fully sanitize the labels or tags. Granted, you could update the plugin to allow such complexity, but it will be just that... complexity that will bleed through how you invoke the plugin. You will now need to not only specify which actions or controllers use this sanitizing, but also which parameters are excluded from being sanitized.

All of the above pales in comparison to the biggest sin of sanitizing the parameters, and it is one of my biggest pet peeves. It is one of the key points for why Maurício chose the path he did. Premature optimization.

The argument goes that rather than waste the CPU cycles every time you load the page (which is hopefully a lot), you should waste the cycles once as the input is being passed in and saved to the database. Premature optimization usually rears its ugly head in the form of much more insane choices, like insisting on how you should concat your strings. Thankfully, Jeff Atwood has already done metrics showing us that it doesn't matter.

Is sanitizing as quick as string concatenation? Probably not. I would be willing to bet, though, that it is fast enough for a small website. Why waste extra consideration on it until you have the awesome problem of having too many users?

Let's take a step back. Is pushing View logic into Model/Controller territory even worth the possible performance benefit? If I am going to throw proper MVC separation concerns out the window, it better be for a damn good reason. Allowing us to get orders of magnitude more pageviews might be worth it (if metrics proved that it was the best possible improvement, which is doubtful, but let's consider it). The whole concept is to cache something you are doing a lot by doing it once, before it even goes to the database. Let's extrapolate that concept. Instead of sanitizing first so we don't have to sanitize on every page view, why don't we cache the result of the action invocation itself? For every action/params pair that produces a view based strictly on data in the database, we could invoke the action once and just cache the rendered view for future use. Then, simply blow the cache away and re-render when you change the related database row(s). It is typically much more common to view than to update, so I expect this approach would give significantly better performance benefits than simply avoiding a bunch of sanitization calls.

With some careful thinking, we now have a much better solution to remove all the redundant sanitize invocations... and we've even removed redundant calls to the database, and any other costly algorithms we have done within our actions! All while preserving proper MVC separation of concerns. You can bet that I will explore this space when I have the fortunate problem of having too many users (and I wouldn't be surprised if there are available solutions that match my description).

Sorry for going off so much on your very well-meaning post, Maurício! I think you brought a very interesting possible solution, and it's always great to see code brought to the table. However, I do feel we should all seriously consider all approaches, and fully consider the consequences of the path we choose... not just to this particular problem, but any problem. It's best to drill down early and think about what issues our code may cause for us in the future. Don't consider this an excuse to dwell on issues to the point of failing to release useful functionality, though.

Thursday, November 5, 2009

Easy Partials in Rails

I created something at my job that has proven to be extremely useful that I think many people could benefit from. I like to call it Easy Partials, and the goal is to make using partials a bit easier (in case the name didn't make that glaringly obvious). The problem is that rendering a partial requires method calls when a little extra work will allow simpler and more readable partial invocation via convention.

You are probably lost, so let me give you some examples. As it stands today, to render a partial you would do:

<%= render :partial => "my_partial" %>


Which would take the partial "_my_partial.erb" from the same directory and render it.

It works, but what if you could take the whole "convention over configuration" idea and change it into:

<% _my_partial %>


A lot simpler, and pretty intuitive, right? Since partials by Rails conventions start with "_", it makes sense to just name a method as such to render the partial. Note that there is no "=" in the scriptlet, the _my_partial method will concat the partial, so you won't obtain a string to render.

We could create a helper method for every single partial we want to render like that, but that's rather cumbersome, isn't it? It's also not very DRY. You won't have exactly the same code, but you will find yourself with a lot of helpers that look rather similar. Instead, let's try overriding method_missing in our application_helper so we can avoid all those repeated helpers!

module ApplicationHelper
alias_method :method_missing_without_easy_partials, :method_missing

def method_missing_with_easy_partials(method_name, *args, &block)
method_str = method_name.to_s

if method_str =~ /^_.+$/
partial_name = method_str[/^_(.+)$/, 1]
concat_partial partial_name
else
method_missing_without_easy_partials method_name, *args, &block
end
end

alias_method :method_missing, :method_missing_with_easy_partials

# Concat the given partial.
def concat_partial(partial_name)
content = render :partial => partial_name
concat content
nil
end
end


What we've done here is check on method_missing to see if the method name starts with "_", and, if so, treat it as a partial and concat it. If the method doesn't start with "_", we fall back to the original method_missing implementation.

This works, but what if the partial needs some local variables? Before you would do:

<%= render :partial => "my_partial", :locals => { :var => "123" } %>


Instead, let's do:

<% _my_partial :var => "123" %>


Again, a lot simpler, and quite intuitive. To achieve this, our code will look like this:

module ApplicationHelper
alias_method :method_missing_without_easy_partials, :method_missing

def method_missing_with_easy_partials(method_name, *args, &block)
method_str = method_name.to_s

if method_str =~ /^_.+$/
partial_name = method_str[/^_(.+)$/, 1]
concat_partial partial_name, *args
else
method_missing_without_easy_partials method_name, *args, &block
end
end

alias_method :method_missing, :method_missing_with_easy_partials

# Concat the given partial.
def concat_partial(partial_name, options = {})
content = render :partial => partial_name, :locals => options
concat content
nil
end
end


Now we are passing the Hash passed in from the view on to concat_partial so we can specify the locals we want to render. We could check that there is no more than 1 argument passed into method_missing, but I prefer not to (feel free to use and improve anything you see here, in case that wasn't clear).

The next improvement we can make is to allow blocks to be passed in. There is no direct correlation for this next part, that I know of, except to build it yourself with helper methods. It was inspired by Ilya Grigorik.

Here is an example of what we will build:

<% _my_partial :var => "123" do %>
<p>
Some block content.
</p>
<% end %>


This will allow us to effectively pass in a block to the partial, so we can abstract some of the content in the partial so that the caller can define it. And now for the code:

module ApplicationHelper
alias_method :method_missing_without_easy_partials, :method_missing

def method_missing_with_easy_partials(method_name, *args, &block)
method_str = method_name.to_s

if method_str =~ /^_.+$/
partial_name = method_str[/^_(.+)$/, 1]
concat_partial partial_name, *args, &block
else
method_missing_without_easy_partials method_name, *args, &block
end
end

alias_method :method_missing, :method_missing_with_easy_partials

# Concat the given partial.
def concat_partial(partial_name, options = {}, &block)
unless block.nil?
options.merge! :body => capture(&block)
end

content = render :partial => partial_name, :locals => options
concat content
nil
end
end


Within your partial, you will use a "body" variable to output the contents of the block passed in. If you try to use a variable named "body" along with partial blocks, the block will override the body variable, so keep that in mind.

For the final improvement, consider a partial that belongs to more than one controller. What do we do then? Well, how about we maintain a shared directory that we pull from if the partial cannot be found within the local directory. Thus:

module ApplicationHelper
alias_method :method_missing_without_easy_partials, :method_missing

def method_missing_with_easy_partials(method_name, *args, &block)
method_str = method_name.to_s

if method_str =~ /^_.+$/
partial_name = method_str[/^_(.+)$/, 1]

begin
concat_partial partial_name, *args, &block
rescue ActionView::MissingTemplate
partial_name = "shared/#{partial_name}"
concat_partial partial_name, *args, &block
end
else
method_missing_without_easy_partials method_name, *args, &block
end
end

alias_method :method_missing, :method_missing_with_easy_partials

# Concat the given partial.
def concat_partial(partial_name, options = {}, &block)
unless block.nil?
options.merge! :body => capture(&block)
end

content = render :partial => partial_name, :locals => options
concat content
nil
end
end


So, if you use the _my_partial examples from above within the views for "person_controller", but there is no views/person/_my_partial.erb, it will fall back on views/shared/_my_partial.erb.

Using Easy Partials, you can avoid redundant helper methods, keep html in easy to access ERB templates, improve the readability of your views that use partials, and make your code more accessible for your non-programmer UI designers. Note that you can even invoke Easy Partials from within helper methods.

Special thanks to Ilya Grigorik's post on block helpers, which planted the seeds for Easy Partials.

Wednesday, October 21, 2009

Quick Lesson from DevDays

Ok, so I went to Stack Overflow DevDays in San Francisco this last Monday, and it was a lot of fun. I went with a friend from work, and we both walked away happy, stickers in hand.

I took some notes, and I would like to share what I thought of all the speakers and their presentations, but we'll save that for another day... if I actually end up writing it (post a comment if you are interested, and hopefully that will push me to write it up).

What I want to talk about is a small bit I learned from one of the presenters. If you don't know me, I'm a big Linux fan, I heart Google, I have an Android G1, I love open source and Ruby and Rails, and Microsoft is practically the devil. Given that, you might find it interesting that the thing that struck me most... enough to actually write about it... came from Microsoft. That's right, it came from the near-devil. Well, it came from Scott Hanselman, who was a pretty good speaker.

Scott talked about Microsoft .NET MVC. It seems like a relatively cool product, if you can bear to be in the Microsoft world, and if you can bear to deal with such a clunky language as C#. Ok, I gotta give props that C# is a decent language... for a statically typed language... but the platform restrictions (excluding Mono), and it not being nearly as awesome as Ruby, will keep me from ever dealing with it again (I worked in C# professionally for a few years). Yes, .NET provides access to a fleet of languages, but C# is still their flagship. Anyway, from the discussion, MVC seems like it does a decent job at mimicking Rails, though it still didn't seem nearly as elegant.

That was a long lead-up for a simple point I extracted from the Big M... <%: value %>. That's it. I have wished the default of scriptlets was to sanitize html output for a long time, and I'm pretty sure Jeff Atwood had a post on it at one point. That tag, if I understood Scott right, differs from <%= value %> in that it will sanitize the output automatically.

On the train ride back to the south bay, I set it upon myself to implement this auto sanitization in Rails for my current side project. Initially I thought I could do something like:

def :(arg)
h arg
end


But alas, that doesn't work. Ruby won't let you define ":" as a method... it throws a syntax error... drat!

So, I figured, maybe I could monkeypatch the ERB handling so that <%: value %> works as I want, so I looked around the Rails source for how ERB compiles the views. It didn't take long to find erb.rb, which deals with the compilation. I inspected it for a bit, and toyed with some ways to deal with the problem, and ultimately came up with:

class ERB::Compiler::Scanner
alias_method :initialize_without_sanitize, :initialize

def initialize_with_sanitize(src, trim_mode, percent)
initialize_without_sanitize src, trim_mode, percent
@src.gsub! /<%:(.*?)%>/, "<%= h(\\1) %>"
end

alias_method :initialize, :initialize_with_sanitize
end


If the above code is cryptic for you, it basically overrides the initialize method (which is the constructor method for all you non-Rubyists) and uses a regex to replace all of the new sanitizing scriptlets with what would actually work to sanitize it. So, <%: value %> gets transformed to <%= h( value ) %>. I can't vouch for the performance (because I haven't done any performance testing), but it works.

Put the above snippet wherever you put your monkeypatches that you want run once. I have mine in a monkeypatches.rb file set to load after Rails has initialized (so it runs just once, at startup time).

Go forth in sanitized goodness.

Friday, April 17, 2009

Dealing with Errors in Rails

Jeff Atwood's recent blog post on "Exception-Driven Development" has driven me to write a post that I've been thinking about writing for the last couple weeks.

I'm working on a Rails application in my spare time, and one of the concepts I wanted to drill into the codebase from the beginning was easy management of the system from within the system. This means I can log in and, given that I have the proper access, view errors that have happened in the site, migrate the database, restart the application, and perform various other actions that would otherwise require tedious activities like digging in log files, or obtaining shell access and executing various commands. While I am quite comfortable on the shell (I run Linux on all the machines that I use with any regularity), I would much rather have an easy to use GUI that gives me access in the context of when I need it (ie, when I am actually checking up on the website... from within it).

So, this post will basically show you how to add error logging within your Rails application, as I've done in mine.

First up: the database. Here's the database migration script you will need:

class CreateErrors < ActiveRecord::Migration
def self.up
create_table :errors do |t|
t.string :level
t.string :title
t.boolean :handled
t.text :description

t.timestamps
end
end

def self.down
drop_table :errors
end
end


The level column is so you can keep track of errors vs warnings, or have informational messages as well. It's not strictly necessary, but I want the flexibility, plus I'm used to the concept from all the logging frameworks that support multiple levels of messages.

The title and description should be self explanatory. In case you are slow though, the title is just a header so you can at a glance see what errors have cropped up, and the description provides a detailed look at the error (such as a backtrace).

The handled column is critical so you can keep track of which errors you have dealt with, and which need looking at. It acts as a soft delete.

Since you've seen the database end, you might as well peak at the model next:

class Error < ActiveRecord::Base
def self.error(title, description)
log :error, title, description
end

def self.warn(title, description)
log :warn, title, description
end

def self.info(title, description)
log :info, title, description
end

def self.log(level, title, description)
Error.new(:level => level.to_s, :title => title,
:description => description, :handled => false).save
end
end


The error, warn and info methods are to easily log something at the given level, much like a typical logging library. All of those methods use the one logging method which will actually save the error as a row in the database. Pretty simple, as is the usual case with ActiveRecord classes! You could make a case for making the log method private, but I don't really want to, so I won't.

Next up, the errors controller:

class ErrorsController < ApplicationController
title "Errors"
require_admin

def index
@total = Error.count :all, :conditions => ["handled = ?", false]
@errors = Error.find :all,
:conditions => ["handled = ?", false],
:order => "created_at DESC", :limit => 50
@start = [1, @total].min
@amount = @errors.size
end

def show
@error = Error.find params[:id]
end

def handle
@error = Error.find params[:id]
@error.update_attributes :handled => true
redirect_to errors_url
end
end


The title and require_admin methods are some utility methods I baked into the base controller... I may publish those at some point, but they aren't the focus of this post, so not now. If I get any comments requesting the code, I will probably follow up with the code, so let me know if it is intriguing you! Their purpose are to set the title on all views associated with this controller, and check for administrator privileges respectively.

If you will notice, the index will only load the most recent 50 errors... I figured it would be possible for the application to get in a weird loop where you trigger tons and tons of errors. I wouldn't want to haplessly check out the current list of errors, only to watch in horror as I am downloading a list of 100,000 errors. The handle method is the only means to update an error... and it simple turns on the soft delete so the error won't show anymore. It might make sense to set up a mass handled action which would handle a group of selected errors all at once... but I don't really need that yet... I will build it when the need arises.

The rest in index should make sense as you check out the index.html.erb view:

<h1>Listing errors</h1>

<p>
<%= @start %> - <%= @amount %> of <%= @total %> errors.
</p>

<table>
<tr>
<th>Date</th>
<th>Level</th>
<th>Title</th>
</tr>

<% @errors.each do |error| %>
<tr>
<td><%= h format_datetime(error.created_at) %></td>
<td><%= h error.level %></td>
<td><%= h truncate(error.title, :length => 100) %></td>
<td><%= h error.handled %></td>
<td><%= link_to "Show", error %></td>
<td><a href="/errors/handle/<%= error.id %>">Handled</a></td>
</tr>
<% end %>
</table>


Pretty straightforward, huh? Well, here's show.html.erb

<p>
<b>Title:</b>
<%= h @error.title %>
</p>

<p>
<b>Time:</b>
<%= h format_datetime(@error.created_at) %>
</p>

<p>
<b>Level:</b>
<%= h @error.level %>
</p>

<p>
<b>Description:</b>

<br/>

<%= format_multilines h(@error.description) %>
</p>

<a href="/errors">Back</a>


Also pretty simple. If you are curious about format_datetime and format_multilines, they are simple helpers I created to respectively format a date as I want them formatted, and take a string with newlines and convert the newlines to <br/> elements. Pretty simple helpers, but I like keeping as much logic off my views as possible.

That's it! Or wait... that's all the stuff that rails practically generates for you (tweaked for simplicity, but still). How can this stuff get put into practice? Well, I'll give you that code too... since you've gotten this far. In your base application controller:

class ApplicationController < ActionController::Base
RENDER_LIMIT = 0.5
around_filter :time_page

...

private
def time_page
before = Time.new
yield
after = Time.new
delta = after - before

if delta >= RENDER_LIMIT
title = "Page took #{delta} seconds to render"
description = "Execution of #{request.path} took #{delta} seconds to render."
Error.warn title, description
end
end

def rescue_action(e)
title = "Caught exception '#{e}'"
description = "Exception was caught: '#{e}'\n\nBacktrace:\n#{e.backtrace.join("\n")}"
Error.error title, description
super
end
end


With this addition, your new in-site error reporting mechanism will report any page that takes over half a second to load, or absolutely any exception that crops up. This includes failing to parse a Ruby file, actions or controllers that were browsed to that don't exist, or any random exception the user encounters. You may want to adjust the RENDER_LIMIT constant to the threshold of your liking. At some point I would like to add a separate timer for all database queries, but I haven't had the time or interest to do it yet... and besides, the render limit should encompass all queries anyways, so it would be slightly redundant (though I would like to have a much lower threshold of query time than render time).

And as a caveat, this will not report a failure to parse the application controller Ruby file... because... well... think about it and you should realize why it can't :-)

Go forth and practice Exception-Driven Development in Rails.