pptx: fix the review's first ten findings (safety and bugs)

[dvejsada]

Fixes items 1–10 of the "Fix first" list from the combined PowerPoint review. Item 11, per-request resource limits, is deliberately out of scope.

Safety

• Exponential backtracking in the bold regex (inline_markdown._BOLD, used by both Word and PowerPoint). Inside a bold span, every *x could be read two ways: as a lone star, or as the opener of a nested italic. An unclosed span therefore had exponentially many parses to reject. A 67-character title held the GIL for 1.5 s, and every two more characters multiplied that by about 2.6.
  • The nested-italic unit now appears only in the closer. A 20k-character input takes about 40 ms.
  • I compared the old and new grammar on every string of up to 10 characters from {*, a, space}. They differ in two degenerate inputs, and in both the new result matches the documented behaviour.
• Admin session with no password. With ADMIN_ENABLED set but no ADMIN_PASSWORD or API_KEY, the cookie-signing secret was a hash of a constant in the source. The gate accepted any signed session, so a hand-signed cookie opened the admin UI. Now the gate is locked (make_before(locked=True)) and the secret is random per process.
• Merge bounds. Each merge was expanded into a set of cells before the bounds check. One 1000×1000 span on a 2×2 table allocated about 110 MB. The bounds are now checked first, and overlap is tested by rectangle intersection.
• Quadratic regexes in schema.py. The markdown heading regex took 4 s at 20k characters and runs on the event loop inside argument validation. The blank-slide position regex took 1.6 s. Both are rewritten to run in linear time and give the same results as before on all short inputs.
• Link scheme allow-list (Word and PowerPoint). Only http, https, mailto and tel targets become clickable, checked by inline_markdown.is_safe_link_target. Any other link keeps its label as plain text and is reported once as link_refused (warning).

Bugs

• Invalid table theme colours. Table fills with dark1, dark2, light1 or light2 wrote those names into <a:schemeClr>, but that attribute only accepts dk1, dk2, lt1 and lt2. The file could not be read back. The names are now mapped through scheme_color_val().
• Chart label position. data_labels: true wrote dLblPos="outEnd" on all 11 chart types. It is now set only on bar, column and pie, where PowerPoint accepts it.
• Control characters. A control character in the footer, a section title or chart data crashed the whole deck. Caller text is now cleaned once after validation and reported as control_chars_removed (info). A vertical tab or form feed becomes a line break.
• NaN and Infinity. In chart values, scatter points, table widths or blank-slide positions, these failed deep inside the build. They are now rejected during validation, with the field's path in the error.
• Crashes reported as bad input. Any exception from a slide builder was re-raised as ValueError, without its traceback, and reported as "Invalid presentation input". Only a ValueError still takes that path. Anything else becomes a RuntimeError chained to the original and logged with exc_info.

Behaviour changes

• A link with no scheme, such as [site](example.com), is no longer clickable. Office resolved it as a file path relative to the document.
• A vertical tab in a slide body becomes a line break, so it can start a new bullet line.

Tests and docs

• New tests: test_admin_no_password.py, test_link_schemes.py, test_pptx_control_chars.py, test_pptx_non_finite_numbers.py, test_pptx_internal_errors.py. Regression tests were added to the inline-markdown, table-formatting, schema, client-compat and blank-slide tests.
• Each new test fails on the previous code. The full offline suite passes (2637), and ruff is clean.
• Updated docs:
  • Tool descriptions in main.py.
  • User docs: docs/powerpoint-slides.md, docs/markdown-reference.md, SECURITY.md, docs/admin-ui.md, docs/deployment.md.
  • Developer docs: powerpoint.md (pipeline, invariants, tests), word.md, shared-modules.md, architecture.md, admin-ui.md.
  • AGENTS.md has a new rule for links.

Known follow-up

Five other unclosed-marker shapes are still quadratic, at about 2.5 s for 32k characters: italic with spaces, strikethrough, underline, highlight, and a link that is never closed. They grow with field length, so they belong with the per-request length limits (item 11).

🤖 Generated with Claude Code

https://claude.ai/code/session_012SPJrnAL7ChjLJ78yMxPJm

---

Generated by Claude Code

[dvejsada]

@claude please review this PR for correctness bugs and security regressions.

Where to look:

• The _BOLD grammar change in inline_markdown.py, which Word and PowerPoint both use.
• The link-scheme allow-list and the pre-scan that reports refused links.
• The locked admin gate when no password is set.
• Whether cleaning control characters from the validated slide models in place in _scrub_caller_text is safe.

Also flag any behaviour change the PR description doesn't mention.

---

Generated by Claude Code

[claude]

Claude finished @dvejsada's task in 5m 52s —— View job

---

Review: correctness bugs and security regressions in the pptx safety/bugs fixes

• Gather context / read diff
• Review _BOLD grammar change in inline_markdown.py
• Review link-scheme allow-list and refusal pre-scan
• Review locked admin gate with no password
• Review _scrub_caller_text in-place mutation safety
• Check for undocumented behaviour changes
• Post final review

Finding: the refused-link pre-scan silently undercounts refusals for ![](target)-shaped text outside a real image block

inline_markdown.refused_link_targets() deliberately skips any match preceded by ! (_LINK_ANYWHERE_RE's (?<!!), inline_markdown.py:52-54), on the theory that ![](src) "is a block the renderer loads, not a link." That's only actually true for Word, and only when the image markdown is the entire line: docx_tools/patterns.py's IMAGE_PATTERN = re.compile(r'^!\[([^\]]*)\]\(([^)]+)\)$') is matched with .match() against the whole stripped line in markdown_processor.py:413, so it only fires for a standalone image paragraph.

