diff options
-rw-r--r-- | app/assets/javascripts/snippets/components/show.vue | 13 | ||||
-rw-r--r-- | app/assets/javascripts/snippets/mixins/snippets.js | 1 | ||||
-rw-r--r-- | app/graphql/queries/snippet/snippet.query.graphql | 1 | ||||
-rw-r--r-- | app/graphql/queries/snippet/snippet_blob_content.query.graphql | 1 | ||||
-rw-r--r-- | app/graphql/resolvers/snippets/blobs_resolver.rb | 24 | ||||
-rw-r--r-- | app/graphql/types/snippets/blob_connection_type.rb | 16 | ||||
-rw-r--r-- | app/graphql/types/snippets/blob_type.rb | 2 | ||||
-rw-r--r-- | app/models/snippet.rb | 10 | ||||
-rw-r--r-- | config/initializers/action_mailer_hooks.rb | 1 | ||||
-rw-r--r-- | doc/api/graphql/reference/index.md | 1 | ||||
-rw-r--r-- | lib/gitlab/email/hook/validate_addresses_interceptor.rb | 32 | ||||
-rw-r--r-- | locale/gitlab.pot | 3 | ||||
-rw-r--r-- | spec/frontend/snippets/components/show_spec.js | 19 | ||||
-rw-r--r-- | spec/graphql/resolvers/snippets/blobs_resolver_spec.rb | 13 | ||||
-rw-r--r-- | spec/lib/gitlab/email/hook/validate_addresses_interceptor_spec.rb | 52 | ||||
-rw-r--r-- | spec/models/snippet_spec.rb | 34 |
16 files changed, 204 insertions, 19 deletions
diff --git a/app/assets/javascripts/snippets/components/show.vue b/app/assets/javascripts/snippets/components/show.vue index 35d88d5ec8e..ee8b00c1f5d 100644 --- a/app/assets/javascripts/snippets/components/show.vue +++ b/app/assets/javascripts/snippets/components/show.vue @@ -1,5 +1,5 @@ <script> -import { GlLoadingIcon } from '@gitlab/ui'; +import { GlAlert, GlLoadingIcon } from '@gitlab/ui'; import eventHub from '~/blob/components/eventhub'; import { SNIPPET_MARK_VIEW_APP_START, @@ -23,6 +23,7 @@ export default { EmbedDropdown, SnippetHeader, SnippetTitle, + GlAlert, GlLoadingIcon, SnippetBlob, CloneDropdownButton, @@ -35,6 +36,9 @@ export default { canBeCloned() { return Boolean(this.snippet.sshUrlToRepo || this.snippet.httpUrlToRepo); }, + hasUnretrievableBlobs() { + return this.snippet.hasUnretrievableBlobs; + }, }, beforeCreate() { performanceMarkAndMeasure({ mark: SNIPPET_MARK_VIEW_APP_START }); @@ -66,6 +70,13 @@ export default { data-qa-selector="clone_button" /> </div> + <gl-alert v-if="hasUnretrievableBlobs" variant="danger" class="gl-mb-3" :dismissible="false"> + {{ + __( + 'WARNING: This snippet contains hidden files which might be used to mask malicious behavior. Exercise caution if cloning and executing code from this snippet.', + ) + }} + </gl-alert> <snippet-blob v-for="blob in blobs" :key="blob.path" diff --git a/app/assets/javascripts/snippets/mixins/snippets.js b/app/assets/javascripts/snippets/mixins/snippets.js index b72befef56b..0b3cca4e53a 100644 --- a/app/assets/javascripts/snippets/mixins/snippets.js +++ b/app/assets/javascripts/snippets/mixins/snippets.js @@ -17,6 +17,7 @@ export const getSnippetMixin = { // Set `snippet.blobs` since some child components are coupled to this. if (!isEmpty(res)) { + res.hasUnretrievableBlobs = res.blobs?.hasUnretrievableBlobs || false; // It's possible for us to not get any blobs in a response. // In this case, we should default to current blobs. res.blobs = res.blobs ? res.blobs.nodes : blobsDefault; diff --git a/app/graphql/queries/snippet/snippet.query.graphql b/app/graphql/queries/snippet/snippet.query.graphql index 24b268ec853..5c0c7ebaa1b 100644 --- a/app/graphql/queries/snippet/snippet.query.graphql +++ b/app/graphql/queries/snippet/snippet.query.graphql @@ -15,6 +15,7 @@ query GetSnippetQuery($ids: [SnippetID!]) { sshUrlToRepo blobs { __typename + hasUnretrievableBlobs nodes { __typename binary diff --git a/app/graphql/queries/snippet/snippet_blob_content.query.graphql b/app/graphql/queries/snippet/snippet_blob_content.query.graphql index 005f42ff726..4459a5e4316 100644 --- a/app/graphql/queries/snippet/snippet_blob_content.query.graphql +++ b/app/graphql/queries/snippet/snippet_blob_content.query.graphql @@ -12,6 +12,7 @@ query SnippetBlobContent($ids: [ID!], $rich: Boolean!, $paths: [String!]) { richData @include(if: $rich) plainData @skip(if: $rich) } + hasUnretrievableBlobs } } } diff --git a/app/graphql/resolvers/snippets/blobs_resolver.rb b/app/graphql/resolvers/snippets/blobs_resolver.rb index cbbc65d7263..29716ce1394 100644 --- a/app/graphql/resolvers/snippets/blobs_resolver.rb +++ b/app/graphql/resolvers/snippets/blobs_resolver.rb @@ -19,18 +19,18 @@ module Resolvers def resolve(paths: []) return [snippet.blob] if snippet.empty_repo? - if paths.empty? - snippet.blobs - else - snippet.repository.blobs_at(transformed_blob_paths(paths)) - end - end - - private - - def transformed_blob_paths(paths) - ref = snippet.default_branch - paths.map { |path| [ref, path] } + paths = snippet.all_files if paths.empty? + blobs = snippet.blobs(paths) + + # TODO: Some blobs, e.g. those with non-utf8 filenames, are returned as nil from the + # repository. We need to provide a flag to notify the user of this until we come up with a + # way to retrieve and display these blobs. We will be exploring a more holistic solution for + # this general problem of making all blobs retrievable as part + # of https://gitlab.com/gitlab-org/gitlab/-/issues/323082, at which point this attribute may + # be removed. + context[:unretrievable_blobs?] = blobs.size < paths.size + + blobs end end end diff --git a/app/graphql/types/snippets/blob_connection_type.rb b/app/graphql/types/snippets/blob_connection_type.rb new file mode 100644 index 00000000000..15d26af7374 --- /dev/null +++ b/app/graphql/types/snippets/blob_connection_type.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module Types + module Snippets + # rubocop: disable Graphql/AuthorizeTypes + class BlobConnectionType < GraphQL::Types::Relay::BaseConnection + field :has_unretrievable_blobs, GraphQL::Types::Boolean, null: false, + description: 'Indicates if the snippet has unretrievable blobs.', + resolver_method: :unretrievable_blobs? + + def unretrievable_blobs? + !!context[:unretrievable_blobs?] + end + end + end +end diff --git a/app/graphql/types/snippets/blob_type.rb b/app/graphql/types/snippets/blob_type.rb index 2b9b76a6194..80702c71f63 100644 --- a/app/graphql/types/snippets/blob_type.rb +++ b/app/graphql/types/snippets/blob_type.rb @@ -8,6 +8,8 @@ module Types description 'Represents the snippet blob' present_using SnippetBlobPresenter + connection_type_class(Types::Snippets::BlobConnectionType) + field :rich_data, GraphQL::Types::String, description: 'Blob highlighted data.', null: true diff --git a/app/models/snippet.rb b/app/models/snippet.rb index 6a8123b3c08..b04fca64c87 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -237,15 +237,19 @@ class Snippet < ApplicationRecord end end + def all_files + list_files(default_branch) + end + def blob @blob ||= Blob.decorate(SnippetBlob.new(self), self) end - def blobs + def blobs(paths = []) return [] unless repository_exists? - files = list_files(default_branch) - items = files.map { |file| [default_branch, file] } + paths = all_files if paths.empty? + items = paths.map { |path| [default_branch, path] } repository.blobs_at(items).compact end diff --git a/config/initializers/action_mailer_hooks.rb b/config/initializers/action_mailer_hooks.rb index 46d5e387d9d..fb09ed34bf6 100644 --- a/config/initializers/action_mailer_hooks.rb +++ b/config/initializers/action_mailer_hooks.rb @@ -8,6 +8,7 @@ end ActionMailer::Base.register_interceptors( ::Gitlab::Email::Hook::AdditionalHeadersInterceptor, ::Gitlab::Email::Hook::EmailTemplateInterceptor, + ::Gitlab::Email::Hook::ValidateAddressesInterceptor, ::Gitlab::Email::Hook::DeliveryMetricsObserver ) diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 18c99b7d151..49c0361123e 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -7841,6 +7841,7 @@ The connection type for [`SnippetBlob`](#snippetblob). | Name | Type | Description | | ---- | ---- | ----------- | | <a id="snippetblobconnectionedges"></a>`edges` | [`[SnippetBlobEdge]`](#snippetblobedge) | A list of edges. | +| <a id="snippetblobconnectionhasunretrievableblobs"></a>`hasUnretrievableBlobs` | [`Boolean!`](#boolean) | Indicates if the snippet has unretrievable blobs. | | <a id="snippetblobconnectionnodes"></a>`nodes` | [`[SnippetBlob]`](#snippetblob) | A list of nodes. | | <a id="snippetblobconnectionpageinfo"></a>`pageInfo` | [`PageInfo!`](#pageinfo) | Information to aid in pagination. | diff --git a/lib/gitlab/email/hook/validate_addresses_interceptor.rb b/lib/gitlab/email/hook/validate_addresses_interceptor.rb new file mode 100644 index 00000000000..e63f047e63d --- /dev/null +++ b/lib/gitlab/email/hook/validate_addresses_interceptor.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module Gitlab + module Email + module Hook + # Check for unsafe characters in the envelope-from and -to addresses. + # These are passed directly as arguments to sendmail and are liable to shell injection attacks: + # https://github.com/mikel/mail/blob/2.7.1/lib/mail/network/delivery_methods/sendmail.rb#L53-L58 + class ValidateAddressesInterceptor + UNSAFE_CHARACTERS = /(\\|[^[:print:]])/.freeze + + def self.delivering_email(message) + addresses = Array(message.smtp_envelope_from) + Array(message.smtp_envelope_to) + + addresses.each do |address| + next unless address.match?(UNSAFE_CHARACTERS) + + Gitlab::AuthLogger.info( + message: 'Skipping email with unsafe characters in address', + address: address, + subject: message.subject + ) + + message.perform_deliveries = false + + break + end + end + end + end + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 37f8a3fd7b4..6cdc5ba18e2 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -40768,6 +40768,9 @@ msgstr "" msgid "WARNING:" msgstr "" +msgid "WARNING: This snippet contains hidden files which might be used to mask malicious behavior. Exercise caution if cloning and executing code from this snippet." +msgstr "" + msgid "Wait for the file to load to copy its contents" msgstr "" diff --git a/spec/frontend/snippets/components/show_spec.js b/spec/frontend/snippets/components/show_spec.js index af61f4ea54f..c73bf8f80a2 100644 --- a/spec/frontend/snippets/components/show_spec.js +++ b/spec/frontend/snippets/components/show_spec.js @@ -1,4 +1,4 @@ -import { GlLoadingIcon } from '@gitlab/ui'; +import { GlLoadingIcon, GlAlert } from '@gitlab/ui'; import { shallowMount } from '@vue/test-utils'; import { Blob, BinaryBlob } from 'jest/blob/components/mock_data'; import EmbedDropdown from '~/snippets/components/embed_dropdown.vue'; @@ -106,6 +106,23 @@ describe('Snippet view app', () => { }); }); + describe('hasUnretrievableBlobs alert rendering', () => { + it.each` + hasUnretrievableBlobs | condition | isRendered + ${false} | ${'not render'} | ${false} + ${true} | ${'render'} | ${true} + `('does $condition gl-alert by default', ({ hasUnretrievableBlobs, isRendered }) => { + createComponent({ + data: { + snippet: { + hasUnretrievableBlobs, + }, + }, + }); + expect(wrapper.findComponent(GlAlert).exists()).toBe(isRendered); + }); + }); + describe('Clone button rendering', () => { it.each` httpUrlToRepo | sshUrlToRepo | shouldRender | isRendered diff --git a/spec/graphql/resolvers/snippets/blobs_resolver_spec.rb b/spec/graphql/resolvers/snippets/blobs_resolver_spec.rb index ebe286769cf..b5db7ac5748 100644 --- a/spec/graphql/resolvers/snippets/blobs_resolver_spec.rb +++ b/spec/graphql/resolvers/snippets/blobs_resolver_spec.rb @@ -13,11 +13,14 @@ RSpec.describe Resolvers::Snippets::BlobsResolver do let_it_be(:current_user) { create(:user) } let_it_be(:snippet) { create(:personal_snippet, :private, :repository, author: current_user) } + let(:query_context) { {} } + context 'when user is not authorized' do let(:other_user) { create(:user) } it 'redacts the field' do expect(resolve_blobs(snippet, user: other_user)).to be_nil + expect(query_context[:unretrievable_blobs?]).to eq(false) end end @@ -28,6 +31,7 @@ RSpec.describe Resolvers::Snippets::BlobsResolver do expect(result).to match_array(snippet.list_files.map do |file| have_attributes(path: file) end) + expect(query_context[:unretrievable_blobs?]).to eq(false) end end @@ -37,12 +41,14 @@ RSpec.describe Resolvers::Snippets::BlobsResolver do path = 'CHANGELOG' expect(resolve_blobs(snippet, paths: [path])).to contain_exactly(have_attributes(path: path)) + expect(query_context[:unretrievable_blobs?]).to eq(false) end end context 'the argument does not match anything' do it 'returns an empty result' do expect(resolve_blobs(snippet, paths: ['does not exist'])).to be_empty + expect(query_context[:unretrievable_blobs?]).to eq(true) end end @@ -53,12 +59,15 @@ RSpec.describe Resolvers::Snippets::BlobsResolver do expect(resolve_blobs(snippet, paths: paths)).to match_array(paths.map do |file| have_attributes(path: file) end) + expect(query_context[:unretrievable_blobs?]).to eq(false) end end end end - def resolve_blobs(snippet, user: current_user, paths: [], args: { paths: paths }) - resolve(described_class, args: args, ctx: { current_user: user }, obj: snippet) + def resolve_blobs(snippet, user: current_user, paths: [], args: { paths: paths }, has_unretrievable_blobs: false) + query_context[:current_user] = user + query_context[:unretrievable_blobs?] = has_unretrievable_blobs + resolve(described_class, args: args, ctx: query_context, obj: snippet) end end diff --git a/spec/lib/gitlab/email/hook/validate_addresses_interceptor_spec.rb b/spec/lib/gitlab/email/hook/validate_addresses_interceptor_spec.rb new file mode 100644 index 00000000000..a3f0158db40 --- /dev/null +++ b/spec/lib/gitlab/email/hook/validate_addresses_interceptor_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Email::Hook::ValidateAddressesInterceptor do + describe 'UNSAFE_CHARACTERS' do + subject { described_class::UNSAFE_CHARACTERS } + + it { is_expected.to match('\\') } + it { is_expected.to match("\x00") } + it { is_expected.to match("\x01") } + it { is_expected.not_to match('') } + it { is_expected.not_to match('user@example.com') } + it { is_expected.not_to match('foo-123+bar_456@example.com') } + end + + describe '.delivering_email' do + let(:mail) do + ActionMailer::Base.mail(to: 'test@mail.com', from: 'info@mail.com', subject: 'title', body: 'hello') + end + + let(:unsafe_email) { "evil+\x01$HOME@example.com" } + + it 'sends emails to normal addresses' do + expect(Gitlab::AuthLogger).not_to receive(:info) + expect { mail.deliver_now }.to change(ActionMailer::Base.deliveries, :count) + end + + [:from, :to, :cc, :bcc].each do |header| + it "does not send emails if the #{header.inspect} header contains unsafe characters" do + mail[header] = unsafe_email + + expect(Gitlab::AuthLogger).to receive(:info).with( + message: 'Skipping email with unsafe characters in address', + address: unsafe_email, + subject: mail.subject + ) + + expect { mail.deliver_now }.not_to change(ActionMailer::Base.deliveries, :count) + end + end + + [:reply_to].each do |header| + it "sends emails if the #{header.inspect} header contains unsafe characters" do + mail[header] = unsafe_email + + expect(Gitlab::AuthLogger).not_to receive(:info) + expect { mail.deliver_now }.to change(ActionMailer::Base.deliveries, :count) + end + end + end +end diff --git a/spec/models/snippet_spec.rb b/spec/models/snippet_spec.rb index 5d4a78bb15f..92e4bc7d1a9 100644 --- a/spec/models/snippet_spec.rb +++ b/spec/models/snippet_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe Snippet do + include FakeBlobHelpers + describe 'modules' do subject { described_class } @@ -526,6 +528,21 @@ RSpec.describe Snippet do end end + describe '#all_files' do + let(:snippet) { create(:snippet, :repository) } + let(:files) { double(:files) } + + subject(:all_files) { snippet.all_files } + + before do + allow(snippet.repository).to receive(:ls_files).with(snippet.default_branch).and_return(files) + end + + it 'lists files from the repository with the default branch' do + expect(all_files).to eq(files) + end + end + describe '#blobs' do context 'when repository does not exist' do let(:snippet) { create(:snippet) } @@ -552,6 +569,23 @@ RSpec.describe Snippet do end end end + + context 'when some blobs are not retrievable from repository' do + let(:snippet) { create(:snippet, :repository) } + let(:container) { double(:container) } + let(:retrievable_filename) { 'retrievable_file'} + let(:unretrievable_filename) { 'unretrievable_file'} + + before do + allow(snippet).to receive(:list_files).and_return([retrievable_filename, unretrievable_filename]) + blob = fake_blob(path: retrievable_filename, container: container) + allow(snippet.repository).to receive(:blobs_at).and_return([blob, nil]) + end + + it 'does not include unretrievable blobs' do + expect(snippet.blobs.map(&:name)).to contain_exactly(retrievable_filename) + end + end end describe '#to_json' do |