Add improved handling of PDF hash links

gjtorikian · Jan 20, 2023 · 1dbb0b8 · 1dbb0b8
1 parent 3aa7073
commit 1dbb0b8
Show file tree

Hide file tree

Showing 8 changed files with 201 additions and 11 deletions.
diff --git a/html-proofer.gemspec b/html-proofer.gemspec
@@ -27,6 +27,7 @@ Gem::Specification.new do |spec|
   spec.add_dependency("addressable",     "~> 2.3")
   spec.add_dependency("async",           "~> 2.1")
   spec.add_dependency("nokogiri",        "~> 1.13")
+  spec.add_dependency("pdf-reader",      "~> 2.11")
   spec.add_dependency("rainbow",         "~> 3.0")
   spec.add_dependency("typhoeus",        "~> 1.3")
   spec.add_dependency("yell",            "~> 2.0")

diff --git a/lib/html_proofer/attribute/url.rb b/lib/html_proofer/attribute/url.rb
@@ -141,12 +141,16 @@ def file_path
           # either overwrite with root_dir; or, if source is directory, use that; or, just get the current file's dirname
           @runner.options[:root_dir] || (File.directory?(@runner.current_source) ? @runner.current_source : File.dirname(@runner.current_source))
         # relative links, path is a file
-        elsif File.exist?(File.expand_path(path,
-          @runner.current_source)) || File.exist?(File.expand_path(path_dot_ext, @runner.current_source))
+        elsif File.exist?(File.expand_path(
+          path,
+          @runner.current_source,
+        )) || File.exist?(File.expand_path(path_dot_ext, @runner.current_source))
           File.dirname(@runner.current_filename)
         # relative links in nested dir, path is a file
-        elsif File.exist?(File.join(File.dirname(@runner.current_filename),
-          path)) || File.exist?(File.join(File.dirname(@runner.current_filename), path_dot_ext))
+        elsif File.exist?(File.join(
+          File.dirname(@runner.current_filename),
+          path,
+        )) || File.exist?(File.join(File.dirname(@runner.current_filename), path_dot_ext))
           File.dirname(@runner.current_filename)
         # relative link, path is a directory
         else

diff --git a/lib/html_proofer/url_validator/external.rb b/lib/html_proofer/url_validator/external.rb
@@ -1,7 +1,9 @@
 # frozen_string_literal: true
 
 require "typhoeus"
-require "uri"
+require "open-uri"
+# require "uri"
+require "pdf-reader"
 
 module HTMLProofer
   class UrlValidator
@@ -88,8 +90,12 @@ def response_handler(response, url, filenames)
         return if @runner.options[:ignore_status_codes].include?(response_code)
 
         if response_code.between?(200, 299)
-          @cache.add_external(href, filenames, response_code, "OK", true) unless check_hash_in_2xx_response(href, url,
-            response, filenames)
+          @cache.add_external(href, filenames, response_code, "OK", true) unless check_hash_in_2xx_response(
+            href,
+            url,
+            response,
+            filenames,
+          )
         elsif response.timed_out?
           handle_timeout(href, filenames, response_code)
         elsif response_code.zero?
@@ -116,6 +122,26 @@ def check_hash_in_2xx_response(href, url, response, filenames)
 
         hash = url.hash
 
+        # attempt to verify PDF hash ref; see #787 for more details
+        # FIXME: this is re-reading the PDF response
+        if /pdf/.match?(response.options[:headers]["content-type"])
+          io = URI.parse(url.to_s).open
+          reader = PDF::Reader.new(io)
+
+          pages = reader.pages
+          if hash =~ /\Apage=(\d+)\z/
+            page = Regexp.last_match[1].to_i
+
+            unless pages[page - 1]
+              msg = "External link #{href} failed: #{url.sans_hash} exists, but the hash '#{hash}' does not"
+              add_failure(filenames, msg, response.code)
+              @cache.add_external(href, filenames, response.code, msg, false)
+            end
+
+            return true
+          end
+        end
+
         body_doc = create_nokogiri(response.body)
 
         unencoded_hash = Addressable::URI.unescape(hash)

