Error 500: Filenames with non-UTF8 characters are erroneously rendered with DiffViewer::Image

https://sentry.gitlab.net/gitlab/gitlabcom/issues/822333/

NoMethodError: undefined method `size' for nil:NilClass
  /opt/gitlab/embedded/service/gitlab-rails/app/views/projects/diffs/_single_image_diff.html.haml:14:in `_app_views_projects_diffs__single_image_diff_html_haml__1156166223028541489_70353837377460'
    %p.image-info= number_to_human_size(blob.size)
  action_view/template.rb:157:in `block in render'
    view.send(method_name, locals, buffer, &block)
  active_support/notifications.rb:168:in `instrument'
    yield payload if block_given?
  action_view/template.rb:352:in `instrument_render_template'
    ActiveSupport::Notifications.instrument("!render_template.action_view".freeze, instrument_payload, &block)
  action_view/template.rb:155:in `render'
    instrument_render_template do
...
(234 additional frame(s) were not displayed)

ActionView::Template::Error: undefined method `size' for nil:NilClass

ActionView::Template::Error: undefined method `size' for nil:NilClass

added typebug label

@pokstad1 ran into this today in https://gitlab.com/gitlab-org/gitlab-ce/-/jobs/240782593 while trying to test https://gitlab.com/gitlab-org/gitlab-test/compare/master...invalid-utf8-diff-paths.

It looks like the diff for this commit has this data:

