Nokogiri: [bug] The `strict` parsing mode does not raise for invalid HTML documents

[dgollahon]

Please describe the bug

Hi,

According to these docs it should be possible to enter a strict parsing mode that raises on malformed XML. This works for XML. It seems implied that strict can be set the same as with Nokogiri::XML, here. Unfortunately, I can't figure out anything that will cause Nokogiri::HTML with strict config to raise.

Hopefully I have not missed any documentation or open issues related to this, but all I ran across was https://github.com/sparklemotion/nokogiri/issues/1806 which was closed for lack of detail. If this is the intended behavior, I think the differences between XML and HTML parse options should be documented more clearly.

Help us reproduce what you're seeing

#! /usr/bin/env ruby

require 'nokogiri'

# OK
Nokogiri::XML('<') # => #(Document:0x3fc297862fc4 { name = "document" })

# OK, there's the error I would expect
begin
  Nokogiri::XML('<', &:strict)
rescue => error
 pp error # => #<Nokogiri::XML::SyntaxError: 1:2: FATAL: StartTag: invalid element name>
end

# OK
 Nokogiri::HTML('<') # => #(Document:0x3fc29751450c { name = "document", children = [ #(DTD:0x3fc2975188dc { name = "html" })]})

# What? Why doesn't this raise?
Nokogiri::HTML('<', &:strict) # => #(Document:0x3fc29752c378 { name = "document", children = [ #(DTD:0x3fc297534780 { name = "html" })]})

# I have tried various other wonky bits of partial HTML and I can't figure out anything that makes a difference / causes `Nokogiri::HTML` to raise.

# NOTE: The `#errors` method does show errors in this case:
Nokogiri::HTML('<').errors # => [#<Nokogiri::XML::SyntaxError: 1:2: ERROR: htmlParseStartTag: invalid element name>]
Nokogiri::XML('<').errors # => [#<Nokogiri::XML::SyntaxError: 1:2: FATAL: StartTag: invalid element name>]

Expected behavior

I would expect Nokogiri to raise a syntax error for HTML documents but nothing is raised, unlike with the XML parsing mode. The #errors array seems to be populated in either case.

Environment

# Nokogiri (1.10.10)
    ---
    warnings: []
    nokogiri: 1.10.10
    ruby:
      version: 2.6.6
      platform: x86_64-darwin19
      description: ruby 2.6.6p146 (2020-03-31 revision 67876) [x86_64-darwin19]
      engine: ruby
    libxml:
      binding: extension
      source: packaged
      libxml2_path: "/Users/gollahon/.rvm/gems/ruby-2.6.6/gems/nokogiri-1.10.10/ports/x86_64-apple-darwin19.6.0/libxml2/2.9.10"
      libxslt_path: "/Users/gollahon/.rvm/gems/ruby-2.6.6/gems/nokogiri-1.10.10/ports/x86_64-apple-darwin19.6.0/libxslt/1.1.34"
      libxml2_patches:
      - 0001-Revert-Do-not-URI-escape-in-server-side-includes.patch
      - 0002-Remove-script-macro-support.patch
      - 0003-Update-entities-to-remove-handling-of-ssi.patch
      - 0004-libxml2.la-is-in-top_builddir.patch
      - 0005-Fix-infinite-loop-in-xmlStringLenDecodeEntities.patch
      libxslt_patches: []
      compiled: 2.9.10
      loaded: 2.9.10

Additional context

I am writing a small scraper and I would like to bail if pages I am trying to parse are corrupt/invalid.

Thanks!

[flavorjones]

@dgollahon Thank you for submitting such a clear issue for this! And I'm sorry that you're having trouble.

I'll get some time later today to reproduce what you're seeing and dig in. My gut is that this is libxml2 acting inconsistently, but I will definitely dig in and tell you what I find.

[flavorjones]

@dgollahon Thanks for your patience. I spent some time tonight looking into this, and I'll try to explain what's going on.

TL;DR: Yeah, this is a bug in the CRuby implementation (but not the JRuby one!).

CRuby / libxml2 HTMLparser.c

The logic used in the CRuby/libxml2 implementation for raising an exception on HTML parse errors is as follows:

• call the parsing function
• if no document is returned, raise an exception
• otherwise assume all is well: save all (presumably nonfatal?) errors on the @errors instance variable and return the document

Libxml2's HTML parser considers almost nothing to be fatal and nearly always recovers -- regardless of what parse options are passed in. The RECOVER parse flag is used very sparingly to control specific behavior around CDATA strings and element names, and does not control whether the parse function returns a successfully-parsed document.

In contrast, if we look at libxml2's XML parser in parser.c, the following code is in the top-level wrapper function xmlDoRead:

    xmlParseDocument(ctxt);
    if ((ctxt->wellFormed) || ctxt->recovery)
        ret = ctxt->myDoc;
    else {
        ret = NULL;
        /* ... some code elided ... */
    }
    /* ... some code elided ... */
    return (ret);

