Skip to content

Conversation

@johnnyshields
Copy link
Collaborator

@johnnyshields johnnyshields commented Jan 11, 2025

UPDATE: This PR is abandoned, but please leave it open. I will close it when Nokogiri migration work is completed.


Fixes #729
Fixes #707

Currently Ruby Saml uses a mix of REXML and Nokogiri. This is not ideal. We should use Nokogiri everywhere, because it can do everything REXML can do.

In addition, note:

  • Ruby Saml is currently setting REXML::Security.entity_expansion_limit = 0 which affects it for all gems in the project.
  • The above is not thread-safe, while Nokogiri is thread-safe by design.
  • REXML does not use Semver, and exposes users to sudden breakage.

This refactor will probably get rid of RubySaml::XML::Document, etc.. Currently these are subclasses of REXML::Document. What we should do instead is make them procedural classes like DocumentSigner and SignedDocumentValidator. Hence I'm thinking it will be v3.0

@pitbulk
Copy link
Collaborator

pitbulk commented Jan 13, 2025

@johnnyshields, let's replace REXML with Nokogiri on V2 and in V3 in addition, we can let's get rid of RubySaml::XML::Document as you suggested.

@johnnyshields johnnyshields changed the title [WIP] v3.x - Replace REXML with Nokogiri [WIP] v3.0 - Replace REXML with Nokogiri Jan 19, 2025
@pitbulk
Copy link
Collaborator

pitbulk commented Jan 19, 2025

@johnnyshields , any progress here? we need to drop REXML before the v2 release and I want to also plan to do that on master branch.

@johnnyshields
Copy link
Collaborator Author

@pitbulk it's way too big a change, and potentially could introduce issues. Let's aim to do it in 3.0 and release what we have as 2.0

@kwent
Copy link

kwent commented Feb 14, 2025

Looking forward to this.

@johnnyshields johnnyshields marked this pull request as draft March 8, 2025 14:35
@johnnyshields
Copy link
Collaborator Author

Abandoned this branch, see my other PRs

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 this pull request may close these issues.

3 participants