Escape Velocity(github.com)

138 pointsby remivor 13 Jahren37 comments

https://github.com/blog/1475-escape-velocity

40 comments

tmoertelvor 13 Jahren

That Rails has an escape_once method is a big part of the reason I stopped using Rails. If you think "double escaping" is a problem to be solved by creating a helper method that won't escape what's already escaped, now you have two problems.

EDIT TO ELABORATE:

Problem 1 is the original problem, that parts of your code do not agree upon what a particular string represents. This is the "strings problem," the mother of XSS and injection vulnerabilities. [1]

Problem 2 is that the escape_once method papers over these problems, making them harder to detect, and preventing you from hunting down the logic errors that cause them. (Since these errors often occur in upstream code, you need to find them before they execute to be safe, which is why compile-time methods [2] work best.)

[1] http://blog.moertel.com/posts/2007-08-15-a-bright-future-sec...

[2] http://blog.moertel.com/posts/2006-10-18-a-type-based-soluti...

lobster_johnsonvor 13 Jahren

The article should have pointed out that Github is running Rails 2.3, and that this is where the problem exists.

Rails 3.x has reworked the entire escaping situation and now avoids the re-escaping trap (strings must be flagged as "HTML safe", otherwise they are escaped on final injection into a document). It still has escape_once, but it's not used by Rails itself.

You are of course right to blame 2.x for its flaws, but let's not blame entire projects for problems that have been fixed. (Not that Rails 3.x does not have other egregious inefficiencies, but that's another story.)

tmoertelvor 13 Jahren

I stopped using Rails not so much because of the existence of the escape_once method but because of what its introduction in 2006, and its continued use in mainstream Rails methods (until 2010 when Rails 3.0 was released), said about the direction of the project.

lobster_johnsonvor 13 Jahren

What did it say about the direction of the project, in your opinion? I can see how one might conclude that the developers are idiots based on that method, but I have to say that what I get out of Rails vastly outweighs that stupidity.

tmoertelvor 13 Jahren

It made it hard to believe that the project wouldn't be plagued by security problems. For me, that was enough.

rhizomevor 13 Jahren

But the 3.0 change didn't say anything?

tmoertelvor 13 Jahren

Of course it did, but what it said wasn't enough to make me want to start using Rails again.

TylerEvor 13 Jahren

Why is it a problem?

JulianMorrisonvor 13 Jahren

Because it hides a problem of repeating work. Even re-re-rescanning it with fast C is slower than scanning it once.

Tuna-Fishvor 13 Jahren

That's not the problem. The amount of work done scanning short strings is completely irrelevant. Using escape_once is a problem because it shows that you have no clue what strings in your codebase are escaped and what are not. If you knew, it would be completely pointless. Instead, people resort to cargo-cult like solutions "hey let's escape everything again here just to be sure, that's gotta catch them all".

I've now worked a while in Yesod (Haskell), and I now feel very strongly that this is something they got right. Unescaped strings and html strings should be two completely separate types that can't be cast to each other, except by using transforming functions that clearly define intent.

saraid216vor 13 Jahren

I could actually see an argument to subclass String for this kind of clarity. I don't know if it'd solve it (and I'm likely not to try it out since I don't have an active Ruby project at all right now, let alone an active Rails project), but someone might try that?

stousetvor 13 Jahren

That's essentially how Rails 3+ does this.

rmckayflemingvor 13 Jahren

And here I thought that Ambrosia just released the Escape Velocity source code.

wsc981vor 13 Jahren

Same here. Was an extremely funny space trade / exploration / shooting game. PC users missed out on this great piece of classic Mac shareware.

Small "let's play" video: https://www.youtube.com/watch?v=4VX6TZEfqf4

gilgoomeshvor 13 Jahren

Windows users only missed out on the first two games when they came out. EV Nova (the third game) is available for Windows and the first two games are available as free plugins for Nova.

whatusernamevor 13 Jahren

EV:Nova did finally make a PC release. But very late compared to the Mac.

codewrightvor 13 Jahren

PC users had similar games like Solar Wolf.

pyrocatvor 13 Jahren

Came here hoping it was this. Ah well, there's always Transcendence: http://transcendence.kronosaur.com/

jspirosvor 13 Jahren

There's also http://naev.org/

jfbvor 13 Jahren

Ooh! Nifty. Thanks for the link.

ScotterCvor 13 Jahren

Man I wish. Enough talk about Monkey Island. Escape Velocity was a fantastic adventure game that could use some more sequels.

basseqvor 13 Jahren

Clearly we all (myself included) have one-track minds.

jspirosvor 13 Jahren

Me too, me too. So disappointed.

funkastervor 13 Jahren

lobster_johnsonvor 13 Jahren

