mikebridge commented on code in PR #45553: URL: https://github.com/apache/superset/pull/45553#discussion_r4383371383
########## superset/semantic_layers/transport.py: ########## @@ -0,0 +1,376 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Host-owned provider HTTP policy with shared deadline enforcement.""" + +from __future__ import annotations + +import heapq +import logging +import os +import socket +import time +from collections.abc import Callable, Mapping +from contextvars import Token +from dataclasses import dataclass, field +from io import BytesIO +from threading import Condition, Lock, Thread +from typing import Any, TYPE_CHECKING +from urllib.parse import SplitResult, urljoin, urlsplit + +import requests +from flask import current_app, has_app_context +from superset_core.semantic_layers.transport import ( + ProviderRequestError, + ProviderRequestFailure as Failure, + ProviderResponse, + ProviderTransport, + RequestBudget, +) +from urllib3.exceptions import HTTPError + +from superset.utils.network import ( + ConnectionDeadline, + NonPublicAddressError, + PinnedHTTPAdapter, + PROVIDER_DEADLINE, +) + +if TYPE_CHECKING: + from requests.sessions import _Settings + +logger: logging.Logger = logging.getLogger(__name__) +MAX_REDIRECTS: int = 3 +_CHUNK_BYTES: int = 65536 + + +class HostProviderTransport(ProviderTransport): + """Fix operator policy at construction; keep budgets and sockets call-local.""" + + def __init__(self, *, allow_internal_hosts: bool = False) -> None: + self._allow_internal: bool = allow_internal_hosts + + def request( + self, + provider: str, + method: str, + url: str, + *, + budget: RequestBudget, + headers: Mapping[str, str] | None = None, + params: Mapping[str, Any] | None = None, + json: Any = None, + consume: Callable[[int], None] | None = None, + ) -> ProviderResponse: + # Do not coerce arbitrary objects while handling a URL-bearing failure. + provider = provider if isinstance(provider, str) else "Provider" + bound: _Deadline = _Deadline(budget.deadline) + token: Token[ConnectionDeadline | None] = PROVIDER_DEADLINE.set(bound) + try: + bound.remaining() # No parsing, resolution or socket work after expiry. + self._validated_url(url) + with _DirectSession(allow_environment_ca=self._allow_internal) as session: + adapter: PinnedHTTPAdapter = PinnedHTTPAdapter( + public_only=not self._allow_internal + ) + session.mount("https://";, adapter) + session.mount("http://";, adapter) + hop: int + for hop in range(MAX_REDIRECTS + 1): + with session.request( + method, + url, + headers={**(headers or {}), "Accept-Encoding": "identity"}, + params=params, + json=json, + timeout=bound.remaining(), + stream=True, + allow_redirects=False, + ) as response: + target: str | None = ( + self._next_redirect(method, response) + if hop < MAX_REDIRECTS + else None + ) + if target is not None: + url, params = target, None + continue # Context closes the unread redirect body. + if not 200 <= response.status_code < 300: + raise ProviderRequestError( + Failure.HTTP_STATUS, status=response.status_code + ) + if ( + response.headers.get("Content-Encoding", "identity").lower() + != "identity" + ): + raise ProviderRequestError(Failure.UNSUPPORTED_ENCODING) + body: bytes = self._read_body( + response, bound, budget.max_bytes, consume + ) + result: ProviderResponse = ProviderResponse( + response.status_code, response.headers, body, response.url + ) + bound.remaining() + return result + raise AssertionError("Redirect loop must return or fail") + except ( + requests.RequestException, + HTTPError, + OSError, + ProviderRequestError, + ) as exc: + failure: ProviderRequestError = self._failure_for(exc, budget.deadline) + error: ProviderRequestError = ProviderRequestError( + failure.failure, status=failure.status, provider=provider + ) + logger.warning( + "%s request failed: category=%s status=%s", + error.provider, + error.failure.value, + error.status, + ) + raise error from None + finally: + PROVIDER_DEADLINE.reset(token) + bound.close() + + def _validated_url(self, url: str) -> SplitResult: + try: + parts: SplitResult = urlsplit(url) + parts.port # noqa: B018 Validate malformed ports before any I/O. + except ValueError: + raise ProviderRequestError(Failure.INVALID_URL) from None + if not parts.hostname: + raise ProviderRequestError(Failure.INVALID_URL) + if parts.scheme not in ( + {"https", "http"} if self._allow_internal else {"https"} + ): + raise ProviderRequestError(Failure.REFUSED) + return parts + + def _next_redirect(self, method: str, response: requests.Response) -> str | None: + if ( + method.upper() != "GET" + or response.status_code not in {301, 302, 303, 307, 308} + or not response.headers.get("Location") + ): + return None + before: SplitResult = self._validated_url(response.url) + try: + target: str = urljoin(response.url, response.headers["Location"]) + after: SplitResult = urlsplit(target) + after.port # noqa: B018 + except ValueError: + raise ProviderRequestError(Failure.INVALID_URL) from None + + def origin(parts: SplitResult) -> tuple[str, str | None, int]: + return ( + parts.scheme, + parts.hostname, + parts.port + if parts.port is not None + else (443 if parts.scheme == "https" else 80), + ) + + return target if origin(before) == origin(after) else None Review Comment: _AI-assisted reply on Mike's behalf._ Good catch, thanks. Fixed in this push: - Redirect targets are now checked against the URL Requests will actually connect to (after Requests' own URL preparation), so `https://evil.example\@provider.example/` is compared as `evil.example` and refused. - Locations containing a backslash, control characters or whitespace are refused outright. - Redirects that carry a username or password (`user:pw@`, including the encoded `%5C@` form) are refused, so Requests can't replace the caller's `Authorization` header with redirect-supplied credentials. - The first-hop URL check uses the same preparation. Adapter-level tests reproduce your case and the userinfo variants before the fix, and pass after. Relative and same-origin default-port redirects are still followed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
