From c64c2c1ba230dd3cdc7c980222abc71b6e45eacd Mon Sep 17 00:00:00 2001 From: baraline Date: Tue, 8 Sep 2026 09:20:53 +0200 Subject: [PATCH 01/14] fix(content): raise GlpiContentError and bound HTML nesting depth markdownify walks the parsed document recursively at about two CPython frames per nesting level, so roughly 494 levels of HTML exhausted the default 1000-frame limit. The converter ran as a pydantic inbound validator, so the RecursionError surfaced from inside model_validate -- a bare builtin escaping a package whose error surface is supposed to derive from GlpiError. from_transport now measures nesting first with a flat non-recursive O(n) scan and, past MAX_HTML_DEPTH (200), strips tags instead of parsing. It degrades and never truncates: every character the converting path would have produced also appears in the degraded rendering. Anything the parser still raises is wrapped in the new GlpiContentError, which inherits GlpiError only -- no ValueError, since no release ever raised a bare ValueError at these sites. The limit is set from headroom rather than from the observed cliff: a 200-level document peaks at a measured 403 frames and the client contributes 8 to 37 at the call site, so it holds until the caller's own stack passes roughly 590 frames. sys.setrecursionlimit is rejected, with the reasoning recorded beside the constant and enforced by an AST guard in the raise-site audit: a library must not mutate global interpreter state on its consumers' behalf, and raising the limit past the C stack turns a catchable RecursionError into an interpreter crash. Both halves were fuzzed against html.parser over 15000 documents with zero depth under-counts, zero over-counts and zero text losses. Four rules the scan needs that are not the obvious ones: a closing tag pops by name or is ignored, an attribute value may contain < and >, a childless leaf is still a level, and the markup forms must be matched in one left-to-right alternation to reproduce the parser's dispatch order. Co-Authored-By: Claude Opus 5 (1M context) --- glpi_python_client/__init__.py | 2 + glpi_python_client/_errors.py | 39 +- glpi_python_client/content/conversion.py | 446 ++++++++++++++- .../content/tests/test_conversion.py | 516 +++++++++++++++++- .../testing/tests/test_raise_site_audit.py | 63 ++- glpi_python_client/tests/test_errors.py | 15 + 6 files changed, 1056 insertions(+), 25 deletions(-) diff --git a/glpi_python_client/__init__.py b/glpi_python_client/__init__.py index 13e3eb8..2e1d48c 100644 --- a/glpi_python_client/__init__.py +++ b/glpi_python_client/__init__.py @@ -20,6 +20,7 @@ from glpi_python_client._async.clients import AsyncGlpiClient from glpi_python_client._errors import ( GlpiAuthError, + GlpiContentError, GlpiError, GlpiNotFoundError, GlpiProtocolError, @@ -145,6 +146,7 @@ "GetUser", "GlpiAuthError", "GlpiClient", + "GlpiContentError", "GlpiEnum", "GlpiError", "GlpiGlobalValidation", diff --git a/glpi_python_client/_errors.py b/glpi_python_client/_errors.py index 20e41b6..dca37f3 100644 --- a/glpi_python_client/_errors.py +++ b/glpi_python_client/_errors.py @@ -1,10 +1,10 @@ """Public exception hierarchy raised by :mod:`glpi_python_client`. Every exception the client raises for a bad argument, an unexpected HTTP -status, an unusable response body, or a network-level fault deliberately -derives from :class:`GlpiError`, so callers can catch the library's failure -surface with a single ``except`` clause and never need to import the -underlying HTTP library. +status, an unusable response body, a network-level fault, or content it +cannot convert deliberately derives from :class:`GlpiError`, so callers can +catch the library's failure surface with a single ``except`` clause and +never need to import the underlying HTTP library. Two deliberate exceptions to that rule remain: @@ -17,6 +17,8 @@ :class:`GlpiStatusError`, :class:`GlpiValidationError` and :class:`GlpiProtocolError` also inherit :class:`ValueError` so code written against earlier releases — which raised bare ``ValueError`` — keeps working. +:class:`GlpiContentError` and :class:`GlpiTransportError` do not, for the +reason given on each: nothing was passed in wrongly. """ from __future__ import annotations @@ -137,6 +139,34 @@ class GlpiProtocolError(GlpiError, ValueError): """ +class GlpiContentError(GlpiError): + """A rich-text content value could not be converted. + + Raised when :class:`~glpi_python_client.content.GlpiContentConverter` + cannot translate a value between GLPI's HTML transport format and the + package's canonical Markdown — in either direction. The underlying + fault is always attached as ``__cause__``. + + This exists so that no failure of the content layer escapes the + package's taxonomy. The conversion runs third-party parsers + (``markdownify`` inbound, ``markdown`` outbound), and a parser fault + used to reach the caller as a bare builtin — most visibly a + ``RecursionError``, which ``except GlpiError`` does not catch and which + a caller reading a ticket has no reason to expect from + ``get_ticket``. Deeply nested HTML is handled before it gets that far + (see :data:`glpi_python_client.content.conversion.MAX_HTML_DEPTH`); + this is the backstop for everything else. + + Unlike :class:`GlpiStatusError`, :class:`GlpiValidationError` and + :class:`GlpiProtocolError` this does **not** inherit ``ValueError``. + Those three do so for back-compatibility with releases that raised + bare ``ValueError`` at the same sites; there was never a + ``ValueError`` here to be compatible with, and a parser exhausting the + interpreter's stack is not a value the caller got wrong. The reasoning + matches :class:`GlpiTransportError`. + """ + + def status_error_class(status_code: int) -> type[GlpiStatusError]: """Return the most specific status-error class for one status code. @@ -164,6 +194,7 @@ def status_error_class(status_code: int) -> type[GlpiStatusError]: __all__ = [ "GlpiAuthError", + "GlpiContentError", "GlpiError", "GlpiNotFoundError", "GlpiProtocolError", diff --git a/glpi_python_client/content/conversion.py b/glpi_python_client/content/conversion.py index bacfeb4..f714a26 100644 --- a/glpi_python_client/content/conversion.py +++ b/glpi_python_client/content/conversion.py @@ -2,15 +2,27 @@ This module translates between GLPI's HTML transport format and the package's canonical Markdown representation used by the rich content models. + +Both conversions run third-party parsers, and both walk the document +recursively, so both have a nesting ceiling. Inbound content is measured +before it is parsed and degraded past :data:`MAX_HTML_DEPTH` rather than +allowed to hit that ceiling; anything else that goes wrong in either +direction surfaces as :class:`~glpi_python_client.GlpiContentError` so no +parser fault escapes the package's exception taxonomy. """ from __future__ import annotations import re +from collections import Counter +from html import unescape +from html.entities import html5 as _HTML5_REFERENCES from markdown import markdown as markdown_to_html from markdownify import markdownify as html_to_markdown +from glpi_python_client._errors import GlpiContentError + #: Element names that make a ``<...>`` sequence markup rather than text. #: #: The HTML5 element set, which is what the parser behind ``markdownify`` @@ -55,6 +67,181 @@ #: place. _CANDIDATE_TAG = re.compile(r"]*>") +#: Elements that cannot contain anything, so nothing nests below them. +#: +#: ``html.parser`` -- the parser ``markdownify`` builds its tree with -- +#: closes these itself, so ``
`` a thousand times over is a thousand +#: siblings, not a thousand levels. Measured: ``"
" * 5000`` parses one +#: level deep and converts fine, while ``"
" * 5000`` parses 5000 deep +#: and raises. +#: +#: The HTML5 void set, plus the ten legacy names ``bs4``'s HTML-parser tree +#: builder also treats as empty. **The invariant is that this stays a +#: subset of what the parser treats as empty**, and the direction matters: +#: a name missing from here is counted as nesting when it does not, which +#: costs an unnecessary degradation, while a name wrongly *in* here hides +#: real nesting, which is a ``RecursionError``. Listing the legacy names +#: only makes the count exact on old markup. Copied rather than imported -- +#: it lives in ``bs4.builder._htmlparser``, which is private -- and the +#: subset invariant is asserted against a real parse in the unit tests, so +#: a future ``bs4`` cannot quietly break it. +_VOID_ELEMENTS = frozenset( + """ + area base basefont bgsound br col command embed frame hr image img + input isindex keygen link menuitem meta nextid param source spacer + track wbr + """.split() +) + +#: Elements whose boundary becomes a line break when tags are stripped. +#: +#: Used only by :func:`_strip_tags`. Removing a block element outright runs +#: its neighbours together -- ``

a

b

`` becomes ``ab`` -- while +#: putting a separator at *every* tag breaks words apart, turning +#: ``offline`` into ``off line``. Splitting on the block/inline line +#: keeps both readable. An unrecognised name counts as inline, matching how +#: the normal path treats it: markup dropped, body kept in place. +_BLOCK_ELEMENTS = frozenset( + """ + address article aside blockquote br col dd details dialog div dl dt + fieldset figcaption figure footer form h1 h2 h3 h4 h5 h6 header hgroup + hr li main menu nav ol p pre search section summary table tbody td + tfoot th thead tr ul + """.split() +) + +#: Nesting depth past which inbound HTML is stripped instead of converted. +#: +#: ``markdownify`` walks the parsed tree recursively and spends about two +#: CPython frames per nesting level. Measured against the default +#: 1000-frame limit, from a shallow stack, the deepest document that +#: converts is 492 levels -- and the same 492 whether the nesting is +#: ``
``, ``

``, ``

``, ``