Anywhere else, the same ![]() text reaches the shared inline-formatting regex instead, which has no special case for a leading !:

• PowerPoint has no inline image markdown support at all — pptx_tools/inline_formatting.py's _parse_segment has no !-aware branch, so ![](javascript:evil) in any slide field (bullet, title, table cell, KPI label, …) is split into a literal ! plus a real link token [alt](javascript:evil), matched by _LINK_RE.
• In Word, the same happens for ![]() written mid-paragraph rather than as its own line (e.g. "See for details"), since IMAGE_PATTERN won't match a non-image line.

In both cases the renderer's own is_safe_link_target() check still correctly refuses an unsafe scheme and keeps the label as plain text — this is not a clickability/security bypass. But refused_link_targets() (the pre-scan pptx_tools/slide_builder.py:185-188 and docx_tools/base_docx_tool.py:122 use to populate the link_refused warning) never sees these targets, because they're excluded by the !-lookbehind before is_safe_link_target is even consulted. The caller/model is never told its link was dropped — a silent degradation, which is exactly what this project's warnings-channel rule (AGENTS.md: "A branch that skips, substitutes or degrades something the caller asked for reports it") says must not happen.

This is even visible in the PR's own new test (tests/test_link_schemes.py:41-42): refused_link_targets("![](data:image/png;base64,xx) ...") is asserted to exclude the data: target — but that string isn't a standalone image line (it shares the line with other content), so in an actual Word/PowerPoint render it would not take the image path; it takes the same silently-dropped-warning path described above.

Not a security regression (the renderer's own scheme check still holds at render time), but a genuine warnings-channel correctness bug and worth fixing before/soon after merge, since it's easy for a caller to end up with a slide that silently lost a link with no explanation.

Fix this →

The four requested areas, in detail

_BOLD grammar (inline_markdown.py:90-115) — sound. The old grammar let every *x inside a bold span be read two ways (a lone star, or the opener of a nested italic), which is what produced the exponential blowup. The new body alternation ([^*] vs \*(?!\*)) is disjoint — a star and a non-star can never both match the same position — so each character has exactly one parse per position, and only the stopping point of the non-greedy loop is ambiguous (linear positions). The one place I'd double check further: pathological 3+-star runs like **a*** (not covered by the PR's stated 10-char/3-symbol brute-force comparison in combination with _BOLD_ITALIC's ordering) fail to match anything and fall through as literal text — plausible and pre-existing-shaped, not a new regression, but wasn't explicitly called out.

Link-scheme allow-list (inline_markdown.py:58-64, rendering call sites) — the actual enforcement (is_safe_link_target() gating run.hyperlink.address / add_hyperlink()) is correct and applied consistently in both pptx_tools/inline_formatting.py:261 and docx_tools/inline_formatting.py:43. The only issue is the pre-scan undercount above.

Locked admin gate (admin/auth.py, admin/app.py:482-493) — correct. check_password() returns False unconditionally when expected is falsy, so login can never succeed with no password configured, independent of the gate; make_before(locked=True) ignores session state entirely rather than trusting a signed cookie, closing the actual vulnerability (a hand-signed cookie against the old constant-derived secret). tests/test_admin_no_password.py covers the regression directly, including the "old constant secret" cookie-forgery scenario. No gaps found.

_scrub_caller_text in-place mutation (pptx_tools/slide_builder.py:162-189, pptx_tools/helpers.py:351-387) — safe. scrub_control_chars() mutates lists/dicts by index/key assignment and pydantic model instances via setattr, and none of the schema models in pptx_tools/schema.py set frozen=True or validate_assignment=True, so the plain attribute writes neither raise nor re-trigger validation. Tuples are the one immutable container, but the recursive rebuild-and-reassign pattern handles those correctly too (a tuple nested in a mutable container gets written back; a tuple that is a model field gets setattr back onto the model). Scrubbing runs before _build_slides(), so builders only ever see cleaned text. No aliasing/double-mutation issues found.

Other undocumented behaviour change

Beyond the pre-scan bug above, I didn't find other unmentioned behaviour changes — the "no-scheme link" and "vertical tab → line break" changes are both called out in the PR description already, and the chart label-position fix (pptx_tools/chart_utils.py) correctly gates on the schema's own chart_type string (bar/column/pie, not *_stacked), matching what's described.
· Branch: claude/admiring-goodall-6vwwd6

[dvejsada]

Fixed the refused-link undercount in b17c98a. Thanks for catching it.

• refused_link_targets() now counts text shaped like [alt](src), because PowerPoint and mid-line Word text render it as ! followed by a link.

• Word removes two things before it scans, because neither makes a link:

  • lines that are only an image, matched with the same IMAGE_PATTERN the block dispatcher uses;
  • fenced code blocks, closed by the same rule _render_code_block uses.

  That removal is in _inline_text().

• New tests cover a PowerPoint bullet, a Word paragraph with the image mid-line, a Word line that is only an image, and a link inside a code fence.

On **a***: my exhaustive comparison of the old and new grammar covered every string of up to 10 characters from {*, a, space}, and that includes this 6-character input. Both grammars tokenise it the same way, so this change doesn't alter its behaviour.

---

Generated by Claude Code