Skip to content

Validate format name against known formatters before string eval - #77

Merged
davorg merged 4 commits into
masterfrom
copilot/be-more-careful-with-format-name
Sep 8, 2026
Merged

davorg merged 4 commits into
masterfrom
copilot/be-more-careful-with-format-name

Conversation

Copilot AI commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

XML::Feed->new, XML::Feed->parse, and XML::Feed::Entry->new built a class name by interpolating the caller-supplied format argument and loaded it via string eval. Nothing constrained that value, so a format string containing a semicolon (e.g. format=Atom; system(...)) would run arbitrary Perl code — a realistic risk given format is often sourced from a query parameter.

Changes

  • lib/XML/Feed.pm: extended the existing BEGIN block to build a %formatters hash alongside @formatters, mapping each format discovered via Module::Pluggable (e.g. Atom, RSS) to a true value.
  • XML::Feed->new / XML::Feed->parse: validate format against %formatters and fail (croak / error) before the string ever reaches eval "use $format_class".
  • lib/XML/Feed/Entry.pm: applies the same check via %XML::Feed::formatters (added use XML::Feed (); to ensure it's populated when loaded standalone), since XML::Feed::Entry->new has the identical vulnerable pattern.
# before: 'Atom; system("rm -rf /")' would execute the injected code
# after: rejected immediately
eval { XML::Feed->new('Atom; system("...")') };
# $@ =~ /Unsupported format Atom; system\(.*\): not a recognised feed format/
  • Tests: added t/32-format-injection.t covering %XML::Feed::formatters contents and confirming injected format strings are rejected without execution across new/parse/Entry->new.

Copilot AI linked an issue Sep 8, 2026 that may be closed by this pull request
Co-authored-by: davorg <24642+davorg@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix format name handling in XML::Feed Validate format name against known formatters before string eval Sep 8, 2026
Copilot AI requested a review from davorg September 8, 2026 07:08
Co-authored-by: davorg <24642+davorg@users.noreply.github.com>
@davorg
davorg marked this pull request as ready for review September 8, 2026 07:15
@davorg
davorg merged commit 976fd24 into master Sep 8, 2026
4 checks passed
@coveralls

Copy link
Copy Markdown

Coverage Report for CI Build 34198444906

Warning

No base build found for commit ae6e736 on master.
Coverage changes can't be calculated without a base build.
If a base build is processing, this comment will update automatically when it completes.

Coverage: 96.517%

Details

  • Patch coverage: 7 of 7 lines across 2 files are fully covered (100%).

Uncovered Changes

No uncovered changes found.

Coverage Regressions

Requires a base build to compare against. How to fix this →


Coverage Stats

Coverage Status
Relevant Lines: 603
Covered Lines: 582
Line Coverage: 96.52%
Coverage Strength: 44.28 hits per line

💛 - Coveralls

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.

Be more careful with format name

3 participants