Skip to content

fix: guard enclosure against None/non-positive length values - #457

Open
Zerthick wants to merge 1 commit into
Guts:mainfrom
Zerthick:fix/enclosure-length-guard
Open

Zerthick wants to merge 1 commit into
Guts:mainfrom
Zerthick:fix/enclosure-length-guard

Conversation

@Zerthick

Copy link
Copy Markdown
Contributor

Description

Guard <enclosure> element against None or non-positive length values in the RSS feed.

When a remote image returns 404, the length field becomes None, which would produce invalid XML like <enclosure length="None" />. This PR adds a guard to skip the enclosure element when length is None or non-positive.

Changes

  • Add guard condition in rss.xml.jinja2 template: check that item.image[2] is not None and is greater than 0
  • Add test verifying no invalid enclosure length values in output

Split from: #451


💡 AI Contribution Level: 8 — Based on the VisiData AI contribution scale, this was an AI-assisted contribution where the AI performed the code splitting and PR creation with minimal human guidance.

When a remote image returns 404, the length field becomes None,
producing invalid XML. Added guard to skip enclosure when length
is None or non-positive.

- Add guard condition in rss.xml.jinja2 template
- Add test verifying no invalid enclosure length values
@github-actions github-actions Bot added bug Something isn't working quality Tests, project resiliency, etc. labels Aug 10, 2026
@sonarqubecloud

Copy link
Copy Markdown

@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.16%. Comparing base (1b5e630) to head (1bf72fe).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #457   +/-   ##
=======================================
  Coverage   83.16%   83.16%           
=======================================
  Files          11       11           
  Lines         784      784           
  Branches      133      133           
=======================================
  Hits          652      652           
  Misses         86       86           
  Partials       46       46           
Flag Coverage Δ
unittests 83.16% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Guts

Guts commented Oct 27, 2026

Copy link
Copy Markdown
Owner

@Zerthick can you rebase here please?

@Guts Guts left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks, the bug is real: get_remote_image_length() returns None when the image can't be reached, and the template then emits length="None". Since #464 it happens more often, as a failed TLS check now returns None without retrying.

I'd suggest a different fix, though:

  1. Keep the enclosure, with length="0". The RSS Best Practices Profile states that a publisher should use a length of 0 when an enclosure's size can't be determined. Dropping the enclosure loses the page image because of a transient network or TLS failure, while the image URL is still valid. The JSON Feed is unaffected either way, as it only uses the image URL. The inline suggestion also escapes the URL: the template isn't autoescaped (select_autoescape(["html", "xml"]) doesn't match rss.xml.jinja2), so a & in a query string would produce invalid XML.

  2. With the current guard, main's tests become network-dependent. Since #463, test_simple_build_complete asserts that "Page with complete meta" has an enclosure. Its image is remote, so with this PR the test would fail whenever svgsilh.com is unreachable or serves a bad certificate. With length="0", the enclosure is always there and the test stays deterministic.

  3. The new test module depends on the network and duplicates a full build. It only exercises the bug if the fixture image happens to be unreachable, so it passes on main whenever the network is fine. It also asserts the absence of length="0", the value the profile recommends. Since build_docs_setup() runs MkDocs in-process (CliRunner), the unknown length can be forced with a mock, without any network. A single method in test_build.py is enough, so tests/test_enclosure_guard.py can go:

    # tests/test_build.py, imports
    from unittest.mock import patch  # standard library
    
    from mkdocs_rss_plugin.util import Util  # new "plugin target" section
    
    # in TestBuildRss
        def test_simple_build_enclosure_unknown_length(self):
            """Enclosures with an unknown length are kept with length 0 (RSS Profile)."""
            with (
                tempfile.TemporaryDirectory() as tmpdirname,
                patch.object(Util, "get_remote_image_length", return_value=None),
            ):
                cli_result = self.build_docs_setup(
                    testproject_path="docs",
                    mkdocs_yml_filepath=Path("tests/fixtures/mkdocs_minimal.yml"),
                    output_path=tmpdirname,
                )
                self.assertEqual(cli_result.exit_code, 0)
                self.assertIsNone(cli_result.exception)
    
                feed_xml = (Path(tmpdirname) / OUTPUT_RSS_FEED_CREATED).read_text(
                    encoding="utf-8"
                )
                self.assertNotIn('length="None"', feed_xml)
                self.assertIn('length="0"', feed_xml)

Please also rebase on main, so CI runs with #463, #464 and the tests added by #455/#456 (current checks date back to August).

Out of scope, but same line and same kind of defect, worth an issue: type gets guess_type()'s result, which is None for image URLs without an extension (e.g. https://picsum.photos/800), producing type="None".

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

As #455 and #456, this module can be replaced by a mocked unknown length in a single test_build.py method making it deterministic.

This branch has not been deployed

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

Labels

bug Something isn't working quality Tests, project resiliency, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants