Repository: yt-dlp/yt-dlp
Pull request #7604 (2023-07-15 23:14:49)

[networking/rh] Add strict extension checking
This adds strict checks for extensions in RequestHandlers.

Previously the logic meant any extension received was optional to support. However, this causes issues if an extension is required to be supported for the request.

This PR changes it so RequestHandlers have to specify what extensions they support. If they don't support an extension then the request is marked as unsupported by the handler. This is done through removing extensions from a copy of the extensions dict when checking.

Since the architecture is very new, compat is not a concern yet.

I won't do it as part of this PR, but if we need the support for optional extensions there is lots of ways we can go about adding it if need be, such as :
- extensions starting with underscore
- `__optional__` extension containing list of optional extensions to ignore


<details open><summary>Template</summary> <!-- OPEN is intentional -->

<!--

# PLEASE FOLLOW THE GUIDE BELOW

- You will be asked some questions, please read them **carefully** and answer honestly
- Put an `x` into all the boxes `[ ]` relevant to your *pull request* (like [x])
- Use *Preview* tab to see how your *pull request* will actually look like

-->

### Before submitting a *pull request* make sure you have:
- [X] At least skimmed through [contributing guidelines](https://github.com/yt-dlp/yt-dlp/blob/master/CONTRIBUTING.md#developer-instructions) including [yt-dlp coding conventions](https://github.com/yt-dlp/yt-dlp/blob/master/CONTRIBUTING.md#yt-dlp-coding-conventions)
- [X] [Searched](https://github.com/yt-dlp/yt-dlp/search?q=is%3Apr&type=Issues) the bugtracker for similar pull requests
- [X] Checked the code with [flake8](https://pypi.python.org/pypi/flake8) and [ran relevant tests](https://github.com/yt-dlp/yt-dlp/blob/master/CONTRIBUTING.md#developer-instructions)

### In order to be accepted and merged into yt-dlp each piece of code must be in public domain or released under [Unlicense](http://unlicense.org/). Check all of the following options that apply:
- [X] I am the original author of this code and I am willing to release it under [Unlicense](http://unlicense.org/)
- [ ] I am not the original author of this code but it is in public domain or released under [Unlicense](http://unlicense.org/) (provide reliable evidence)

### What is the purpose of your *pull request*?
- [ ] Fix or improvement to an extractor (Make sure to add/update tests)
- [ ] New extractor ([Piracy websites will not be accepted](https://github.com/yt-dlp/yt-dlp/blob/master/CONTRIBUTING.md#is-the-website-primarily-used-for-piracy))
- [X] Core bug fix/improvement
- [X] New feature (It is strongly [recommended to open an issue first](https://github.com/yt-dlp/yt-dlp/blob/master/CONTRIBUTING.md#adding-new-feature-or-making-overarching-changes))


<!-- Do NOT edit/remove anything below this! -->
</details><details><summary>Copilot Summary</summary>  

<!--
copilot:all
-->
### <samp>🤖 Generated by Copilot at 8265877</samp>

### Summary
🌐🧪🔧

<!--
1.  🌐 - This emoji represents the `urllib` module and the network requests that the `UrllibRH` class handles. It also suggests the global scope of the networking functionality.
2.  🧪 - This emoji represents the testing and refactoring of the extension validation logic in `test_networking.py`. It also suggests the experimental and scientific nature of the tests.
3.  🔧 - This emoji represents the support and processing of request extensions in the `RequestHandler` class and its subclasses. It also suggests the tool-like and customizable nature of the extensions.
-->
This pull request adds support for request extensions to the `RequestHandler` class and its subclasses, which are used to perform network requests in `yt-dlp`. It also refactors the extension validation tests in `test_networking.py` to use a parametrized method.

> _Oh we are the coders of the sea_
> _We write our `RequestHandler`s with glee_
> _We check our extensions on the count of three_
> _And raise an `UnsupportedRequest` if we disagree_

### Walkthrough
*  Add and document `_SUPPORTED_EXTENSIONS` attribute to `RequestHandler` and its subclasses to validate request extensions ([link](https://github.com/yt-dlp/yt-dlp/pull/7604/files?diff=unified&w=0#diff-e92498eb46ad8369b16aed0d65fa7c8d56c81b2d595f6510981c6cfb0c0f7e78R826), [link](https://github.com/yt-dlp/yt-dlp/pull/7604/files?diff=unified&w=0#diff-c7b2bfc3ad326c23db07694e9137f86e58e526681dab17e7680a968a7190e232R380), [link](https://github.com/yt-dlp/yt-dlp/pull/7604/files?diff=unified&w=0#diff-ead7fca4af365bb8c89c1a0257a027147d0533eb2e0dfb395d08a9a12e58d4abR150-R154), [link](https://github.com/yt-dlp/yt-dlp/pull/7604/files?diff=unified&w=0#diff-ead7fca4af365bb8c89c1a0257a027147d0533eb2e0dfb395d08a9a12e58d4abR191))
*  Implement core extension check in `_check_timeout_extension` method of `RequestHandler` to raise `UnsupportedRequest` exception for unsupported extensions ([link](https://github.com/yt-dlp/yt-dlp/pull/7604/files?diff=unified&w=0#diff-ead7fca4af365bb8c89c1a0257a027147d0533eb2e0dfb395d08a9a12e58d4abR285-R288))
*  Refactor and parametrize extension tests in `test_networking.py` using `EXTENSION_TESTS` attribute and `test_extension` method ([link](https://github.com/yt-dlp/yt-dlp/pull/7604/files?diff=unified&w=0#diff-e92498eb46ad8369b16aed0d65fa7c8d56c81b2d595f6510981c6cfb0c0f7e78R871-R885), [link](https://github.com/yt-dlp/yt-dlp/pull/7604/files?diff=unified&w=0#diff-e92498eb46ad8369b16aed0d65fa7c8d56c81b2d595f6510981c6cfb0c0f7e78L912-R935))



</details>


pukkandan: I aumed thee will just be added to SUPPORTED_FEATURES. Why not do that?

Patch:
diff --git a/yt_dlp/compat/__init__.py b/yt_dlp/compat/__init__.py
index a41a80ebb..832a9138d 100644
--- a/yt_dlp/compat/__init__.py
+++ b/yt_dlp/compat/__init__.py
@@ -1,14 +1,11 @@
 import os
 import sys
-import warnings
 import xml.etree.ElementTree as etree
 
-from ._deprecated import *  # noqa: F401, F403
 from .compat_utils import passthrough_module
 
-# XXX: Implement this the same way as other DeprecationWarnings without circular import
-passthrough_module(__name__, '._legacy', callback=lambda attr: warnings.warn(
-    DeprecationWarning(f'{__name__}.{attr} is deprecated'), stacklevel=5))
+passthrough_module(__name__, '._deprecated')
+del passthrough_module
 
 
 # HTMLParseError has been deprecated in Python 3.3 and removed in
diff --git a/yt_dlp/compat/_deprecated.py b/yt_dlp/compat/_deprecated.py
index 14d37b236..607bae999 100644
--- a/yt_dlp/compat/_deprecated.py
+++ b/yt_dlp/compat/_deprecated.py
@@ -1,4 +1,12 @@
 """Deprecated - New code should avoid these"""
+import warnings
+
+from .compat_utils import passthrough_module
+
+# XXX: Implement this the same way as other DeprecationWarnings without circular import
+passthrough_module(__name__, '.._legacy', callback=lambda attr: warnings.warn(
+    DeprecationWarning(f'{__name__}.{attr} is deprecated'), stacklevel=6))
+del passthrough_module
 
 import base64
 import urllib.error
diff --git a/yt_dlp/compat/urllib/__init__.py b/yt_dlp/compat/urllib/__init__.py
index 6b6b8e103..b27cc6133 100644
--- a/yt_dlp/compat/urllib/__init__.py
+++ b/yt_dlp/compat/urllib/__init__.py
@@ -1,6 +1,9 @@
 # flake8: noqa: F405
 from urllib import *  # noqa: F403
 
+del request
+from . import request  # noqa: F401
+
 from ..compat_utils import passthrough_module
 
 passthrough_module(__name__, 'urllib')
diff --git a/yt_dlp/extractor/patreon.py b/yt_dlp/extractor/patreon.py
index 447087436..9316789df 100644
--- a/yt_dlp/extractor/patreon.py
+++ b/yt_dlp/extractor/patreon.py
@@ -2,21 +2,21 @@
 
 from .common import InfoExtractor
 from .vimeo import VimeoIE
-
 from ..compat import compat_urllib_parse_unquote
 from ..networking.exceptions import HTTPError
 from ..utils import (
+    KNOWN_EXTENSIONS,
+    ExtractorError,
     clean_html,
     determine_ext,
-    ExtractorError,
     int_or_none,
-    KNOWN_EXTENSIONS,
     mimetype2ext,
     parse_iso8601,
     str_or_none,
     traverse_obj,
     try_get,
     url_or_none,
+    urljoin,
 )
 
 
@@ -404,8 +404,8 @@ def _entries(self, campaign_id):
             posts_json = self._call_api('posts', campaign_id, query=params, note='Downloading posts page %d' % page)
 
             cursor = traverse_obj(posts_json, ('meta', 'pagination', 'cursors', 'next'))
-            for post in posts_json.get('data') or []:
-                yield self.url_result(url_or_none(traverse_obj(post, ('attributes', 'patreon_url'))), 'Patreon')
+            for post_url in traverse_obj(posts_json, ('data', ..., 'attributes', 'patreon_url')):
+                yield self.url_result(urljoin('https://www.patreon.com/', post_url), PatreonIE)
 
             if cursor is None:
                 break
diff --git a/yt_dlp/networking/_urllib.py b/yt_dlp/networking/_urllib.py
index ff3a22c8c..3fe5fa52e 100644
--- a/yt_dlp/networking/_urllib.py
+++ b/yt_dlp/networking/_urllib.py
@@ -385,6 +385,11 @@ def __init__(self, *, enable_file_urls: bool = False, **kwargs):
         if self.enable_file_urls:
             self._SUPPORTED_URL_SCHEMES = (*self._SUPPORTED_URL_SCHEMES, 'file')
 
+    def _check_extensions(self, extensions):
+        super()._check_extensions(extensions)
+        extensions.pop('cookiejar', None)
+        extensions.pop('timeout', None)
+
     def _create_instance(self, proxies, cookiejar):
         opener = urllib.request.OpenerDirector()
         handlers = [
diff --git a/yt_dlp/networking/common.py b/yt_dlp/networking/common.py
index 7f7457978..ab26a0628 100644
--- a/yt_dlp/networking/common.py
+++ b/yt_dlp/networking/common.py
@@ -21,6 +21,7 @@
     TransportError,
     UnsupportedRequest,
 )
+from ..compat.types import NoneType
 from ..utils import (
     bug_reports_message,
     classproperty,
@@ -147,6 +148,7 @@ class RequestHandler(abc.ABC):
         a proxy url with an url scheme not in this list will raise an UnsupportedRequest.
 
     - `_SUPPORTED_FEATURES`: a tuple of supported features, as defined in Features enum.
+
     The above may be set to None to disable the checks.
 
     Parameters:
@@ -169,9 +171,14 @@ class RequestHandler(abc.ABC):
     Requests may have additional optional parameters defined as extensions.
      RequestHandler subclasses may choose to support custom extensions.
 
+    If an extension is supported, subclasses should extend _check_extensions(extensions)
+    to pop and validate the extension.
+    - Extensions left in `extensions` are treated as unsupported and UnsupportedRequest will be raised.
+
     The following extensions are defined for RequestHandler:
-    - `cookiejar`: Cookiejar to use for this request
-    - `timeout`: socket timeout to use for this request
+    - `cookiejar`: Cookiejar to use for this request.
+    - `timeout`: socket timeout to use for this request.
+    To enable these, add extensions.pop('<extension>', None) to _check_extensions
 
     Apart from the url protocol, proxies dict may contain the following keys:
     - `all`: proxy to use for all protocols. Used as a fallback if no proxy is set for a specific protocol.
@@ -263,26 +270,19 @@ def _check_proxies(self, proxies):
             if scheme not in self._SUPPORTED_PROXY_SCHEMES:
                 raise UnsupportedRequest(f'Unsupported proxy type: "{scheme}"')
 
-    def _check_cookiejar_extension(self, extensions):
-        if not extensions.get('cookiejar'):
-            return
-        if not isinstance(extensions['cookiejar'], CookieJar):
-            raise UnsupportedRequest('cookiejar is not a CookieJar')
-
-    def _check_timeout_extension(self, extensions):
-        if extensions.get('timeout') is None:
-            return
-        if not isinstance(extensions['timeout'], (float, int)):
-            raise UnsupportedRequest('timeout is not a float or int')
-
     def _check_extensions(self, extensions):
-        self._check_cookiejar_extension(extensions)
-        self._check_timeout_extension(extensions)
+        """Check extensions for unsupported extensions. Subclasses should extend this."""
+        assert isinstance(extensions.get('cookiejar'), (CookieJar, NoneType))
+        assert isinstance(extensions.get('timeout'), (float, int, NoneType))
 
     def _validate(self, request):
         self._check_url_scheme(request)
         self._check_proxies(request.proxies or self.proxies)
-        self._check_extensions(request.extensions)
+        extensions = request.extensions.copy()
+        self._check_extensions(extensions)
+        if extensions:
+            # TODO: add support for optional extensions
+            raise UnsupportedRequest(f'Unsupported extensions: {", ".join(extensions.keys())}')
 
     @wrap_request_errors
     def validate(self, request: Request):
diff --git a/yt_dlp/utils/__init__.py b/yt_dlp/utils/__init__.py
index 0b00adddb..c267e326f 100644
--- a/yt_dlp/utils/__init__.py
+++ b/yt_dlp/utils/__init__.py
@@ -1,6 +1,4 @@
-# flake8: noqa: F401, F403
-import warnings
-
+# flake8: noqa: F403
 from ..compat.compat_utils import passthrough_module
 
 passthrough_module(__name__, '._deprecated')
@@ -9,4 +7,4 @@
 # isort: off
 from .traversal import *
 from ._utils import *
-from ._utils import _configuration_args, _get_exe_version_output
+from ._utils import _configuration_args, _get_exe_version_output  # noqa: F401


Test patch:
diff --git a/test/test_compat.py b/test/test_compat.py
index 003a97abf..71ca7f99f 100644
--- a/test/test_compat.py
+++ b/test/test_compat.py
@@ -9,15 +9,16 @@
 
 
 import struct
-import urllib.parse
 
 from yt_dlp import compat
+from yt_dlp.compat import urllib  # isort: split
 from yt_dlp.compat import (
     compat_etree_fromstring,
     compat_expanduser,
     compat_urllib_parse_unquote,
     compat_urllib_parse_urlencode,
 )
+from yt_dlp.compat.urllib.request import getproxies
 
 
 class TestCompat(unittest.TestCase):
@@ -28,8 +29,7 @@ def test_compat_passthrough(self):
         with self.assertWarns(DeprecationWarning):
             compat.WINDOWS_VT_MODE
 
-        # TODO: Test submodule
-        # compat.asyncio.events  # Must not raise error
+        self.assertEqual(urllib.request.getproxies, getproxies)
 
         with self.assertWarns(DeprecationWarning):
             compat.compat_pycrypto_AES  # Must not raise error
diff --git a/test/test_networking.py b/test/test_networking.py
index d4eba2a5d..1bd6afc88 100644
--- a/test/test_networking.py
+++ b/test/test_networking.py
@@ -804,10 +804,10 @@ def test_httplib_validation_errors(self, handler):
             assert not isinstance(exc_info.value, TransportError)
 
 
-def run_validation(handler, fail, req, **handler_kwargs):
+def run_validation(handler, error, req, **handler_kwargs):
     with handler(**handler_kwargs) as rh:
-        if fail:
-            with pytest.raises(UnsupportedRequest):
+        if error:
+            with pytest.raises(error):
                 rh.validate(req)
         else:
             rh.validate(req)
@@ -824,6 +824,9 @@ class NoCheckRH(ValidationRH):
         _SUPPORTED_PROXY_SCHEMES = None
         _SUPPORTED_URL_SCHEMES = None
 
+        def _check_extensions(self, extensions):
+            extensions.clear()
+
     class HTTPSupportedRH(ValidationRH):
         _SUPPORTED_URL_SCHEMES = ('http',)
 
@@ -834,26 +837,26 @@ class HTTPSupportedRH(ValidationRH):
             ('https', False, {}),
             ('data', False, {}),
             ('ftp', False, {}),
-            ('file', True, {}),
+            ('file', UnsupportedRequest, {}),
             ('file', False, {'enable_file_urls': True}),
         ]),
         (NoCheckRH, [('http', False, {})]),
-        (ValidationRH, [('http', True, {})])
+        (ValidationRH, [('http', UnsupportedRequest, {})])
     ]
 
     PROXY_SCHEME_TESTS = [
         # scheme, expected to fail
         ('Urllib', [
             ('http', False),
-            ('https', True),
+            ('https', UnsupportedRequest),
             ('socks4', False),
             ('socks4a', False),
             ('socks5', False),
             ('socks5h', False),
-            ('socks', True),
+            ('socks', UnsupportedRequest),
         ]),
         (NoCheckRH, [('http', False)]),
-        (HTTPSupportedRH, [('http', True)]),
+        (HTTPSupportedRH, [('http', UnsupportedRequest)]),
     ]
 
     PROXY_KEY_TESTS = [
@@ -863,8 +866,22 @@ class HTTPSupportedRH(ValidationRH):
             ('unrelated', False),
         ]),
         (NoCheckRH, [('all', False)]),
-        (HTTPSupportedRH, [('all', True)]),
-        (HTTPSupportedRH, [('no', True)]),
+        (HTTPSupportedRH, [('all', UnsupportedRequest)]),
+        (HTTPSupportedRH, [('no', UnsupportedRequest)]),
+    ]
+
+    EXTENSION_TESTS = [
+        ('Urllib', [
+            ({'cookiejar': 'notacookiejar'}, AssertionError),
+            ({'cookiejar': CookieJar()}, False),
+            ({'timeout': 1}, False),
+            ({'timeout': 'notatimeout'}, AssertionError),
+            ({'unsupported': 'value'}, UnsupportedRequest),
+        ]),
+        (NoCheckRH, [
+            ({'cookiejar': 'notacookiejar'}, False),
+            ({'somerandom': 'test'}, False),  # but any extension is allowed through
+        ]),
     ]
 
     @pytest.mark.parametrize('handler,scheme,fail,handler_kwargs', [
@@ -907,15 +924,16 @@ def test_empty_proxy(self, handler):
     @pytest.mark.parametrize('proxy_url', ['//example.com', 'example.com', '127.0.0.1'])
     @pytest.mark.parametrize('handler', ['Urllib'], indirect=True)
     def test_missing_proxy_scheme(self, handler, proxy_url):
-        run_validation(handler, True, Request('http://', proxies={'http': 'example.com'}))
-
-    @pytest.mark.parametrize('handler', ['Urllib'], indirect=True)
-    def test_cookiejar_extension(self, handler):
-        run_validation(handler, True, Request('http://', extensions={'cookiejar': 'notacookiejar'}))
+        run_validation(handler, UnsupportedRequest, Request('http://', proxies={'http': 'example.com'}))
 
-    @pytest.mark.parametrize('handler', ['Urllib'], indirect=True)
-    def test_timeout_extension(self, handler):
-        run_validation(handler, True, Request('http://', extensions={'timeout': 'notavalidtimeout'}))
+    @pytest.mark.parametrize('handler,extensions,fail', [
+        (handler_tests[0], extensions, fail)
+        for handler_tests in EXTENSION_TESTS
+        for extensions, fail in handler_tests[1]
+    ], indirect=['handler'])
+    def test_extension(self, handler, extensions, fail):
+        run_validation(
+            handler, fail, Request('http://', extensions=extensions))
 
     def test_invalid_request_type(self):
         rh = self.ValidationRH(logger=FakeLogger())