Skip to content

Conversation

@cataphract
Copy link
Contributor

  • Partial XML supporterd.
  • No more PHP code run during RINIT, which is problematic for some 3rd party extensions. Instead libxml2 is bundled and called directly.

Description

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@cataphract cataphract requested review from a team as code owners January 7, 2026 17:41
@cataphract
Copy link
Contributor Author

@codex review

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 06500381d5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@codecov-commenter
Copy link

codecov-commenter commented Jan 7, 2026

Codecov Report

❌ Patch coverage is 71.08108% with 107 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.90%. Comparing base (4a3a987) to head (ef4a88e).

Files with missing lines Patch % Lines
appsec/src/extension/xml_truncated_parser.c 70.11% 72 Missing and 35 partials ⚠️

❌ Your patch status has failed because the patch coverage (71.08%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3558      +/-   ##
==========================================
+ Coverage   61.79%   61.90%   +0.10%     
==========================================
  Files         139      140       +1     
  Lines       13114    13281     +167     
  Branches     1728     1758      +30     
==========================================
+ Hits         8104     8221     +117     
- Misses       4240     4272      +32     
- Partials      770      788      +18     
Files with missing lines Coverage Δ
appsec/src/extension/ddappsec.c 75.25% <100.00%> (+0.08%) ⬆️
appsec/src/extension/entity_body.c 78.70% <100.00%> (+5.22%) ⬆️
appsec/src/extension/xml_truncated_parser.c 70.11% <70.11%> (ø)

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a3a987...ef4a88e. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cataphract cataphract force-pushed the glopes/xml-parser branch 3 times, most recently from de228f3 to df15b71 Compare January 7, 2026 19:09
* Partial XML supporterd.
* No more PHP code run during RINIT, which is problematic for some 3rd
  party extensions. Instead libxml2 is bundled and called directly.
@cataphract cataphract merged commit 64813c5 into master Jan 8, 2026
2000 of 2008 checks passed
@cataphract cataphract deleted the glopes/xml-parser branch January 8, 2026 11:57
@github-actions github-actions bot added this to the 1.16.0 milestone Jan 8, 2026
@realFlowControl
Copy link
Member

realFlowControl commented Jan 8, 2026

Hey @cataphract,

just for my understanding:

  • Would it have been an option to dlsym() libxml2 from PHP if PHP is compiled with libxml?
  • Could we git submodule add https://gitlab.gnome.org/GNOME/libxml2.git (and change to the right tag/commit) instead?

I might just not be aware of it (but I assume as there are other third party deps already) if there is some kind of process that helps us in monitor those upstream dependencies for new releases / bug fixes / security issues?

@cataphract
Copy link
Contributor Author

cataphract commented Jan 9, 2026

@realFlowControl

Would it have been an option to dlsym() libxml2 from PHP if PHP is compiled with libxml?

I don't think so. We don't compile together with PHP, so we don't know which version of the headers of libxml PHP used. Therefore we could have mismatch between the data structures in that runtime and build time versions.

Could we git submodule add https://gitlab.gnome.org/GNOME/libxml2.git (and change to the right tag/commit) instead?

This is add a bit of delay on downloading the submodule and prevents us from changing patching configuration files... However, I'll improve the vendoring script so it doesn't require extensive modifications (e.g. in the file list) for each new version.

There's also the disadvantage the build would break not just when github is down but also when the gitlab instacne of gnome is down.

I might just not be aware of it (but I assume as there are other third party deps already) if there is some kind of process that helps us in monitor those upstream dependencies for new releases / bug fixes / security issues?

I'm not sure about any such process... But we could add a check during CI that we're on the latest version of libxml2

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants