Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Not compatible with Nokogiri >= 1.12.0 #30

Closed
jweir opened this issue Aug 27, 2021 · 5 comments · Fixed by #31
Closed

Not compatible with Nokogiri >= 1.12.0 #30

jweir opened this issue Aug 27, 2021 · 5 comments · Fixed by #31

Comments

@jweir
Copy link

jweir commented Aug 27, 2021

There is a problem with add_namespace_definition and the Signer in Nokogiri 1.12 and up. I am adding this ticket here so @flavorjones can see see the impact on this gem as he is working on a solution.

See sparklemotion/nokogiri#2317

There are a number of failing tests

Finished in 0.04294 seconds (files took 0.52242 seconds to load)
18 examples, 6 failures

Here is a sample via Nokogiri 1.12.3

node.add_namespace_definition('','wsse')
node.to_xml
=> "<BinarySecurityToken xmlns:=\"wsse\" ValueType=\"http://docs.oasis-open.org/wss/2004/01/oasis-200401-wss-x509-token-profile-1.0#X509v3\" EncodingType=\"http://docs.oasis-open.org/wss/2004/01/oasis-200401-wss-soap-message-security-1.0#Base64Binary\">MIICs<<CUT>>>jlyrGYJlLli1NxHiBz7FCEJaVI8=</BinarySecurityToken>"

Here is another sample via Nokogiri 1.11.7

node.add_namespace_definition('','wsse')
node.to_xml
=> "<wsse:BinarySecurityToken xmlns:=\"wsse\" ValueType=\"http://docs.oasis-open.org/wss/2004/01/oasis-200401-wss-x509-token-profile-1.0#X509v3\" EncodingType=\"http://docs.oasis-open.org/wss/2004/01/oasis-200401-wss-soap-message-security-1.0#Base64Binary\">MIICs<<CUT>>>jlyrGYJlLli1NxHiBz7FCEJaVI8=</wsse:BinarySecurityToken>"

The difference is the wsse is NOT added to the node in the 1.12.3 version.

@flavorjones
Copy link
Contributor

ACK. #2317 is the underlying issue to follow. Hoping to have a fix out this weekend.

@flavorjones
Copy link
Contributor

See sparklemotion/nokogiri#2320 for the changes I'd like to make. Comments and feedback welcome.

If that PR is what lands in a patch release of Nokogiri v1.12, then the signer gem would need to make only one small change to accomodate, and I'd be happy to submit it as a PR. The patch would be something like:

diff --git a/lib/signer.rb b/lib/signer.rb
index 16b9d25..4941355 100644
--- a/lib/signer.rb
+++ b/lib/signer.rb
@@ -65,6 +65,7 @@ def initialize(document, noblanks: true, wss: true, canonicalize_algorithm: :c14
     self.document = Nokogiri::XML(document.to_s) do |config|
       config.noblanks if noblanks
     end
+    self.document.namespace_inheritance = true if self.document.respond_to?(:namespace_inheritance)
     self.digest_algorithm = :sha1
     self.wss = wss
     self.canonicalize_algorithm = canonicalize_algorithm
diff --git a/signer.gemspec b/signer.gemspec
index ad5ca9f..405d6af 100644
--- a/signer.gemspec
+++ b/signer.gemspec
@@ -21,5 +21,5 @@ Gem::Specification.new do |gem|
   gem.add_development_dependency 'rake'
   gem.add_development_dependency 'rspec'
 
-  gem.add_runtime_dependency 'nokogiri', '>= 1.5.1'
+  gem.add_runtime_dependency 'nokogiri', '>= 1.5.1', '!=1.12.3', '!=1.12.2', '!=1.12.1', '!=1.12.0'
 end

flavorjones added a commit to flavorjones/signer that referenced this issue Aug 29, 2021
Nokogiri v1.12 introduced a breaking change related to namespace
inheritance of reparented child nodes. This commit using the feature added in
v1.12.4 to opt

- for new versions of Nokogiri that support it, set `Document#namespace_inheritance`
- intermediate versions of Nokogiri will be avoided via gemspec version specification
- old versions of Nokogiri will continue to work

See sparklemotion/nokogiri#2320 for more details.

Fixes ebeigarts#30
@flavorjones
Copy link
Contributor

I expect to ship Nokogiri v1.12.4 today making a workaround available. I've submitted a PR to signer at #31 which I'll change from "draft" to "ready" shortly thereafter.

@flavorjones
Copy link
Contributor

Nokogiri 1.12.4 is out, and #31 is ready for review.

@Envek
Copy link
Collaborator

Envek commented Aug 30, 2021

#29 seems to be related to this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants