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

Add support for configuration of XSLT security #1789

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

maths22
Copy link

@maths22 maths22 commented Aug 21, 2018

No description provided.

@flavorjones
Copy link
Member

@maths22 Thank you for submitting this! And apologies for my slow reply. Will look at it and optimistically would like to ship in v1.11.0 (see milestone).

Base automatically changed from master to main January 17, 2021 21:52
@flavorjones flavorjones modified the milestones: v1.12.0, v1.13.0 Aug 2, 2021
@flavorjones flavorjones modified the milestones: v1.13.0, v1.14.0 Jan 6, 2022
@flavorjones
Copy link
Member

I just rebased this off current main and added an assertion about NotImplementedError to the tests for JRuby.

@flavorjones
Copy link
Member

Again, apologies for the long delay in reviewing this and providing feedback.

There are a few things about this PR that I'd like to change:

  • naming. Currently Nokogiri uses the word "options" (e.g., ParseOptions, SaveOptions) and this PR introduces two new words for the same concept: "prefs" and "config". I see that the libxslt docs use "prefs" but in the past the places where we've closely mirrored the libxml2 API are generally the parts of the API I regret most. I think we can improve on libxslt's API.
  • initial values. It's not clear from this code or the tests what the initial values of these options is, and the setter isn't called by default.
  • tests. I'd really like to have some tests exercising each of these libxslt features so we know whether we accidentally break them in a future release. this again is a hard-won lesson from not doing so when implementing ParseOptions or SaveOptions.

If you're not interested in working on this PR, given the long time delay, I understand. But please let me know if you think you'll be able to work on it? Otherwise I'll keep it open and hopefully find time to address some of this feedback.

@flavorjones flavorjones removed this from the v1.14.0 milestone Aug 28, 2022
@maths22
Copy link
Author

maths22 commented May 23, 2023

Sorry for the big delay here. I've just pushed up a new version that I think exposes a more idiomatic ruby API, has more logical defaults, and checks that the underlying library gets all the config params as expected.

(I'm starting to work on getting nokogiri-xmlsec-instructure passing the build inside nokogiri proper, and currently this xmlsec stuff is shoved into that library where it definitely doesn't belong)

@flavorjones
Copy link
Member

@maths22 ❤️ ❤️ ❤️ Thanks for tackling this and the nokogiri-xmlsec integration.

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

Successfully merging this pull request may close these issues.

None yet

2 participants