Security Agent Report

SECURITY_PASS

Security Review — PR #105

Scope

All 7 changed files are test files under tests/. No production source code was modified.

tests/conftest.py
tests/image/test_svg.py
tests/oxml/test_footnotes.py
tests/oxml/test_numbering.py
tests/oxml/test_shape.py
tests/oxml/test_shared.py
tests/oxml/test_simpletypes.py

Checks Performed

Details

• No new dependencies: The only new import is unittest.mock.MagicMock from the Python standard library.
• No XXE risk: XML is constructed via the project's own cxml.element() helper with static, controlled strings. No external entities or DTD references are present.
• No path traversal: All file I/O uses io.BytesIO in-memory buffers — no filesystem paths involved.
• Test data is static and benign: SVG byte strings, numeric literals, and date strings used in parametrize fixtures contain no executable or injectable content.
• No secrets: Reviewed all string literals; none resemble API keys, tokens, or credentials.

Conclusion

PR #105 introduces test coverage only. There are no security concerns.

Review Agent

REVIEW_CHANGES_NEEDED

Summary

PR #105 adds test coverage for several previously untested modules: simpletypes, numbering, shape, shared, SVG image parsing, and footnote edge cases. The tests follow project conventions (BDD-style naming, cxml.element() for XML fixtures, cast() for type safety). Most are correct and valuable. Two issues require changes before merging.

Issues

1. Duplicate test — tests/oxml/test_footnotes.py

DescribeCT_Footnotes_EdgeCases.it_returns_2_when_only_reserved_ids_exist (lines 130–140) is an exact duplicate of the pre-existing DescribeCT_Footnotes.it_can_add_a_footnote (lines 24–44): same input (footnotes with w:id=0 and w:id=1), same core assertion (footnote.id == 2). The existing test just also verifies the structure of the returned element.

The only genuinely new scenario in DescribeCT_Footnotes_EdgeCases is it_returns_2_when_no_footnotes_exist (empty w:footnotes element), which provides real coverage.

Action: Remove it_returns_2_when_only_reserved_ids_exist; it adds maintenance burden without covering any new code path.

2. Unused fixture — tests/conftest.py

fake_package (lines 24–28) is added to the shared conftest.py but is not referenced by any test in this PR:

@pytest.fixture
def fake_package() -> OpcPackage:
    """A mock OpcPackage suitable for constructing parts in tests."""
    return MagicMock(name="package")

Adding dead fixtures to the shared conftest makes the test infrastructure harder to reason about. The project convention (see fake_parent) is to add fixtures when they are actually used.

Action: Remove fake_package (including its OpcPackage TYPE_CHECKING import) and add it back when a test actually needs it.

Minor observation (no change required)

DescribeBaseSimpleType.it_can_convert_to_xml in tests/oxml/test_simpletypes.py:

def it_can_convert_to_xml(self):
    assert BaseSimpleType.to_xml(42) == BaseSimpleType.convert_to_xml(42)

BaseSimpleType.convert_to_xml is a stub (...) that returns None, so this assertion reduces to None == None. The test passes but validates nothing meaningful. This is not a blocker, but worth noting if coverage quality is a goal.

What looks good

• test_simpletypes.py: Comprehensive coverage of all ST_* and Xsd* types. Unit conversions for ST_UniversalMeasure, ST_HpsMeasure, ST_SignedTwipsMeasure, ST_TwipsMeasure are arithmetically correct.
• test_svg.py: All expected pixel values for unit conversions are correct (verified against the _parse_length implementation). Non-UTF-8 and invalid-XML fallback tests are well-targeted.
• test_numbering.py: Gap-filling logic for _next_numId is tested correctly; assertions match the implementation.
• test_shape.py: CT_ShapeProperties and CT_Transform2D coverage is correct and matches the source.
• test_shared.py: Accurate and concise.

Security Agent Report

SECURITY_PASS

Security Review — PR #105

Summary

PR #105 contains test-only changes — no production source code was modified. All changes are additions to the tests/ directory.

Files Reviewed

Checks

Injection Risks

No injection risks. All XML in the tests is hardcoded static literals passed through the existing cxml.element() test utility or io.BytesIO. No user-controlled input flows into XML parsers.

XXE Attacks

No XXE risk. Tests use the project's existing parse_xml / cxml infrastructure which is already covered by production code review. No new XML parsing paths are introduced.

Path Traversal

No file paths constructed from test data. All imports and file references are static.

New Dependencies

No new dependencies added (requirements*.txt, pyproject.toml, setup.cfg are unchanged).

Secrets / Credentials

No API keys, tokens, passwords, or credentials present anywhere in the diff.

