diff --git a/.buildkite/pull-requests.org-wide.json b/.buildkite/pull-requests.org-wide.json index fa7d2ddb737ce..9c77344dcc229 100644 --- a/.buildkite/pull-requests.org-wide.json +++ b/.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)? ?(?warnlinkcheck)? ?(?skiplinkcheck)?\\s*?$", "always_trigger_comment_regex": "^buildkite test this ?(?rebuild)? ?(?warnlinkcheck)? ?(?skiplinkcheck)?\\s*?$", "skip_ci_labels": [ diff --git a/lib/ES/Util.pm b/lib/ES/Util.pm index f1a0a358ad1c2..dfc86f405f563 100644 --- a/lib/ES/Util.pm +++ b/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 diff --git a/resources/asciidoctor/lib/copy_images/copier.rb b/resources/asciidoctor/lib/copy_images/copier.rb index 666ac86ffd85b..07c77bb46188a 100644 --- a/resources/asciidoctor/lib/copy_images/copier.rb +++ b/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 diff --git a/resources/asciidoctor/spec/copy_images_spec.rb b/resources/asciidoctor/spec/copy_images_spec.rb index 9e0f2d38536a9..faf8837ce4fa6 100644 --- a/resources/asciidoctor/spec/copy_images_spec.rb +++ b/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