diff --git a/spec/html-proofer/fixtures/links/pdf_w_hash.html b/spec/html-proofer/fixtures/links/pdf_w_hash.html
@@ -0,0 +1,2 @@
+<!DOCTYPE html>
+<a href='https://checkoway.net/teaching/cs210/2021-fall/slides/Lec19%20ClocksandTiming.pdf#page=25'>X</a>
diff --git a/spec/html-proofer/fixtures/links/pdf_w_hash_broken.html b/spec/html-proofer/fixtures/links/pdf_w_hash_broken.html
@@ -0,0 +1,2 @@
+<!DOCTYPE html>
+<a href='https://checkoway.net/teaching/cs210/2021-fall/slides/Lec19%20ClocksandTiming.pdf#page=2111115'>X</a>
diff --git a/...roofer/fixtures/vcr_cassettes/links/pdf_w_hash_broken_html_log_level_error_type_file_.yml b/...roofer/fixtures/vcr_cassettes/links/pdf_w_hash_broken_html_log_level_error_type_file_.yml
diff --git a/.../html-proofer/fixtures/vcr_cassettes/links/pdf_w_hash_html_log_level_error_type_file_.yml b/.../html-proofer/fixtures/vcr_cassettes/links/pdf_w_hash_html_log_level_error_type_file_.yml
diff --git a/spec/html-proofer/links_spec.rb b/spec/html-proofer/links_spec.rb
@@ -144,16 +144,22 @@
 
   it "fails on redirects if not following" do
     link_with_redirect_filepath = File.join(FIXTURES_DIR, "links", "link_with_redirect.html")
-    proofer = run_proofer(link_with_redirect_filepath, :file,
-      { enforce_https: false, typhoeus: { followlocation: false } })
+    proofer = run_proofer(
+      link_with_redirect_filepath,
+      :file,
+      { enforce_https: false, typhoeus: { followlocation: false } },
+    )
     expect(proofer.failed_checks.first.description).to(match(/failed/))
   end
 
   it "does not fail on redirects we're not following" do
     # this test should emit a 301--see above--but we're intentionally suppressing it
     link_with_redirect_filepath = File.join(FIXTURES_DIR, "links", "link_with_redirect.html")
-    proofer = run_proofer(link_with_redirect_filepath, :file,
-      { only_4xx: true, enforce_https: false, typhoeus: { followlocation: false } })
+    proofer = run_proofer(
+      link_with_redirect_filepath,
+      :file,
+      { only_4xx: true, enforce_https: false, typhoeus: { followlocation: false } },
+    )
     expect(proofer.failed_checks).to(eq([]))
   end
 
@@ -750,4 +756,15 @@
     expect(proofer.failed_checks.first.description).to(match(/internally linking to exists.pdf#page=2; the file exists, but the hash 'page=2' does not/))
     expect(proofer.failed_checks.last.description).to(match(/internally linking to missing.pdf#page=2, which does not exist/))
   end
+
+  it "tries reading PDFs with hashes" do
+    file = File.join(FIXTURES_DIR, "links", "pdf_w_hash.html")
+    proofer = run_proofer(file, :file)
+    expect(proofer.failed_checks.count).to(eq(0))
+
+    file = File.join(FIXTURES_DIR, "links", "pdf_w_hash_broken.html")
+    proofer = run_proofer(file, :file)
+    expect(proofer.failed_checks.count).to(eq(1))
+    expect(proofer.failed_checks.first.description).to(match(/ClocksandTiming.pdf exists, but the hash 'page=2111115' does not/))
+  end
 end
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		<!DOCTYPE html>
		<a href='https://checkoway.net/teaching/cs210/2021-fall/slides/Lec19%20ClocksandTiming.pdf#page=25'>X</a>