Commit b423fe90 authored by Luke Duncalfe's avatar Luke Duncalfe Committed by Mayra Cabrera

Add migrations to limit Design filename

This change adds two post-migrations that enforce a character limit
for design_management_designs.filename of 255.

The first post-migration adds the limit to the filename column.

The second post-migration modifies any design filenames larger than 255
characters to the string "gitlab-modified-#{id}".

This will ensure the filename remains valid (by having a valid image
extension .jpg and being unique).

Any records modified by this data migration will appear as a broken
image when viewed within the GitLab app, as filenames are used to look
up the design file from the design git repository.

However, this is an intentional consequence given the 255 character
limit was selected because that is the largest filename that most
filesystems support, and also because GitLab.com has 6 records with
filenames larger than this but they are all abuse.

https://gitlab.com/gitlab-org/gitlab/-/issues/209025
parent a6b8ec46
---
title: Add database migrations to design_management_designs.filename to enforce
a 255 character limit, and modify any filenames that exceed that limit
merge_request: 33565
author:
type: changed
# frozen_string_literal: true
class AddLimitToDesignsFilename < ActiveRecord::Migration[6.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
def up
add_text_limit(:design_management_designs, :filename, 255, validate: false)
end
def down
remove_text_limit(:design_management_designs, :filename)
end
end
# frozen_string_literal: true
class CapDesignsFilenameLengthToNewLimit < ActiveRecord::Migration[6.0]
DOWNTIME = false
CHAR_LENGTH = 255
MODIFIED_NAME = 'gitlab-modified-'
MODIFIED_EXTENSION = '.jpg'
def up
arel_table = Arel::Table.new(:design_management_designs)
# Design filenames larger than the limit will be renamed to "gitlab-modified-{id}.jpg",
# which will be valid and unique. The design file itself will appear broken, as it is
# understood that no designs with filenames that exceed this limit will be legitimate.
# See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/33565/diffs#note_355789080
new_value_clause = Arel::Nodes::NamedFunction.new(
'CONCAT',
[
Arel::Nodes.build_quoted(MODIFIED_NAME),
arel_table[:id],
Arel::Nodes.build_quoted(MODIFIED_EXTENSION)
]
)
where_clause = Arel::Nodes::NamedFunction.new(
'CHAR_LENGTH',
[arel_table[:filename]]
).gt(CHAR_LENGTH)
update_arel = Arel::UpdateManager.new.table(arel_table)
.set([[arel_table[:filename], new_value_clause]])
.where(where_clause)
ActiveRecord::Base.connection.execute(update_arel.to_sql)
end
def down
# no-op : the original filename is lost forever
end
end
...@@ -8121,6 +8121,9 @@ ALTER TABLE ONLY public.chat_names ...@@ -8121,6 +8121,9 @@ ALTER TABLE ONLY public.chat_names
ALTER TABLE ONLY public.chat_teams ALTER TABLE ONLY public.chat_teams
ADD CONSTRAINT chat_teams_pkey PRIMARY KEY (id); ADD CONSTRAINT chat_teams_pkey PRIMARY KEY (id);
ALTER TABLE public.design_management_designs
ADD CONSTRAINT check_07155e2715 CHECK ((char_length((filename)::text) <= 255)) NOT VALID;
ALTER TABLE public.ci_job_artifacts ALTER TABLE public.ci_job_artifacts
ADD CONSTRAINT check_27f0f6dbab CHECK ((file_store IS NOT NULL)) NOT VALID; ADD CONSTRAINT check_27f0f6dbab CHECK ((file_store IS NOT NULL)) NOT VALID;
...@@ -13832,6 +13835,8 @@ COPY "schema_migrations" (version) FROM STDIN; ...@@ -13832,6 +13835,8 @@ COPY "schema_migrations" (version) FROM STDIN;
20200528125905 20200528125905
20200528171933 20200528171933
20200601210148 20200601210148
20200602013900
20200602013901
20200603073101 20200603073101
20200604143628 20200604143628
\. \.
......
# frozen_string_literal: true
require 'spec_helper'
require Rails.root.join('db', 'post_migrate', '20200602013901_cap_designs_filename_length_to_new_limit')
describe CapDesignsFilenameLengthToNewLimit, :migration, schema: 20200528125905 do
let(:namespaces) { table(:namespaces) }
let(:projects) { table(:projects) }
let(:issues) { table(:issues) }
let(:designs) { table(:design_management_designs) }
let(:filename_below_limit) { generate_filename(254) }
let(:filename_at_limit) { generate_filename(255) }
let(:filename_above_limit) { generate_filename(256) }
let!(:namespace) { namespaces.create!(name: 'foo', path: 'foo') }
let!(:project) { projects.create!(name: 'gitlab', path: 'gitlab-org/gitlab', namespace_id: namespace.id) }
let!(:issue) { issues.create!(description: 'issue', project_id: project.id) }
def generate_filename(length, extension: '.png')
name = 'a' * (length - extension.length)
"#{name}#{extension}"
end
def create_design(filename)
designs.create!(
issue_id: issue.id,
project_id: project.id,
filename: filename
)
end
it 'correctly sets filenames that are above the limit' do
[
filename_below_limit,
filename_at_limit,
filename_above_limit
].each(&method(:create_design))
migrate!
expect(designs.find(1).filename).to eq(filename_below_limit)
expect(designs.find(2).filename).to eq(filename_at_limit)
expect(designs.find(3).filename).to eq([described_class::MODIFIED_NAME, 3, described_class::MODIFIED_EXTENSION].join)
end
it 'runs after filename limit has been set' do
# This spec file uses the `schema:` keyword to run these tests
# against a schema version before the one that sets the limit,
# as otherwise we can't create the design data with filenames greater
# than the limit.
#
# For this test, we migrate any skipped versions up to this migration.
migration_context.migrate(20200602013901)
create_design(filename_at_limit)
expect { create_design(filename_above_limit) }.to raise_error(ActiveRecord::StatementInvalid)
end
end
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