[bug] Possible regression in XML generation #2317

mscrivo · 2021-08-24T19:44:06Z

Please describe the bug

We use the Viewpoint gem to communicate with on-prem & office 365 exchange instances. The Viewpoint gem has a dependency on nokogiri and appears to use both the Nokogiri::XML and Nokogiri::XML:SAX namespaces in its code. After upgrading our version of nokogiri from 1.11.7 to 1.12, all integrations with exchange servers broke with a non-helpful SOAP XML error. I did a little testing of what the XML generated looks like between the 1.11.7 version and 1.12 and it looks like this:

1.11.7:

<soap:Envelope xmlns:soap="http://schemas.xmlsoap.org/soap/envelope/" xmlns:t="http://schemas.microsoft.com/exchange/services/2006/types" xmlns:m="http://schemas.microsoft.com/exchange/services/2006/messages">
  <soap:Header>
    <t:RequestServerVersion Version="Exchange2010"/>
  </soap:Header>
  <soap:Body>
    <SyncFolderHierarchy xmlns="http://schemas.microsoft.com/exchange/services/2006/messages">
      <FolderShape>
        <t:BaseShape>Default</t:BaseShape>
      </FolderShape>
    </SyncFolderHierarchy>
  </soap:Body>
</soap:Envelope>

1.12+:

<soap:Envelope xmlns:soap="http://schemas.xmlsoap.org/soap/envelope/" xmlns:t="http://schemas.microsoft.com/exchange/services/2006/types" xmlns:m="http://schemas.microsoft.com/exchange/services/2006/messages">
  <Header>
    <t:RequestServerVersion Version="Exchange2010"/>
  </Header>
  <Body>
    <SyncFolderHierarchy xmlns="http://schemas.microsoft.com/exchange/services/2006/messages">
      <FolderShape>
        <t:BaseShape>Default</t:BaseShape>
      </FolderShape>
    </SyncFolderHierarchy>
  </Body>
</soap:Envelope>

Both are valid XML, but notice how the new one does not have any namespace prefix on the Header, Body, etc. I don't know the XML spec that well, but this page seems to indicate that unless your parent has a default namespace (in this case Envelope doesn't), then the child elements don't inherit it and are thus unnamespaced?

I believe the issue was introduced with this commit: cd6941f

Help us reproduce what you're seeing

reproducing requires access to an exchange server, but the XML output above should suffice to show the problem

Expected behavior

The XML generated should properly follow the spec, or how the spec has been adapted in the wild. It's entirely possible that the issue here is with the Viewpoint code since it's quite old and hasn't been updated, or how Exchange is interpreting the XML, I'm not sure, but since it broke with this upgrade, I'd figure this would be a good place to start just in case there was a regression in the aforementioned commit.

Environment

# Nokogiri (1.12.3)
    ---
    warnings: []
    nokogiri:
      version: 1.12.3
      cppflags:
      - "-I/Users/michaelscrivo/affinity/vendor/bundle/ruby/3.0.0/gems/nokogiri-1.12.3-x86_64-darwin/ext/nokogiri"
      - "-I/Users/michaelscrivo/affinity/vendor/bundle/ruby/3.0.0/gems/nokogiri-1.12.3-x86_64-darwin/ext/nokogiri/include"
      - "-I/Users/michaelscrivo/affinity/vendor/bundle/ruby/3.0.0/gems/nokogiri-1.12.3-x86_64-darwin/ext/nokogiri/include/libxml2"
      ldflags: []
    ruby:
      version: 3.0.2
      platform: x86_64-darwin20
      gem_platform: x86_64-darwin-20
      description: ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-darwin20]
      engine: ruby
    libxml:
      source: packaged
      precompiled: true
      patches:
      - 0001-Remove-script-macro-support.patch
      - 0002-Update-entities-to-remove-handling-of-ssi.patch
      - 0003-libxml2.la-is-in-top_builddir.patch
      - 0004-use-glibc-strlen.patch
      - 0005-avoid-isnan-isinf.patch
      - 0006-update-automake-files-for-arm64.patch
      - 0007-Fix-XPath-recursion-limit.patch
      libxml2_path: "/Users/michaelscrivo/affinity/vendor/bundle/ruby/3.0.0/gems/nokogiri-1.12.3-x86_64-darwin/ext/nokogiri"
      memory_management: ruby
      iconv_enabled: true
      compiled: 2.9.12
      loaded: 2.9.12
    libxslt:
      source: packaged
      precompiled: true
      patches:
      - 0001-update-automake-files-for-arm64.patch
      - 0002-Fix-xml2-config-check-in-configure-script.patch
      datetime_enabled: true
      compiled: 1.1.34
      loaded: 1.1.34
    other_libraries:
      zlib: 1.2.11
      libiconv: '1.15'
      libgumbo: 1.0.0-nokogiri

The text was updated successfully, but these errors were encountered:

flavorjones · 2021-08-25T11:28:41Z

@mscrivo Thanks for opening this issue, and I'm sorry you're having trouble.

I will try to find some time today to take a look, but it seems likely that this problem was introduced when we cleaned up the namespaces logic related to reparenting nodes (maybe not the exact commit you pointed at, but I did a bit of work in this area recently). Certainly if the Builder behavior has changed it was unintentional and we should fix it.

mscrivo · 2021-08-25T14:03:18Z

Thanks @flavorjones, really appreciate it. Let me know if there's anything I can help test.

flavorjones · 2021-08-26T03:20:41Z

Yeah, yikes, the Viewpoint test suite has quite a few failures with Nokogiri 1.12, I'll try to prioritize fixing this.

flavorjones · 2021-08-26T03:46:42Z

git bisect reveals this is the commit that introduced the breakage: 1f483f0

flavorjones · 2021-08-26T19:19:09Z

Ah, OK, thanks for your patience as I piece this together, my memory isn't what it used to be.

The current behavior is intentional, and the change was noted in the CHANGELOG linking to a couple of historical issues complaining about automatic namespace inheritance:

The code in Viewpoint that looks like this:

    def build!(opts = {}, &block)
      @nbuild.Envelope(NAMESPACES) do |node|
        node.parent.namespace = parent_namespace(node)
        node.Header {
          set_version_header! opts[:server_version]
          set_impersonation! opts[:impersonation_type], opts[:impersonation_mail]
          set_time_zone_context_header! opts[:time_zone_context]
          yield(:header, self) if block_given?
        }
        node.Body {
          yield(:body, self) if block_given?
        }
      end
      @nbuild.doc
    end

now needs to explicitly specify the namespace, like this:

node["soap"].Header {
 ...
}

For now, please stay on Nokogiri v1.11 and I'll ship a PR to Viewpoint.

mscrivo · 2021-08-26T19:34:19Z

amazing, thank you!

flavorjones · 2021-08-26T19:38:32Z

🤔 I'm still thinking about this, and after looking at the Viewpoint code it feels like this was a bigger breaking change than I thought it would be ...

I'm thinking now about perhaps making an attribute on a document (which can also be set in the Builder initializer) controlling whether namespaces would be inherited by reparented nodes. I'm going to play with it a bit before making a decision.

larskanis · 2021-08-26T20:22:04Z

I'm also affected by this change in code for xml signature generation. Although it's a breaking change, the new behavior makes more sense to me. And after adding explicit namespaces to child nodes the code still runs on older nokogiri versions.