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

QuickJS: xml module. #861

Merged
merged 4 commits into from
Mar 27, 2025
Merged

QuickJS: xml module. #861

merged 4 commits into from
Mar 27, 2025

Conversation

xeioex
Copy link
Contributor

@xeioex xeioex commented Mar 5, 2025

No description provided.

@xeioex xeioex requested a review from VadimZhestikov March 5, 2025 07:20
@xeioex xeioex force-pushed the xml branch 3 times, most recently from 28f10ca to ba34e2f Compare March 7, 2025 03:01
@xeioex xeioex force-pushed the xml branch 5 times, most recently from 593cf7b to c1348c5 Compare March 20, 2025 16:07
xeioex added 4 commits March 26, 2025 21:24
Previously, serializeToString() was exclusiveC14n() which returned
string instead of Buffer. According to the published documentation it
should be c14n().
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces a comprehensive set of tests for the new XML module, including parsing, canonicalization, modification, external entity handling, and SAML signature verification. Changes include adding and updating XML tests in the test/xml directory, updating the workflow dependencies to include libxml2-dev for i386, and refining file system tests in test/fs/methods.t.mjs to conditionally skip tests for QuickJS.

Reviewed Changes

Copilot reviewed 6 out of 10 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test/xml/xml.t.mjs Adds extensive tests for XML parsing, canonicalization, modification, and error handling.
.github/workflows/check-pr.yml Updates dependency installation to include libxml2-dev:i386 along with other required packages.
test/xml/external_entity_ignored.t.js Removes the compatXml.js include and adjusts the condition to run the test without checking has_xml().
test/xml/saml_verify.t.mjs Updates the SAML verification tests to import xml directly and adjusts error message handling and skip logic.
test/fs/methods.t.mjs Adds a QuickJS detection helper and updates file system tests to conditionally skip tests under QuickJS.
Files not reviewed (4)
  • auto/qjs_modules: Language not supported
  • external/njs_xml_module.c: Language not supported
  • src/qjs.h: Language not supported
  • src/test/njs_unit_test.c: Language not supported
Comments suppressed due to low confidence (2)

test/xml/external_entity_ignored.t.js:16

  • Removing the has_xml() check may cause the test to run even when the xml module is unavailable. Consider verifying that the xml module is always present or reinstate the condition if necessary.
if (has_njs()) {

test/xml/saml_verify.t.mjs:277

  • Removing the has_xml() condition in the skip function could lead to test execution in environments without the xml module. Please ensure the xml module is consistently available in the test environment or adjust the condition accordingly.
skip: () => (!has_njs() || !has_webcrypto()),

Copy link
Contributor

@VadimZhestikov VadimZhestikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@xeioex xeioex merged commit cec9a16 into nginx:master Mar 27, 2025
1 check passed
@xeioex xeioex deleted the xml branch March 27, 2025 16:04
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.

2 participants