diff --git a/README.md b/README.md index 8800901..f196d6d 100644 --- a/README.md +++ b/README.md @@ -58,11 +58,11 @@ deployment instructions) is the configuration format: * the `server` section is renamed `web` * `scan.tempDirectory` is renamed `scan.temp_directory` -* `scan.baseUrl` is renamed `download.base_homeserver_url` (and becomes optional) +* `scan.baseUrl` is renamed `download.base_homeserver_url` and is now required. All media is + downloaded via the homeserver at this URL, regardless of which homeserver it originated from. * `scan.doNotCacheExitCodes` is renamed `result_cache.exit_codes_to_ignore` -* `scan.directDownload` is removed. Direct download always happens when `download.base_homeserver_url` - is absent from the configuration file, and setting a value for it will always cause files to be - downloaded from the server configured. +* `scan.directDownload` is removed. Files are always downloaded from the homeserver configured + in `download.base_homeserver_url`. * `proxy` is renamed `download.proxy` * `middleware.encryptedBody.pickleKey` is renamed `crypto.pickle_key` * `middleware.encryptedBody.picklePath` is renamed `crypto.pickle_path` diff --git a/config.sample.yaml b/config.sample.yaml index fb94f0e..bebcabc 100644 --- a/config.sample.yaml +++ b/config.sample.yaml @@ -87,18 +87,13 @@ result_cache: # Configuration for downloading files. -# When downloading files directly from their respective homeservers (which is the default -# behaviour), the homeservers' default URLs are determined using .well-known discovery -# (defaults to using the homeserver's domain if not available). -# See https://spec.matrix.org/latest/client-server-api/#server-discovery for more info. -# Settings in this section (apart from `base_homeserver_url`) apply to .well-known -# discovery requests as well as file download ones. download: - # If provided, all files are downloaded using the homeserver at this URL. If this - # setting is provided, .well-known discovery is not used to determine the base URL - # to use. - # Optional, defaults to downloading files directly from their respective homeservers. - base_homeserver_url: "https://matrix.org" + # All files are downloaded via the homeserver at this URL, regardless of which + # homeserver they originated from. The content scanner never contacts a media's + # origin homeserver, so that it never sends an Authorization header + # (and thus an access token) to a server other than this one. + # It is expected that there is one instance of content scanner per one homeserver. + base_homeserver_url: "https://matrix-client.matrix.org" # HTTP(S) proxy to use when sending requests. # Optional, defaults to no proxy. diff --git a/src/matrix_content_scanner/config.py b/src/matrix_content_scanner/config.py index 33d33a9..c2ec5f1 100644 --- a/src/matrix_content_scanner/config.py +++ b/src/matrix_content_scanner/config.py @@ -58,7 +58,7 @@ def _parse_size(size: Optional[Union[str, float]]) -> Optional[float]: # Schema to validate the raw configuration dictionary against. _config_schema = { "type": "object", - "required": ["web", "scan", "crypto"], + "required": ["web", "scan", "crypto", "download"], "additionalProperties": False, "properties": { "web": { @@ -84,6 +84,7 @@ def _parse_size(size: Optional[Union[str, float]]) -> Optional[float]: }, "download": { "type": "object", + "required": ["base_homeserver_url"], "additionalProperties": False, "properties": { "base_homeserver_url": {"type": "string"}, @@ -153,7 +154,7 @@ class ResultCacheConfig: class DownloadConfig: """Configuration for downloading files.""" - base_homeserver_url: Optional[str] = None + base_homeserver_url: str proxy: Optional[str] = None additional_headers: Optional[Dict[str, str]] = None headers_to_forward: Optional[List[str]] = None diff --git a/src/matrix_content_scanner/scanner/file_downloader.py b/src/matrix_content_scanner/scanner/file_downloader.py index e26e274..e2b879c 100644 --- a/src/matrix_content_scanner/scanner/file_downloader.py +++ b/src/matrix_content_scanner/scanner/file_downloader.py @@ -6,16 +6,13 @@ import logging import urllib.parse from http import HTTPStatus -from typing import TYPE_CHECKING, Dict, Optional, Tuple +from typing import TYPE_CHECKING, Optional, Tuple import aiohttp from multidict import CIMultiDict, CIMultiDictProxy, MultiMapping from matrix_content_scanner.utils.constants import ErrCode -from matrix_content_scanner.utils.errors import ( - ContentScannerRestError, - WellKnownDiscoveryError, -) +from matrix_content_scanner.utils.errors import ContentScannerRestError from matrix_content_scanner.utils.types import MediaDescription if TYPE_CHECKING: @@ -38,7 +35,6 @@ class FileDownloader: def __init__(self, mcs: "MatrixContentScanner"): self._base_url = mcs.config.download.base_homeserver_url - self._well_known_cache: Dict[str, Optional[str]] = {} self._proxy_url = mcs.config.download.proxy self._additional_headers = ( mcs.config.download.additional_headers @@ -149,37 +145,17 @@ async def _build_https_url( this is either "v3" or "r0". Returns: - An https URL to use. If `base_homeserver_url` is set in the config, this - will be used as the base of the URL. + A URL to use, always based on `base_homeserver_url`. """ server_name, media_id = media_path.split("/") - # Figure out what base URL to use. If one is specified in the configuration file, - # use it, otherwise try to discover one using .well-known. If that fails, use the - # server name with an HTTPS scheme. - if self._base_url is not None: - base_url = self._base_url - else: - base_url = None - - try: - base_url = await self._discover_via_well_known(server_name) - except WellKnownDiscoveryError as e: - # We don't catch ContentScannerRestErrors here because if one makes its - # way up here then it likely means that trying to reach https://server_name - # failed, in which case we're unlikely to be able to reach it again when - # downloading the file, so we let the error escalate. - logger.info("Failed to discover server via well-known: %s", e) - - if base_url is None: - # base_url might be None if either .well-known discovery failed, or we - # didn't find a .well-known file. - base_url = "https://" + server_name - - # Build the full URL. + # Build the full URL, always against the configured base homeserver URL. We + # never contact the media's origin homeserver directly, so that we never send + # an Authorization header to a server other than the one we've been configured + # to trust. path_prefix = prefix % endpoint_version url = "%s/%s/%s/%s" % ( - base_url, + self._base_url, path_prefix, urllib.parse.quote(server_name), urllib.parse.quote(media_id), @@ -279,88 +255,6 @@ async def _get_file_content( response_headers=headers, ) - async def _discover_via_well_known(self, domain: str) -> Optional[str]: - """Try to discover the base URL for the given domain via .well-known client - discovery. - - Args: - domain: The domain to discover the base URL for. - - Returns: - The base URL to use, or None if no .well-known client file exist for this - domain. - - Raises: - WellKnownDiscoveryError if an error happened during the discovery attempt. - """ - # Check if we already have a result cached, and if so return with it straight - # away. - if domain in self._well_known_cache: - logger.info("Fetching .well-known discovery result from cache") - return self._well_known_cache[domain] - - # Attempt to download the .well-known file. - try: - url = f"https://{domain}/.well-known/matrix/client" - code, body, _ = await self._get(url) - except ContentScannerRestError: - raise WellKnownDiscoveryError(f"Failed to reach web server at {domain}") - - if code != 200: - if code == 404: - # If the response status is 404, then the homeserver hasn't set up - # .well-known discovery, in which case we tell the caller that there's - # no base URL to use rather than raising an error. - # The difference is that we want to cache this result here, but we don't - # want to do that when the discovery fails due to an incorrectly set up - # file or an unavailable homeserver, which might be fixed later on. - logger.info( - ".well-known discover has not been set up for this homeserver" - ) - self._well_known_cache[domain] = None - return None - - raise WellKnownDiscoveryError( - f"Server responded with non-200 status {code}" - ) - - # Try to parse the JSON content. - try: - parsed_body = json.loads(body) - except json.decoder.JSONDecodeError as e: - raise WellKnownDiscoveryError(e) - - # Check if the parsed content has a base URL in the right place. - try: - base_url: str = parsed_body["m.homeserver"]["base_url"] - except (KeyError, TypeError): - # We might get a KeyError if we're trying to reach a key that doesn't exist, - # and we might get a TypeError if parsed_body or parsed_body["m.homeserver"] - # isn't a dictionary. - raise WellKnownDiscoveryError("Response did not include a usable URL") - - # Remove the trailing slash if there is one. - if base_url.endswith("/"): - base_url = base_url[:-1] - - # Check if the base URL is one for a working homeserver. - url = base_url + "/_matrix/client/versions" - try: - code, _, _ = await self._get(url) - except ContentScannerRestError: - raise WellKnownDiscoveryError( - "Base URL does not seem to point to a working homeserver" - ) - - if code != 200: - raise WellKnownDiscoveryError( - "Base URL does not seem to point to a working homeserver" - ) - - # Cache and return the result. - self._well_known_cache[domain] = base_url - return base_url - async def _get( self, url: str, diff --git a/src/matrix_content_scanner/utils/errors.py b/src/matrix_content_scanner/utils/errors.py index 928a6ee..e6654aa 100644 --- a/src/matrix_content_scanner/utils/errors.py +++ b/src/matrix_content_scanner/utils/errors.py @@ -53,7 +53,3 @@ def __init__(self, info: Optional[str]) -> None: class ConfigError(Exception): """An error indicating an issue with the configuration file.""" - - -class WellKnownDiscoveryError(Exception): - """An error indicating a failure when attempting a .well-known discovery.""" diff --git a/tests/scanner/test_file_downloader.py b/tests/scanner/test_file_downloader.py index 5a4b0b6..979a28c 100644 --- a/tests/scanner/test_file_downloader.py +++ b/tests/scanner/test_file_downloader.py @@ -2,18 +2,13 @@ # # SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial # Please see LICENSE files in the repository root for full details. -import json -from typing import Dict, List, Optional, Tuple, Union +from typing import Dict, List, Optional, Tuple from unittest import IsolatedAsyncioTestCase from unittest.mock import Mock, call from multidict import CIMultiDict, CIMultiDictProxy, MultiDictProxy, MultiMapping -from matrix_content_scanner.utils.errors import ( - ContentScannerRestError, - WellKnownDiscoveryError, -) -from matrix_content_scanner.utils.types import JsonDict +from matrix_content_scanner.utils.errors import ContentScannerRestError from tests.testutils import ( MEDIA_PATH, @@ -26,7 +21,6 @@ class FileDownloaderTestCase(IsolatedAsyncioTestCase): def setUp(self) -> None: - # Set a fixed base URL so that .well-known discovery doesn't get in the way. content_scanner = get_content_scanner( {"download": {"base_homeserver_url": "http://my-site.com"}} ) @@ -42,9 +36,7 @@ async def _get( query: Optional[MultiDictProxy[str]] = None, auth_header: Optional[str] = None, ) -> Tuple[int, bytes, CIMultiDictProxy[str]]: - """Mock for the _get method on the file downloader that doesn't serve a - .well-known client file. - """ + """Mock for the _get method on the file downloader.""" if ( url.endswith( ( @@ -64,8 +56,6 @@ async def _get( return self.media_status, self.media_body, self.media_headers else: return 404, b"Not found", CIMultiDictProxy(CIMultiDict()) - elif url.endswith("/.well-known/matrix/client"): - return 404, b"Not found", CIMultiDictProxy(CIMultiDict()) raise RuntimeError("Unexpected request on %s" % url) @@ -144,30 +134,6 @@ async def test_download_auth_media_missing_token(self) -> None: self.assertTrue(args[0].startswith("http://my-site.com/")) self.assertIn("/_matrix/client/v1/media/download/" + MEDIA_PATH, args[0]) - async def test_no_base_url(self) -> None: - """Tests that configuring a base homeserver URL means files are downloaded from - that homeserver (rather than the one the files were uploaded to) and .well-known - discovery is bypassed. - """ - self.downloader._base_url = None - await self.downloader.download_file(MEDIA_PATH) - - # Check that we've tried making a .well-known discovery request before - # downloading the file. - self.assertEqual(self.get_mock.call_count, 2) - self.assertEqual( - self.get_mock.mock_calls[0], call("https://foo/.well-known/matrix/client") - ) - self.assertEqual( - self.get_mock.mock_calls[1], - call( - "https://foo/_matrix/media/v3/download/" + MEDIA_PATH, - None, - query=None, - auth_header=None, - ), - ) - async def test_retry_on_404(self) -> None: """Tests that if we get a 404 when trying to download a file on a v3 path, we retry with an r0 path for backwards compatibility. @@ -320,99 +286,3 @@ def _set_headers(self, headers: Dict[str, List[str]]) -> None: md.add(k, el) self.media_headers = CIMultiDictProxy(md) - - -class WellKnownDiscoveryTestCase(IsolatedAsyncioTestCase): - def setUp(self) -> None: - self.downloader = get_content_scanner().file_downloader - - self.well_known_status = 200 - self.well_known_body: Union[bytes, JsonDict] = b"" - - self.versions_status = 200 - - async def _get( - url: str, - req_headers: Optional[MultiMapping[str]] = None, - query: Optional[MultiDictProxy[str]] = None, - auth_header: Optional[str] = None, - ) -> Tuple[int, bytes, CIMultiDictProxy[str]]: - """Mock for the _get method on the file downloader that serves a .well-known - client file. - """ - if url.endswith("/.well-known/matrix/client"): - if isinstance(self.well_known_body, bytes): - body_bytes = self.well_known_body - else: - body_bytes = json.dumps(self.well_known_body).encode("utf-8") - - return ( - self.well_known_status, - body_bytes, - CIMultiDictProxy(CIMultiDict()), - ) - elif url.endswith("/_matrix/client/versions"): - return self.versions_status, b"{}", CIMultiDictProxy(CIMultiDict()) - elif url.endswith("/_matrix/media/v3/download/" + MEDIA_PATH): - return 200, SMALL_PNG, get_base_media_headers() - - raise RuntimeError("Unexpected request on %s" % url) - - # Mock _get so we don't actually try to download files. - self.get_mock = Mock(side_effect=_get) - self.downloader._get = self.get_mock # type: ignore[method-assign] - - async def test_discover(self) -> None: - """Checks that the base URL to use to download files can be discovered via - .well-known discovery. - """ - self.well_known_body = {"m.homeserver": {"base_url": "https://foo.bar"}} - - await self.downloader.download_file(MEDIA_PATH) - - # Check that we got 3 calls: - # * one to retrieve the .well-known file - # * one to check that the base URL can be used to interact with a homeserver - # (by hitting the /_matrix/client/versions endpoint) - # * one to download the file - self.assertEqual(self.get_mock.call_count, 3, self.get_mock.mock_calls) - - calls = self.get_mock.mock_calls - - self.assertEqual(calls[0], call("https://foo/.well-known/matrix/client")) - self.assertTrue(calls[1], call("https://foo.bar/_matrix/client/versions")) - self.assertTrue( - calls[2], call("https://foo.bar/_matrix/media/v3/download/" + MEDIA_PATH) - ) - - async def test_error_status(self) -> None: - """Tests that we raise a WellKnownDiscoveryError if the server responded with an - error.""" - self.well_known_status = 401 - await self._assert_discovery_fail() - - async def test_malformed_content(self) -> None: - """Tests that we raise a WellKnownDiscoveryError if the server responded with a - body that isn't compliant with the Matrix specification.""" - self.well_known_body = {"m.homeserver": "https://foo.bar"} - await self._assert_discovery_fail() - - async def test_not_valid_homeserver(self) -> None: - """Tests that we raise a WellKnownDiscoveryError if the server at the provided - base URL isn't a Matrix homeserver.""" - self.versions_status = 404 - await self._assert_discovery_fail() - - async def test_404_no_fail(self) -> None: - """Tests that we don't raise a WellKnownDiscoveryError if the .well-known file - couldn't be found, and that we return None instead of the discovered base URL in - this case. - """ - self.well_known_status = 404 - res = await self.downloader._discover_via_well_known("foo") - self.assertIsNone(res) - - async def _assert_discovery_fail(self) -> None: - """Checks that .well-known discovery fails and raises a WellKnownDiscoveryError.""" - with self.assertRaises(WellKnownDiscoveryError): - await self.downloader._discover_via_well_known("foo") diff --git a/tests/testutils.py b/tests/testutils.py index d7b5efa..66ec07c 100644 --- a/tests/testutils.py +++ b/tests/testutils.py @@ -119,6 +119,9 @@ def get_content_scanner(config: Optional[JsonDict] = None) -> MatrixContentScann "crypto": { "request_secret_path": "tests/testdata/request_secret", }, + "download": { + "base_homeserver_url": "http://my-site.com", + }, } if config is None: