Fix image path check: anchor to book output dir not chapter dir

.buildkite/pull-requests.org-wide.json

@@ -91,6 +91,7 @@
       "allowed_list": ["github-actions[bot]", "renovate[bot]", "mergify[bot]"],
       "build_on_commit": true,
       "build_on_comment": true,
+      "ignore_pipeline_branch_filters": true,
       "trigger_comment_regex": "^run docs-build ?(?<rebuild_opt>rebuild)? ?(?<warn_opt>warnlinkcheck)? ?(?<skip_opt>skiplinkcheck)?\\s*?$",
       "always_trigger_comment_regex": "^buildkite test this ?(?<rebuild_opt>rebuild)? ?(?<warn_opt>warnlinkcheck)? ?(?<skip_opt>skiplinkcheck)?\\s*?$",
       "skip_ci_labels": [

lib/ES/Util.pm

@@ -228,6 +228,9 @@ sub build_single {
             $page_header ? ('-a' => "page-header=$page_header") : (),
             # Turn on asciidoctor's table of contents generation if we want a TOC
             $toc ? ('-a' => 'toc') : (),
+            # Asciidoctor doesn't pass the destination directory down to
+            # the converter so we do so here explicitly
+            '-a' => 'outdir=' . $raw_dest,
             '--destination-dir=' . $raw_dest,
             docinfo($index),
             $index

resources/asciidoctor/lib/copy_images/copier.rb

@@ -12,7 +12,7 @@ module CopyImages
   class Copier
     include LogUtil
 
-    ALLOWED_IMAGE_EXTENSIONS = %w[.png .jpg .jpeg .gif .svg .webp].freeze
+    ALLOWED_IMAGE_EXTENSIONS = %w[.png .jpg .jpeg .gif .svg .ico .webp].freeze
 
     def initialize
       # TODO: store this set on the document so we don't duplicate copies
@@ -38,30 +38,27 @@ def copy_image(block, uri)
 
     def perform_copy(block, uri, source)
       unless ALLOWED_IMAGE_EXTENSIONS.include?(File.extname(uri).downcase)
-        warn block: block, message: "Refusing to copy non-image file: #{uri}"
-        return
+        return warn block: block, message: "Non-image extension: #{uri}"
       end
 
       if File.symlink?(source)
-        warn block: block, message: "Refusing to copy symlink: #{source}"
-        return
+        return warn block: block, message: "Refusing to copy symlink: #{source}"
       end
 
-      to_dir = File.expand_path(block.document.options[:to_dir])
-      destination = File.expand_path(File.join(to_dir, uri))
+      doc = block.document
+      outdir = book_outdir doc
+      dest = File.expand_path(uri, doc.options[:to_dir])
 
-      unless destination.start_with?("#{to_dir}/")
-        warn block: block, message: "Image path escapes output dir: #{uri}"
-        return
+      unless dest.start_with?("#{outdir}/")
+        return warn block: block, message: "Image outside output dir: #{uri}"
       end
 
-      if File.symlink?(destination)
-        warn block: block, message: "Destination is a symlink: #{destination}"
-        return
+      if File.symlink?(dest)
+        return warn block: block, message: "Destination is a symlink: #{dest}"
       end
 
-      FileUtils.mkdir_p(File.dirname(destination))
-      FileUtils.cp source, destination
+      FileUtils.mkdir_p(File.dirname(dest))
+      FileUtils.cp source, dest
     end
 
     ##
@@ -123,5 +120,12 @@ def log_missing(block, checked, uri)
       warn block: block,
            message: "can't read image [#{uri}] at any of #{checked}"
     end
+
+    private
+
+    def book_outdir(doc)
+      raw = doc.attr('outdir') || doc.options[:to_dir]
+      File.expand_path(File.dirname(raw))
+    end
   end
 end

resources/asciidoctor/spec/copy_images_spec.rb

@@ -361,72 +361,6 @@
     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
-      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
@@ -457,4 +391,34 @@ def make_block(to_dir)
       expect(logs).to eq(expected_logs.strip)
     end
   end
+
+  context 'input/output validation' do
+    let(:tmp) { Dir.mktmpdir }
+    let(:source_file) do
+      path = File.join(tmp, 'example.png')
+      FileUtils.touch(path)
+      path
+    end
+    let(:copier) { CopyImages::Copier.new }
+
+    after(:example) { FileUtils.remove_entry tmp }
+
+    def make_block(to_dir)
+      doc = double('document')
+      allow(doc).to receive(:options).and_return(to_dir: to_dir)
+      allow(doc).to receive(:attr).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 uses path traversal with a non-image extension' do
+      it 'rejects it via the extension check' 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
+  end
 end