diff --git a/.github/workflows/validate-pr.yml b/.github/workflows/validate-pr.yml index 372d10f09..ee2d6ee74 100644 --- a/.github/workflows/validate-pr.yml +++ b/.github/workflows/validate-pr.yml @@ -5,6 +5,8 @@ on: paths: - README.md - CONTRIBUTING.md + - scripts/** + - tests/** permissions: contents: read @@ -23,6 +25,9 @@ jobs: with: fetch-depth: 0 + - name: Run unit tests + run: python3 -m unittest discover -s tests -v + - name: Lint README grammar, duplicates, and Table of Contents run: python3 scripts/check_readme.py lint >> "$GITHUB_STEP_SUMMARY" diff --git a/scripts/check_readme.py b/scripts/check_readme.py index b3011684d..8161cf248 100644 --- a/scripts/check_readme.py +++ b/scripts/check_readme.py @@ -656,6 +656,82 @@ def check_url(url, policy=None, timeout=15, max_redirects=5, retries=2, backoff= return _classify_result(url, current, redirect_count, status, headers, body, exc, policy) +def _canonical_redirect_host(hostname): + """Normalize hostnames for conservative redirect comparisons.""" + host = (hostname or "").lower().rstrip(".") + if host.startswith("www."): + host = host[4:] + return host + + +def _normalized_redirect_path(path): + """Normalize harmless trailing-slash differences.""" + path = path or "/" + if path != "/": + path = path.rstrip("/") + return path + + +def _resource_slug(path): + """Return a conservative identity slug for a resource path.""" + path = _normalized_redirect_path(path) + if path == "/": + return "" + + slug = path.rsplit("/", 1)[-1].lower() + slug = re.sub(r"\.(?:html?|php)$", "", slug) + slug = re.sub(r"^\d+[-_]", "", slug) + return slug + + +def _nondefault_port(parts): + """Return an explicit non-default port, otherwise None.""" + port = parts.port + if port is None: + return None + + default = {"http": 80, "https": 443}.get(parts.scheme.lower()) + return None if port == default else port + + +def _is_safe_redirect(original_url, final_url): + """Return True only for redirects safe enough for automatic rewriting. + + Scheme, www, trailing-slash, and same-site path migrations may be handled + automatically. Cross-domain, query-changing, or non-default-port redirects + require human review. + """ + original = urlsplit(original_url) + final = urlsplit(final_url) + + if ( + _canonical_redirect_host(original.hostname) + != _canonical_redirect_host(final.hostname) + ): + return False + + if original.query != final.query: + return False + + if _nondefault_port(original) != _nondefault_port(final): + return False + + if ( + _normalized_redirect_path(original.path) + == _normalized_redirect_path(final.path) + ): + return True + + original_slug = _resource_slug(original.path) + final_slug = _resource_slug(final.path) + + return bool( + original_slug + and final_slug + and original_slug == final_slug + ) + + def _classify_result(original_url, final_url, redirect_count, status, headers, body, exc, policy): host = urlsplit(final_url).netloc.lower() note = None @@ -693,6 +769,9 @@ def _classify_result(original_url, final_url, redirect_count, status, headers, b if fp.path in ("", "/") and op.path not in ("", "/"): cls = "SUSPECT" note = "redirected to domain root" + elif not _is_safe_redirect(original_url, final_url): + cls = "SUSPECT" + note = "redirect target appears to be a different resource" else: cls = "OK" else: diff --git a/tests/test_check_readme.py b/tests/test_check_readme.py new file mode 100644 index 000000000..0e79ee942 --- /dev/null +++ b/tests/test_check_readme.py @@ -0,0 +1,93 @@ +import importlib.util +import unittest +from pathlib import Path + + +ROOT = Path(__file__).resolve().parents[1] +SCRIPT = ROOT / "scripts" / "check_readme.py" + +spec = importlib.util.spec_from_file_location("check_readme", SCRIPT) +check_readme = importlib.util.module_from_spec(spec) +spec.loader.exec_module(check_readme) + + +class RedirectSafetyTests(unittest.TestCase): + def classify_redirect(self, original_url, final_url): + return check_readme._classify_result( + original_url=original_url, + final_url=final_url, + redirect_count=1, + status=200, + headers={}, + body=b"Example tutorial", + exc=None, + policy=set(), + ) + + def assert_safe(self, original, final): + result = self.classify_redirect(original, final) + self.assertEqual(result["class"], "OK") + self.assertEqual(result["redirect_to"], final) + + def assert_suspect(self, original, final): + result = self.classify_redirect(original, final) + self.assertEqual(result["class"], "SUSPECT") + self.assertIsNone(result["redirect_to"]) + + def test_http_to_https_same_path_is_safe(self): + self.assert_safe( + "http://example.com/tutorial", + "https://example.com/tutorial", + ) + + def test_www_change_same_path_is_safe(self): + self.assert_safe( + "https://www.example.com/tutorial/", + "https://example.com/tutorial", + ) + + def test_same_domain_path_move_same_slug_is_safe(self): + self.assert_safe( + "https://example.com/blog/build-a-chat-app", + "https://example.com/articles/build-a-chat-app", + ) + + def test_numeric_prefix_change_same_slug_is_safe(self): + self.assert_safe( + "https://example.com/134049-building-ios-apps-with-xamarin", + "https://example.com/1044-building-ios-apps-with-xamarin", + ) + + def test_redirect_to_domain_root_is_suspect(self): + self.assert_suspect( + "https://example.com/tutorial", + "https://example.com/", + ) + + def test_different_article_is_suspect(self): + self.assert_suspect( + "https://example.com/build-a-song-recommender", + "https://example.com/evaluation-metrics", + ) + + def test_cross_domain_same_slug_is_suspect(self): + self.assert_suspect( + "https://old.example/build-a-chat-app", + "https://new.example/build-a-chat-app", + ) + + def test_query_change_is_suspect(self): + self.assert_suspect( + "https://example.com/tutorial?id=123", + "https://example.com/tutorial?id=456", + ) + + def test_nondefault_port_change_is_suspect(self): + self.assert_suspect( + "https://example.com:8443/tutorial", + "https://example.com:9443/tutorial", + ) + + +if __name__ == "__main__": + unittest.main()