That is, unless the parser found zero errors (wellFormed is truthy) or the RECOVER parse option is set, no document is returned.

JRuby / NekoHTML

Here's where it gets messy. The JRuby implementation does not rely on the return value from the parse function to determine success, and instead uses different error handlers (strict or non-strict) to control whether to raise an exception. As a result, it behaves consistently across both XML and HTML parsers, and matches what your expectations are in this issue.

Why has nobody reported this before?

Why has nobody reported this before? I think the honest truth is that there is so much broken HTML out there in the world that the overwhelming majority of use cases need the default behavior -- to act like a browser and try to recover from parse errors as best we can. Put another way, strict parsing of an arbitrary HTML document from the internet is highly likely to fail.

How do we resolve this?

I think the right thing to do here is to change the behavior of the HTML::Document parse functions to raise an exception if _any_ parse error is encountered; but this is likely to be a breaking change for some folks and so I want to think hard about putting it into a patch release.

Here are some tests that all pass on JRuby:

require "helper"

class Test2130 < Nokogiri::TestCase
  describe "HTML::Document.parse" do
    describe "read memory" do
      let(:input) { "<html><body><div" }

      describe "strict parsing" do
        let(:parse_options) { Nokogiri::XML::ParseOptions.new.strict }

        it "raises exception on parse error" do
          assert_raises Nokogiri::SyntaxError do
            Nokogiri::HTML.parse(input, nil, nil, parse_options)
          end
        end
      end

      describe "default options" do
        it "does not raise exception on parse error" do
          doc = Nokogiri::HTML.parse(input)
          assert_operator(doc.errors.length, :>, 0)
        end
      end
    end

    describe "read io" do
      let(:input) { StringIO.new("<html><body><div") }

      describe "strict parsing" do
        let(:parse_options) { Nokogiri::XML::ParseOptions.new.strict }

        it "raises exception on parse error" do
          assert_raises Nokogiri::SyntaxError do
            Nokogiri::HTML.parse(input, nil, nil, parse_options)
          end
        end
      end

      describe "default options" do
        it "does not raise exception on parse error" do
          doc = Nokogiri::HTML.parse(input)
          assert_operator(doc.errors.length, :>, 0)
        end
      end
    end
  end
end

But fail on CRuby in embarrasingly bad ways -- including read_io not trapping errors correctly.

$ be ruby -Ilib:test 2130-html-strict-behavior/test.rb 
/home/flavorjones/code/oss/nokogiri/test/helper.rb:30: version info:
---
warnings: []
nokogiri: 1.11.0.rc3
ruby:
  version: 2.7.2
  platform: x86_64-linux
  gem_platform: x86_64-linux
  description: ruby 2.7.2p137 (2020-10-01 revision 5445e04352) [x86_64-linux]
  engine: ruby
libxml:
  source: system
  iconv_enabled: true
  compiled: 2.9.10
  loaded: 2.9.10
libxslt:
  source: system
  compiled: 1.1.34
  loaded: 1.1.34
Run options: --seed 37272

# Running:

FHTML parser error : Couldn't find end of Start Tag div
<html><body><div
                ^
FF.

Fabulous run in 0.001873s, 2135.2260 runs/s, 2135.2260 assertions/s.

  1) Failure:
HTML::Document.parse::read io::default options#test_0001_does not raise exception on parse error [2130-html-strict-behavior/test.rb:42]:
Expected 0 to be > 0.

  2) Failure:
HTML::Document.parse::read io::strict parsing#test_0001_raises exception on parse error [2130-html-strict-behavior/test.rb:33]:
Nokogiri::SyntaxError expected but nothing was raised.

  3) Failure:
HTML::Document.parse::read memory::strict parsing#test_0001_raises exception on parse error [2130-html-strict-behavior/test.rb:12]:
Nokogiri::SyntaxError expected but nothing was raised.

4 runs, 4 assertions, 3 failures, 0 errors, 0 skips

[dgollahon]

Thanks for the update! Interesting that it works on JRuby. Regardless of the fix path, it seems to me that MRI and JRuby should have the same behavior.

Understood re: broken HTML and the risk of breaking users. I am sure some people like me have strict enabled but do not realize it would fail if this bug were resolved. It does seem, however, like something that should be fixed, imo. I wonder how widespread the use is? Also, is this a regression or has it never worked? I suppose you could always wait for a major release if the risk of breakage was bad enough but I am guessing that is not on the horizon.

[flavorjones]

It does seem, however, like something that should be fixed

I agree and I'll try to get this into the next release.

is this a regression or has it never worked?

Neither libxml2 nor Nokogiri has ever behaved any differently. It's been like this since the first release of Nokogiri.

[flavorjones]

PR #2150 will address this.