To people horrified by the Rails code, keep in mind that Github is running Rails 2.3, which is very old and near the end of LTS. Rails 3.x has reworked the entire escaping situation and now avoids the re-escaping trap (strings must be flagged as "HTML safe", otherwise they are escaped on final injection into a document).

codewrightvor 13 Jahren

So, what's their upgrade path situation? Are they going to try to hold out indefinitely on 2.3? Upgrade at the last possible moment? (Ick) Switch out of Rails when the situation becomes intolerable?

lobster_johnsonvor 13 Jahren

I wouldn't know. I don't know anyone at Github who might answer this.

minimaxvor 13 Jahren

"Just by replacing the escaping function with a more optimized one, we've reduced the average request time by 45ms, and we're allocating 20,000 less Ruby objects per request. That was a lot of escaped HTML right there!"

I generally stay away from web development so forgive me if this one is obvious, but why does so much text need to have HTML escaping performed in order to render the page? Also is there a way to quantify how much text that is? Like a few K per page or a few hundred K?

JangoStevevor 13 Jahren

My guess is that it has something to do with the fact that Github is made up almost entirely of user generated content. Just load a Github project page. From top to bottom, you have:

    * Username in the header (which is probably validated with a specific format,
      so probably doesn't need escaping).
    * Project summary entered when the project is created.
    * Drop-down with list of branch and tag names.
    * Latest commit message.
    * List of names for all folders and files tracked in git for the project.
    * Readme text.

That's just the homepage for a project, and each one of those things could potentially contain malicious HTML or JS, meaning it should be escaped before rendered on the page.

theycallmemortyvor 13 Jahren

Not to mention the pages that display actual code.

lobster_johnsonvor 13 Jahren

The problem occurs with templates such as in this (HAML example):

    - posts.each do |post|
      %li
         = link_to post.title, post_url(post), class: "post_link"

Note that in the case of Rails 2.3 (which the article describes), Rails doesn't know if post.title, post_url() or "post_link" have been escaped or not. It originates at a time when the Rails people were fairly lax and ignorant about encodings and sanitization in general.

For the post.title bit, the link_to method just passes it along, assuming it's valid HTML. Which means it's an injection point if the post was user-submitted. The URL and the {class: "post_link"} are passed through escape_once, described in the article.

As you can see, the amount of data that must potentially be sanitized/encoded can be a lot; lots of small pieces of text that add up to a lot of overhead.

Rails 3.x fixes this by assuming that no strings contain sanitized HTML; therefore, when you insert data into a template, it will automatically escape everything. This is good. If you know that something is already sanitized, you can declare it as such:

     = link_to post.title.html_safe

This flags the string as safe, and link_to will not need to escape it.

Cyranixvor 13 Jahren

`phillmv has identified the biggest concern for sure, but more broadly speaking, you have issues whenever you cross one of two boundaries: 1) escaping context [characters get escaped differently in URLs, JavaScript, HTML, etc.] and 2) user-defined content.

The latter is the source of XSS attacks, but the former is a pain too. Let's say that you run a stock photo website, and your database has images and captions. You decide you want to use captions for the alt attribute of your images, and one of your captions is Cat saying "meow!". If you blindly insert that caption into your HTML without escaping, you could end up with

  <img src="cat.jpg" alt="Cat saying "meow!"">

And now you've got crappy HTML on your hands. It's the same principle as XSS attacks, but you do it to yourself with less disastrous (but still unwanted) effects.

EDIT: Hm, seems like the comment I referenced is now gone? `phillmv pointed out that cross-site scripting (XSS) is an important reason to use escaping appropriately (and often widely).

[deleted]vor 13 Jahren

Irishstevevor 13 Jahren

What a disappointing post. After the ambrosia software announcement the other day I thought they had open sourced escape velocity.

purephasevor 13 Jahren

Very cool. I'll have to check this out. That profile tool looks interesting too. I've been using the rack-mini-profiler gem but it might be a good idea to do a deeper dive.

As always GH, thanks for sharing.

jamesaguilarvor 13 Jahren

I'm always amazed by these startups doing what _seems_ like really simple optimizations and reaping these enormous benefits. If your tools support profiling, you should at least give it a shot once in a while. The fruit are hanging low indeed if Github can reap this kind of reward with this small a tweak.

mbellvor 13 Jahren

FTA: "273,006 objs avg/req"

Wow, that is...a lot...is that mostly rails or is that in user code?

drbawbvor 13 Jahren

Seems kinda par for the course for a Rails app.

http://meta.discourse.org/t/tuning-ruby-and-rails-for-discou...

Scroll down and you'll see that on an average request, Discourse eats up 230,000 strings [which are all objects in Ruby] per request.

The entire thread is about relaxing the garbage collector so that it won't run until the request is over.

[deleted]vor 13 Jahren

silasbvor 13 Jahren

I love stories like this. Make me wish I could spend more time in Ruby land creating/fixing slow code.