#<Gitlab::Diff::File:0x00007fee864966b0 @diff=#<Gitlab::Git::Diff:0x00007fee86497290 @expanded=false, @diff="@@ -0,0 +1,16 @@\n+testme\n+======\n+\n+Sample repo for testing gitlab features\n+\n+## Usage in testing and development\n+\n+`gitlab-test` is used to seed your local gdk GitLab application and is also used in rspec tests.\n+Because of this, when building and testing features that require a specific type of file, you can\n+add them to the `gitlab-test` repo in order to access that blob during development or testing.\n+\n+1. Push a new file on a new branch to [gitlab-org/gitlab-test](https://gitlab.com/gitlab-org/gitlab-test).\n+2. Execute `rm -rf tmp/tests` in your gitlab repo.\n+3. Add your branch and its head commit to the `BRANCH_SHA` hash in [`test_env.rb`](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/spec/support/helpers/test_env.rb#L10-68).\n+\n+In rspec, you can use `create(:project)` to create an instance of `gitlab-test` that has a `path` of `gitlabhq`.\n", @new_path="helloworld", @old_path="helloworld", @a_mode="0", @b_mode="100644", @new_file=true, @renamed_file=false, @deleted_file=false, @too_large=false, @collapsed=false>, @stats=nil, @repository=#<Repository:@hashed/6b/86/6b86b273ff34fce19d6b804eff5a3f5747ada4eaa22f1d49c01e52ddb7875b4b>, @diff_refs=#<Gitlab::Diff::DiffRefs:0x00007fee7e04e9c0 @base_sha="b83d6e391c22777fca1ed3012fce84f633d7fed0", @start_sha="b83d6e391c22777fca1ed3012fce84f633d7fed0", @head_sha="19950f03c765f7ac8723a73a0599764095f52fc0">, @fallback_diff_refs=nil, @unique_identifier=nil, @unfolded=false, @new_content_sha="19950f03c765f7ac8723a73a0599764095f52fc0", @diff_lines=[#<Gitlab::Diff::Line:0x00007fee806fe570 @index=0, @type="new", @text="+testme", @new_pos=1, @old_pos=0, @parent_file=#<Gitlab::Diff::File:0x00007fee864966b0 ...>, @rich_text=nil, @line_code="6adfb183a4a2c94a2f92dab5ade762a47889a5a1_0_1">, #<Gitlab::Diff::Line:0x00007fee806fe228 @index=1, @type="new", @text="+======", @new_pos=2, @old_pos=0, @parent_file=#<Gitlab::Diff::File:0x00007fee864966b0 ...>, @rich_text=nil, @line_code="6adfb183a4a2c94a2f92dab5ade762a47889a5a1_0_2">, #<Gitlab::Diff::Line:0x00007fee806fdeb8 @index=2, @type="new", @text="+", @new_pos=3, @old_pos=0, @parent_file=#<Gitlab::Diff::File:0x00007fee864966b0 ...>, @rich_text=nil, @line_code="6adfb183a4a2c94a2f92dab5ade762a47889a5a1_0_3">, #<Gitlab::Diff::Line:0x00007fee806fdaa8 @index=3, @type="new", @text="+Sample repo for testing gitlab features", @new_pos=4, @old_pos=0, @parent_file=#<Gitlab::Diff::File:0x00007fee864966b0 ...>, @rich_text=nil, @line_code="6adfb183a4a2c94a2f92dab5ade762a47889a5a1_0_4">, #<Gitlab::Diff::Line:0x00007fee806fd760 @index=4, @type="new", @text="+", @new_pos=5, @old_pos=0, @parent_file=#<Gitlab::Diff::File:0x00007fee864966b0 ...>, @rich_text=nil, @line_code="6adfb183a4a2c94a2f92dab5ade762a47889a5a1_0_5">, #<Gitlab::Diff::Line:0x00007fee806fd490 @index=5, @type="new", @text="+## Usage in testing and development", @new_pos=6, @old_pos=0, @parent_file=#<Gitlab::Diff::File:0x00007fee864966b0 ...>, @rich_text=nil, @line_code="6adfb183a4a2c94a2f92dab5ade762a47889a5a1_0_6">, #<Gitlab::Diff::Line:0x00007fee806fd1e8 @index=6, @type="new", @text="+", @new_pos=7, @old_pos=0, @parent_file=#<Gitlab::Diff::File:0x00007fee864966b0 ...>, @rich_text=nil, @line_code="6adfb183a4a2c94a2f92dab5ade762a47889a5a1_0_7">, #<Gitlab::Diff::Line:0x00007fee806fce50 @index=7, @type="new", @text="+`gitlab-test` is used to seed your local gdk GitLab application and is also used in rspec tests.", @new_pos=8, @old_pos=0, @parent_file=#<Gitlab::Diff::File:0x00007fee864966b0 ...>, @rich_text=nil, @line_code="6adfb183a4a2c94a2f92dab5ade762a47889a5a1_0_8">, #<Gitlab::Diff::Line:0x00007fee806fcb58 @index=8, @type="new", @text="+Because of this, when building and testing features that require a specific type of file, you can", @new_pos=9, @old_pos=0, @parent_file=#<Gitlab::Diff::File:0x00007fee864966b0 ...>, @rich_text=nil, @line_code="6adfb183a4a2c94a2f92dab5ade762a47889a5a1_0_9">, #<Gitlab::Diff::Line:0x00007fee806fc6f8 @index=9, @type="new", @text="+add them to the `gitlab-test` repo in order to access that blob during development or testing.", @new_pos=10, @old_pos=0, @parent_file=#<Gitlab::Diff::File:0x00007fee864966b0 ...>, @rich_text=nil, @line_code="6adfb183a4a2c94a2f92dab5ade762a47889a5a1_0_10">, #<Gitlab::Diff::Line:0x00007fee806fc3d8 @index=10, @type="new", @text="+", @new_pos=11, @old_pos=0, @parent_file=#<Gitlab::Diff::File:0x00007fee864966b0 ...>, @rich_text=nil, @line_code="6adfb183a4a2c94a2f92dab5ade762a47889a5a1_0_11">, #<Gitlab::Diff::Line:0x00007fee806fc0e0 @index=11, @type="new", @text="+1. Push a new file on a new branch to [gitlab-org/gitlab-test](https://gitlab.com/gitlab-org/gitlab-test).", @new_pos=12, @old_pos=0, @parent_file=#<Gitlab::Diff::File:0x00007fee864966b0 ...>, @rich_text=nil, @line_code="6adfb183a4a2c94a2f92dab5ade762a47889a5a1_0_12">, #<Gitlab::Diff::Line:0x00007fee80703db8 @index=12, @type="new", @text="+2. Execute `rm -rf tmp/tests` in your gitlab repo.", @new_pos=13, @old_pos=0, @parent_file=#<Gitlab::Diff::File:0x00007fee864966b0 ...>, @rich_text=nil, @line_code="6adfb183a4a2c94a2f92dab5ade762a47889a5a1_0_13">, #<Gitlab::Diff::Line:0x00007fee80703ae8 @index=13, @type="new", @text="+3. Add your branch and its head commit to the `BRANCH_SHA` hash in [`test_env.rb`](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/spec/support/helpers/test_env.rb#L10-68).", @new_pos=14, @old_pos=0, @parent_file=#<Gitlab::Diff::File:0x00007fee864966b0 ...>, @rich_text=nil, @line_code="6adfb183a4a2c94a2f92dab5ade762a47889a5a1_0_14">, #<Gitlab::Diff::Line:0x00007fee80703778 @index=14, @type="new", @text="+", @new_pos=15, @old_pos=0, @parent_file=#<Gitlab::Diff::File:0x00007fee864966b0 ...>, @rich_text=nil, @line_code="6adfb183a4a2c94a2f92dab5ade762a47889a5a1_0_15">, #<Gitlab::Diff::Line:0x00007fee807033b8 @index=15, @type="new", @text="+In rspec, you can use `create(:project)` to create an instance of `gitlab-test` that has a `path` of `gitlabhq`.", @new_pos=16, @old_pos=0, @parent_file=#<Gitlab::Diff::File:0x00007fee864966b0 ...>, @rich_text=nil, @line_code="6adfb183a4a2c94a2f92dab5ade762a47889a5a1_0_16">], @new_blob=nil, @old_blob=nil>