Data Exposure / Unsafe File Handling

No file I/O beyond in-memory io.BytesIO streams. No temp files created, no disk writes.

Notable Observations (Non-Security)

• test_svg.py adds tests for malformed inputs (b"\x80\x81..." invalid UTF-8, b"<svg><not-valid-xml" truncated XML). These exercise defensive behaviour in the production SVG parser — they are security-positive additions, not risks.
• test_simpletypes.py tests input validation and rejection of out-of-range values — again security-positive coverage.

Conclusion

No security issues found. The PR consists entirely of unit tests that improve coverage of existing production code with no new attack surface.

Review Agent

REVIEW_CHANGES_NEEDED

Summary

PR #105 adds test coverage for several modules: SVG image parsing, footnotes oxml, numbering oxml, shape oxml, shared oxml, and simpletypes. The tests are generally well-structured and follow the project's BDD naming conventions. However, there are two real issues:

Issue 1: Convention violation — DescribeCT_Footnotes_EdgeCases should not be a separate class

File: tests/oxml/test_footnotes.py:122

The project convention is one Describe* class per class under test (see CLAUDE.md: "Test classes: Describe* pattern"). The new class DescribeCT_Footnotes_EdgeCases tests CT_Footnotes — the same class already covered by DescribeCT_Footnotes. There is no reason to introduce a second describe class for the same subject.

Move the single test method into the existing DescribeCT_Footnotes class:

# In DescribeCT_Footnotes (already exists):
def it_returns_2_when_no_footnotes_exist(self):
    footnotes = cast(CT_Footnotes, element("w:footnotes"))
    footnote = footnotes.add_footnote()
    assert footnote.id == 2

Remove the DescribeCT_Footnotes_EdgeCases class entirely.

Issue 2: Vacuous test in DescribeBaseSimpleType

File: tests/oxml/test_simpletypes.py — DescribeBaseSimpleType.it_can_convert_to_xml

def it_can_convert_to_xml(self):
    assert BaseSimpleType.to_xml(42) == BaseSimpleType.convert_to_xml(42)

BaseSimpleType.convert_to_xml is a stub method body (...) that returns None. BaseSimpleType.to_xml also ultimately returns None because it delegates to the same stub. This assertion reduces to None == None, which is always True — it does not exercise any real behavior.

Either:

• Remove this test (it provides no coverage value), or
• Test the delegation using a concrete subclass that actually implements convert_to_xml, or
• Test that to_xml calls validate before serializing (which is the real contract of that method).

What's Good

• SVG tests (it_handles_non_utf8_binary_data, it_handles_invalid_xml, it_parses_various_unit_types, it_handles_viewBox_with_comma_separators) correctly match the from_stream fallback logic and _parse_dimensions implementation.
• test_numbering.py is comprehensive: covers CT_Num, CT_NumLvl, CT_NumPr, CT_Numbering, including the gap-filling behavior of _next_numId.
• test_shape.py and test_shared.py are clean, follow project conventions, and cover both present and absent element states.
• test_simpletypes.py covers a wide range of types with parametrized tests where appropriate. The DescribeST_DateTime suite correctly tests the epoch fallback, fractional seconds, and naive datetime handling.

Security Agent Report

SECURITY_PASS

Security Review — PR #105

Reviewer: Security Agent
Date: 2026-04-05
Branch: agent/issue-83 → master

Summary

PR #105 is a test-only change. All 698 lines added are in the tests/ directory. No production source code was modified.

Checks

Injection risks (XML injection, XXE, path traversal): NONE
All XML in test files is constructed via the project's own cxml.element() helper or inline io.BytesIO byte strings with static content. No user-controlled input is fed into XML parsers. The SVG tests (it_handles_invalid_xml, it_handles_non_utf8_binary_data) are specifically verifying that the library handles malformed or binary input gracefully — a security-positive addition.

Dependency risks: NONE
No new dependencies introduced. Only pytest, io, typing, and existing project imports are used.

Data exposure / unsafe file handling: NONE
No file I/O to disk, no network access, no use of tempfile, open(), or os.path. All test data is in-memory via io.BytesIO.

Secrets in code: NONE
No API keys, tokens, passwords, or credentials present.

Conclusion

No security issues found. The PR is clean.

Review Agent

REVIEW_CHANGES_NEEDED

PR #105 — Test Coverage Additions

Overall this is a solid test-coverage PR. The new test files follow project conventions
(Describe* classes, it_* methods, cxml.element() helpers), and the logic in
test_numbering, test_shape, test_shared, and test_svg is correct.

Issues requiring changes

1. Tautological convert_to_xml assertions (tests/oxml/test_simpletypes.py)

