From 4b16a00af84d1f0a2b28075594628d2e3eb4f65a Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Sun, 3 May 2026 18:30:20 +0200 Subject: [PATCH 1/5] Tighten image copy input/output validation Co-Authored-By: Claude Sonnet 4.6 (1M context) --- .../asciidoctor/lib/copy_images/copier.rb | 26 ++++++- .../asciidoctor/spec/copy_images_spec.rb | 68 +++++++++++++++++++ 2 files changed, 93 insertions(+), 1 deletion(-) diff --git a/resources/asciidoctor/lib/copy_images/copier.rb b/resources/asciidoctor/lib/copy_images/copier.rb index 07d01f02d3064..0ed85fe3faa9e 100644 --- a/resources/asciidoctor/lib/copy_images/copier.rb +++ b/resources/asciidoctor/lib/copy_images/copier.rb @@ -12,6 +12,8 @@ module CopyImages class Copier include LogUtil + ALLOWED_IMAGE_EXTENSIONS = %w[.png .jpg .jpeg .gif .svg .webp].freeze + def initialize # TODO: store this set on the document so we don't duplicate copies # for sub-documents caused by alternative examples @@ -35,7 +37,29 @@ def copy_image(block, uri) end def perform_copy(block, uri, source) - destination = File.join block.document.options[:to_dir], uri + unless ALLOWED_IMAGE_EXTENSIONS.include?(File.extname(uri).downcase) + warn block: block, message: "Refusing to copy non-image file: #{uri}" + return + end + + if File.symlink?(source) + warn block: block, message: "Refusing to copy symlink: #{source}" + return + end + + to_dir = File.expand_path(block.document.options[:to_dir]) + destination = File.expand_path(File.join(to_dir, uri)) + + unless destination.start_with?(to_dir + File::SEPARATOR) + warn block: block, message: "Refusing to copy image outside output dir: #{uri}" + return + end + + if File.symlink?(destination) + warn block: block, message: "Refusing to overwrite symlink at destination: #{destination}" + return + end + destination_dir = File.dirname destination FileUtils.mkdir_p destination_dir FileUtils.cp source, destination diff --git a/resources/asciidoctor/spec/copy_images_spec.rb b/resources/asciidoctor/spec/copy_images_spec.rb index e658ebcf6646a..41e4b039da8eb 100644 --- a/resources/asciidoctor/spec/copy_images_spec.rb +++ b/resources/asciidoctor/spec/copy_images_spec.rb @@ -361,6 +361,74 @@ end end + context 'input/output validation' do + let(:tmp) { Dir.mktmpdir } + after(:example) { FileUtils.remove_entry tmp } + + let(:source_file) do + path = File.join(tmp, 'real_image.png') + FileUtils.cp( + File.join(spec_dir, 'resources', 'copy_images', 'example1.png'), + path + ) + path + end + + let(:copier) { CopyImages::Copier.new } + + def make_block(to_dir) + doc = double('document') + allow(doc).to receive(:options).and_return(to_dir: to_dir) + allow(doc).to receive(:attr).with('copy_image').and_return(nil) + block = double('block') + allow(block).to receive(:document).and_return(doc) + allow(block).to receive(:source_location).and_return(nil) + block + end + + context 'when the URI contains path traversal' do + it 'rejects it with a warning and does not write outside to_dir' do + block = make_block(tmp) + outside = File.expand_path(File.join(tmp, '../../../outside.png')) + copier.perform_copy(block, '../../../outside.png', source_file) + expect(File.exist?(outside)).to be false + # Verify a warning was issued (the logger on Copier is the global logger) + # We rely on the fact that no file was written as the main assertion. + end + end + + context 'when the URI has a non-image extension' do + it 'rejects it with a warning and does not copy the file' do + block = make_block(tmp) + copier.perform_copy(block, 'evil.yml', source_file) + expect(File.exist?(File.join(tmp, 'evil.yml'))).to be false + end + end + + context 'when the source path is a symlink' do + it 'rejects it with a warning and does not copy' do + symlink_path = File.join(tmp, 'link_image.png') + File.symlink(source_file, symlink_path) + block = make_block(tmp) + copier.perform_copy(block, 'output_image.png', symlink_path) + expect(File.exist?(File.join(tmp, 'output_image.png'))).to be false + end + end + + context 'when the destination is already a symlink' do + it 'rejects it with a warning and does not overwrite the symlink' do + dest_path = File.join(tmp, 'dest_image.png') + # Create a symlink at destination pointing somewhere else + File.symlink('/dev/null', dest_path) + block = make_block(tmp) + copier.perform_copy(block, 'dest_image.png', source_file) + # The symlink should still point to /dev/null, not be replaced + expect(File.symlink?(dest_path)).to be true + expect(File.readlink(dest_path)).to eq('/dev/null') + end + end + end + context 'when the same image is referenced more than once' do let(:input) do <<~ASCIIDOC From 7e9b8128eabe7bedd4ba8dcf5b69e92443c2a2c9 Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Sun, 3 May 2026 18:46:55 +0200 Subject: [PATCH 2/5] Inline destination_dir to stay within method length limit --- resources/asciidoctor/lib/copy_images/copier.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/resources/asciidoctor/lib/copy_images/copier.rb b/resources/asciidoctor/lib/copy_images/copier.rb index 0ed85fe3faa9e..05edfa90f5227 100644 --- a/resources/asciidoctor/lib/copy_images/copier.rb +++ b/resources/asciidoctor/lib/copy_images/copier.rb @@ -60,8 +60,7 @@ def perform_copy(block, uri, source) return end - destination_dir = File.dirname destination - FileUtils.mkdir_p destination_dir + FileUtils.mkdir_p(File.dirname(destination)) FileUtils.cp source, destination end From 6ee0a47817fc7ed5e25e249dc778d81fe4afe1dd Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Sun, 3 May 2026 19:07:29 +0200 Subject: [PATCH 3/5] Use string interpolation to reduce ABC metric below limit --- resources/asciidoctor/lib/copy_images/copier.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/resources/asciidoctor/lib/copy_images/copier.rb b/resources/asciidoctor/lib/copy_images/copier.rb index 05edfa90f5227..b7958ab485dc4 100644 --- a/resources/asciidoctor/lib/copy_images/copier.rb +++ b/resources/asciidoctor/lib/copy_images/copier.rb @@ -50,7 +50,7 @@ def perform_copy(block, uri, source) to_dir = File.expand_path(block.document.options[:to_dir]) destination = File.expand_path(File.join(to_dir, uri)) - unless destination.start_with?(to_dir + File::SEPARATOR) + unless destination.start_with?("#{to_dir}/") warn block: block, message: "Refusing to copy image outside output dir: #{uri}" return end From 3c57e159bb1c4aa4c58847d15bbc05903cff448a Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Sun, 3 May 2026 19:30:01 +0200 Subject: [PATCH 4/5] Fix Metrics/LineLength violations (rubocop max is 80) --- resources/asciidoctor/lib/copy_images/copier.rb | 6 ++++-- resources/asciidoctor/spec/copy_images_spec.rb | 2 -- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/resources/asciidoctor/lib/copy_images/copier.rb b/resources/asciidoctor/lib/copy_images/copier.rb index b7958ab485dc4..308b0ff9ed348 100644 --- a/resources/asciidoctor/lib/copy_images/copier.rb +++ b/resources/asciidoctor/lib/copy_images/copier.rb @@ -51,12 +51,14 @@ def perform_copy(block, uri, source) destination = File.expand_path(File.join(to_dir, uri)) unless destination.start_with?("#{to_dir}/") - warn block: block, message: "Refusing to copy image outside output dir: #{uri}" + warn block: block, + message: "Refusing to copy image outside output dir: #{uri}" return end if File.symlink?(destination) - warn block: block, message: "Refusing to overwrite symlink at destination: #{destination}" + warn block: block, + message: "Refusing to overwrite symlink at dest: #{destination}" return end diff --git a/resources/asciidoctor/spec/copy_images_spec.rb b/resources/asciidoctor/spec/copy_images_spec.rb index 41e4b039da8eb..9e0f2d38536a9 100644 --- a/resources/asciidoctor/spec/copy_images_spec.rb +++ b/resources/asciidoctor/spec/copy_images_spec.rb @@ -392,8 +392,6 @@ def make_block(to_dir) outside = File.expand_path(File.join(tmp, '../../../outside.png')) copier.perform_copy(block, '../../../outside.png', source_file) expect(File.exist?(outside)).to be false - # Verify a warning was issued (the logger on Copier is the global logger) - # We rely on the fact that no file was written as the main assertion. end end From 04577a6bf9e62157628eb1e35016f29e612d0f85 Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Sun, 3 May 2026 19:43:08 +0200 Subject: [PATCH 5/5] Shorten warn messages to restore MethodLength to 20 lines --- resources/asciidoctor/lib/copy_images/copier.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/resources/asciidoctor/lib/copy_images/copier.rb b/resources/asciidoctor/lib/copy_images/copier.rb index 308b0ff9ed348..666ac86ffd85b 100644 --- a/resources/asciidoctor/lib/copy_images/copier.rb +++ b/resources/asciidoctor/lib/copy_images/copier.rb @@ -51,14 +51,12 @@ def perform_copy(block, uri, source) destination = File.expand_path(File.join(to_dir, uri)) unless destination.start_with?("#{to_dir}/") - warn block: block, - message: "Refusing to copy image outside output dir: #{uri}" + warn block: block, message: "Image path escapes output dir: #{uri}" return end if File.symlink?(destination) - warn block: block, - message: "Refusing to overwrite symlink at dest: #{destination}" + warn block: block, message: "Destination is a symlink: #{destination}" return end