Commit e5d7fa81 authored by Magdalena Frankiewicz's avatar Magdalena Frankiewicz

Remove Markdown support for bio field

Supporting Markdown in bio makes it vulnerable to abuse
which raises security concerns

Changelog: removed
parent 5a0ea85a
import Vue from 'vue'; import Vue from 'vue';
import { sanitize } from '~/lib/dompurify';
import UsersCache from './lib/utils/users_cache'; import UsersCache from './lib/utils/users_cache';
import UserPopover from './vue_shared/components/user_popover/user_popover.vue'; import UserPopover from './vue_shared/components/user_popover/user_popover.vue';
...@@ -41,7 +38,6 @@ const populateUserInfo = (user) => { ...@@ -41,7 +38,6 @@ const populateUserInfo = (user) => {
name: userData.name, name: userData.name,
location: userData.location, location: userData.location,
bio: userData.bio, bio: userData.bio,
bioHtml: sanitize(userData.bio_html),
workInformation: userData.work_information, workInformation: userData.work_information,
websiteUrl: userData.website_url, websiteUrl: userData.website_url,
pronouns: userData.pronouns, pronouns: userData.pronouns,
......
...@@ -82,11 +82,7 @@ export default { ...@@ -82,11 +82,7 @@ export default {
<div class="gl-text-gray-500"> <div class="gl-text-gray-500">
<div v-if="user.bio" class="gl-display-flex gl-mb-2"> <div v-if="user.bio" class="gl-display-flex gl-mb-2">
<gl-icon name="profile" class="gl-text-gray-400 gl-flex-shrink-0" /> <gl-icon name="profile" class="gl-text-gray-400 gl-flex-shrink-0" />
<span <span ref="bio" class="gl-ml-2 gl-overflow-hidden">{{ user.bio }}</span>
ref="bio"
class="gl-ml-2 gl-overflow-hidden"
v-html="user.bioHtml /* eslint-disable-line vue/no-v-html */"
></span>
</div> </div>
<div v-if="user.workInformation" class="gl-display-flex gl-mb-2"> <div v-if="user.workInformation" class="gl-display-flex gl-mb-2">
<gl-icon name="work" class="gl-text-gray-400 gl-flex-shrink-0" /> <gl-icon name="work" class="gl-text-gray-400 gl-flex-shrink-0" />
......
...@@ -316,7 +316,7 @@ class User < ApplicationRecord ...@@ -316,7 +316,7 @@ class User < ApplicationRecord
delegate :path, to: :namespace, allow_nil: true, prefix: true delegate :path, to: :namespace, allow_nil: true, prefix: true
delegate :job_title, :job_title=, to: :user_detail, allow_nil: true delegate :job_title, :job_title=, to: :user_detail, allow_nil: true
delegate :other_role, :other_role=, to: :user_detail, allow_nil: true delegate :other_role, :other_role=, to: :user_detail, allow_nil: true
delegate :bio, :bio=, :bio_html, to: :user_detail, allow_nil: true delegate :bio, :bio=, to: :user_detail, allow_nil: true
delegate :webauthn_xid, :webauthn_xid=, to: :user_detail, allow_nil: true delegate :webauthn_xid, :webauthn_xid=, to: :user_detail, allow_nil: true
delegate :pronouns, :pronouns=, to: :user_detail, allow_nil: true delegate :pronouns, :pronouns=, to: :user_detail, allow_nil: true
delegate :pronunciation, :pronunciation=, to: :user_detail, allow_nil: true delegate :pronunciation, :pronunciation=, to: :user_detail, allow_nil: true
......
...@@ -2,7 +2,8 @@ ...@@ -2,7 +2,8 @@
class UserDetail < ApplicationRecord class UserDetail < ApplicationRecord
extend ::Gitlab::Utils::Override extend ::Gitlab::Utils::Override
include CacheMarkdownField include IgnorableColumns
ignore_columns %i[bio_html cached_markdown_version], remove_with: '13.6', remove_after: '2021-10-22'
belongs_to :user belongs_to :user
...@@ -13,20 +14,6 @@ class UserDetail < ApplicationRecord ...@@ -13,20 +14,6 @@ class UserDetail < ApplicationRecord
before_save :prevent_nil_bio before_save :prevent_nil_bio
cache_markdown_field :bio
def bio_html
read_attribute(:bio_html) || bio
end
# For backward compatibility.
# Older migrations (and their tests) reference the `User.migration_bot` where the `bio` attribute is set.
# Here we disable writing the markdown cache when the `bio_html` column does not exist.
override :invalidated_markdown_cache?
def invalidated_markdown_cache?
self.class.column_names.include?('bio_html') && super
end
private private
def prevent_nil_bio def prevent_nil_bio
......
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
- @hide_breadcrumbs = true - @hide_breadcrumbs = true
- @no_container = true - @no_container = true
- page_title user_display_name(@user) - page_title user_display_name(@user)
- page_description @user.bio_html - page_description @user.bio
- header_title @user.name, user_path(@user) - header_title @user.name, user_path(@user)
- page_itemtype 'http://schema.org/Person' - page_itemtype 'http://schema.org/Person'
- link_classes = "flex-grow-1 mx-1 " - link_classes = "flex-grow-1 mx-1 "
...@@ -127,7 +127,7 @@ ...@@ -127,7 +127,7 @@
- if @user.bio.present? - if @user.bio.present?
.gl-text-gray-900 .gl-text-gray-900
.profile-user-bio .profile-user-bio
= markdown(@user.bio_html) = @user.bio
- unless profile_tabs.empty? - unless profile_tabs.empty?
......
...@@ -124,7 +124,6 @@ GET /users ...@@ -124,7 +124,6 @@ GET /users
"created_at": "2012-05-23T08:00:58Z", "created_at": "2012-05-23T08:00:58Z",
"is_admin": false, "is_admin": false,
"bio": "", "bio": "",
"bio_html": "",
"location": null, "location": null,
"skype": "", "skype": "",
"linkedin": "", "linkedin": "",
...@@ -164,7 +163,6 @@ GET /users ...@@ -164,7 +163,6 @@ GET /users
"created_at": "2012-05-23T08:01:01Z", "created_at": "2012-05-23T08:01:01Z",
"is_admin": false, "is_admin": false,
"bio": "", "bio": "",
"bio_html": "",
"location": null, "location": null,
"skype": "", "skype": "",
"linkedin": "", "linkedin": "",
...@@ -283,7 +281,6 @@ Parameters: ...@@ -283,7 +281,6 @@ Parameters:
"web_url": "http://localhost:3000/john_smith", "web_url": "http://localhost:3000/john_smith",
"created_at": "2012-05-23T08:00:58Z", "created_at": "2012-05-23T08:00:58Z",
"bio": "", "bio": "",
"bio_html": "",
"bot": false, "bot": false,
"location": null, "location": null,
"public_email": "john@example.com", "public_email": "john@example.com",
...@@ -322,7 +319,6 @@ Example Responses: ...@@ -322,7 +319,6 @@ Example Responses:
"created_at": "2012-05-23T08:00:58Z", "created_at": "2012-05-23T08:00:58Z",
"is_admin": false, "is_admin": false,
"bio": "", "bio": "",
"bio_html": "",
"location": null, "location": null,
"public_email": "john@example.com", "public_email": "john@example.com",
"skype": "", "skype": "",
...@@ -551,7 +547,6 @@ GET /user ...@@ -551,7 +547,6 @@ GET /user
"web_url": "http://localhost:3000/john_smith", "web_url": "http://localhost:3000/john_smith",
"created_at": "2012-05-23T08:00:58Z", "created_at": "2012-05-23T08:00:58Z",
"bio": "", "bio": "",
"bio_html": "",
"location": null, "location": null,
"public_email": "john@example.com", "public_email": "john@example.com",
"skype": "", "skype": "",
...@@ -601,7 +596,6 @@ GET /user ...@@ -601,7 +596,6 @@ GET /user
"created_at": "2012-05-23T08:00:58Z", "created_at": "2012-05-23T08:00:58Z",
"is_admin": false, "is_admin": false,
"bio": "", "bio": "",
"bio_html": "",
"location": null, "location": null,
"public_email": "john@example.com", "public_email": "john@example.com",
"skype": "", "skype": "",
......
...@@ -4,8 +4,10 @@ module API ...@@ -4,8 +4,10 @@ module API
module Entities module Entities
class User < UserBasic class User < UserBasic
include UsersHelper include UsersHelper
include ActionView::Helpers::SanitizeHelper
expose :created_at, if: ->(user, opts) { Ability.allowed?(opts[:current_user], :read_user_profile, user) } expose :created_at, if: ->(user, opts) { Ability.allowed?(opts[:current_user], :read_user_profile, user) }
expose :bio, :bio_html, :location, :public_email, :skype, :linkedin, :twitter, :website_url, :organization, :job_title, :pronouns expose :bio, :location, :public_email, :skype, :linkedin, :twitter, :website_url, :organization, :job_title, :pronouns
expose :bot?, as: :bot expose :bot?, as: :bot
expose :work_information do |user| expose :work_information do |user|
work_information(user) work_information(user)
...@@ -16,6 +18,12 @@ module API ...@@ -16,6 +18,12 @@ module API
expose :following, if: ->(user, opts) { Ability.allowed?(opts[:current_user], :read_user_profile, user) } do |user| expose :following, if: ->(user, opts) { Ability.allowed?(opts[:current_user], :read_user_profile, user) } do |user|
user.followees.size user.followees.size
end end
# This is only for multi version compatibility reasons, as we removed user.bio_html
# to be removed in 14.4
expose :bio_html do |user|
strip_tags(user.bio)
end
end end
end end
end end
...@@ -45,7 +45,6 @@ RSpec.describe 'User edit profile' do ...@@ -45,7 +45,6 @@ RSpec.describe 'User edit profile' do
twitter: 'testtwitter', twitter: 'testtwitter',
website_url: 'http://testurl.com', website_url: 'http://testurl.com',
bio: 'I <3 GitLab :tada:', bio: 'I <3 GitLab :tada:',
bio_html: '<p data-sourcepos="1:1-1:18" dir="auto">I &lt;3 GitLab <gl-emoji title="party popper" data-name="tada" data-unicode-version="6.0">🎉</gl-emoji></p>',
job_title: 'Frontend Engineer', job_title: 'Frontend Engineer',
organization: 'GitLab' organization: 'GitLab'
) )
......
...@@ -5,7 +5,7 @@ require 'spec_helper' ...@@ -5,7 +5,7 @@ require 'spec_helper'
RSpec.describe 'User page' do RSpec.describe 'User page' do
include ExternalAuthorizationServiceHelpers include ExternalAuthorizationServiceHelpers
let_it_be(:user) { create(:user, bio: '**Lorem** _ipsum_ dolor sit [amet](https://example.com)') } let_it_be(:user) { create(:user, bio: '<b>Lorem</b> <i>ipsum</i> dolor sit <a href="https://example.com">amet</a>') }
subject(:visit_profile) { visit(user_path(user)) } subject(:visit_profile) { visit(user_path(user)) }
......
...@@ -94,7 +94,7 @@ describe('User Popover Component', () => { ...@@ -94,7 +94,7 @@ describe('User Popover Component', () => {
const bio = 'My super interesting bio'; const bio = 'My super interesting bio';
it('should show only bio if work information is not available', () => { it('should show only bio if work information is not available', () => {
const user = { ...DEFAULT_PROPS.user, bio, bioHtml: bio }; const user = { ...DEFAULT_PROPS.user, bio };
createWrapper({ user }); createWrapper({ user });
...@@ -117,7 +117,6 @@ describe('User Popover Component', () => { ...@@ -117,7 +117,6 @@ describe('User Popover Component', () => {
const user = { const user = {
...DEFAULT_PROPS.user, ...DEFAULT_PROPS.user,
bio, bio,
bioHtml: bio,
workInformation: 'Frontend Engineer at GitLab', workInformation: 'Frontend Engineer at GitLab',
}; };
...@@ -127,16 +126,15 @@ describe('User Popover Component', () => { ...@@ -127,16 +126,15 @@ describe('User Popover Component', () => {
expect(findWorkInformation().text()).toBe('Frontend Engineer at GitLab'); expect(findWorkInformation().text()).toBe('Frontend Engineer at GitLab');
}); });
it('should not encode special characters in bio', () => { it('should encode special characters in bio', () => {
const user = { const user = {
...DEFAULT_PROPS.user, ...DEFAULT_PROPS.user,
bio: 'I like CSS', bio: 'I like <b>CSS</b>',
bioHtml: 'I like <b>CSS</b>',
}; };
createWrapper({ user }); createWrapper({ user });
expect(findBio().html()).toContain('I like <b>CSS</b>'); expect(findBio().html()).toContain('I like &lt;b&gt;CSS&lt;/b&gt;');
}); });
it('shows icon for bio', () => { it('shows icon for bio', () => {
......
...@@ -25,29 +25,4 @@ RSpec.describe UserDetail do ...@@ -25,29 +25,4 @@ RSpec.describe UserDetail do
it { is_expected.to validate_length_of(:bio).is_at_most(255) } it { is_expected.to validate_length_of(:bio).is_at_most(255) }
end end
end end
describe '#bio_html' do
let(:user) { create(:user, bio: 'some **bio**') }
subject { user.user_detail.bio_html }
it 'falls back to #bio when the html representation is missing' do
user.user_detail.update!(bio_html: nil)
expect(subject).to eq(user.user_detail.bio)
end
it 'stores rendered html' do
expect(subject).to include('some <strong>bio</strong>')
end
it 'does not try to set the value when the column is not there' do
without_bio_html_column = UserDetail.column_names - ['bio_html']
expect(described_class).to receive(:column_names).at_least(:once).and_return(without_bio_html_column)
expect(user.user_detail).not_to receive(:bio_html=)
subject
end
end
end end
...@@ -82,7 +82,6 @@ RSpec.describe User do ...@@ -82,7 +82,6 @@ RSpec.describe User do
it { is_expected.to delegate_method(:bio).to(:user_detail).allow_nil } it { is_expected.to delegate_method(:bio).to(:user_detail).allow_nil }
it { is_expected.to delegate_method(:bio=).to(:user_detail).with_arguments(:args).allow_nil } it { is_expected.to delegate_method(:bio=).to(:user_detail).with_arguments(:args).allow_nil }
it { is_expected.to delegate_method(:bio_html).to(:user_detail).allow_nil }
end end
describe 'associations' do describe 'associations' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment