Fix error when editing Snippets with image blob

Views do not currently support binary blob
data and thus will throw an error. This change
will redirect the user if they try to edit or update
a snippet with binary data.

app/controllers/concerns/snippets_actions.rb

@@ -3,6 +3,10 @@
 module SnippetsActions
   extend ActiveSupport::Concern
 
+  included do
+    before_action :redirect_if_binary, only: [:edit, :update]
+  end
+
   def edit
     # We need to load some info from the existing blob
     snippet.content = blob.data
@@ -52,4 +56,8 @@ module SnippetsActions
     flash.now[:alert] = repository_errors.first if repository_errors.present?
     recaptcha_check_with_fallback(repository_errors.empty?) { render :edit }
   end
+
+  def redirect_if_binary
+    redirect_to gitlab_snippet_path(snippet) if blob&.binary?
+  end
 end

changelogs/unreleased/212662-edit-snippet-images.yml

+---
+title: Resolve Snippet actions with binary data
+merge_request: 28191
+author:
+type: fixed

spec/controllers/projects/snippets_controller_spec.rb

@@ -116,7 +116,7 @@ describe Projects::SnippetsController do
       end
 
       context 'when the snippet is public' do
-        it 'rejects the shippet' do
+        it 'rejects the snippet' do
           expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }
             .not_to change { Snippet.count }
           expect(response).to render_template(:new)
@@ -164,6 +164,7 @@ describe Projects::SnippetsController do
 
   describe 'PUT #update' do
     let(:project) { create :project, :public }
+    let(:visibility_level) { Snippet::PUBLIC }
     let(:snippet) { create :project_snippet, author: user, project: project, visibility_level: visibility_level }
 
     def update_snippet(snippet_params = {}, additional_params = {})
@@ -174,13 +175,27 @@ describe Projects::SnippetsController do
       put :update, params: {
         namespace_id: project.namespace.to_param,
         project_id: project,
-        id: snippet.id,
+        id: snippet,
         project_snippet: { title: 'Title', content: 'Content' }.merge(snippet_params)
       }.merge(additional_params)
 
       snippet.reload
     end
 
+    it_behaves_like 'updating snippet checks blob is binary' do
+      let_it_be(:title) { 'Foo' }
+      let(:params) do
+        {
+          namespace_id: project.namespace.to_param,
+          project_id: project,
+          id: snippet.id,
+          project_snippet: { title: title }
+        }
+      end
+
+      subject { put :update, params: params }
+    end
+
     context 'when the snippet is spam' do
       before do
         allow_next_instance_of(Spam::AkismetService) do |instance|
@@ -198,9 +213,7 @@ describe Projects::SnippetsController do
       end
 
       context 'when the snippet is public' do
-        let(:visibility_level) { Snippet::PUBLIC }
-
-        it 'rejects the shippet' do
+        it 'rejects the snippet' do
           expect { update_snippet(title: 'Foo') }
             .not_to change { snippet.reload.title }
         end
@@ -245,7 +258,7 @@ describe Projects::SnippetsController do
       context 'when the private snippet is made public' do
         let(:visibility_level) { Snippet::PRIVATE }
 
-        it 'rejects the shippet' do
+        it 'rejects the snippet' do
           expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }
             .not_to change { snippet.reload.title }
         end
@@ -549,4 +562,19 @@ describe Projects::SnippetsController do
       end
     end
   end
+
+  describe 'GET #edit' do
+    it_behaves_like 'editing snippet checks blob is binary' do
+      let(:snippet) { create(:project_snippet, :private, project: project, author: user) }
+      let(:params) do
+        {
+          namespace_id: project.namespace,
+          project_id: project,
+          id: snippet
+        }
+      end
+
+      subject { get :edit, params: params }
+    end
+  end
 end

spec/controllers/snippets_controller_spec.rb

@@ -308,7 +308,7 @@ describe SnippetsController do
       end
 
       context 'when the snippet is public' do
-        it 'rejects the shippet' do
+        it 'rejects the snippet' do
           expect { create_snippet(visibility_level: Snippet::PUBLIC) }
             .not_to change { Snippet.count }
         end
@@ -354,6 +354,7 @@ describe SnippetsController do
 
   describe 'PUT #update' do
     let(:project) { create :project }
+    let(:visibility_level) { Snippet::PUBLIC }
     let(:snippet) { create :personal_snippet, author: user, project: project, visibility_level: visibility_level }
 
     def update_snippet(snippet_params = {}, additional_params = {})
@@ -367,6 +368,12 @@ describe SnippetsController do
       snippet.reload
     end
 
+    it_behaves_like 'updating snippet checks blob is binary' do
+      let_it_be(:title) { 'Foo' }
+
+      subject { put :update, params: { id: snippet, personal_snippet: { title: title } } }
+    end
+
     context 'when the snippet is spam' do
       before do
         allow_next_instance_of(Spam::AkismetService) do |instance|
@@ -429,9 +436,7 @@ describe SnippetsController do
       end
 
       context 'when the snippet is public' do
-        let(:visibility_level) { Snippet::PUBLIC }
-
-        it 'rejects the shippet' do
+        it 'rejects the snippet' do
           expect { update_snippet(title: 'Foo') }
             .not_to change { snippet.reload.title }
         end
@@ -779,4 +784,12 @@ describe SnippetsController do
       end
     end
   end
+
+  describe 'GET #edit' do
+    it_behaves_like 'editing snippet checks blob is binary' do
+      let_it_be(:snippet) { create(:personal_snippet, :public, :repository, author: user) }
+
+      subject { get :edit, params: { id: snippet } }
+    end
+  end
 end

spec/support/shared_examples/controllers/binary_blob_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'editing snippet checks blob is binary' do
+  before do
+    sign_in(user)
+
+    allow_next_instance_of(Blob) do |blob|
+      allow(blob).to receive(:binary?).and_return(binary)
+    end
+
+    subject
+  end
+
+  context 'when blob is text' do
+    let(:binary) { false }
+
+    it 'responds with status 200' do
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(response).to render_template(:edit)
+    end
+  end
+
+  context 'when blob is binary' do
+    let(:binary) { true }
+
+    it 'redirects away' do
+      expect(response).to redirect_to(gitlab_snippet_path(snippet))
+    end
+  end
+end
+
+RSpec.shared_examples 'updating snippet checks blob is binary' do
+  before do
+    sign_in(user)
+
+    allow_next_instance_of(Blob) do |blob|
+      allow(blob).to receive(:binary?).and_return(binary)
+    end
+
+    subject
+  end
+
+  context 'when blob is text' do
+    let(:binary) { false }
+
+    it 'updates successfully' do
+      expect(snippet.reload.title).to eq title
+      expect(response).to redirect_to(gitlab_snippet_path(snippet))
+    end
+  end
+
+  context 'when blob is binary' do
+    let(:binary) { true }
+
+    it 'redirects away without updating' do
+      expect(response).to redirect_to(gitlab_snippet_path(snippet))
+      expect(snippet.reload.title).not_to eq title
+    end
+  end
+end