Repository: sphinx-doc/sphinx

Issue:
linkchecker: update handling of HTTP 401 unauthorized responses
### Describe the bug

In Sphinx releases since v1.3 the `linkcheck` builder has [considered an HTTP 401 (unauthorized) response to be acceptable](https://github.com/sphinx-doc/sphinx/blob/b87ce32e7dc09773f9e71305e66e8d6aead53dd1/sphinx/builders/linkcheck.py#L186-L189) as confirmation that a hyperlinked reference is available and working (see #1687 for context).

That made sense at the time -- support for authentication in the `linkcheck` builder didn't arrive until Sphinx release v2.3.0 (see #6816) -- but I think that it might be an improvement to update the linkchecker to report references that return HTTP 401 as either 'ignored' or 'broken'.

Basically I think the ideal is to communicate to documentation authors and reviewers -- the likely users of the `linkcheck` builder -- where there are gaps or problems in the links that downstream readers may experience.

Sure, in some environments downstream readers may be expected to provide authentication information to access documents -- but for many projects the documentation is provided public and without requiring such auth.  In either case, if an unauthorized response is received, the linkcheck user should be notified that something may not be accessible for their readers, and this could hint to them that they may need to update their config to add authentication to match user expectations.

### How to Reproduce

I don't have a repro case to offer for this at the moment, but may add one later.

### Environment Information

```text
Affects sphinx v1.3 onwards.
```


### Sphinx extensions

```python
N/A
```


### Additional context

Discovered during refactoring in #11432.

Discussion:
AA-Turner: For this to go into 7.3, it should be opt-in; we can flip to opt-out in 8.0.

A
jayaddison: Ok, thanks @AA-Turner.

I could wait until 8.0 to merge this, if that's simpler -- or I'll add a config option called `linkcheck_permit_unauthorized_responses`, default `True` (existing behaviour).

More concise and/or better config option name ideas welcome.. I'd like to communicate the fact that it's server-communicated auth status that is the condition, and for the way those cases are handled to be fairly clear from the option name.
picnixz: An alternative: `linkcheck_ignore_error_codes` containing 401 by default. 

Or an alternative name: `linkcheck_ignore_unauthorized` set to true.
jayaddison: Hmm.. the error-code list approach could be a neat solution, but gives me a sense of too much configurability.  Roughly speaking, I think HTTP status codes are well-enough defined to determine success and failure in the case of linkchecking.

The `ignore` keyword would be nice because it'd visually align with other configuration options and re-uses existing terminology.. but the use case is different here, I think.  We _do_ check these unauthorized URLs, so they're not ignored.

`strict` could also be OK; my concern there is that it infers that there could be a category of checks -- but since this is intended to be a temporary flag until 8.0, I'd prefer to pick something that can't accidentally become repurposed for other things prior to removal.

To shoot down one of my own other alternative ideas as well: `linkcheck_allow_unauthorized` - concise, re-uses the word allow -- but could be confusing because there is client `auth`, and it's perfectly fine for the linkchecker to make unauthorized requests in most cases.  What we care about is whether the server is saying the request was not authorized.

So.. I suppose `linkcheck_allow_unauthorized_responses` could be OK.  `linkcheck_tolerate_unauthorized_responses`?
picnixz: I'm not fond of long configuration value names. And I would prefer using standard terminology (i.e., not "tolerate"). I think we should pick something from: allow, ignore, skip or strict.

We could also have something like `linkcheck_ignore_errors = ['unauthorized']` (meaning, you do check but ignore specific errors, and not the link itself). Since we also have a bunch of other errors that turn into warnings, we could also suppress them like that. 
jayaddison: > I'm not fond of long configuration value names. And I would prefer using standard terminology (i.e., not "tolerate"). I think we should pick something from: allow, ignore, skip or strict.

Ok, agreed on both of those.
 
> We could also have something like `linkcheck_ignore_errors = ['unauthorized']` (meaning, you do check but ignore specific errors, and not the link itself). Since we also have a bunch of other errors that turn into warnings, we could also suppress them like that.

I'd like to keep the scope of this change (and the config option) small, and the management of log-level for some of those errors/warnings seems a bit unrelated.

However: it does make me wonder whether it'd be OK to add a warning message even when HTTP 401 is treated as success?

Regarding the option name: probably enough bikeshedding, so `linkcheck_allow_unauthorized` is it, I think.  I resolved my own concern about the ambiguity with client `auth` because I suppose technically that refers to authentication and not authorization.
picnixz: > whether it'd be OK to add a warning message even when HTTP 401 is treated as success?

Not good if people are turning warnings into errors.
jayaddison: I don't have any further changes/commits planned here at the moment and would be glad for more review when possible.
AA-Turner: > I could wait until 8.0 to merge this

For X.0 releases, I prefer to have a 'clean' release with only removals/changes to defaults -- a new feature I think should be merged in a point release, with an opt-in.
jayaddison: Ok, I've reached another review-pause now (no further commits planned).

The main question I have open / to resolve is how and when we should emit deprecation warnings -- if at all -- and whether my [assumptions in this subthread](https://github.com/sphinx-doc/sphinx/pull/11684#discussion_r1337404066) are correct.
jayaddison: Ok, reading back through the commentary, there's one detail I missed:

> For this to go into 7.3, it should be opt-in; we can flip to opt-out in 8.0.

So we should switch the default, not remove the configuration value entirely.  I'll update the deprecation notice about this correspondingly -- and as I understand from the other comments, deprecation warnings don't become errors, so that should be innocuous, and we can remove the warning from 8.0 onwards.
jayaddison: I think this is ready - no further commits/changes planned.
jayaddison: @AA-Turner @picnixz any further feedback for these changes?  I don't know if it'll have much effect in practice, but if the number of people who have broken hyperlinks and don't realize it yet is greater than zero, I'd like to help with that.
picnixz: Personally I don't have anything else in mind. However note that I willl be extremely busy for the next months as I am leaning towards the end of my thesis.

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 471d99626..73cb7409d 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -53,6 +53,10 @@ Bugs fixed
 * #11715: Apply ``tls_verify`` and ``tls_cacerts`` config to
   ``ImageDownloader``.
   Patch by Nick Touran.
+* #11433: Added the ``linkcheck_allow_unauthorized`` configuration option.
+  Set this option to ``False`` to report HTTP 401 (unauthorized) server
+  responses as broken.
+  Patch by James Addison.
 
 Testing
 -------
diff --git a/doc/usage/configuration.rst b/doc/usage/configuration.rst
index f2367a8e5..5eff8eb57 100644
--- a/doc/usage/configuration.rst
+++ b/doc/usage/configuration.rst
@@ -2915,6 +2915,18 @@ Options for the linkcheck builder
 
    .. versionadded:: 4.4
 
+.. confval:: linkcheck_allow_unauthorized
+
+   When a webserver responds with an HTTP 401 (unauthorized) response, the
+   current default behaviour of Sphinx is to treat the link as "working".  To
+   change that behaviour, set this option to ``False``.
+
+   The default value for this option will be changed in Sphinx 8.0; from that
+   version onwards, HTTP 401 responses to checked hyperlinks will be treated
+   as "broken" by default.
+
+   .. versionadded:: 7.3
+
 
 Options for the XML builder
 ---------------------------
diff --git a/sphinx/builders/linkcheck.py b/sphinx/builders/linkcheck.py
index 460598ef2..85351b948 100644
--- a/sphinx/builders/linkcheck.py
+++ b/sphinx/builders/linkcheck.py
@@ -7,6 +7,7 @@
 import re
 import socket
 import time
+import warnings
 from html.parser import HTMLParser
 from os import path
 from queue import PriorityQueue, Queue
@@ -18,6 +19,7 @@
 from requests.exceptions import ConnectionError, HTTPError, SSLError, TooManyRedirects
 
 from sphinx.builders.dummy import DummyBuilder
+from sphinx.deprecation import RemovedInSphinx80Warning
 from sphinx.locale import __
 from sphinx.transforms.post_transforms import SphinxPostTransform
 from sphinx.util import encode_uri, logging, requests
@@ -66,6 +68,15 @@ def init(self) -> None:
         # set a timeout for non-responding servers
         socket.setdefaulttimeout(5.0)
 
+        if not self.config.linkcheck_allow_unauthorized:
+            deprecation_msg = (
+                "The default value for 'linkcheck_allow_unauthorized' will change "
+                "from `True` in Sphinx 7.3+ to `False`, meaning that HTTP 401 "
+                "unauthorized responses will be reported as broken by default. "
+                "See https://github.com/sphinx-doc/sphinx/issues/11433 for details."
+            )
+            warnings.warn(deprecation_msg, RemovedInSphinx80Warning, stacklevel=1)
+
     def finish(self) -> None:
         checker = HyperlinkAvailabilityChecker(self.config)
         logger.info('')
@@ -283,6 +294,7 @@ def __init__(self, config: Config,
         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
 
         self.user_agent = config.user_agent
         self.tls_verify = config.tls_verify
@@ -437,9 +449,30 @@ def _check_uri(self, uri: str, hyperlink: Hyperlink) -> tuple[str, str, int]:
             except HTTPError as err:
                 error_message = str(err)
 
-                # Unauthorised: the reference probably exists
+                # Unauthorized: the client did not provide required credentials
                 if status_code == 401:
-                    return 'working', 'unauthorized', 0
+                    if self._allow_unauthorized:
+                        deprecation_msg = (
+                            "\n---\n"
+                            "The linkcheck builder encountered an HTTP 401 "
+                            "(unauthorized) response, and will report it as "
+                            "'working' in this version of Sphinx to maintain "
+                            "backwards-compatibility."
+                            "\n"
+                            "This logic will change in Sphinx 8.0 which will "
+                            "report the hyperlink as 'broken'."
+                            "\n"
+                            "To explicitly continue treating unauthorized "
+                            "hyperlink responses as 'working', set the "
+                            "'linkcheck_allow_unauthorized' config option to "
+                            "``True``."
+                            "\n"
+                            "See sphinx-doc/sphinx#11433 for details."
+                            "\n---"
+                        )
+                        warnings.warn(deprecation_msg, RemovedInSphinx80Warning, stacklevel=1)
+                    status = 'working' if self._allow_unauthorized else 'broken'
+                    return status, 'unauthorized', 0
 
                 # Rate limiting; back-off if allowed, or report failure otherwise
                 if status_code == 429:
@@ -625,6 +658,7 @@ def setup(app: Sphinx) -> dict[str, Any]:
     app.add_config_value('linkcheck_anchors_ignore', ['^!'], '')
     app.add_config_value('linkcheck_anchors_ignore_for_url', (), '', (tuple, list))
     app.add_config_value('linkcheck_rate_limit_timeout', 300.0, '')
+    app.add_config_value('linkcheck_allow_unauthorized', True, '')
 
     app.add_event('linkcheck-process-uri')
 


Test patch:
diff --git a/tests/test_build_linkcheck.py b/tests/test_build_linkcheck.py
index 2ba817579..28529d9b0 100644
--- a/tests/test_build_linkcheck.py
+++ b/tests/test_build_linkcheck.py
@@ -348,8 +348,12 @@ class CustomHandler(http.server.BaseHTTPRequestHandler):
 
         def authenticated(method):
             def method_if_authenticated(self):
-                if (expected_token is None
-                        or self.headers["Authorization"] == f"Basic {expected_token}"):
+                if expected_token is None:
+                    return method(self)
+                elif not self.headers["Authorization"]:
+                    self.send_response(401, "Unauthorized")
+                    self.end_headers()
+                elif self.headers["Authorization"] == f"Basic {expected_token}":
                     return method(self)
                 else:
                     self.send_response(403, "Forbidden")
@@ -392,6 +396,21 @@ def test_auth_header_uses_first_match(app):
     assert content["status"] == "working"
 
 
+@pytest.mark.filterwarnings('ignore::sphinx.deprecation.RemovedInSphinx80Warning')
+@pytest.mark.sphinx(
+    'linkcheck', testroot='linkcheck-localserver', freshenv=True,
+    confoverrides={'linkcheck_allow_unauthorized': False})
+def test_unauthorized_broken(app):
+    with http_server(custom_handler(valid_credentials=("user1", "password"))):
+        app.build()
+
+    with open(app.outdir / "output.json", encoding="utf-8") as fp:
+        content = json.load(fp)
+
+    assert content["info"] == "unauthorized"
+    assert content["status"] == "broken"
+
+
 @pytest.mark.sphinx(
     'linkcheck', testroot='linkcheck-localserver', freshenv=True,
     confoverrides={'linkcheck_auth': [(r'^$', ('user1', 'password'))]})
@@ -402,10 +421,9 @@ def test_auth_header_no_match(app):
     with open(app.outdir / "output.json", encoding="utf-8") as fp:
         content = json.load(fp)
 
-    # TODO: should this test's webserver return HTTP 401 here?
-    # https://github.com/sphinx-doc/sphinx/issues/11433
-    assert content["info"] == "403 Client Error: Forbidden for url: http://localhost:7777/"
-    assert content["status"] == "broken"
+    # This link is considered working based on the default linkcheck_allow_unauthorized=true
+    assert content["info"] == "unauthorized"
+    assert content["status"] == "working"
 
 
 @pytest.mark.sphinx(