Better binary blob detection

Just check the first 1024 bytes if there's any null bytes, instead of using
the error prone mime-type detection, which gives us a smaller chance of
false-positives with the null byte detection

6 files changed, 28 insertions, 29 deletions

diff --git a/app/controllers/blobs_controller.rb b/app/controllers/blobs_controller.rb
index 4766d0d..c2240c1 100644
--- a/app/controllers/blobs_controller.rb
+++ b/app/controllers/blobs_controller.rb
@@ -58,7 +58,7 @@ class BlobsController < ApplicationController
       @blob = @git.tree(@commit.tree.id, ["#{@path.join("/")}"]).contents.first
       render_not_found and return unless @blob
       if @blob.size > 500.kilobytes
-        flash[:error] = I18n.t "blogs_controller.raw_error"
+        flash[:error] = I18n.t "blobs_controller.raw_error", :size => @blob.size
         redirect_to project_repository_path(@project, @repository) and return
       end
       expires_in 30.minutes
diff --git a/app/helpers/blobs_helper.rb b/app/helpers/blobs_helper.rb
index 8e9bcd2..0c6b97e 100644
--- a/app/helpers/blobs_helper.rb
+++ b/app/helpers/blobs_helper.rb
@@ -37,14 +37,11 @@ module BlobsHelper
   ASCII_MIME_TYPES_EXCEPTIONS = [ /^text/ ]
   
   def textual?(blob)
-    types = MIME::Types.type_for(blob.name)
-    if types.first && types.first.ascii?
-      return true
-    end
-    if ASCII_MIME_TYPES_EXCEPTIONS.find{|r| r =~ blob.mime_type }
-      return true
-    end
-    false
+    !binary?(blob)
+  end
+  
+  def binary?(blob)
+    blob.data[0..1024].include?("\000")
   end
   
   def image?(blob)
diff --git a/app/views/blobs/show.html.erb b/app/views/blobs/show.html.erb
index d82676a..4042b53 100644
--- a/app/views/blobs/show.html.erb
+++ b/app/views/blobs/show.html.erb
@@ -65,7 +65,7 @@
   <% end -%>
 <% else -%>
   <p>
-	<%= t("views.blobs.message").call(self, @blob.mime_type, raw_blob_path(@commit.id, current_path)) %>
+	<%= t("views.blobs.message").call(self, raw_blob_path(@commit.id, current_path)) %>
   </p>
 <% end -%>
 
diff --git a/config/locales/en.rb b/config/locales/en.rb
index 36bf432..519cf50 100644
--- a/config/locales/en.rb
+++ b/config/locales/en.rb
@@ -24,7 +24,7 @@
       :activated => 'Your account has been activated!',
     },
     :blobs_controller => {
-      :raw_error => "Blob is too big. Clone the repository locally to see it",
+      :raw_error => "Blob is too big ({{size}} bytes). Clone the repository locally to see it",
     },
     :comments_controller => {
       :create_success => "Your comment was added",
@@ -271,9 +271,10 @@
         :heading => "History for {{ref}}:{{path}}",
         :too_big => lambda { |this, path| "This file is too big to be rendered within reasonable time, " +
           this.link_to("try viewing the raw data", path) },
-        :message => lambda { |this, mime, path| "Not sure we can display this blob nicely (it's a \"#{mime}\" mimetype), " +
-          this.link_to("try viewing the raw data", path) + 
-          "and see if your browser figures it out." },
+        :message => lambda { |this, path|
+          "This blob appears to be binary data, if you like you can " +
+          this.link_to("download the raw data", path) + " (right click, save as)"
+        },
       },
       :comments => {
         :commit => "on commit {{sha1}}",
diff --git a/test/functional/blobs_controller_test.rb b/test/functional/blobs_controller_test.rb
index 36ec4a8..2a04dd3 100644
--- a/test/functional/blobs_controller_test.rb
+++ b/test/functional/blobs_controller_test.rb
@@ -113,7 +113,7 @@ class BlobsControllerTest < ActionController::TestCase
       should "redirects if blob is too big" do
         blob_mock = mock("blob")
         blob_mock.stubs(:contents).returns([blob_mock]) #meh
-        blob_mock.expects(:size).returns(501.kilobytes)
+        blob_mock.expects(:size).twice.returns(501.kilobytes)
         commit_stub = mock("commit")
         commit_stub.stubs(:id).returns("a"*40)
         commit_stub.stubs(:tree).returns(commit_stub)
diff --git a/test/unit/helpers/blobs_helper_test.rb b/test/unit/helpers/blobs_helper_test.rb
index b687394..310a9fd 100644
--- a/test/unit/helpers/blobs_helper_test.rb
+++ b/test/unit/helpers/blobs_helper_test.rb
@@ -54,25 +54,26 @@ class BlobsHelperTest < ActionView::TestCase
   
   context "ascii/binary detection" do
     should "know that a plain text file is fine" do
-      assert textual?(blob_with_name("foo.txt"))
+      assert textual?(blob_with_name("jack.txt", "all work and no play"))
     end
     
-    should "know that a ruby and python file is fine" do
-      assert textual?(blob_with_name("foo.rb"))
-      assert textual?(blob_with_name("foo.py"))
-      assert textual?(blob_with_name("foo.c"))
-      assert textual?(blob_with_name("foo.h"))
-      assert textual?(blob_with_name("foo.cpp"))
-      assert textual?(blob_with_name("foo.m"))
+    should "know that text files are fine" do
+      assert textual?(blob_with_name("foo.rb", "class Lulz; end"))
+      assert textual?(blob_with_name("foo.py", "class Lulz: pass"))
+      assert textual?(blob_with_name("foo.c", "void lulz()"))
+      assert textual?(blob_with_name("foo.m", "[lulz isForTehLaffs:YES]"))
     end
     
-    should "know that binary aren't ok" do
-      assert !textual?(blob_with_name("foo.png"))
-      assert !textual?(blob_with_name("foo.gif"))
-      assert !textual?(blob_with_name("foo.exe"))
+    should "know that binary aren't textual" do
+      assert !textual?(blob_with_name("foo.png", File.read(File.join(RAILS_ROOT, "public/images/rails.png"))))
+      assert !textual?(blob_with_name("foo.gif", "GIF89a\v\x00\r\x00\xD5!\x00\xBD"))
+      assert !textual?(blob_with_name("foo.exe", "rabuf\06si\000ezodniw"))
+      assert !textual?(blob_with_name("foo", "a"*1024 + "\000"))
       
       assert image?(blob_with_name("foo.png"))
       assert image?(blob_with_name("foo.gif"))
+      assert image?(blob_with_name("foo.jpg"))
+      assert image?(blob_with_name("foo.jpeg"))
       assert !image?(blob_with_name("foo.exe"))
     end
   end
@@ -96,9 +97,9 @@ class BlobsHelperTest < ActionView::TestCase
     end
   end
   
-  def blob_with_name(name)
+  def blob_with_name(name, data = "")
     repo = mock("grit repo")
-    Grit::Blob.create(repo, {:name => name})
+    Grit::Blob.create(repo, {:name => name, :data => data})
   end
   
 end