Notice new_blob and old_blob are nil, so the can_render_blob? call in https://gitlab.com/gitlab-org/gitlab-ce/blob/e688f3db660e783a64ba7aadee13238ceb84f595/app/models/diff_viewer/base.rb#L50 is returning true.

However, Diff:ImageViewer is erroneously trying to render this blob as an image. @DouweM Do you know the answer to these questions?

1. Why is it sufficient for ImageViewer that blob.nil? is true? I'd expect we'd want to check the file extensions and types as well. Or at least we should check that at least one of the blobs is not nil.
2. Is new_blob and old_blob nil in the first place? I added some debugging to BlobSnitcher and validated that the data in the README.md wasn't considered binary.

added Create [DEPRECATED] devopscreate labels

1. Why is it sufficient for ImageViewer that blob.nil? is true? I'd expect we'd want to check the file extensions and types as well.

@stanhu Since can_render_blob?(blob) is only used from can_render?(diff_file), the idea was that if a diff file represents a creation (old_blob nil, new_blob set) or a deletion (new_blob nil, old_blob set), we only need to look at the extension/type of the other blob, while we look at both for regular changes or renames that have both set.

As you've identified, this only works if we can assume that at least one of old_blob and new_blob will always be present.

Or at least we should check that at least one of the blobs is not nil.

Yep, I think we should. Diff files that have neither old_blob or new_blob are really invalid, but that shouldn't trip up the diff viewer the way it has here.

1. Is new_blob and old_blob nil in the first place? I added some debugging to BlobSnitcher and validated that the data in the README.md wasn't considered binary.

@stanhu Could the issue be that Gitlab::Diff::File#old_blob or Gitlab::Diff::File#new_blob tries to look up the diff file's blob by path, but gets no result back from Gitlab::GitalyClient::BlobService#get_blobs because Gitaly doesn't handle the invalid UTF8 in the path correctly?

@zj-gitlab What do you think?

It starts to feel much more like an encoding issue where Gitlab::EncodingHelper.encode! needs to be used before the conversion to bytes by the gRPC layer. I've got no time today to investigate, maybe I'll take a look tomorrow to narrow it down further.

added groupsource code + 1 deleted label

@zj-gitlab Since this is starting to look like an issue at the gitlab-ce~11839077 layer, I'm removing the Create and gitlab-ce10309854 labels. Let me know if you think that's unwarranted :)

removed Create [DEPRECATED] groupsource code labels

Setting gitlab-ce10309854 based on gitlab-ce3103452.

added groupsource code label

added [deprecated] Accepting merge requests label

removed [deprecated] Accepting merge requests label

added groupgitaly label and removed groupsource code + 1 deleted label

mentioned in issue #31996 (closed)

mentioned in issue #32511 (closed)

mentioned in issue #32984 (closed)

mentioned in issue #33511 (closed)

mentioned in issue #33968 (closed)

mentioned in issue #34475 (closed)

mentioned in issue #35039 (closed)

Setting Category:Gitaly based on groupgitaly.

added Category:Gitaly label

mentioned in issue #35492 (closed)

mentioned in issue #35986 (closed)

mentioned in issue #36577 (closed)

changed milestone to %Backlog

added priority4 severity4 labels

added [deprecated] Accepting merge requests label

added sectiondev label

added vintage label

Hi @jobbot ,

Thanks for raising this bug.

Contributions like this are vital to help make GitLab a better product.

We would be grateful for your help in verifying whether your bug report requires further attention from the team. If you think this bug still exists, and is reproducible with the latest stable version of GitLab, please comment on this issue.

This issue has been inactive for more than 12 months now and based on the policy for inactive bugs, will be closed in 7 days.

Thanks for your contributions to make GitLab better!

added stale label

Hi @jobbot ,

Based on the policy for inactive bugs, this issue is now being closed.

If you think this bug still exists, and is reproducible with the latest stable version of GitLab, please reopen this issue.

Thanks for your contributions to make GitLab better!

closed

added auto closed label