Two tests derive the expected value using the same computation as the implementation,
making them circular—they cannot detect a wrong multiplier or broken roundtrip.

DescribeST_SignedTwipsMeasure.it_converts_to_xml (lines 342–345):

# current — will always pass even if convert_to_xml is broken
def it_converts_to_xml(self):
    result = ST_SignedTwipsMeasure.convert_to_xml(Twips(720))
    twips_val = Emu(Twips(720)).twips   # same computation as the implementation
    assert result == str(twips_val)

Should be:

def it_converts_to_xml(self):
    result = ST_SignedTwipsMeasure.convert_to_xml(Twips(720))
    assert result == "720"

DescribeST_TwipsMeasure.it_converts_to_xml (lines 383–386):

# current — same problem
def it_converts_to_xml(self):
    result = ST_TwipsMeasure.convert_to_xml(Twips(1440))
    twips_val = Emu(Twips(1440)).twips
    assert result == str(twips_val)

Should be:

def it_converts_to_xml(self):
    result = ST_TwipsMeasure.convert_to_xml(Twips(1440))
    assert result == "1440"

The hardcoded expected values are easy to verify:
Twips(720) stores 720 × 635 = 457200 EMU → 457200 / 635 = 720 twips → "720".
Twips(1440) stores 1440 × 635 = 914400 EMU → 914400 / 635 = 1440 twips → "1440".

Minor notes (no change required)

• DescribeST_Coordinate.it_validates (line 210–211): only exercises the happy path with
  no assertion — acceptable for a coverage test, but doesn't verify any error condition.
• DescribeST_DateTime.it_converts_naive_datetime_to_xml (line 247–251): only checks
  endswith("Z") because the converted time depends on the system timezone, which is
  correct given the implementation.
• DescribeXsdLong.it_validates_in_range only tests the upper bound; the lower bound
  (-9223372036854775808) is exercised indirectly via XsdInt which shares the same
  validate_int_in_range path. Acceptable.

Review Agent

REVIEW_CHANGES_NEEDED

PR #105 Review

This PR adds test coverage across several modules: tests/image/test_svg.py, tests/oxml/test_footnotes.py, tests/oxml/test_numbering.py, tests/oxml/test_shape.py, tests/oxml/test_shared.py, and tests/oxml/test_simpletypes.py. No production code is changed.

Most tests are well-written, follow project conventions (Describe* classes, it_* methods), and correctly validate the implementations. Two issues require attention:

Issue 1: Tautological assertions in test_simpletypes.py (real test quality bug)

tests/oxml/test_simpletypes.py, DescribeST_SignedTwipsMeasure.it_converts_to_xml and
DescribeST_TwipsMeasure.it_converts_to_xml

Both tests compute the expected value using the same formula as the production code, providing no meaningful correctness check:

# ST_SignedTwipsMeasure
def it_converts_to_xml(self):
    result = ST_SignedTwipsMeasure.convert_to_xml(Twips(720))
    twips_val = Emu(Twips(720)).twips   # <-- reproduces implementation logic
    assert result == str(twips_val)     # comparing output to itself

# ST_TwipsMeasure
def it_converts_to_xml(self):
    result = ST_TwipsMeasure.convert_to_xml(Twips(1440))
    twips_val = Emu(Twips(1440)).twips  # <-- reproduces implementation logic
    assert result == str(twips_val)

The production implementations (simpletypes.py:369-371 and ~:405-407) do exactly Emu(value).twips → str(twips). If Emu.twips had a bug, these tests would still pass.

Use literal expected values instead:

def it_converts_to_xml(self):
    result = ST_SignedTwipsMeasure.convert_to_xml(Twips(720))
    assert result == "720"

def it_converts_to_xml(self):
    result = ST_TwipsMeasure.convert_to_xml(Twips(1440))
    assert result == "1440"

(720 twips × 635 EMU/twip = 457200 EMU → 457200/635 = 720 twips; 1440 twips → "1440")

Everything else looks good

• test_footnotes.py: it_returns_2_when_no_footnotes_exist correctly tests the reserved-ID boundary (_next_available_footnote_id returns 2 when used_ids=[]).
• test_numbering.py: All five test cases are correct and cover the full public API including gap-filling behavior (tested against the documented _next_numId algorithm).
• test_shape.py: Tests correctly verify the delegation chain CT_ShapeProperties → CT_Transform2D → CT_PositiveSize2D.
• test_shared.py: All assertions match the implementations.
• test_svg.py: Unit and parametrized tests correctly exercise all code paths in Svg.from_stream and _parse_dimensions, including the UTF-8 fallback, invalid-XML fallback, and all CSS unit conversions.