From c9d6d6f40164f140d9ff6570b862921aee9ea302 Mon Sep 17 00:00:00 2001 From: James Addison Date: Fri, 4 Apr 2025 18:35:42 +0100 Subject: [PATCH 1/9] Tests: uncover a quirk in our ``linkcheck`` tests --- tests/test_builders/test_build_linkcheck.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_builders/test_build_linkcheck.py b/tests/test_builders/test_build_linkcheck.py index bdd8dea54c1..2677cb3d33c 100644 --- a/tests/test_builders/test_build_linkcheck.py +++ b/tests/test_builders/test_build_linkcheck.py @@ -715,6 +715,7 @@ def log_date_time_string(self): ) def test_follows_redirects_on_HEAD(app, capsys): with serve_application(app, make_redirect_handler(support_head=True)) as address: + compile_linkcheck_allowed_redirects(app, app.config) app.build() _stdout, stderr = capsys.readouterr() content = (app.outdir / 'output.txt').read_text(encoding='utf8') @@ -738,6 +739,7 @@ def test_follows_redirects_on_HEAD(app, capsys): ) def test_follows_redirects_on_GET(app, capsys): with serve_application(app, make_redirect_handler(support_head=False)) as address: + compile_linkcheck_allowed_redirects(app, app.config) app.build() _stdout, stderr = capsys.readouterr() content = (app.outdir / 'output.txt').read_text(encoding='utf8') From 5afe46d31ac1adf994f11ff45aa95f39392c84b4 Mon Sep 17 00:00:00 2001 From: James Addison Date: Mon, 14 Apr 2025 10:02:49 +0100 Subject: [PATCH 2/9] linkcheck: prevent `linkcheck_allowed_redirects` value of `None` --- sphinx/builders/linkcheck.py | 7 ++++--- tests/test_builders/test_build_linkcheck.py | 12 ++++++++++-- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/sphinx/builders/linkcheck.py b/sphinx/builders/linkcheck.py index e1a80a47c0f..c15e9fa7aeb 100644 --- a/sphinx/builders/linkcheck.py +++ b/sphinx/builders/linkcheck.py @@ -179,7 +179,7 @@ def process_result(self, result: CheckResult) -> None: text = 'with unknown code' linkstat['text'] = text redirection = f'{text} to {result.message}' - if self.config.linkcheck_allowed_redirects is not None: + if self.config.linkcheck_allowed_redirects is not _sentinel_lar: msg = f'redirect {res_uri} - {redirection}' logger.warning(msg, location=(result.docname, result.lineno)) else: @@ -387,7 +387,7 @@ def __init__( ) self.check_anchors: bool = config.linkcheck_anchors self.allowed_redirects: dict[re.Pattern[str], re.Pattern[str]] - self.allowed_redirects = config.linkcheck_allowed_redirects or {} + self.allowed_redirects = config.linkcheck_allowed_redirects self.retries: int = config.linkcheck_retries self.rate_limit_timeout = config.linkcheck_rate_limit_timeout self._allow_unauthorized = config.linkcheck_allow_unauthorized @@ -722,6 +722,8 @@ def handle_starttag(self, tag: Any, attrs: Any) -> None: def _allowed_redirect( url: str, new_url: str, allowed_redirects: dict[re.Pattern[str], re.Pattern[str]] ) -> bool: + if allowed_redirects is _sentinel_lar: + return True return any( from_url.match(url) and to_url.match(new_url) for from_url, to_url in allowed_redirects.items() @@ -751,7 +753,6 @@ def rewrite_github_anchor(app: Sphinx, uri: str) -> str | None: def compile_linkcheck_allowed_redirects(app: Sphinx, config: Config) -> None: """Compile patterns to the regexp objects.""" if config.linkcheck_allowed_redirects is _sentinel_lar: - config.linkcheck_allowed_redirects = None return if not isinstance(config.linkcheck_allowed_redirects, dict): raise ConfigError diff --git a/tests/test_builders/test_build_linkcheck.py b/tests/test_builders/test_build_linkcheck.py index 2677cb3d33c..1f636b29ffc 100644 --- a/tests/test_builders/test_build_linkcheck.py +++ b/tests/test_builders/test_build_linkcheck.py @@ -712,6 +712,7 @@ def log_date_time_string(self): 'linkcheck', testroot='linkcheck-localserver', freshenv=True, + confoverrides={'linkcheck_allowed_redirects': {}}, # do not follow any redirects ) def test_follows_redirects_on_HEAD(app, capsys): with serve_application(app, make_redirect_handler(support_head=True)) as address: @@ -729,13 +730,17 @@ def test_follows_redirects_on_HEAD(app, capsys): 127.0.0.1 - - [] "HEAD /?redirected=1 HTTP/1.1" 204 - """, ) - assert app.warning.getvalue() == '' + assert ( + f'WARNING: redirect http://{address}/' + f' - with Found to http://{address}/?redirected=1' + ) in strip_escape_sequences(app.warning.getvalue()) @pytest.mark.sphinx( 'linkcheck', testroot='linkcheck-localserver', freshenv=True, + confoverrides={'linkcheck_allowed_redirects': {}}, # do not follow any redirects ) def test_follows_redirects_on_GET(app, capsys): with serve_application(app, make_redirect_handler(support_head=False)) as address: @@ -754,7 +759,10 @@ def test_follows_redirects_on_GET(app, capsys): 127.0.0.1 - - [] "GET /?redirected=1 HTTP/1.1" 204 - """, ) - assert app.warning.getvalue() == '' + assert ( + f'WARNING: redirect http://{address}/' + f' - with Found to http://{address}/?redirected=1' + ) in strip_escape_sequences(app.warning.getvalue()) def test_linkcheck_allowed_redirects_config( From a8df874236cb59e6342df4666cdfc63645d149f1 Mon Sep 17 00:00:00 2001 From: James Addison Date: Mon, 14 Apr 2025 10:20:25 +0100 Subject: [PATCH 3/9] linkcheck: adjust test coverage --- tests/test_builders/test_build_linkcheck.py | 47 +++++++++++++-------- 1 file changed, 29 insertions(+), 18 deletions(-) diff --git a/tests/test_builders/test_build_linkcheck.py b/tests/test_builders/test_build_linkcheck.py index 1f636b29ffc..5feb105d7d1 100644 --- a/tests/test_builders/test_build_linkcheck.py +++ b/tests/test_builders/test_build_linkcheck.py @@ -680,7 +680,7 @@ def check_headers(self): assert content['status'] == 'working' -def make_redirect_handler(*, support_head: bool) -> type[BaseHTTPRequestHandler]: +def make_redirect_handler(*, support_head: bool = True) -> type[BaseHTTPRequestHandler]: class RedirectOnceHandler(BaseHTTPRequestHandler): protocol_version = 'HTTP/1.1' @@ -712,7 +712,6 @@ def log_date_time_string(self): 'linkcheck', testroot='linkcheck-localserver', freshenv=True, - confoverrides={'linkcheck_allowed_redirects': {}}, # do not follow any redirects ) def test_follows_redirects_on_HEAD(app, capsys): with serve_application(app, make_redirect_handler(support_head=True)) as address: @@ -720,27 +719,20 @@ def test_follows_redirects_on_HEAD(app, capsys): app.build() _stdout, stderr = capsys.readouterr() content = (app.outdir / 'output.txt').read_text(encoding='utf8') - assert content == ( - 'index.rst:1: [redirected with Found] ' - f'http://{address}/ to http://{address}/?redirected=1\n' - ) + assert content == '' assert stderr == textwrap.dedent( """\ 127.0.0.1 - - [] "HEAD / HTTP/1.1" 302 - 127.0.0.1 - - [] "HEAD /?redirected=1 HTTP/1.1" 204 - """, ) - assert ( - f'WARNING: redirect http://{address}/' - f' - with Found to http://{address}/?redirected=1' - ) in strip_escape_sequences(app.warning.getvalue()) + assert app.warning.getvalue() == '' @pytest.mark.sphinx( 'linkcheck', testroot='linkcheck-localserver', freshenv=True, - confoverrides={'linkcheck_allowed_redirects': {}}, # do not follow any redirects ) def test_follows_redirects_on_GET(app, capsys): with serve_application(app, make_redirect_handler(support_head=False)) as address: @@ -748,21 +740,40 @@ def test_follows_redirects_on_GET(app, capsys): app.build() _stdout, stderr = capsys.readouterr() content = (app.outdir / 'output.txt').read_text(encoding='utf8') + assert content == '' + assert stderr == textwrap.dedent( + """\ + 127.0.0.1 - - [] "HEAD / HTTP/1.1" 405 - + 127.0.0.1 - - [] "GET / HTTP/1.1" 302 - + 127.0.0.1 - - [] "GET /?redirected=1 HTTP/1.1" 204 - + """, + ) + assert app.warning.getvalue() == '' + + +@pytest.mark.sphinx( + 'linkcheck', + testroot='linkcheck-localserver', + freshenv=True, + confoverrides={'linkcheck_allowed_redirects': {}}, # do not follow any redirects +) +def test_warns_disallowed_redirects(app, capsys): + with serve_application(app, make_redirect_handler()) as address: + compile_linkcheck_allowed_redirects(app, app.config) + app.build() + _stdout, stderr = capsys.readouterr() + content = (app.outdir / 'output.txt').read_text(encoding='utf8') assert content == ( 'index.rst:1: [redirected with Found] ' f'http://{address}/ to http://{address}/?redirected=1\n' ) assert stderr == textwrap.dedent( """\ - 127.0.0.1 - - [] "HEAD / HTTP/1.1" 405 - - 127.0.0.1 - - [] "GET / HTTP/1.1" 302 - - 127.0.0.1 - - [] "GET /?redirected=1 HTTP/1.1" 204 - + 127.0.0.1 - - [] "HEAD / HTTP/1.1" 302 - + 127.0.0.1 - - [] "HEAD /?redirected=1 HTTP/1.1" 204 - """, ) - assert ( - f'WARNING: redirect http://{address}/' - f' - with Found to http://{address}/?redirected=1' - ) in strip_escape_sequences(app.warning.getvalue()) + assert len(app.warning.getvalue().splitlines()) == 1 def test_linkcheck_allowed_redirects_config( From 3fa174b0865db3747aca59b1e8f243ec109c823c Mon Sep 17 00:00:00 2001 From: James Addison Date: Mon, 14 Apr 2025 10:20:51 +0100 Subject: [PATCH 4/9] Update CHANGES.rst --- CHANGES.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.rst b/CHANGES.rst index fede8b5177b..25ca682aff0 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -18,7 +18,7 @@ Features added Patch by Till Hoffmann. * #13439: linkcheck: Permit warning on every redirect with ``linkcheck_allowed_redirects = {}``. - Patch by Adam Turner. + Patch by Adam Turner and James Addison. Bugs fixed ---------- From bf12dfd426dd61127f96e3ad97592e95fc426691 Mon Sep 17 00:00:00 2001 From: James Addison Date: Tue, 6 May 2025 21:36:08 +0100 Subject: [PATCH 5/9] linkcheck: use boolean literal `True` instead of sentinel as wildcard-allow-redirects value --- sphinx/builders/linkcheck.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/sphinx/builders/linkcheck.py b/sphinx/builders/linkcheck.py index 024a1d3cb18..aa38659be18 100644 --- a/sphinx/builders/linkcheck.py +++ b/sphinx/builders/linkcheck.py @@ -179,7 +179,7 @@ def process_result(self, result: CheckResult) -> None: text = 'with unknown code' linkstat['text'] = text redirection = f'{text} to {result.message}' - if self.config.linkcheck_allowed_redirects is not _sentinel_lar: + if self.config.linkcheck_allowed_redirects is not True: msg = f'redirect {res_uri} - {redirection}' logger.warning(msg, location=(result.docname, result.lineno)) else: @@ -722,7 +722,7 @@ def handle_starttag(self, tag: Any, attrs: Any) -> None: def _allowed_redirect( url: str, new_url: str, allowed_redirects: dict[re.Pattern[str], re.Pattern[str]] ) -> bool: - if allowed_redirects is _sentinel_lar: + if allowed_redirects is True: return True return any( from_url.match(url) and to_url.match(new_url) @@ -752,7 +752,7 @@ def rewrite_github_anchor(app: Sphinx, uri: str) -> str | None: def compile_linkcheck_allowed_redirects(app: Sphinx, config: Config) -> None: """Compile patterns to the regexp objects.""" - if config.linkcheck_allowed_redirects is _sentinel_lar: + if config.linkcheck_allowed_redirects is True: return if not isinstance(config.linkcheck_allowed_redirects, dict): msg = __( @@ -773,9 +773,6 @@ def compile_linkcheck_allowed_redirects(app: Sphinx, config: Config) -> None: config.linkcheck_allowed_redirects = allowed_redirects -_sentinel_lar = object() - - def setup(app: Sphinx) -> ExtensionMetadata: app.add_builder(CheckExternalLinksBuilder) app.add_post_transform(HyperlinkCollector) @@ -785,7 +782,7 @@ def setup(app: Sphinx) -> ExtensionMetadata: 'linkcheck_exclude_documents', [], '', types=frozenset({list, tuple}) ) app.add_config_value( - 'linkcheck_allowed_redirects', _sentinel_lar, '', types=frozenset({dict}) + 'linkcheck_allowed_redirects', True, '', types=frozenset({dict}) ) app.add_config_value('linkcheck_auth', [], '', types=frozenset({list, tuple})) app.add_config_value('linkcheck_request_headers', {}, '', types=frozenset({dict})) From 6356f75f3c826e9434bf1ad6aa8246a0b7687fbd Mon Sep 17 00:00:00 2001 From: James Addison Date: Tue, 6 May 2025 23:17:39 +0100 Subject: [PATCH 6/9] linkcheck: invert `linkcheck_allowed_redirects` boolean value to more-closely-resemble existing defaults --- sphinx/builders/linkcheck.py | 10 +++++----- tests/test_builders/test_build_linkcheck.py | 10 ++++++++-- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/sphinx/builders/linkcheck.py b/sphinx/builders/linkcheck.py index aa38659be18..531d7bf6870 100644 --- a/sphinx/builders/linkcheck.py +++ b/sphinx/builders/linkcheck.py @@ -179,7 +179,7 @@ def process_result(self, result: CheckResult) -> None: text = 'with unknown code' linkstat['text'] = text redirection = f'{text} to {result.message}' - if self.config.linkcheck_allowed_redirects is not True: + if self.config.linkcheck_allowed_redirects is not False: msg = f'redirect {res_uri} - {redirection}' logger.warning(msg, location=(result.docname, result.lineno)) else: @@ -722,8 +722,8 @@ def handle_starttag(self, tag: Any, attrs: Any) -> None: def _allowed_redirect( url: str, new_url: str, allowed_redirects: dict[re.Pattern[str], re.Pattern[str]] ) -> bool: - if allowed_redirects is True: - return True + if allowed_redirects is False: + return False return any( from_url.match(url) and to_url.match(new_url) for from_url, to_url in allowed_redirects.items() @@ -752,7 +752,7 @@ def rewrite_github_anchor(app: Sphinx, uri: str) -> str | None: def compile_linkcheck_allowed_redirects(app: Sphinx, config: Config) -> None: """Compile patterns to the regexp objects.""" - if config.linkcheck_allowed_redirects is True: + if config.linkcheck_allowed_redirects is False: return if not isinstance(config.linkcheck_allowed_redirects, dict): msg = __( @@ -782,7 +782,7 @@ def setup(app: Sphinx) -> ExtensionMetadata: 'linkcheck_exclude_documents', [], '', types=frozenset({list, tuple}) ) app.add_config_value( - 'linkcheck_allowed_redirects', True, '', types=frozenset({dict}) + 'linkcheck_allowed_redirects', False, '', types=frozenset({dict}) ) app.add_config_value('linkcheck_auth', [], '', types=frozenset({list, tuple})) app.add_config_value('linkcheck_request_headers', {}, '', types=frozenset({dict})) diff --git a/tests/test_builders/test_build_linkcheck.py b/tests/test_builders/test_build_linkcheck.py index 5feb105d7d1..a2209273be3 100644 --- a/tests/test_builders/test_build_linkcheck.py +++ b/tests/test_builders/test_build_linkcheck.py @@ -719,7 +719,10 @@ def test_follows_redirects_on_HEAD(app, capsys): app.build() _stdout, stderr = capsys.readouterr() content = (app.outdir / 'output.txt').read_text(encoding='utf8') - assert content == '' + assert content == ( + 'index.rst:1: [redirected with Found] ' + f'http://{address}/ to http://{address}/?redirected=1\n' + ) assert stderr == textwrap.dedent( """\ 127.0.0.1 - - [] "HEAD / HTTP/1.1" 302 - @@ -740,7 +743,10 @@ def test_follows_redirects_on_GET(app, capsys): app.build() _stdout, stderr = capsys.readouterr() content = (app.outdir / 'output.txt').read_text(encoding='utf8') - assert content == '' + assert content == ( + 'index.rst:1: [redirected with Found] ' + f'http://{address}/ to http://{address}/?redirected=1\n' + ) assert stderr == textwrap.dedent( """\ 127.0.0.1 - - [] "HEAD / HTTP/1.1" 405 - From 90cd874d765d53114ac828b1e84182a9a127ac41 Mon Sep 17 00:00:00 2001 From: James Addison Date: Tue, 6 May 2025 23:23:51 +0100 Subject: [PATCH 7/9] linkcheck: rectify test comment --- tests/test_builders/test_build_linkcheck.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_builders/test_build_linkcheck.py b/tests/test_builders/test_build_linkcheck.py index a2209273be3..f01e1b1cef6 100644 --- a/tests/test_builders/test_build_linkcheck.py +++ b/tests/test_builders/test_build_linkcheck.py @@ -761,7 +761,7 @@ def test_follows_redirects_on_GET(app, capsys): 'linkcheck', testroot='linkcheck-localserver', freshenv=True, - confoverrides={'linkcheck_allowed_redirects': {}}, # do not follow any redirects + confoverrides={'linkcheck_allowed_redirects': {}}, # warn about any redirects ) def test_warns_disallowed_redirects(app, capsys): with serve_application(app, make_redirect_handler()) as address: From 57e4d084954a904835caf8e93c9e591ce9893917 Mon Sep 17 00:00:00 2001 From: James Addison Date: Mon, 2 Jun 2025 10:38:11 +0100 Subject: [PATCH 8/9] linkcheck: undo a couple of `allowed_redirects` config logic statements, per code self-review Partially reverts commit 5afe46d31ac1adf994f11ff45aa95f39392c84b4. --- sphinx/builders/linkcheck.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/sphinx/builders/linkcheck.py b/sphinx/builders/linkcheck.py index 531d7bf6870..4ca5bd99428 100644 --- a/sphinx/builders/linkcheck.py +++ b/sphinx/builders/linkcheck.py @@ -387,7 +387,7 @@ def __init__( ) self.check_anchors: bool = config.linkcheck_anchors self.allowed_redirects: dict[re.Pattern[str], re.Pattern[str]] - self.allowed_redirects = config.linkcheck_allowed_redirects + self.allowed_redirects = config.linkcheck_allowed_redirects or {} self.retries: int = config.linkcheck_retries self.rate_limit_timeout = config.linkcheck_rate_limit_timeout self._allow_unauthorized = config.linkcheck_allow_unauthorized @@ -722,8 +722,6 @@ def handle_starttag(self, tag: Any, attrs: Any) -> None: def _allowed_redirect( url: str, new_url: str, allowed_redirects: dict[re.Pattern[str], re.Pattern[str]] ) -> bool: - if allowed_redirects is False: - return False return any( from_url.match(url) and to_url.match(new_url) for from_url, to_url in allowed_redirects.items() From 69153629bfba13349ecf0b35cac5bbb6dd121533 Mon Sep 17 00:00:00 2001 From: James Addison Date: Mon, 2 Jun 2025 10:47:09 +0100 Subject: [PATCH 9/9] linkcheck: tests: add coverage for status/info-level redirection log message --- tests/test_builders/test_build_linkcheck.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/test_builders/test_build_linkcheck.py b/tests/test_builders/test_build_linkcheck.py index 24de0227c35..a09a4a42216 100644 --- a/tests/test_builders/test_build_linkcheck.py +++ b/tests/test_builders/test_build_linkcheck.py @@ -729,6 +729,9 @@ def test_follows_redirects_on_HEAD(app, capsys): 127.0.0.1 - - [] "HEAD /?redirected=1 HTTP/1.1" 204 - """, ) + assert ( + f'redirect http://{address}/ - with Found to http://{address}/?redirected=1\n' + ) in strip_escape_sequences(app.status.getvalue()) assert app.warning.getvalue() == '' @@ -754,6 +757,9 @@ def test_follows_redirects_on_GET(app, capsys): 127.0.0.1 - - [] "GET /?redirected=1 HTTP/1.1" 204 - """, ) + assert ( + f'redirect http://{address}/ - with Found to http://{address}/?redirected=1\n' + ) in strip_escape_sequences(app.status.getvalue()) assert app.warning.getvalue() == ''