From eeff8eb7ce3900cd213cca3eb9c128b9dafd9036 Mon Sep 17 00:00:00 2001 From: Matt Conflitti Date: Thu, 8 Oct 2026 17:11:56 -0400 Subject: [PATCH 1/5] fix(security)!: harden credential handling Keep the deferred security changes separate from the additive agent workflows. Protect default credential writes with private atomic replacement, suppress sensitive auth diagnostics, and exclude CLI configuration from generated manifests and bundles. BREAKING CHANGE: saves require a writable parent, replace destination symlinks, and leave hardlink aliases unchanged. Diagnostics provide less detail and configuration files cannot be forced into bundles. Trusted custom openers and prepared archives keep existing behavior. Refs #859 --- docs/CHANGELOG.md | 19 +- docs/commands/deploy.md | 8 + docs/commands/write-manifest.md | 5 + rsconnect/api.py | 38 ++-- rsconnect/bundle.py | 154 ++++++++++++--- rsconnect/connect_cloud.py | 4 - rsconnect/device_login.py | 22 ++- rsconnect/http_support.py | 71 +++---- rsconnect/metadata.py | 71 +++++-- rsconnect/oauth.py | 74 ++++---- tests/test_agent_cli_e2e.py | 136 ++++++++++++- tests/test_api.py | 65 ++++++- tests/test_bundle.py | 327 ++++++++++++++++++++++++++++++++ tests/test_connect_cloud.py | 24 +-- tests/test_device_login.py | 27 +-- tests/test_git_metadata.py | 4 +- tests/test_http_support.py | 235 +++++++++++++---------- tests/test_metadata.py | 231 ++++++++++++++++++++++ tests/test_oauth.py | 98 +++++++--- 19 files changed, 1270 insertions(+), 343 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 42f97cb70..7bb6de8df 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -9,12 +9,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - OAuth clients now register with Posit Connect as "Posit CLI", the name shown on the consent screen. Previously registered clients keep their existing name. +- Credential files written through the default file backend now use private + temporary files and atomic replacement, requiring write access to the parent + directory. Unchanged regular files have their permissions repaired. Default + protected saves replace a destination symlink even when its bytes are unchanged; hard-linked + copies no longer share subsequent updates. +- Authentication errors and verbose HTTP diagnostics omit provider-supplied + error descriptions, response reasons and redirect headers that could expose + credentials. OAuth response bodies and redirect destinations are also omitted + from verbose logs, along with all OAuth response header values. + Resumable Cloud account lookup uses the same protection. Encoded credential + query names are redacted, and cookie diagnostics omit names as well as values. +- Generated manifests and bundles exclude `.rsconnect-python` directories and + the configured CLI credential directory, including explicit extra files and + symlink aliases. Publishing from inside that directory is rejected, including + static notebook rendering before execution. Prepared + bundle archives remain unchanged. - `login` and `add --connect-cloud` can start device-code authentication and return approval details as JSON. Finish a pending login by nickname with `--finish`. Use `--timeout` to bound the entire finish invocation, including account lookup. Resumable login requires POSIX (Linux or macOS). Pending state is validated - before saving and uses owner-only permissions. Resumable authentication omits - sensitive provider diagnostics. Connect Cloud finish retains the original OAuth client. + before saving and uses owner-only permissions. Connect Cloud finish retains the original OAuth client. - Added `rsconnect preflight` to check Python and Node.js runtime availability on self-hosted Posit Connect and Snowpark Container Services (SPCS), from POSIX systems. For first publishes, it considers server installations marked publishable. Python diff --git a/docs/commands/deploy.md b/docs/commands/deploy.md index 9d5b05e5c..eef23fb0e 100644 --- a/docs/commands/deploy.md +++ b/docs/commands/deploy.md @@ -1,3 +1,11 @@ +Generated bundles exclude `.rsconnect-python` directories and the configured +CLI credential directory, including symlink aliases and files supplied as +explicit extras. Keep application files outside that directory; publishing +from inside it is rejected. + +Deploying an already prepared bundle uploads the archive as supplied. Check its +contents before deployment. + ::: mkdocs-click :module: rsconnect.main :command: deploy diff --git a/docs/commands/write-manifest.md b/docs/commands/write-manifest.md index 7b87cca61..da6868d7c 100644 --- a/docs/commands/write-manifest.md +++ b/docs/commands/write-manifest.md @@ -1,3 +1,8 @@ +Generated manifests exclude `.rsconnect-python` directories and the configured +CLI credential directory, including symlink aliases and files supplied as +explicit extras. Keep application files outside that directory; generating +a manifest from inside it is rejected. + ::: mkdocs-click :module: rsconnect.main :command: write_manifest diff --git a/rsconnect/api.py b/rsconnect/api.py index 19c491d10..96b1ef331 100644 --- a/rsconnect/api.py +++ b/rsconnect/api.py @@ -167,7 +167,8 @@ def handle_bad_response(self, response: HTTPResponse | T, is_httpresponse: bool safe_uri = _redacted_uri_for_log(response.full_uri) if response.exception: raise RSConnectException( - "Could not connect to %s - %s" % (_redacted_uri_for_log(self.url), response.exception), + "Could not connect to %s (%s)" + % (_redacted_uri_for_log(self.url), type(response.exception).__name__), cause=response.exception, ) # Sometimes an ISP will respond to an unknown server name by returning a friendly @@ -188,12 +189,11 @@ def handle_bad_response(self, response: HTTPResponse | T, is_httpresponse: bool raise RSConnectException(error, status=response.status) if response.status is None or response.status < 200 or response.status > 299: raise RSConnectException( - "Received an unexpected response from %s (calling %s): %s %s" + "Received an unexpected response from %s (calling %s): %s" % ( self.remote_name, safe_uri, response.status, - response.reason, ), status=response.status, ) @@ -203,12 +203,11 @@ def handle_bad_response(self, response: HTTPResponse | T, is_httpresponse: bool # prior function call was not converted from a HTTPResponse to JSON. This # should never happen, so raise an exception. raise RSConnectException( - "Received an unexpected response from %s (calling %s): %s %s" + "Received an unexpected response from %s (calling %s): %s" % ( self.remote_name, safe_uri, response.status, - response.reason, ) ) return response @@ -400,6 +399,7 @@ def fmt_payload(self): def exchange_token(self) -> str: try: server = HTTPServer(url=self.token_endpoint()) + server._suppress_oauth_response_logging = True payload = self.fmt_payload() response = server.request( @@ -412,17 +412,17 @@ def exchange_token(self) -> str: # since we don't want to pick up its json decoding assumptions if response.exception is not None: raise RSConnectException( - "Could not connect to %s - %s" % (self.token_endpoint(), response.exception), + "Could not connect to %s (%s)" + % (_redacted_uri_for_log(self.token_endpoint()), type(response.exception).__name__), cause=response.exception, ) if response.status is None or response.status < 200 or response.status > 299: raise RSConnectException( - "Received an unexpected response from %s (calling %s): %s %s" + "Received an unexpected response from %s (calling %s): %s" % ( self.url, - response.full_uri, + _redacted_uri_for_log(response.full_uri), response.status, - response.reason, ) ) @@ -598,10 +598,12 @@ def _attempt_token_refresh(self) -> bool: store._set(entry_name, entry) # type: ignore[possibly-undefined] logger.warning("OAuth client was re-registered; please run `rsconnect login` again.") except Exception as exc: - logger.warning(f"OAuth client re-registration failed: {exc}. Please run `rsconnect login` again.") + logger.warning( + "OAuth client re-registration failed (%s). Please run `rsconnect login` again." % type(exc).__name__ + ) return False except Exception as exc: - logger.warning(f"OAuth token refresh failed: {exc}") + logger.warning("OAuth token refresh failed (%s)." % type(exc).__name__) return False new_access = token_response["access_token"] @@ -3411,16 +3413,8 @@ def _refresh_user_token(self) -> dict[str, Any]: request_options["request_deadline"] = self.request_deadline if self._server.oauth_client_id is not None: request_options["client_id_override"] = self._server.oauth_client_id - if self._suppress_oauth_response_logging: - request_options["suppress_response_logging"] = True return connect_cloud.refresh(cast(str, self._server.refresh_token), self._server.environment, **request_options) - def _warn_refresh_failure(self, exception: Exception) -> None: - if self._suppress_oauth_response_logging: - logger.warning("Posit Connect Cloud token refresh failed (%s)." % type(exception).__name__) - else: - logger.warning("Posit Connect Cloud token refresh failed: %s" % exception) - def _attempt_token_refresh(self) -> bool: """Mint a new access token and apply it to this client. @@ -3448,7 +3442,7 @@ def _attempt_token_refresh(self) -> bool: except InvalidClientError as exc: if not service_account: # This CLI's own OAuth client, not the user's credential. - self._warn_refresh_failure(exc) + logger.warning("Posit Connect Cloud token refresh failed (%s)." % type(exc).__name__) return False raise RSConnectException( "The Posit Connect Cloud service account credential was rejected — it has been revoked or " @@ -3457,7 +3451,7 @@ def _attempt_token_refresh(self) -> bool: ) from exc except InvalidGrantError as exc: if service_account: - self._warn_refresh_failure(exc) + logger.warning("Posit Connect Cloud token refresh failed (%s)." % type(exc).__name__) return False self._persist_tokens(None, None) raise RSConnectException( @@ -3465,7 +3459,7 @@ def _attempt_token_refresh(self) -> bool: "Authenticate again with `%s`." % self._add_command() ) from exc except RSConnectException as exc: - self._warn_refresh_failure(exc) + logger.warning("Posit Connect Cloud token refresh failed (%s)." % type(exc).__name__) return False access_token = tokens.get("access_token") diff --git a/rsconnect/bundle.py b/rsconnect/bundle.py index ea5edde14..7ca82db8b 100644 --- a/rsconnect/bundle.py +++ b/rsconnect/bundle.py @@ -42,6 +42,8 @@ cast, ) +from . import metadata + # Even though TypedDict is available in Python 3.8, because it's used with NotRequired, # they should both come from the same typing module. # https://peps.python.org/pep-0655/#usage-in-python-3-11 @@ -79,10 +81,53 @@ "node_modules/", ] directories_to_ignore = {Path(d) for d in directories_ignore_list} +_config_dir_names = {".rsconnect-python"} mimetypes.add_type("text/ipynb", ".ipynb") +def _resolved_config_dir() -> Path: + return Path(metadata.config_dirname()).expanduser().resolve() + + +def _path_is_within(path: Path, directory: Path) -> bool: + return path == directory or directory in path.parents + + +def _is_config_path( + path: str | Path, + base_dir: str | Path | None = None, + config_dir: Path | None = None, +) -> bool: + candidate = Path(path) + if _config_dir_names.intersection(candidate.parts): + return True + if config_dir is None: + if base_dir is None and not candidate.is_absolute(): + return False + config_dir = _resolved_config_dir() + if base_dir is not None and not candidate.is_absolute(): + candidate = Path(base_dir) / candidate + return _path_is_within(candidate.resolve(), config_dir) + + +def _ensure_content_root_is_safe(path: str | Path, config_dir: Path) -> None: + content_root = Path(path) + if content_root.is_file(): + content_root = content_root.parent + content_root = content_root.resolve() + if _path_is_within(content_root, config_dir): + raise RSConnectException( + f"Cannot bundle content from '{content_root}' because it is inside the " + f"rsconnect-python configuration directory '{config_dir}'. " + "Choose a content directory outside the configuration directory." + ) + + +def _filter_config_paths(paths: Sequence[str], base_dir: str | Path, config_dir: Path) -> list[str]: + return [path for path in paths if not _is_config_path(path, base_dir, config_dir)] + + class ManifestDataFile(TypedDict): checksum: str @@ -623,6 +668,11 @@ def make_notebook_source_bundle( extra_files = [] base_dir = dirname(file) nb_name = basename(file) + config_dir = _resolved_config_dir() + _ensure_content_root_is_safe(base_dir, config_dir) + if _is_config_path(file, config_dir=config_dir): + raise RSConnectException("A credential configuration file cannot be bundled as notebook content.") + extra_files = _filter_config_paths(extra_files, base_dir, config_dir) manifest = make_source_manifest( AppModes.JUPYTER_NOTEBOOK, @@ -740,6 +790,11 @@ def make_notebook_html_bundle( hide_tagged_input: bool, check_output: Callable[..., bytes] = subprocess.check_output, ) -> typing.IO[bytes]: + config_dir = _resolved_config_dir() + _ensure_content_root_is_safe(dirname(filename), config_dir) + if _is_config_path(filename, config_dir=config_dir): + raise RSConnectException("A credential configuration file cannot be bundled as notebook content.") + # noinspection SpellCheckingInspection cmd = [ python, @@ -780,7 +835,12 @@ def make_notebook_html_bundle( return bundle_file -def keep_manifest_specified_file(relative_path: str, ignore_path_set: set[Path] = directories_to_ignore) -> bool: +def keep_manifest_specified_file( + relative_path: str, + ignore_path_set: set[Path] = directories_to_ignore, + base_dir: str | Path | None = None, + config_dir: Path | None = None, +) -> bool: """ A helper to see if the relative path given, which is assumed to have come from a manifest.json file, should be kept or ignored. @@ -788,6 +848,8 @@ def keep_manifest_specified_file(relative_path: str, ignore_path_set: set[Path] :param relative_path: the relative path name to check. :return: True, if the path should kept or False, if it should be ignored. """ + if _is_config_path(relative_path, base_dir, config_dir): + return False p = Path(relative_path) for parent in p.parents: if parent in ignore_path_set: @@ -950,7 +1012,16 @@ def make_manifest_bundle(manifest_path: str | Path) -> typing.IO[bytes]: manifest, raw_manifest = read_manifest_file(manifest_path) base_dir = dirname(manifest_path) - files = list(filter(keep_manifest_specified_file, manifest.get("files", {}).keys())) + config_dir = _resolved_config_dir() + _ensure_content_root_is_safe(base_dir, config_dir) + manifest_files = manifest.get("files", {}) + config_files = {path for path in manifest_files if _is_config_path(path, base_dir, config_dir)} + files = [ + path for path in manifest_files if keep_manifest_specified_file(path, base_dir=base_dir, config_dir=config_dir) + ] + if config_files: + manifest["files"] = {path: value for path, value in manifest_files.items() if path not in config_files} + raw_manifest = json.dumps(manifest, indent=2) if "manifest.json" in files: # this will be created @@ -1230,6 +1301,48 @@ def make_tensorflow_bundle( return bundle.to_file(directory) +def _should_skip_walk_directory( + cur_dir: str, + sub_dirs: list[str], + exclude_paths: set[Path], + config_dir: Path, +) -> bool: + sub_dirs[:] = [ + sub_dir for sub_dir in sub_dirs if not _is_config_path(join(cur_dir, sub_dir), config_dir=config_dir) + ] + current_dir = Path(cur_dir) + return current_dir in exclude_paths or any(parent in exclude_paths for parent in current_dir.parents) + + +def _iter_directory_files( + path: str, + extra_files: Sequence[str], + excludes: Sequence[str], + use_abspath: bool, + config_dir: Path, +) -> Iterator[str]: + glob_set = create_glob_set(path, excludes) + exclude_paths = {Path(p) for p in excludes} + for cur_dir, sub_dirs, files in os.walk(path): + if _should_skip_walk_directory(cur_dir, sub_dirs, exclude_paths, config_dir): + continue + for file in files: + cur_path = os.path.join(cur_dir, file) + rel_path = relpath(cur_path, path) + if Path(cur_path) in exclude_paths: + continue + if not keep_manifest_specified_file( + rel_path, + exclude_paths | directories_to_ignore, + base_dir=path, + config_dir=config_dir, + ): + continue + if rel_path not in extra_files and glob_set.matches(cur_path): + continue + yield abspath(cur_path) if use_abspath else rel_path + + def create_file_list( path: str, extra_files: Sequence[str], @@ -1247,32 +1360,19 @@ def create_file_list( :return: the list of relevant files, relative to the given directory. """ extra_files = extra_files or [] - excludes = excludes if excludes else [] - glob_set = create_glob_set(path, excludes) - exclude_paths = {Path(p) for p in excludes} - file_set: set[str] = set(extra_files) + excludes = excludes or [] + config_dir = _resolved_config_dir() + content_root = Path(path).parent if isfile(path) else Path(path) + _ensure_content_root_is_safe(content_root, config_dir) + file_set: set[str] = set(_filter_config_paths(extra_files, content_root, config_dir)) if isfile(path): - path_to_add = abspath(path) if use_abspath else path - file_set.add(path_to_add) + if not _is_config_path(path, config_dir=config_dir): + path_to_add = abspath(path) if use_abspath else path + file_set.add(path_to_add) return sorted(file_set) - for cur_dir, _, files in os.walk(path): - if Path(cur_dir) in exclude_paths: - continue - if any(parent in exclude_paths for parent in Path(cur_dir).parents): - continue - for file in files: - cur_path = os.path.join(cur_dir, file) - rel_path = relpath(cur_path, path) - - if Path(cur_path) in exclude_paths: - continue - if keep_manifest_specified_file(rel_path, exclude_paths | directories_to_ignore) and ( - rel_path in extra_files or not glob_set.matches(cur_path) - ): - path_to_add = abspath(cur_path) if use_abspath else rel_path - file_set.add(path_to_add) + file_set.update(_iter_directory_files(path, extra_files, excludes, use_abspath, config_dir)) return sorted(file_set) @@ -1712,7 +1812,11 @@ def make_quarto_manifest( # Standalone Quarto document base_dir = dirname(file_or_directory) file_name = basename(file_or_directory) - relevant_files = [file_name] + list(extra_files or []) + config_dir = _resolved_config_dir() + _ensure_content_root_is_safe(base_dir, config_dir) + if _is_config_path(file_or_directory, config_dir=config_dir): + raise RSConnectException("A credential configuration file cannot be bundled as Quarto content.") + relevant_files = [file_name] + _filter_config_paths(extra_files or [], base_dir, config_dir) manifest = make_source_manifest( app_mode, diff --git a/rsconnect/connect_cloud.py b/rsconnect/connect_cloud.py index dd58dafd2..1872b90e4 100644 --- a/rsconnect/connect_cloud.py +++ b/rsconnect/connect_cloud.py @@ -242,8 +242,6 @@ def refresh( request_timeout: Optional[float] = None, request_deadline: Optional[float] = None, client_id_override: Optional[str] = None, - *, - suppress_response_logging: bool = False, ) -> dict[str, Any]: """Mint a new access token from a refresh token.""" env = environment or environment_name() @@ -252,8 +250,6 @@ def refresh( request_options["request_timeout"] = request_timeout if request_deadline is not None: request_options["request_deadline"] = request_deadline - if suppress_response_logging: - request_options["suppress_response_logging"] = True return refresh_access_token( metadata=urls(env).oauth_metadata(), client_id=client_id_override if client_id_override is not None else client_id(env), diff --git a/rsconnect/device_login.py b/rsconnect/device_login.py index bdb732be0..6d842fe82 100644 --- a/rsconnect/device_login.py +++ b/rsconnect/device_login.py @@ -634,7 +634,6 @@ def _post_form( ca_data, request_timeout, request_deadline, - suppress_response_logging=True, ) @@ -662,13 +661,12 @@ def _register_device_client( ca_data, request_timeout=_remaining(deadline), request_deadline=deadline, - suppress_response_logging=True, ) + return _identifier(client_id, "OAuth client ID") except InvalidClientError: raise except RSConnectException: raise RSConnectException("OAuth client registration failed.") from None - return _identifier(client_id, "OAuth client ID") def _start_device_request( @@ -823,12 +821,21 @@ def _start_connect_login( ca_data, request_timeout=_remaining(deadline), request_deadline=deadline, - suppress_response_logging=True, ), server, ) if not client_id: - client_id = _register_device_client(metadata, server, insecure, ca_data, deadline) + client_id = _identifier( + register_client( + metadata, + server, + insecure, + ca_data, + request_timeout=_remaining(deadline), + request_deadline=deadline, + ), + "OAuth client ID", + ) client_id, response = _start_device_request(metadata, server, client_id, None, insecure, ca_data, deadline) _remaining(deadline) state = _make_state( @@ -1084,10 +1091,9 @@ def _lookup_cloud_account( raise InvalidGrantError() from exc if deadline <= time.monotonic(): raise _FinishDeadline() from exc - if isinstance(exc, RSConnectException): - status_detail = " (HTTP %s)" % exc.status if exc.status is not None else "" + if isinstance(exc, RSConnectException) and exc.status is not None: safe_error = RSConnectException( - "Posit Connect Cloud account lookup failed%s." % status_detail, + "Posit Connect Cloud account lookup failed (HTTP %s)." % exc.status, cause=exc.cause, status=exc.status, ) diff --git a/rsconnect/http_support.py b/rsconnect/http_support.py index c379584b4..ba8f8ec78 100644 --- a/rsconnect/http_support.py +++ b/rsconnect/http_support.py @@ -15,7 +15,7 @@ from http import client as http from http.cookies import SimpleCookie from typing import IO, Any, Dict, List, Mapping, Optional, Tuple, Union, cast -from urllib.parse import urlencode, urljoin, urlparse +from urllib.parse import unquote_plus, urlencode, urljoin, urlparse from warnings import warn from . import VERSION @@ -51,6 +51,7 @@ "x-auth-token", "x-auth-signature", "x-rsc-authorization", + "location", } _SENSITIVE_FIELDS = ( "client_secret", @@ -72,6 +73,8 @@ "subject_token", # PKCE (RFC 7636). "code_verifier", + "error_description", + "user_code", # The bootstrap response body carries a freshly minted admin API key. "api_key", "id_token", @@ -82,6 +85,7 @@ # redaction: in JSON bodies a bare "code" key is an error code (Connect, # shinyapps.io), which the debug log must keep readable. _SENSITIVE_FORM_ONLY_FIELDS = _SENSITIVE_FIELDS + ("code",) +_SENSITIVE_QUERY_FIELDS = frozenset(_SENSITIVE_FORM_ONLY_FIELDS) _SENSITIVE_FORM_FIELD = re.compile(r"\b(%s)=[^&\s'\"]*" % "|".join(_SENSITIVE_FORM_ONLY_FIELDS), re.IGNORECASE) _SENSITIVE_JSON_FIELD = re.compile(r'"(%s)"\s*:\s*"[^"]*"' % "|".join(_SENSITIVE_FIELDS), re.IGNORECASE) @@ -96,8 +100,26 @@ def _redacted_header_for_log(key: str, value: str) -> str: def _redacted_uri_for_log(uri: str) -> str: - """Redact credential-bearing query parameters, e.g. a presigned upload URL's.""" - return _SENSITIVE_FORM_FIELD.sub(r"\1=", uri) + """Redact credential fields in query strings and OAuth-style fragments.""" + uri = _SENSITIVE_FORM_FIELD.sub(r"\1=", uri) + before_fragment, fragment_marker, fragment = uri.partition("#") + suffix = fragment_marker + _redacted_query_for_log(fragment) if fragment_marker else "" + path, query_marker, query = before_fragment.partition("?") + if not query_marker: + return before_fragment + suffix + return path + query_marker + _redacted_query_for_log(query) + suffix + + +def _redacted_query_for_log(query: str) -> str: + """Redact sensitive query values while preserving parameter spelling and order.""" + parameters: list[str] = [] + for parameter in query.split("&"): + name, separator, _ = parameter.partition("=") + if separator and unquote_plus(name).casefold() in _SENSITIVE_QUERY_FIELDS: + parameters.append(name + "=") + else: + parameters.append(parameter) + return "&".join(parameters) def _redact_json_value(value: JsonData) -> JsonData: @@ -121,8 +143,8 @@ def _redacted_body_for_log(body: object) -> object: Only affects what is logged; the body itself is sent untouched. Streams and other non-text bodies are logged as their repr, which carries no content. JSON bodies are parsed and redacted structurally, since a secret containing - an escaped quote would leak past a regex; everything else falls back to the - form-encoded pattern. + an escaped quote would leak past a regex. Malformed JSON is logged as a + placeholder instead of falling back to patterns that may miss escaped data. """ if isinstance(body, bytes): text = body.decode("utf-8", errors="replace") @@ -136,7 +158,7 @@ def _redacted_body_for_log(body: object) -> object: try: return json.dumps(_redact_json_value(json.loads(text))) except (json.JSONDecodeError, ValueError): - pass + return "" text = _SENSITIVE_FORM_FIELD.sub(r"\1=", text) return _SENSITIVE_JSON_FIELD.sub(r'"\1": ""', text) @@ -639,8 +661,7 @@ def _log_request( ) -> None: if not logger.is_debugging(): return - private_uri = suppress_uri_logging or self._suppress_oauth_response_logging - logged_uri = "" if private_uri else _redacted_uri_for_log(full_uri) + logged_uri = "" if suppress_uri_logging else _redacted_uri_for_log(full_uri) logger.debug(f"Request: {method} {logged_uri}") logger.debug("Headers:") for key, value in headers.items(): @@ -651,10 +672,7 @@ def _log_request( def _log_response(self, response: http.HTTPResponse, response_body: str | bytes) -> None: if not logger.is_debugging(): return - if self._suppress_oauth_response_logging: - logger.debug(f"Response: {response.status}") - else: - logger.debug(f"Response: {response.status} {response.reason}") + logger.debug(f"Response: {response.status}") logger.debug("Headers:") if self._suppress_oauth_response_logging: logger.debug("--> ") @@ -737,10 +755,7 @@ def _do_request( socket.gaierror, socket.timeout, ) as exception: - if self._suppress_oauth_response_logging: - logger.debug("An exception occurred processing the HTTP request (%s)." % type(exception).__name__) - else: - logger.debug("An exception occurred processing the HTTP request.", exc_info=True) + logger.debug("An exception occurred processing the HTTP request (%s)." % type(exception).__name__) return HTTPResponse(full_uri, exception=exception) def _follow_redirect( @@ -789,19 +804,12 @@ def _tweak_response(self, response: HTTPResponse) -> JsonData | HTTPResponse: return response def _handle_set_cookie(self, response: http.HTTPResponse): - if self._suppress_oauth_response_logging: - self._cookies.store_cookies(response, suppress_logs=True) - else: - self._cookies.store_cookies(response) + self._cookies.store_cookies(response) self._inject_cookies() def _inject_cookies(self): if len(self._cookies) > 0: - self._headers["Cookie"] = ( - self._cookies.get_cookie_header_value(suppress_logs=True) - if self._suppress_oauth_response_logging - else self._cookies.get_cookie_header_value() - ) + self._headers["Cookie"] = self._cookies.get_cookie_header_value() elif "Cookie" in self._headers: del self._headers["Cookie"] @@ -879,7 +887,7 @@ def __init__(self) -> None: self._content: dict[str, str] = {} self._reference = SimpleCookie() - def store_cookies(self, response: http.HTTPResponse, *, suppress_logs: bool = False): + def store_cookies(self, response: http.HTTPResponse): headers = filter(lambda h: h[0].lower() == "set-cookie", response.getheaders()) for header in headers: @@ -888,17 +896,12 @@ def store_cookies(self, response: http.HTTPResponse, *, suppress_logs: bool = Fa if morsel.key not in self._keys: self._keys.append(morsel.key) self._content[morsel.key] = morsel.value - # Cookies are session credentials; names only, like the header log. - if not suppress_logs: - logger.debug(f"--> Set cookie {morsel.key}: ") - if not suppress_logs: - logger.debug(f"CookieJar contents: {self._keys}") + logger.debug("CookieJar contains %d cookie(s).", len(self._keys)) - def get_cookie_header_value(self, *, suppress_logs: bool = False): + def get_cookie_header_value(self): result = "; ".join([f"{key}={self._reference.value_encode(self._content[key])[1]}" for key in self._keys]) - if not suppress_logs: - logger.debug(f"Cookie: {'; '.join(f'{key}=' for key in self._keys)}") + logger.debug("Sending Cookie header with %d cookie(s).", len(self._keys)) return result def as_dict(self): diff --git a/rsconnect/metadata.py b/rsconnect/metadata.py index 0905229fa..0b6506e66 100644 --- a/rsconnect/metadata.py +++ b/rsconnect/metadata.py @@ -12,6 +12,8 @@ import shutil import stat import sys +import tempfile +from contextlib import suppress from datetime import datetime, timezone from io import BufferedWriter from os.path import abspath, basename, dirname, exists, join @@ -55,6 +57,8 @@ # App deployment history is small; strict reads cap it at 1 MiB. _MAX_METADATA_BYTES = 1024 * 1024 +_DEFAULT_OPEN = open + def resolve_server_alias(url: str) -> str: """Translate a --server value into the URL a saved server is stored under. @@ -104,6 +108,13 @@ def makedirs(filepath: str): pass +def _chmod_private_file(descriptor: int, path: str) -> None: + if hasattr(os, "fchmod"): + os.fchmod(descriptor, 0o600) + else: + os.chmod(path, 0o600) + + def _normalize_server_url(server_url: str): url = urlparse(server_url) return url.netloc.replace(".", "_").replace(":", "_") @@ -291,13 +302,34 @@ def get_path(self): # noinspection PyShadowingBuiltins def save_to(self, path: str, data: bytes, open: Callable[..., BufferedWriter] = open): """ - Save our data to the specified file. + Save our data to the specified file. Protected stores use an atomic write + with the default opener; supplied openers keep the direct destination-path + write behavior. """ - with open(path, "wb") as f: - f.write(data) + if self._chmod and open is _DEFAULT_OPEN: + descriptor, temporary_path = tempfile.mkstemp(prefix=".%s." % basename(path), dir=dirname(path) or ".") + try: + with os.fdopen(descriptor, "wb") as stream: + descriptor = -1 + _chmod_private_file(stream.fileno(), temporary_path) + stream.write(data) + stream.flush() + os.fsync(stream.fileno()) + os.replace(temporary_path, path) + temporary_path = "" + finally: + with suppress(OSError): + os.close(descriptor) + with suppress(OSError): + os.unlink(temporary_path) + else: + with open(path, "wb") as f: + f.write(data) + if self._chmod: + os.chmod(path, 0o600) self._real_path = path - def _already_holds(self, path: str, data: bytes) -> bool: + def _already_holds(self, path: str, data: bytes, opener: Callable[..., BufferedWriter] = open) -> bool: """ Whether this store was loaded from `path` and that file already holds `data`. @@ -306,6 +338,9 @@ def _already_holds(self, path: str, data: bytes) -> bool: """ if self._real_path != path or not exists(path): return False + # Default protected writes must replace symlinks even when their target matches. + if self._chmod and opener is _DEFAULT_OPEN and os.path.islink(path): + return False try: with open(path, "rb") as f: return f.read() == data @@ -323,18 +358,22 @@ def save(self, open: Callable[..., BufferedWriter] = open): A save that would not change the file is skipped. """ data = json.dumps(self._data, indent=4).encode("utf-8") - if not self._already_holds(self._primary_path, data): - try: - makedirs(self._primary_path) - self.save_to(self._primary_path, data, open) - except OSError: - if not self._secondary_path: - raise - makedirs(self._secondary_path) - self.save_to(self._secondary_path, data, open) - - if self._chmod and self._real_path is not None: - os.chmod(self._real_path, 0o600) + if self._already_holds(self._primary_path, data, open): + if self._chmod and open is _DEFAULT_OPEN: + metadata = os.lstat(self._primary_path) + # Avoid chmod following a symlink to an unrelated target. + if stat.S_ISREG(metadata.st_mode) and stat.S_IMODE(metadata.st_mode) != 0o600: + os.chmod(self._primary_path, 0o600) + return + + try: + makedirs(self._primary_path) + self.save_to(self._primary_path, data, open) + except OSError: + if not self._secondary_path: + raise + makedirs(self._secondary_path) + self.save_to(self._secondary_path, data, open) class ServerDataDict(TypedDict): diff --git a/rsconnect/oauth.py b/rsconnect/oauth.py index 0c40043e1..4623edc29 100644 --- a/rsconnect/oauth.py +++ b/rsconnect/oauth.py @@ -20,7 +20,7 @@ import click from .exception import RSConnectException -from .http_support import HTTPResponse, HTTPServer +from .http_support import HTTPResponse, HTTPServer, _redacted_uri_for_log from .log import logger # pyright: reportMissingTypeStubs=false @@ -29,6 +29,29 @@ # Shown to users on the server's OAuth consent screen. _CLIENT_NAME = "Posit CLI" _CALLBACK_TIMEOUT_SECONDS = 600 +_SAFE_OAUTH_ERROR_CODES = frozenset( + { + "access_denied", + "authorization_pending", + "expired_token", + "invalid_client", + "invalid_grant", + "invalid_request", + "invalid_scope", + "invalid_target", + "no_code", + "server_error", + "slow_down", + "temporarily_unavailable", + "unauthorized_client", + "unsupported_grant_type", + "unsupported_response_type", + } +) + + +def _safe_oauth_error_code(error: str) -> str: + return error if error in _SAFE_OAUTH_ERROR_CODES else "unknown_error" class InvalidClientError(RSConnectException): @@ -48,8 +71,7 @@ class InvalidGrantError(RSConnectException): def __init__(self, description: Optional[str] = None) -> None: self.description = description - detail = f": {description}" if description else "." - super().__init__(f"The OAuth grant is invalid, expired, or has been revoked{detail}") + super().__init__("The OAuth grant is invalid, expired, or has been revoked.") def _check_oauth_error_response(response: HTTPResponse) -> None: @@ -62,7 +84,7 @@ def _check_oauth_error_response(response: HTTPResponse) -> None: if error == "invalid_grant": raise InvalidGrantError(description or None) if description or error: - raise RSConnectException(f"OAuth error: {description or error}") + raise RSConnectException(f"OAuth error: {_safe_oauth_error_code(error)}") def _unwrap_json_response(response: Any) -> dict[str, Any]: @@ -107,12 +129,10 @@ def _post_oauth_form_request( ca_data: Optional[str | bytes] = None, request_timeout: Optional[float] = None, request_deadline: Optional[float] = None, - *, - suppress_response_logging: bool = False, ) -> Any: """POST a form and return the raw response so device polling can inspect pending errors.""" server = HTTPServer(base_url, disable_tls_check=insecure, ca_data=ca_data) - server._suppress_oauth_response_logging = suppress_response_logging + server._suppress_oauth_response_logging = True if request_timeout is not None: server.request_timeout = request_timeout if request_deadline is not None: @@ -132,8 +152,6 @@ def discover_oauth_metadata( ca_data: Optional[str | bytes] = None, request_timeout: Optional[float] = None, request_deadline: Optional[float] = None, - *, - suppress_response_logging: bool = False, ) -> dict[str, Any]: """Fetch OAuth 2.0 Authorization Server Metadata (RFC 8414). @@ -141,7 +159,7 @@ def discover_oauth_metadata( the server does not support OAuth. """ server = HTTPServer(url, disable_tls_check=insecure, ca_data=ca_data) - server._suppress_oauth_response_logging = suppress_response_logging + server._suppress_oauth_response_logging = True server.request_timeout = request_timeout server.request_deadline = request_deadline with server: @@ -171,8 +189,6 @@ def register_client( ca_data: Optional[str | bytes] = None, request_timeout: Optional[float] = None, request_deadline: Optional[float] = None, - *, - suppress_response_logging: bool = False, ) -> str: """Register an OAuth client via Dynamic Client Registration (RFC 7591). @@ -191,7 +207,7 @@ def register_client( grant_types.append("urn:ietf:params:oauth:grant-type:device_code") server = HTTPServer(base, disable_tls_check=insecure, ca_data=ca_data) - server._suppress_oauth_response_logging = suppress_response_logging + server._suppress_oauth_response_logging = True server.request_timeout = request_timeout server.request_deadline = request_deadline with server: @@ -286,7 +302,7 @@ def do_GET(self) -> None: # noqa: N802 self.result_queue.put(("error", "no_code", "No authorization code in callback")) def log_message(self, format: str, *args: object) -> None: - logger.debug(f"OAuth callback server: {format % args}") + logger.debug("OAuth callback server handled a request.") def login_with_browser( @@ -347,7 +363,7 @@ def login_with_browser( result = result_queue.get_nowait() if result[0] == "error": - raise RSConnectException(f"OAuth authentication failed: {result[1]} — {result[2]}") + raise RSConnectException(f"OAuth authentication failed: {_safe_oauth_error_code(result[1] or '')}") _, code, returned_state = result if returned_state != state: @@ -467,8 +483,7 @@ def _poll_for_device_token( elif error == "access_denied": raise RSConnectException("Authorization was denied by the user.") elif error: - description = str(json_data.get("error_description", error)) - raise RSConnectException(f"Device code flow failed: {description}") + raise RSConnectException(f"Device code flow failed: {_safe_oauth_error_code(error)}") else: raise RSConnectException("Device code token request returned an unexpected response.") @@ -484,8 +499,6 @@ def refresh_access_token( scope: Optional[str] = None, request_timeout: Optional[float] = None, request_deadline: Optional[float] = None, - *, - suppress_response_logging: bool = False, ) -> dict[str, Any]: """Refresh an OAuth access token using a refresh token. @@ -506,8 +519,6 @@ def refresh_access_token( request_options["request_timeout"] = request_timeout if request_deadline is not None: request_options["request_deadline"] = request_deadline - if suppress_response_logging: - request_options["suppress_response_logging"] = True data = _post_token_request(str(metadata["token_endpoint"]), params, insecure, ca_data, **request_options) if "access_token" not in data: raise RSConnectException("Token refresh returned an unexpected response.") @@ -551,14 +562,11 @@ def _post_token_request( ca_data: Optional[str | bytes] = None, request_timeout: Optional[float] = None, request_deadline: Optional[float] = None, - *, - suppress_response_logging: bool = False, ) -> dict[str, Any]: """POST a form-encoded request to an OAuth token endpoint and return the JSON body.""" parsed = urlparse(token_endpoint) base = f"{parsed.scheme}://{parsed.netloc}" - logging_options = {"suppress_response_logging": True} if suppress_response_logging else {} response = _post_oauth_form_request( base, parsed.path, @@ -567,7 +575,6 @@ def _post_token_request( ca_data, request_timeout, request_deadline, - **logging_options, ) return _unwrap_json_response(response) @@ -624,6 +631,7 @@ def exchange_token_for_api_key( ).encode("utf-8") server = HTTPServer(base, disable_tls_check=insecure, ca_data=ca_data) + server._suppress_oauth_response_logging = True with server: response = server.request( "POST", @@ -636,7 +644,10 @@ def exchange_token_for_api_key( raise RSConnectException("Unexpected response from the OIDC token exchange.") if response.exception: - raise RSConnectException("Could not connect to %s - %s" % (url, response.exception), cause=response.exception) + raise RSConnectException( + "Could not connect to %s (%s)" % (_redacted_uri_for_log(url), type(response.exception).__name__), + cause=response.exception, + ) status = response.status data = response.json_data if isinstance(response.json_data, dict) else {} @@ -659,24 +670,21 @@ def _token_exchange_error(status: Optional[int], data: dict[str, Any]) -> RSConn lowered = description.lower() if "ambiguous" in lowered: return RSConnectException( - f"The identity token matched more than one service principal on Connect ({description}). " + "The identity token matched more than one service principal on Connect. " "Resolve the duplicate access grants on the server, or authenticate with an API key." ) if "verif" in lowered: return RSConnectException( - f"Connect could not verify the identity token ({description}). " + "Connect could not verify the identity token. " "Check the server clock and the OIDC issuer configuration, or authenticate with an API key." ) return RSConnectException( - f"Connect did not grant access for this identity token ({description or 'no match'}). " + "Connect did not grant access for this identity token. " "Confirm access has been configured for the target content and that the token's " "audience matches it, or authenticate with an API key." ) - detail = error - if description: - detail = f"{error}: {description}" if error else description - suffix = f" ({detail})" if detail else "" + suffix = f" ({_safe_oauth_error_code(error)})" if error else "" return RSConnectException(f"OIDC token exchange failed (HTTP {status}){suffix}.") diff --git a/tests/test_agent_cli_e2e.py b/tests/test_agent_cli_e2e.py index 7aa8add58..f24b56954 100644 --- a/tests/test_agent_cli_e2e.py +++ b/tests/test_agent_cli_e2e.py @@ -8,6 +8,7 @@ import stat import subprocess import sys +import tarfile import threading import time from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer @@ -147,18 +148,24 @@ def do_GET(self) -> None: self._json(200, {"api_enabled": True, "installations": self.server.python_installations}) # type: ignore[attr-defined] elif path == "/__api__/v1/server_settings/nodejs": self._json(200, self.server.nodejs_settings) # type: ignore[attr-defined] - elif path.startswith("/__api__/v1/content/"): - content_id = path.rsplit("/", 1)[-1] - status = self.server.content_statuses.get(content_id) # type: ignore[attr-defined] - if status is not None and status != 200: - self._json(status, {"error": "content lookup failed"}) - else: - self._json(200, {"py_version": "3.12.8", "node_version": "22.22.2"}) + elif path.startswith("/__api__/v1/content"): + self._connect_content_response(path) elif path == "/v1/accounts": self._cloud_accounts(record) else: self._json(404, {"error": "not found"}) + def _connect_content_response(self, path: str) -> None: + if path == "/__api__/v1/content": + self._json(200, []) + return + content_id = path.rsplit("/", 1)[-1] + status = self.server.content_statuses.get(content_id) # type: ignore[attr-defined] + if status is not None and status != 200: + self._json(status, {"error": "content lookup failed"}) + else: + self._json(200, {"py_version": "3.12.8", "node_version": "22.22.2"}) + def _cloud_accounts(self, record: dict[str, object]) -> None: with self.server.lock: # type: ignore[attr-defined] status = self.server.account_statuses.pop(0) if self.server.account_statuses else 200 # type: ignore[attr-defined] @@ -1609,6 +1616,121 @@ def test_cloud_finish_refresh_uses_the_client_id_saved_at_start( assert not _device_states(home, "cloud") +@pytest.mark.parametrize("explicit_extra", [False, True]) +def test_pending_login_inside_project_is_excluded_from_manifest_and_bundle( + tmp_path: Path, local_http_server: _LocalHTTPServer, explicit_extra: bool +) -> None: + project = tmp_path / "project-home" + environment = _cli_environment(project) + environment.pop("XDG_CONFIG_HOME", None) + (project / "app.py").write_text("app = object()\n", encoding="utf-8") + (project / "requirements.txt").write_text("", encoding="utf-8") + (project / "assets").mkdir() + (project / "assets" / "keep.txt").write_text("keep this content\n", encoding="utf-8") + started = _run_cli(["login", local_http_server.base_url, "--name", "inside-project", "--no-wait"], environment) + assert started.returncode == 0, _output(started) + state = _device_states(project, "connect")[0] + original_state = state.read_bytes() + arguments = ["write-manifest", "api", "--entrypoint", "app:app", "--exclude-renv", str(project)] + if explicit_extra: + arguments.append(str(state)) + + generated = _run_cli(arguments, environment) + + assert generated.returncode == 0, _output(generated) + manifest_path = project / "manifest.json" + manifest = json.loads(manifest_path.read_text(encoding="utf-8")) + assert {"app.py", "requirements.txt", "assets/keep.txt"} <= set(manifest["files"]) + assert state.relative_to(project).as_posix() not in manifest["files"] + assert state.read_bytes() == original_state + output = tmp_path / "content.tar.gz" + packaged = subprocess.run( + [ + sys.executable, + "-c", + "import shutil,sys; from rsconnect.bundle import make_manifest_bundle; " + "bundle=make_manifest_bundle(sys.argv[1]); " + "out=open(sys.argv[2],'wb'); shutil.copyfileobj(bundle,out); out.close(); bundle.close()", + str(manifest_path), + str(output), + ], + cwd=str(_REPO_ROOT), + env=environment, + capture_output=True, + text=True, + timeout=30, + ) + assert packaged.returncode == 0, _output(packaged) + with tarfile.open(output, "r:gz") as bundle: + assert {"app.py", "requirements.txt", "assets/keep.txt", "manifest.json"} <= set(bundle.getnames()) + assert state.relative_to(project).as_posix() not in bundle.getnames() + for member in bundle.getmembers(): + if member.isfile(): + file = bundle.extractfile(member) + assert file is not None + assert b"device-code-secret" not in file.read() + + +def test_static_notebook_in_config_is_rejected_before_execution_or_upload( + tmp_path: Path, local_http_server: _LocalHTTPServer +) -> None: + home = tmp_path / "home" + environment = _cli_environment(home) + started = _run_cli( + ["login", "--name", "static-guard", "--server", local_http_server.base_url, "--no-wait"], + environment, + ) + assert started.returncode == 0, _output(started) + state = _device_states(home, "connect")[0] + notebook = state.parent / "restricted.ipynb" + marker = tmp_path / "notebook-executed" + notebook.write_text( + json.dumps( + { + "nbformat": 4, + "nbformat_minor": 5, + "metadata": {}, + "cells": [ + { + "cell_type": "code", + "metadata": {}, + "execution_count": None, + "outputs": [], + "source": f"from pathlib import Path; Path({str(marker)!r}).write_text('executed')", + } + ], + } + ), + encoding="utf-8", + ) + (notebook.parent / "requirements.txt").write_text("", encoding="utf-8") + + deployed = _run_cli( + [ + "deploy", + "notebook", + "--server", + local_http_server.base_url, + "--api-key", + "synthetic-api-key", + "--static", + "--new", + "--exclude-renv", + str(notebook), + ], + environment, + ) + + assert deployed.returncode == 1, _output(deployed) + assert "configuration directory" in _output(deployed) + assert not marker.exists() + assert state.exists() + assert not any( + record["method"] == "POST" and str(record["path"]).startswith("/__api__/") + for record in local_http_server.requests + ) + + @pytest.mark.skipif(os.name != "posix", reason="POSIX FIFOs and symlinks are required") @pytest.mark.parametrize("runtime", ["python", "nodejs"]) @pytest.mark.parametrize("record_kind", ["module-symlink", "manifest-symlink", "module-fifo"]) diff --git a/tests/test_api.py b/tests/test_api.py index d435dddd4..5e3c343f5 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -54,6 +54,54 @@ def test_nodejs_settings_reports_permission_failure(self): with self.assertRaises(RSConnectException): client.nodejs_settings() + def test_http_failure_diagnostics_omit_untrusted_reason_and_exception_text(self): + from http.client import BadStatusLine + + from rsconnect.http_support import HTTPResponse + + server = RSConnectServer("https://connect.example.com", "api-key") + reason_secret = "reason-refresh-token" + response = HTTPResponse("https://connect.example.com/v1/user", body="") + response.status = 503 + response.reason = reason_secret + + with self.assertRaises(RSConnectException) as raised: + server.handle_bad_response(response, is_httpresponse=True) + + self.assertIn("503", raised.exception.message) + self.assertNotIn(reason_secret, raised.exception.message) + + exception_secret = "bad-status-refresh-token" + failure = BadStatusLine(exception_secret) + failed_response = HTTPResponse("https://connect.example.com/v1/user", exception=failure) + with self.assertRaises(RSConnectException) as raised: + server.handle_bad_response(failed_response, is_httpresponse=True) + + self.assertNotIn(exception_secret, raised.exception.message) + self.assertIs(raised.exception.cause, failure) + + def test_oauth_refresh_warning_omits_exception_text(self): + secret = "refresh-token-secret" + client = RSConnectClient( + RSConnectServer( + "https://connect.example.com", + None, + oauth_access_token="stale-access-token", + oauth_client_id="oauth-client", + ) + ) + + with patch("rsconnect.oauth.keyring_get_tokens", return_value=(None, "refresh-token")): + with patch("rsconnect.oauth.discover_oauth_metadata", return_value={}): + with patch("rsconnect.oauth.refresh_access_token", side_effect=RSConnectException(secret)): + with self.assertLogs("rsconnect", level="WARNING") as captured: + self.assertFalse(client._attempt_token_refresh()) + + log_text = "\n".join(captured.output) + self.assertIn("OAuth token refresh failed", log_text) + self.assertIn("RSConnectException", log_text) + self.assertNotIn(secret, log_text) + def test_executor_init(self): connect_server = require_connect() api_key = require_api_key() @@ -198,8 +246,9 @@ def test_verify_api_key_connection_error(self): with patch.object(RSConnectClient, "get", return_value=failed_response): with self.assertRaises(RSConnectException) as cm: ce.verify_api_key() - self.assertEqual(str(cm.exception), "Could not connect to http://test-server/ - connection refused") + self.assertEqual(str(cm.exception), "Could not connect to http://test-server/ (OSError)") self.assertIs(cm.exception.cause, failed_response.exception) + self.assertNotIn("connection refused", str(cm.exception)) # The deprecated module-level verify_api_key() is reached via actions.test_api_key() # during `rsconnect add`, so it must accept the same credentials as the executor path. @@ -553,8 +602,8 @@ def test_exchange_token_error_status(self, mock_fmt_payload, mock_token_endpoint mock_response = Mock() mock_response.status = 401 mock_response.exception = None - mock_response.full_uri = "https://example.snowflakecomputing.com/oauth/token" - mock_response.reason = "Unauthorized" + mock_response.full_uri = "https://example.snowflakecomputing.com/oauth/token?refresh_token=uri-secret" + mock_response.reason = "reason-secret" mock_server_instance.request.return_value = mock_response # Mock the token endpoint and payload @@ -566,13 +615,11 @@ def test_exchange_token_error_status(self, mock_fmt_payload, mock_token_endpoint } # Call the method and verify it raises the expected exception - with pytest.raises(RSConnectException) as raised: + with pytest.raises(RSConnectException, match="Failed to exchange Snowflake token") as raised: server.exchange_token() - self.assertEqual( - raised.value.message, - "Failed to exchange Snowflake token: Received an unexpected response from " - "https://spcs.example.com (calling https://example.snowflakecomputing.com/oauth/token): 401 Unauthorized", - ) + self.assertIs(mock_server_instance._suppress_oauth_response_logging, True) + self.assertNotIn("reason-secret", raised.value.message) + self.assertNotIn("uri-secret", raised.value.message) @patch("rsconnect.api.HTTPServer") @patch("rsconnect.api.SPCSConnectServer.token_endpoint") diff --git a/tests/test_bundle.py b/tests/test_bundle.py index 15d915480..50ce6f813 100644 --- a/tests/test_bundle.py +++ b/tests/test_bundle.py @@ -782,9 +782,13 @@ def test_keep_manifest_specified_file(self): self.assertFalse(keep_manifest_specified_file("rsconnect/bogus.file")) self.assertFalse(keep_manifest_specified_file("rsconnect-python")) self.assertFalse(keep_manifest_specified_file("rsconnect-python/bogus.file")) + self.assertFalse(keep_manifest_specified_file(".rsconnect-python/servers.json")) + self.assertTrue(keep_manifest_specified_file("nested/rsconnect-python/app.json")) + self.assertFalse(keep_manifest_specified_file("nested/.rsconnect-python/pending.json")) self.assertFalse(keep_manifest_specified_file(".svn/bogus.file")) # noinspection SpellCheckingInspection self.assertFalse(keep_manifest_specified_file(".Rproj.user/bogus.file")) + self.assertTrue(keep_manifest_specified_file("nested/node_modules/local-data.js")) def test_manifest_bundle(self): # noinspection SpellCheckingInspection @@ -3245,6 +3249,329 @@ def _make_node_env(**overrides): return NodeEnvironment(**defaults) +def _make_test_environment(): + return Environment.from_dict( + { + "contents": "flask==3.0.0\n", + "filename": "requirements.txt", + "locale": "en_US.UTF-8", + "package_manager": "pip", + "pip": "23.0.1", + "python": "3.12.0", + "source": "file", + } + ) + + +def _bundle_snapshot(bundle_file): + with tarfile.open(mode="r:gz", fileobj=bundle_file) as tar: + members = {name: tar.extractfile(name).read() for name in tar.getnames()} + return members, json.loads(members["manifest.json"].decode("utf-8")) + + +def test_node_bundle_excludes_config_paths_and_explicit_extras(tmp_path, monkeypatch): + project = tmp_path / "project" + project.mkdir() + config_dir = project / "nested" / "custom-state" + config_dir.mkdir(parents=True) + monkeypatch.setattr(rsconnect.bundle.metadata, "config_dirname", lambda: str(config_dir)) + + (project / "app.js").write_text("// app") + (project / "package.json").write_text("{}") + (project / "data.csv").write_text("safe") + (project / "nested" / "keep.txt").write_text("safe") + nested_node_module = project / "feature" / "node_modules" / "kept.js" + nested_node_module.parent.mkdir(parents=True) + nested_node_module.write_text("safe") + + secret_files = [ + project / ".rsconnect-python" / "servers.json", + project / "nested" / ".rsconnect-python" / "pending.json", + config_dir / "servers.json", + config_dir / "pending.json", + ] + for secret_file in secret_files: + secret_file.parent.mkdir(parents=True, exist_ok=True) + secret_file.write_text("SECRET") + project_metadata = project / "nested" / "rsconnect-python" / "application.json" + project_metadata.parent.mkdir(parents=True) + project_metadata.write_text("project metadata") + + extras = [ + "data.csv", + ".rsconnect-python/servers.json", + "nested/.rsconnect-python/pending.json", + "nested/rsconnect-python/application.json", + "nested/custom-state/servers.json", + ] + try: + (project / "config-alias").symlink_to(config_dir, target_is_directory=True) + except (NotImplementedError, OSError): + pass + else: + extras.append("config-alias/pending.json") + + bundle_file = make_nodejs_bundle(str(project), "app.js", _make_node_env(), extras, []) + members, manifest = _bundle_snapshot(bundle_file) + names = set(members) + + expected = { + "manifest.json", + "app.js", + "package.json", + "data.csv", + "nested/keep.txt", + "nested/rsconnect-python/application.json", + "feature/node_modules/kept.js", + } + assert names == expected + assert set(manifest["files"]) == expected - {"manifest.json"} + assert b"SECRET" not in b"".join(members.values()) + + +def test_manifest_bundle_removes_config_entries_only(tmp_path, monkeypatch): + project = tmp_path / "project" + project.mkdir() + config_dir = project / "state" / "custom-store" + config_dir.mkdir(parents=True) + monkeypatch.setattr(rsconnect.bundle.metadata, "config_dirname", lambda: str(config_dir)) + + (project / "app.py").write_text("# app") + nested_node_module = project / "feature" / "node_modules" / "kept.js" + nested_node_module.parent.mkdir(parents=True) + nested_node_module.write_text("safe") + ignored_file = project / "packrat" / "packrat.lock" + ignored_file.parent.mkdir() + ignored_file.write_text("ignored by existing rules") + + secret_paths = [ + ".rsconnect-python/servers.json", + "nested/.rsconnect-python/pending.json", + "state/custom-store/servers.json", + ] + for relative_path in secret_paths: + secret_file = project / relative_path + secret_file.parent.mkdir(parents=True, exist_ok=True) + secret_file.write_text("SECRET") + project_metadata = project / "nested" / "rsconnect-python" / "application.json" + project_metadata.parent.mkdir(parents=True) + project_metadata.write_text("project metadata") + + files = { + "app.py": {"checksum": "app"}, + "nested/rsconnect-python/application.json": {"checksum": "project-metadata"}, + "feature/node_modules/kept.js": {"checksum": "nested-node-module"}, + "packrat/packrat.lock": {"checksum": "existing-ignore"}, + **{path: {"checksum": "secret"} for path in secret_paths}, + } + try: + (project / "config-alias").symlink_to(config_dir, target_is_directory=True) + except (NotImplementedError, OSError): + pass + else: + files["config-alias/servers.json"] = {"checksum": "secret-alias"} + + manifest_path = project / "manifest.json" + manifest_path.write_text(json.dumps({"version": 1, "metadata": {"appmode": "python-api"}, "files": files})) + with make_manifest_bundle(manifest_path) as bundle_file: + members, manifest = _bundle_snapshot(bundle_file) + names = set(members) + + assert names == { + "manifest.json", + "app.py", + "nested/rsconnect-python/application.json", + "feature/node_modules/kept.js", + } + assert set(manifest["files"]) == { + "app.py", + "nested/rsconnect-python/application.json", + "feature/node_modules/kept.js", + "packrat/packrat.lock", + } + assert not any(".rsconnect-python" in name for name in manifest["files"]) + assert "state/custom-store/servers.json" not in manifest["files"] + assert "config-alias/servers.json" not in manifest["files"] + assert b"SECRET" not in b"".join(members.values()) + + +def test_notebook_source_bundle_filters_config_extras(tmp_path, monkeypatch): + project = tmp_path / "project" + project.mkdir() + config_dir = project / "nested" / "custom-state" + config_dir.mkdir(parents=True) + monkeypatch.setattr(rsconnect.bundle.metadata, "config_dirname", lambda: str(config_dir)) + + notebook = project / "notebook.ipynb" + notebook.write_text("{}") + (project / "data.csv").write_text("safe") + (config_dir / "servers.json").write_text("SECRET") + (project / ".rsconnect-python").mkdir() + (project / ".rsconnect-python" / "pending.json").write_text("SECRET") + extras = ["data.csv", "nested/custom-state/servers.json", ".rsconnect-python/pending.json"] + try: + (project / "config-alias").symlink_to(config_dir, target_is_directory=True) + except (NotImplementedError, OSError): + pass + else: + extras.append("config-alias/servers.json") + + environment = _make_test_environment() + with make_notebook_source_bundle( + str(notebook), + environment, + extras, + hide_all_input=False, + hide_tagged_input=False, + ) as bundle_file: + members, manifest = _bundle_snapshot(bundle_file) + names = set(members) + + assert names == {"manifest.json", "notebook.ipynb", "requirements.txt", "data.csv"} + assert set(manifest["files"]) == names - {"manifest.json"} + assert b"SECRET" not in b"".join(members.values()) + + +@pytest.mark.parametrize("path_kind", ["active_config", "reserved_path", "symlink_alias"]) +def test_notebook_html_bundle_rejects_config_paths_before_conversion(tmp_path, monkeypatch, path_kind): + project = tmp_path / "project" + project.mkdir() + if path_kind == "active_config": + config_dir = project / "custom-state" + notebook = config_dir / "private.ipynb" + error_match = "inside the rsconnect-python configuration directory" + elif path_kind == "reserved_path": + config_dir = tmp_path / "user-config" + notebook = project / ".rsconnect-python" / "private.ipynb" + error_match = "credential configuration file" + else: + config_dir = tmp_path / "user-config" + notebook = project / "private.ipynb" + error_match = "credential configuration file" + + config_dir.mkdir(parents=True) + monkeypatch.setattr(rsconnect.bundle.metadata, "config_dirname", lambda: str(config_dir)) + if path_kind == "symlink_alias": + target = config_dir / "private.ipynb" + target.write_text("{}") + try: + notebook.symlink_to(target) + except (NotImplementedError, OSError): + pytest.skip("symlinks are unavailable") + else: + notebook.parent.mkdir(parents=True, exist_ok=True) + notebook.write_text("{}") + + callback_calls = [] + + def check_output(command): + callback_calls.append(command) + return b"Python 3.12" + + with pytest.raises(RSConnectException, match=error_match): + make_notebook_html_bundle( + str(notebook), + sys.executable, + hide_all_input=False, + hide_tagged_input=True, + check_output=check_output, + ) + assert callback_calls == [] + + +def test_standalone_quarto_bundle_filters_config_extras(tmp_path, monkeypatch): + project = tmp_path / "project" + project.mkdir() + config_dir = project / "nested" / "custom-state" + config_dir.mkdir(parents=True) + monkeypatch.setattr(rsconnect.bundle.metadata, "config_dirname", lambda: str(config_dir)) + + document = project / "report.qmd" + document.write_text("# report") + (project / "data.csv").write_text("safe") + (config_dir / "servers.json").write_text("SECRET") + extras = ["data.csv", "nested/custom-state/servers.json"] + try: + (project / "config-alias").symlink_to(config_dir, target_is_directory=True) + except (NotImplementedError, OSError): + pass + else: + extras.append("config-alias/servers.json") + + inspect = {"quarto": {"version": "1.4.0"}, "engines": [], "files": {"input": [str(document)]}} + with make_quarto_source_bundle( + str(document), + inspect, + AppModes.STATIC_QUARTO, + None, + extras, + [], + ) as bundle_file: + members, manifest = _bundle_snapshot(bundle_file) + names = set(members) + + assert names == {"manifest.json", "report.qmd", "data.csv"} + assert set(manifest["files"]) == names - {"manifest.json"} + assert b"SECRET" not in b"".join(members.values()) + + +def test_bundle_rejects_config_directory_as_content_root(tmp_path, monkeypatch): + config_dir = tmp_path / "custom-config" + config_dir.mkdir() + monkeypatch.setattr(rsconnect.bundle.metadata, "config_dirname", lambda: str(config_dir)) + content_root = tmp_path / "config-alias" + try: + content_root.symlink_to(config_dir, target_is_directory=True) + except (NotImplementedError, OSError): + content_root = config_dir + + with pytest.raises(RSConnectException, match="inside the rsconnect-python configuration directory"): + make_nodejs_bundle(str(content_root), "app.js", _make_node_env(), [], []) + + +def test_write_api_manifest_excludes_current_home_config_extra(tmp_path, monkeypatch): + project = tmp_path / "project" + project.mkdir() + monkeypatch.setenv("HOME", str(project)) + monkeypatch.delenv("XDG_CONFIG_HOME", raising=False) + monkeypatch.delenv("APPDATA", raising=False) + + config_dir = Path(rsconnect.bundle.metadata.config_dirname()).resolve() + config_dir.mkdir(parents=True) + pending_state = config_dir / "pending.json" + pending_state.write_text("SECRET") + (project / "app.py").write_text("app = object()") + (project / "data.csv").write_text("safe") + (project / "requirements.txt").write_text("flask==3.0.0\n") + extra_files = [str(pending_state), str(project / "data.csv")] + environment = _make_test_environment() + + assert write_api_manifest_json( + str(project), + "app:app", + environment, + AppModes.PYTHON_FASTAPI, + extra_files, + [], + ) + manifest = json.loads((project / "manifest.json").read_text()) + with make_manifest_bundle(project / "manifest.json") as bundle_file: + members, bundled_manifest = _bundle_snapshot(bundle_file) + names = set(members) + + assert "app.py" in manifest["files"] + assert "data.csv" in manifest["files"] + assert all("pending.json" not in path for path in manifest["files"]) + assert names == {"manifest.json", "app.py", "data.csv", "requirements.txt"} + assert set(bundled_manifest["files"]) == names - {"manifest.json"} + assert "app.py" in bundled_manifest["files"] + assert "data.csv" in bundled_manifest["files"] + assert all("pending.json" not in path for path in bundled_manifest["files"]) + for path in ("app.py", "data.csv", "requirements.txt"): + assert bundled_manifest["files"][path]["checksum"] == file_checksum(project / path) + assert b"SECRET" not in b"".join(members.values()) + + class TestNodeJSManifest: def test_manifest_structure(self): env = _make_node_env() diff --git a/tests/test_connect_cloud.py b/tests/test_connect_cloud.py index 2585f7de0..a6de0cdaf 100644 --- a/tests/test_connect_cloud.py +++ b/tests/test_connect_cloud.py @@ -219,12 +219,6 @@ def test_refresh_can_limit_its_request_timeout(self): connect_cloud.refresh("rt", "production", request_timeout=0.25) self.assertEqual(refresh.call_args.kwargs["request_timeout"], 0.25) - def test_refresh_can_suppress_response_logging(self): - with mock.patch("rsconnect.connect_cloud.refresh_access_token") as refresh: - refresh.return_value = {"access_token": "new"} - connect_cloud.refresh("rt", "production", suppress_response_logging=True) - self.assertIs(refresh.call_args.kwargs["suppress_response_logging"], True) - def test_refresh_honors_saved_client_override_and_deadline(self): with mock.patch.dict( os.environ, @@ -833,21 +827,6 @@ def test_refresh_uses_the_remaining_finish_budget(self): ) self.assertEqual(server.access_token, "fresh") - def test_private_refresh_suppresses_logging_and_limits_warning_details(self): - server = ConnectCloudServer("acme", access_token="stale", refresh_token="rt") - client = ConnectCloudClient(server) - client._suppress_oauth_response_logging = True - failure = RSConnectException("refresh response contained private details") - - with mock.patch("rsconnect.connect_cloud.refresh", side_effect=failure) as refresh: - with self.assertLogs("rsconnect", level="WARNING") as captured: - self.assertFalse(client._attempt_token_refresh()) - - refresh.assert_called_once_with("rt", "production", suppress_response_logging=True) - log_text = "\n".join(captured.output) - self.assertIn("RSConnectException", log_text) - self.assertNotIn("refresh response contained private details", log_text) - def test_expired_finish_budget_does_not_refresh(self): server = ConnectCloudServer("acme", access_token="stale", refresh_token="rt") client = ConnectCloudClient(server) @@ -1062,7 +1041,8 @@ def test_a_transient_refresh_failure_leaves_the_original_401_and_warns(self): self.assertIn("401", exception.message) log_text = "\n".join(captured.output) self.assertIn("token refresh failed", log_text) - self.assertIn(secret, log_text) + self.assertIn("RSConnectException", log_text) + self.assertNotIn(secret, log_text) self.assertEqual(self._stored_entry()["connect_cloud_refresh_token"], "rt") self.assertEqual(len(httpretty.latest_requests()), 1) diff --git a/tests/test_device_login.py b/tests/test_device_login.py index ac065387f..5b308c57e 100644 --- a/tests/test_device_login.py +++ b/tests/test_device_login.py @@ -1072,7 +1072,7 @@ def test_bytes_ca_is_persisted_and_reused_during_finish(login_env: Any, monkeypa [RSConnectException("temporary account lookup failure"), {"id": "team-id"}], [], RSConnectException, - "account lookup failed", + "temporary account lookup failure", ), ( [ @@ -1260,34 +1260,9 @@ def test_cloud_finish_refresh_uses_client_id_saved_at_start(login_env: Any, monk request_timeout=10.0, request_deadline=20.0, client_id_override="started-client", - suppress_response_logging=True, ) -def test_registration_error_is_private_and_can_be_retried(login_env: Any): - login_env.http.responses.extend( - [ - _response(200, METADATA), - _response(503, {"error": "server_error", "error_description": "device-code-secret"}), - ] - ) - - with pytest.raises(RSConnectException, match="client registration failed") as failed: - device_login.start_connect_login(SERVER, "work") - - assert "device-code-secret" not in str(failed.value) - assert not _state_path("connect", "work").exists() - assert login_env.http.instances[-1]._suppress_oauth_response_logging is True - login_env.http.responses.extend( - [ - _response(200, METADATA), - _response(201, {"client_id": "registered-client"}), - _response(200, DEVICE_RESPONSE), - ] - ) - assert device_login.start_connect_login(SERVER, "work")["status"] == "pending" - - def test_missing_cloud_account_removes_checkpoint_for_corrected_start(login_env: Any, monkeypatch: pytest.MonkeyPatch): monkeypatch.setenv(connect_cloud.ENVIRONMENT_ENV_VAR, "production") _queue_device(login_env) diff --git a/tests/test_git_metadata.py b/tests/test_git_metadata.py index a4722a1e3..44ff4d306 100644 --- a/tests/test_git_metadata.py +++ b/tests/test_git_metadata.py @@ -297,4 +297,6 @@ def test_upload_bundle_with_metadata_handles_bad_response(self): with patch.object(RSConnectClient, "post", return_value=bad_response): with pytest.raises(RSConnectException) as cm: client.upload_bundle("guid", io.BytesIO(b"tarball"), metadata={"source": "git"}) - assert "connection refused" in str(cm.value) + assert str(cm.value) == "Could not connect to http://test-server (OSError)" + assert cm.value.cause is bad_response.exception + assert "connection refused" not in str(cm.value) diff --git a/tests/test_http_support.py b/tests/test_http_support.py index 4682c7b33..b5c01b886 100644 --- a/tests/test_http_support.py +++ b/tests/test_http_support.py @@ -355,29 +355,20 @@ def test_as_dict(self): ) def test_cookie_values_do_not_reach_the_debug_log(self): - # Cookies are session credentials; the jar logs names only. + # Cookie names can carry credentials too; diagnostics expose only the count. jar = CookieJar() + cookie_name = "echoed-refresh-token-name" + cookie_value = "echoed-refresh-token-value" with self.assertLogs("rsconnect", level="DEBUG") as captured: - jar.store_cookies(FakeSetCookieResponse(["session=s3ssionv4lue"])) + jar.store_cookies(FakeSetCookieResponse([f"{cookie_name}={cookie_value}"])) header = jar.get_cookie_header_value() - self.assertEqual(header, "session=s3ssionv4lue") - log_text = "\n".join(captured.output) - self.assertNotIn("s3ssionv4lue", log_text) - self.assertIn("session", log_text) - - def test_cookie_names_and_values_can_be_omitted_when_opted_in(self): - from unittest.mock import patch - - jar = CookieJar() - cookie_name = "oauth-refresh-cookie-name" - cookie_value = "oauth-refresh-cookie-value" - with patch("rsconnect.http_support.logger.debug") as debug: - jar.store_cookies(FakeSetCookieResponse([f"{cookie_name}={cookie_value}"]), suppress_logs=True) - header = jar.get_cookie_header_value(suppress_logs=True) - - debug.assert_not_called() self.assertEqual(header, f"{cookie_name}={cookie_value}") + self.assertEqual(jar.as_dict(), {"keys": [cookie_name], "content": {cookie_name: cookie_value}}) + log_text = "\n".join(captured.output) + self.assertNotIn(cookie_name, log_text) + self.assertNotIn(cookie_value, log_text) + self.assertIn("1 cookie(s)", log_text) class TestDebugLogRedaction(TestCase): @@ -459,6 +450,7 @@ def test_json_error_codes_stay_readable(self): body = '{"error": "An object with that name already exists.", "code": 26}' redacted = str(_redacted_body_for_log(body)) + self.assertIn('"error": "An object with that name already exists."', redacted) self.assertIn('"code": 26', redacted) def test_streams_are_left_alone(self): @@ -492,7 +484,72 @@ def test_cookie_values_with_spaces_leave_no_first_token(self): self.assertEqual(_redacted_header_for_log("Cookie", "session=abc; other=def"), "") - def test_oauth_redirect_destination_is_suppressed_when_opted_in(self): + def test_oauth_error_fields_and_redirect_location_are_redacted_in_http_logs(self): + from unittest.mock import Mock, patch + + location = ( + "/callback?refresh%5Ftoken=redirect-secret" + "#access%5Ftoken=fragment-secret&error_description=location-secret&tab=summary" + ) + body = '{"error":"invalid_grant","error_description":"description-secret","user_code":"code-secret"}' + + def make_response(status, response_body, headers, reason): + response = Mock() + response.status = status + response.reason = reason + response.read.return_value = response_body + response.getheaders.return_value = headers + header_values = {key.lower(): value for key, value in headers} + response.getheader.side_effect = lambda key, default=None: header_values.get(key.lower(), default) + return response + + redirect = make_response(302, b"", [("Location", location)], "reason-secret") + final = make_response( + 200, + body.encode(), + [("Content-Type", "application/json"), ("X-Debug-Context", "ordinary-debug-context")], + "reason-secret", + ) + + with HTTPServer("http://example.com") as server: + transport = cast(Any, server._conn) + with self.assertLogs("rsconnect", level="DEBUG") as captured: + with patch.object(transport, "request") as send: + with patch.object(transport, "getresponse", side_effect=[redirect, final]): + response = cast(HTTPResponse, server.get("/start")) + + log_text = "\n".join(captured.output) + for secret in ( + "redirect-secret", + "location-secret", + "description-secret", + "code-secret", + "reason-secret", + "fragment-secret", + ): + self.assertNotIn(secret, log_text) + self.assertIn("Content-Type: application/json", log_text) + self.assertIn("X-Debug-Context: ordinary-debug-context", log_text) + self.assertIn( + "Redirected to: http://example.com/callback?refresh%5Ftoken=" + "#access%5Ftoken=&error_description=&tab=summary", + log_text, + ) + self.assertIn('"error_description": ""', log_text) + self.assertIn('"user_code": ""', log_text) + self.assertEqual(send.call_args_list[1].args[1], location) + self.assertEqual(response.reason, "reason-secret") + self.assertEqual(response.response_body, body) + self.assertEqual( + response.json_data, + { + "error": "invalid_grant", + "error_description": "description-secret", + "user_code": "code-secret", + }, + ) + + def test_oauth_redirect_destination_is_suppressed_without_changing_routing(self): from unittest.mock import Mock, patch location = "http://example.com/opaque/path-refresh-token?resume=opaque-query-token" @@ -529,24 +586,17 @@ def make_response(status, body, headers, reason): self.assertEqual(response.reason, "final-reason-refresh-token") self.assertEqual(response.response_body, "{}") - def test_oauth_response_body_is_omitted_when_opted_in(self): + def test_oauth_response_body_suppression_keeps_response_data_unchanged(self): from unittest.mock import Mock, patch secret = "echoed-refresh-token" - cookie_name = "oauth-cookie-name" - cookie_value = "oauth-cookie-value" - endpoint = "/oauth/token?refresh_token=endpoint-secret" body = f'{{"error":"{secret}","error_description":"{secret}"}}' content_type = f"application/json; debug={secret}" reply = Mock() reply.status = 503 reply.reason = "reason-secret" reply.read.return_value = body.encode() - reply.getheaders.return_value = [ - ("Content-Type", content_type), - ("X-Debug-Context", secret), - ("Set-Cookie", f"{cookie_name}={cookie_value}"), - ] + reply.getheaders.return_value = [("Content-Type", content_type), ("X-Debug-Context", secret)] header_values = {key.lower(): value for key, value in reply.getheaders.return_value} reply.getheader.side_effect = lambda key, default=None: header_values.get(key.lower(), default) @@ -556,33 +606,38 @@ def test_oauth_response_body_is_omitted_when_opted_in(self): with self.assertLogs("rsconnect", level="DEBUG") as captured: with patch.object(transport, "request"): with patch.object(transport, "getresponse", return_value=reply): - response = cast(HTTPResponse, server.get(endpoint)) + response = cast(HTTPResponse, server.get("/oauth/token")) log_text = "\n".join(captured.output) self.assertIn("Response: 503", log_text) self.assertIn("", log_text) self.assertIn("", log_text) - for private_value in (secret, cookie_name, cookie_value, "endpoint-secret", "X-Debug-Context", "reason-secret"): - self.assertNotIn(private_value, log_text) + self.assertNotIn(secret, log_text) + self.assertNotIn("X-Debug-Context", log_text) + self.assertNotIn("reason-secret", log_text) self.assertEqual(response.reason, "reason-secret") self.assertEqual(response.content_type, content_type) self.assertEqual(response._response.getheader("Content-Type"), content_type) self.assertEqual(response._response.getheader("X-Debug-Context"), secret) - self.assertEqual(response._response.getheader("Set-Cookie"), f"{cookie_name}={cookie_value}") self.assertEqual(response.response_body, body) self.assertEqual(response.json_data, {"error": secret, "error_description": secret}) - def test_default_oauth_response_logging_keeps_baseline_diagnostics(self): + def test_malformed_json_is_logged_as_a_placeholder(self): from unittest.mock import Mock, patch - body = '{"message":"ordinary response"}' + from rsconnect.http_support import _redacted_body_for_log + + body = '{"error_description":"malformed-secret", "broken": "' + self.assertEqual(_redacted_body_for_log(body), "") + reply = Mock() - reply.status = 200 - reply.reason = "ordinary reason" + reply.status = 400 + reply.reason = "Bad Request" reply.read.return_value = body.encode() - reply.getheaders.return_value = [("Content-Type", "application/json"), ("X-Debug-Context", "ordinary context")] - header_values = {key.lower(): value for key, value in reply.getheaders.return_value} - reply.getheader.side_effect = lambda key, default=None: header_values.get(key.lower(), default) + reply.getheaders.return_value = [("Content-Type", "application/json")] + reply.getheader.side_effect = lambda key, default=None: ( + "application/json" if key.lower() == "content-type" else default + ) with HTTPServer("http://example.com") as server: transport = cast(Any, server._conn) @@ -592,82 +647,27 @@ def test_default_oauth_response_logging_keeps_baseline_diagnostics(self): response = cast(HTTPResponse, server.get("/token")) log_text = "\n".join(captured.output) - self.assertIn("Response: 200 ordinary reason", log_text) - self.assertIn("X-Debug-Context: ordinary context", log_text) - self.assertIn("ordinary response", log_text) - self.assertNotIn("", log_text) + self.assertNotIn("malformed-secret", log_text) self.assertEqual(response.response_body, body) + self.assertIsNone(response.json_data) - def test_default_redirect_logging_keeps_destination_visible(self): - from unittest.mock import Mock, patch - - location = "http://example.com/reports/current?tab=summary" - request_target = "/reports/current?tab=summary" - - def make_response(status, body, headers, reason): - response = Mock() - response.status = status - response.reason = reason - response.read.return_value = body - response.getheaders.return_value = headers - header_values = {key.lower(): value for key, value in headers} - response.getheader.side_effect = lambda key, default=None: header_values.get(key.lower(), default) - return response - - redirect = make_response(302, b"", [("Location", location)], "Found") - final = make_response( - 200, - b'{"message":"done"}', - [("Content-Type", "application/json")], - "OK", - ) - - with HTTPServer("http://example.com") as server: - transport = cast(Any, server._conn) - with self.assertLogs("rsconnect", level="DEBUG") as captured: - with patch.object(transport, "request") as send: - with patch.object(transport, "getresponse", side_effect=[redirect, final]): - response = cast(HTTPResponse, server.get("/start")) - - log_text = "\n".join(captured.output) - self.assertIn("Location: " + location, log_text) - self.assertIn("Redirected to: " + location, log_text) - self.assertEqual(send.call_args_list[1].args[1], request_target) - self.assertEqual(response.response_body, '{"message":"done"}') - - def test_default_http_failure_logging_keeps_exception_details(self): - from unittest.mock import patch - - failure = OSError("ordinary transport detail") - with HTTPServer("http://example.com") as server: - transport = cast(Any, server._conn) - with self.assertLogs("rsconnect", level="DEBUG") as captured: - with patch.object(transport, "request"): - with patch.object(transport, "getresponse", side_effect=failure): - response = cast(HTTPResponse, server.get("/ordinary")) - - log_text = "\n".join(captured.output) - self.assertIn("Traceback", log_text) - self.assertIn("ordinary transport detail", log_text) - self.assertIs(response.exception, failure) - - def test_http_failure_logging_omits_exception_details_when_opted_in(self): + def test_bad_status_line_text_is_not_logged_with_a_traceback(self): + from http.client import BadStatusLine from unittest.mock import patch - failure = OSError("oauth transport detail") + failure = BadStatusLine("refresh-token-in-status-line") with HTTPServer("http://example.com") as server: - server._suppress_oauth_response_logging = True transport = cast(Any, server._conn) with self.assertLogs("rsconnect", level="DEBUG") as captured: with patch.object(transport, "request"): with patch.object(transport, "getresponse", side_effect=failure): - response = cast(HTTPResponse, server.get("/oauth/token?access_token=endpoint-secret")) + response = cast(HTTPResponse, server.get("/token")) log_text = "\n".join(captured.output) - self.assertIn("OSError", log_text) - self.assertNotIn("oauth transport detail", log_text) - self.assertNotIn("endpoint-secret", log_text) + self.assertNotIn("refresh-token-in-status-line", log_text) self.assertNotIn("Traceback", log_text) + self.assertIn("BadStatusLine", log_text) self.assertIs(response.exception, failure) def test_a_connection_failure_response_has_a_none_status(self): @@ -701,6 +701,35 @@ def test_presigned_url_query_is_redacted(self): self.assertNotIn("AKIA", redacted) self.assertIn("X-Amz-Expires=300", redacted) + def test_encoded_query_names_are_redacted_without_rewriting_other_parameters(self): + from rsconnect.http_support import _redacted_uri_for_log + + uri = "/path?keep=%2f+value&refresh%5Ftoken=echoed-secret&X-Amz%2dSignature=signature-secret&tail=a+b#section" + self.assertEqual( + _redacted_uri_for_log(uri), + "/path?keep=%2f+value&refresh%5Ftoken=&X-Amz%2dSignature=&tail=a+b#section", + ) + + def test_encoded_fragment_fields_are_redacted_with_and_without_a_query(self): + from rsconnect.http_support import _redacted_uri_for_log + + for prefix in ("/callback", "/callback?keep=%2f+value"): + with self.subTest(prefix=prefix): + self.assertEqual( + _redacted_uri_for_log(prefix + "#refresh%5Ftoken=fragment-secret&tab=a+b"), + prefix + "#refresh%5Ftoken=&tab=a+b", + ) + self.assertEqual(_redacted_uri_for_log(prefix + "#ordinary-section"), prefix + "#ordinary-section") + + def test_queryless_uri_text_and_fragment_credentials_remain_redacted(self): + from rsconnect.http_support import _redacted_uri_for_log + + self.assertEqual(_redacted_uri_for_log("account token=plain-secret"), "account token=") + self.assertEqual( + _redacted_uri_for_log("/callback#refresh_token=fragment-secret"), + "/callback#refresh_token=", + ) + def test_azure_sas_sig_param_is_redacted(self): # Azure-style presigned URLs carry the signature in a bare "sig" param. from rsconnect.http_support import _redacted_uri_for_log diff --git a/tests/test_metadata.py b/tests/test_metadata.py index 4cc9f952e..a08a2143b 100644 --- a/tests/test_metadata.py +++ b/tests/test_metadata.py @@ -1,6 +1,7 @@ import json import os import shutil +import stat import subprocess import sys import tempfile @@ -13,6 +14,7 @@ from rsconnect.metadata import ( AppStore, ContentBuildStore, + DataStore, ServerStore, _normalize_server_url, resolve_server_alias, @@ -259,11 +261,240 @@ def test_remove_default_clears_it(self): self.assertIsNone(self.server_store.get_default()) def test_save_skips_rewrite_when_file_unchanged(self): + with patch( + "rsconnect.metadata.tempfile.mkstemp", + side_effect=AssertionError("created a temporary file for unchanged data"), + ): + self.server_store.save() + def fail_open(path_to_open, mode, *args, **kw): self.fail("rewrote %s when nothing had changed" % path_to_open) self.server_store.save(fail_open) + def test_private_store_creates_mode_600_file_under_umask_022(self): + if os.name != "posix": + self.skipTest("POSIX file permissions are unavailable") + + base_dir = join(self.tempDir, "umask") + path = join(base_dir, "servers.json") + previous_umask = os.umask(0o022) + try: + store = ServerStore(base_dir=base_dir) + store.set("private", "https://connect.example.test", api_key="new-secret") + finally: + os.umask(previous_umask) + + self.assertEqual(stat.S_IMODE(os.stat(path).st_mode), 0o600) + self.assertEqual(store.get_path(), path) + + def test_private_store_keeps_new_secret_off_existing_file_until_replace(self): + if os.name != "posix": + self.skipTest("POSIX file permissions are unavailable") + + path = self.server_store_path + os.chmod(path, 0o644) + with open(path, "rb") as metadata_file: + previous_data = metadata_file.read() + new_secret = b"rotated-secret" + self.server_store._data["foo"]["api_key"] = new_secret.decode("ascii") + fdopen = os.fdopen + fchmod = os.fchmod + replace = os.replace + + def check_private_fd(descriptor, mode): + self.assertEqual(stat.S_IMODE(os.fstat(descriptor).st_mode), 0o600) + return fdopen(descriptor, mode) + + def check_fchmod(descriptor, mode): + self.assertEqual(mode, 0o600) + self.assertEqual(stat.S_IMODE(os.fstat(descriptor).st_mode), 0o600) + fchmod(descriptor, mode) + + def check_atomic_replace(source, destination): + self.assertEqual(destination, path) + self.assertEqual(stat.S_IMODE(os.stat(source).st_mode), 0o600) + with open(source, "rb") as temporary_file: + self.assertIn(new_secret, temporary_file.read()) + with open(destination, "rb") as existing_file: + self.assertEqual(existing_file.read(), previous_data) + replace(source, destination) + + with patch("rsconnect.metadata.os.fdopen", side_effect=check_private_fd) as fdopen_mock, patch( + "rsconnect.metadata.os.fchmod", side_effect=check_fchmod + ) as fchmod_mock, patch("rsconnect.metadata.os.replace", side_effect=check_atomic_replace) as replace_mock: + self.server_store.save() + + fdopen_mock.assert_called_once() + fchmod_mock.assert_called_once() + replace_mock.assert_called_once() + self.assertEqual(fchmod_mock.call_args[0][0], fdopen_mock.call_args[0][0]) + self.assertEqual(stat.S_IMODE(os.stat(path).st_mode), 0o600) + with open(path, "rb") as metadata_file: + self.assertIn(new_secret, metadata_file.read()) + + def test_fdopen_failure_closes_descriptor_removes_temp_and_preserves_file(self): + path = join(self.tempDir, "existing.json") + with open(path, "wb") as metadata_file: + metadata_file.write(b"old credentials") + previous_mode = stat.S_IMODE(os.stat(path).st_mode) + store = DataStore(path, chmod=True, autoload=False) + store._data = {"server": {"api_key": "new-secret"}} + files_before = set(os.listdir(self.tempDir)) + mkstemp = tempfile.mkstemp + temporary = {} + + def record_temp(*args, **kwargs): + descriptor, temporary_path = mkstemp(*args, **kwargs) + temporary.update(descriptor=descriptor, path=temporary_path) + return descriptor, temporary_path + + with patch("rsconnect.metadata.tempfile.mkstemp", side_effect=record_temp), patch( + "rsconnect.metadata.os.fdopen", side_effect=OSError("fdopen failed") + ): + with self.assertRaisesRegex(OSError, "fdopen failed"): + store.save() + + with self.assertRaises(OSError): + os.fstat(temporary["descriptor"]) + self.assertFalse(os.path.exists(temporary["path"])) + self.assertEqual(set(os.listdir(self.tempDir)), files_before) + with open(path, "rb") as metadata_file: + self.assertEqual(metadata_file.read(), b"old credentials") + self.assertEqual(stat.S_IMODE(os.stat(path).st_mode), previous_mode) + self.assertIsNone(store._real_path) + + def test_unchanged_private_store_repairs_mode_without_rewriting(self): + if os.name != "posix": + self.skipTest("POSIX file permissions are unavailable") + + path = self.server_store_path + os.chmod(path, 0o644) + before = os.stat(path) + with open(path, "rb") as metadata_file: + previous_data = metadata_file.read() + + with patch( + "rsconnect.metadata.tempfile.mkstemp", + side_effect=AssertionError("rewrote unchanged credentials"), + ): + self.server_store.save() + + after = os.stat(path) + self.assertEqual(stat.S_IMODE(after.st_mode), 0o600) + self.assertEqual(after.st_ino, before.st_ino) + with open(path, "rb") as metadata_file: + self.assertEqual(metadata_file.read(), previous_data) + + def test_private_store_failure_preserves_file_and_real_path(self): + path = join(self.tempDir, "existing.json") + with open(path, "wb") as metadata_file: + metadata_file.write(b"old credentials") + previous_mode = stat.S_IMODE(os.stat(path).st_mode) + store = DataStore(path, chmod=True, autoload=False) + store._data = {"server": {"api_key": "new-secret"}} + files_before = set(os.listdir(self.tempDir)) + + with patch("rsconnect.metadata.os.fsync", side_effect=OSError("sync failed")): + with self.assertRaisesRegex(OSError, "sync failed"): + store.save() + + with open(path, "rb") as metadata_file: + self.assertEqual(metadata_file.read(), b"old credentials") + self.assertEqual(stat.S_IMODE(os.stat(path).st_mode), previous_mode) + self.assertIsNone(store._real_path) + self.assertEqual(set(os.listdir(self.tempDir)), files_before) + + def test_private_store_falls_back_after_atomic_replace_failure(self): + primary = join(self.tempDir, "local", "servers.json") + secondary = join(self.tempDir, "config", "servers.json") + store = DataStore(primary, secondary, chmod=True, autoload=False) + store._data = {"server": {"api_key": "fallback-secret"}} + replace = os.replace + + def fail_primary(source, destination): + if destination == primary: + raise OSError("primary unavailable") + replace(source, destination) + + with patch("rsconnect.metadata.os.replace", side_effect=fail_primary): + store.save() + + self.assertEqual(store.get_path(), secondary) + self.assertFalse(exists(primary)) + self.assertEqual(set(os.listdir(os.path.dirname(primary))), set()) + with open(secondary, "r", encoding="utf-8") as metadata_file: + self.assertEqual(json.load(metadata_file), store._data) + + def test_private_store_replaces_symlink_without_writing_through(self): + if os.name != "posix": + self.skipTest("POSIX symlinks are unavailable") + + path = self.server_store_path + target = join(self.tempDir, "servers-target.json") + os.replace(path, target) + os.chmod(target, 0o644) + with open(target, "rb") as metadata_file: + previous_data = metadata_file.read() + try: + os.symlink(target, path) + except OSError: + self.skipTest("symlinks are unavailable") + + def fail_open(path_to_open, mode, *args, **kw): + self.fail("custom opener was called for unchanged symlink data") + + self.server_store.save(fail_open) + self.assertTrue(os.path.islink(path)) + with open(target, "rb") as metadata_file: + self.assertEqual(metadata_file.read(), previous_data) + self.assertEqual(stat.S_IMODE(os.stat(target).st_mode), 0o644) + + self.server_store.save() + + self.assertFalse(os.path.islink(path)) + self.assertEqual(stat.S_IMODE(os.stat(path).st_mode), 0o600) + with open(path, "rb") as metadata_file: + self.assertEqual(metadata_file.read(), previous_data) + with open(target, "rb") as metadata_file: + self.assertEqual(metadata_file.read(), previous_data) + self.assertEqual(stat.S_IMODE(os.stat(target).st_mode), 0o644) + + self.server_store._data["foo"]["api_key"] = "symlink-secret" + self.server_store.save() + + self.assertFalse(os.path.islink(path)) + with open(target, "rb") as metadata_file: + self.assertEqual(metadata_file.read(), previous_data) + self.assertEqual(stat.S_IMODE(os.stat(target).st_mode), 0o644) + self.assertEqual(stat.S_IMODE(os.stat(path).st_mode), 0o600) + + def test_private_store_does_not_update_other_hardlinks(self): + if os.name != "posix": + self.skipTest("POSIX hard links are unavailable") + + path = self.server_store_path + alias = join(self.tempDir, "servers-alias.json") + try: + os.link(path, alias) + except OSError: + self.skipTest("hard links are unavailable") + os.chmod(path, 0o644) + with open(path, "rb") as metadata_file: + previous_data = metadata_file.read() + self.server_store.save() + self.assertEqual(stat.S_IMODE(os.stat(alias).st_mode), 0o600) + + self.server_store._data["foo"]["api_key"] = "hardlink-secret" + self.server_store.save() + + with open(alias, "rb") as metadata_file: + alias_data = metadata_file.read() + self.assertEqual(alias_data, previous_data) + self.assertNotIn(b"hardlink-secret", alias_data) + self.assertEqual(stat.S_IMODE(os.stat(alias).st_mode), 0o600) + self.assertEqual(stat.S_IMODE(os.stat(path).st_mode), 0o600) + def test_save_rewrites_when_data_changed(self): writes = [] diff --git a/tests/test_oauth.py b/tests/test_oauth.py index 7f7991637..ebd8a1f9a 100644 --- a/tests/test_oauth.py +++ b/tests/test_oauth.py @@ -13,6 +13,7 @@ from .utils import failing_keyring from rsconnect.oauth import ( + _CallbackHandler, InvalidClientError, InvalidGrantError, _exchange_code_for_token, @@ -73,6 +74,7 @@ def test_success(self, mock_http_server: MagicMock): mock_http_server.get.return_value = _make_response(200, FAKE_METADATA) result = discover_oauth_metadata(FAKE_URL) assert result == FAKE_METADATA + assert mock_http_server._suppress_oauth_response_logging is True def test_client_specific_request_timeout_and_deadline(self, mock_http_server: MagicMock): mock_http_server.get.return_value = _make_response(200, FAKE_METADATA) @@ -80,11 +82,6 @@ def test_client_specific_request_timeout_and_deadline(self, mock_http_server: Ma assert mock_http_server.request_timeout == 0.25 assert mock_http_server.request_deadline == 12.5 - def test_response_logging_can_be_suppressed(self, mock_http_server: MagicMock): - mock_http_server.get.return_value = _make_response(200, FAKE_METADATA) - assert discover_oauth_metadata(FAKE_URL, suppress_response_logging=True) == FAKE_METADATA - assert mock_http_server._suppress_oauth_response_logging is True - def test_server_not_supporting_oauth(self, mock_http_server: MagicMock): mock_http_server.get.return_value = _make_response(404, None) with pytest.raises(RSConnectException, match="does not support OAuth"): @@ -101,6 +98,7 @@ def test_success(self, mock_http_server: MagicMock): mock_http_server.post.return_value = _make_response(200, {"client_id": "test-client-123"}) result = register_client(FAKE_METADATA, FAKE_URL) assert result == "test-client-123" + assert mock_http_server._suppress_oauth_response_logging is True def test_registers_human_readable_client_name(self, mock_http_server: MagicMock): mock_http_server.post.return_value = _make_response(200, {"client_id": "test-client-123"}) @@ -113,18 +111,24 @@ def test_client_specific_request_timeout_and_deadline(self, mock_http_server: Ma assert mock_http_server.request_timeout == 0.25 assert mock_http_server.request_deadline == 12.5 - def test_response_logging_can_be_suppressed(self, mock_http_server: MagicMock): - mock_http_server.post.return_value = _make_response(200, {"client_id": "test-client-123"}) - assert register_client(FAKE_METADATA, FAKE_URL, suppress_response_logging=True) == "test-client-123" - assert mock_http_server._suppress_oauth_response_logging is True - def test_failure(self, mock_http_server: MagicMock): mock_http_server.post.return_value = _make_response( - 400, {"error": "invalid_request", "error_description": "bad request"} + 400, {"error": "invalid_request", "error_description": "refresh-token-secret"} ) with pytest.raises(RSConnectException, match="OAuth error") as raised: register_client(FAKE_METADATA, FAKE_URL) - assert "bad request" in str(raised.value) + assert "invalid_request" in str(raised.value) + assert "refresh-token-secret" not in str(raised.value) + + def test_unknown_oauth_error_code_is_not_echoed(self, mock_http_server: MagicMock): + secret = "refresh-token-secret" + mock_http_server.post.return_value = _make_response(400, {"error": secret, "error_description": secret}) + + with pytest.raises(RSConnectException) as raised: + register_client(FAKE_METADATA, FAKE_URL) + + assert "unknown_error" in str(raised.value) + assert secret not in str(raised.value) def test_client_id_response_keeps_legacy_stringification(self, mock_http_server: MagicMock): mock_http_server.post.return_value = _make_response(200, {"client_id": "bad\nclient"}) @@ -166,6 +170,7 @@ def test_success(self, mock_http_server: MagicMock): mock_http_server.request.return_value = _make_response(200, {"access_token": "minted-key"}) result = exchange_token_for_api_key(FAKE_URL, "oidc-token") assert result == "minted-key" + assert mock_http_server._suppress_oauth_response_logging is True # RFC 8693 token-exchange request shape. body = mock_http_server.request.call_args.kwargs["body"] assert b"grant_type=urn%3Aietf%3Aparams%3Aoauth%3Agrant-type%3Atoken-exchange" in body @@ -234,18 +239,47 @@ def test_ambiguous_match(self, mock_http_server: MagicMock): exchange_token_for_api_key(FAKE_URL, "oidc-token") def test_verification_failure(self, mock_http_server: MagicMock): + secret = "subject-token-secret" self._set_metadata(mock_http_server) mock_http_server.request.return_value = _make_response( - 400, {"error": "invalid_grant", "error_description": "could not verify token signature"} + 400, {"error": "invalid_grant", "error_description": f"could not verify token signature {secret}"} ) - with pytest.raises(RSConnectException, match="could not verify the identity token"): + with pytest.raises(RSConnectException, match="could not verify the identity token") as raised: exchange_token_for_api_key(FAKE_URL, "oidc-token") + assert secret not in str(raised.value) def test_generic_failure(self, mock_http_server: MagicMock): self._set_metadata(mock_http_server) mock_http_server.request.return_value = _make_response(500, {"error": "boom", "error_description": "kaboom"}) - with pytest.raises(RSConnectException, match="HTTP 500"): + with pytest.raises(RSConnectException, match="HTTP 500") as raised: exchange_token_for_api_key(FAKE_URL, "oidc-token") + assert "boom" not in str(raised.value) + assert "kaboom" not in str(raised.value) + + +def test_browser_callback_error_does_not_expose_description(): + secret = "callback-token-secret" + with patch("rsconnect.oauth._HTTPServer") as server_factory: + server = server_factory.return_value + server.server_address = ("127.0.0.1", 0) + server.RequestHandlerClass = _CallbackHandler + server.handle_request.side_effect = lambda: _CallbackHandler.result_queue.put( + ("error", "invalid_request", secret) + ) + + with patch("rsconnect.oauth.webbrowser.open", return_value=True): + with pytest.raises(RSConnectException) as raised: + login_with_browser(FAKE_URL, "client-1", FAKE_METADATA) + + assert "invalid_request" in str(raised.value) + assert secret not in str(raised.value) + + +def test_callback_server_log_does_not_include_request_line(): + request = "GET /callback?code=authorization-secret&state=state-secret HTTP/1.1" + with patch("rsconnect.oauth.logger.debug") as debug: + _CallbackHandler.log_message(MagicMock(), "%s", request) + debug.assert_called_once_with("OAuth callback server handled a request.") class TestDeviceCodeFlow: @@ -335,6 +369,19 @@ def test_poll_malformed_response(self, _, mock_http_server: MagicMock): with pytest.raises(RSConnectException, match="unexpected response"): _poll_for_device_token(FAKE_METADATA, "client-1", "device-code-1", 5, 600) + @patch("rsconnect.oauth.time.sleep") + def test_poll_error_uses_code_without_description(self, _, mock_http_server: MagicMock): + secret = "refresh-token-secret" + mock_http_server.request.return_value = _make_response( + 400, {"error": "server_error", "error_description": secret} + ) + + with pytest.raises(RSConnectException) as raised: + _poll_for_device_token(FAKE_METADATA, "client-1", "device-code-1", 5, 600) + + assert "server_error" in str(raised.value) + assert secret not in str(raised.value) + @pytest.mark.parametrize( ("response", "message"), [ @@ -366,19 +413,6 @@ def test_client_specific_request_timeout_and_deadline(self, mock_http_server: Ma ) assert mock_http_server.request_timeout == 0.25 assert mock_http_server.request_deadline == 12.5 - - def test_response_logging_can_be_suppressed(self, mock_http_server: MagicMock): - mock_http_server.request.return_value = _make_response(200, {"access_token": "new-at"}) - refresh_access_token( - FAKE_METADATA, - "client-1", - "old-rt", - request_timeout=0.25, - request_deadline=12.5, - suppress_response_logging=True, - ) - assert mock_http_server.request_timeout == 0.25 - assert mock_http_server.request_deadline == 12.5 assert mock_http_server._suppress_oauth_response_logging is True def test_refresh_keeps_legacy_path_only_for_token_query(self, mock_http_server: MagicMock): @@ -402,12 +436,14 @@ def test_invalid_client(self, mock_http_server: MagicMock): refresh_access_token(FAKE_METADATA, "bad-client", "old-rt") def test_invalid_grant(self, mock_http_server: MagicMock): + secret = "refresh-token-secret" mock_http_server.request.return_value = _make_response( - 400, {"error": "invalid_grant", "error_description": "refresh token expired"} + 400, {"error": "invalid_grant", "error_description": secret} ) - with pytest.raises(InvalidGrantError, match="refresh token expired") as raised: + with pytest.raises(InvalidGrantError) as raised: refresh_access_token(FAKE_METADATA, "client-1", "old-rt") - assert raised.value.description == "refresh token expired" + assert secret not in str(raised.value) + assert raised.value.description == secret def test_invalid_grant_without_a_description(self, mock_http_server: MagicMock): mock_http_server.request.return_value = _make_response(400, {"error": "invalid_grant"}) From 79526cc78c2f0e3e402901792125cc2995245d35 Mon Sep 17 00:00:00 2001 From: Matt Conflitti Date: Thu, 15 Oct 2026 10:05:00 -0400 Subject: [PATCH 2/5] fix(security): preserve credential compatibility Keep credential writes private without replacing symlinks or hardlink aliases, or requiring a writable parent directory. Retain logging keywords and scoped OAuth diagnostics. Preserve registration deadline errors and cover compatibility regressions. Refs #860 --- docs/CHANGELOG.md | 10 +- rsconnect/api.py | 2 + rsconnect/connect_cloud.py | 4 + rsconnect/device_login.py | 25 ++-- rsconnect/http_support.py | 24 ++-- rsconnect/metadata.py | 28 ++--- rsconnect/oauth.py | 19 ++- tests/test_connect_cloud.py | 9 ++ tests/test_device_login.py | 40 ++++++- tests/test_http_support.py | 37 ++++++ tests/test_metadata.py | 223 +++++++++++++++++++++++------------- tests/test_oauth.py | 42 +++++++ 12 files changed, 338 insertions(+), 125 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 7bb6de8df..9f6212af0 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -9,11 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - OAuth clients now register with Posit Connect as "Posit CLI", the name shown on the consent screen. Previously registered clients keep their existing name. -- Credential files written through the default file backend now use private - temporary files and atomic replacement, requiring write access to the parent - directory. Unchanged regular files have their permissions repaired. Default - protected saves replace a destination symlink even when its bytes are unchanged; hard-linked - copies no longer share subsequent updates. +- Credential files written through the default file backend are created with + private permissions, and existing files have their permissions repaired before + new credentials are written. Unchanged files also have their permissions repaired. + Saves retain existing symlinks and hardlinks and can update writable files + without write access to the parent directory. - Authentication errors and verbose HTTP diagnostics omit provider-supplied error descriptions, response reasons and redirect headers that could expose credentials. OAuth response bodies and redirect destinations are also omitted diff --git a/rsconnect/api.py b/rsconnect/api.py index 96b1ef331..33661da80 100644 --- a/rsconnect/api.py +++ b/rsconnect/api.py @@ -3413,6 +3413,8 @@ def _refresh_user_token(self) -> dict[str, Any]: request_options["request_deadline"] = self.request_deadline if self._server.oauth_client_id is not None: request_options["client_id_override"] = self._server.oauth_client_id + if self._suppress_oauth_response_logging: + request_options["suppress_response_logging"] = True return connect_cloud.refresh(cast(str, self._server.refresh_token), self._server.environment, **request_options) def _attempt_token_refresh(self) -> bool: diff --git a/rsconnect/connect_cloud.py b/rsconnect/connect_cloud.py index 1872b90e4..dd58dafd2 100644 --- a/rsconnect/connect_cloud.py +++ b/rsconnect/connect_cloud.py @@ -242,6 +242,8 @@ def refresh( request_timeout: Optional[float] = None, request_deadline: Optional[float] = None, client_id_override: Optional[str] = None, + *, + suppress_response_logging: bool = False, ) -> dict[str, Any]: """Mint a new access token from a refresh token.""" env = environment or environment_name() @@ -250,6 +252,8 @@ def refresh( request_options["request_timeout"] = request_timeout if request_deadline is not None: request_options["request_deadline"] = request_deadline + if suppress_response_logging: + request_options["suppress_response_logging"] = True return refresh_access_token( metadata=urls(env).oauth_metadata(), client_id=client_id_override if client_id_override is not None else client_id(env), diff --git a/rsconnect/device_login.py b/rsconnect/device_login.py index 6d842fe82..acca42edf 100644 --- a/rsconnect/device_login.py +++ b/rsconnect/device_login.py @@ -634,6 +634,7 @@ def _post_form( ca_data, request_timeout, request_deadline, + suppress_response_logging=True, ) @@ -653,20 +654,22 @@ def _register_device_client( ca_data: Optional[str | bytes], deadline: float, ) -> str: + request_timeout = _remaining(deadline) try: client_id = register_client( metadata, server, insecure, ca_data, - request_timeout=_remaining(deadline), + request_timeout=request_timeout, request_deadline=deadline, + suppress_response_logging=True, ) - return _identifier(client_id, "OAuth client ID") except InvalidClientError: raise except RSConnectException: raise RSConnectException("OAuth client registration failed.") from None + return _identifier(client_id, "OAuth client ID") def _start_device_request( @@ -821,21 +824,12 @@ def _start_connect_login( ca_data, request_timeout=_remaining(deadline), request_deadline=deadline, + suppress_response_logging=True, ), server, ) if not client_id: - client_id = _identifier( - register_client( - metadata, - server, - insecure, - ca_data, - request_timeout=_remaining(deadline), - request_deadline=deadline, - ), - "OAuth client ID", - ) + client_id = _register_device_client(metadata, server, insecure, ca_data, deadline) client_id, response = _start_device_request(metadata, server, client_id, None, insecure, ca_data, deadline) _remaining(deadline) state = _make_state( @@ -1091,9 +1085,10 @@ def _lookup_cloud_account( raise InvalidGrantError() from exc if deadline <= time.monotonic(): raise _FinishDeadline() from exc - if isinstance(exc, RSConnectException) and exc.status is not None: + if isinstance(exc, RSConnectException): + status_detail = " (HTTP %s)" % exc.status if exc.status is not None else "" safe_error = RSConnectException( - "Posit Connect Cloud account lookup failed (HTTP %s)." % exc.status, + "Posit Connect Cloud account lookup failed%s." % status_detail, cause=exc.cause, status=exc.status, ) diff --git a/rsconnect/http_support.py b/rsconnect/http_support.py index ba8f8ec78..92f79444c 100644 --- a/rsconnect/http_support.py +++ b/rsconnect/http_support.py @@ -661,7 +661,8 @@ def _log_request( ) -> None: if not logger.is_debugging(): return - logged_uri = "" if suppress_uri_logging else _redacted_uri_for_log(full_uri) + private_uri = suppress_uri_logging or self._suppress_oauth_response_logging + logged_uri = "" if private_uri else _redacted_uri_for_log(full_uri) logger.debug(f"Request: {method} {logged_uri}") logger.debug("Headers:") for key, value in headers.items(): @@ -804,12 +805,19 @@ def _tweak_response(self, response: HTTPResponse) -> JsonData | HTTPResponse: return response def _handle_set_cookie(self, response: http.HTTPResponse): - self._cookies.store_cookies(response) + if self._suppress_oauth_response_logging: + self._cookies.store_cookies(response, suppress_logs=True) + else: + self._cookies.store_cookies(response) self._inject_cookies() def _inject_cookies(self): if len(self._cookies) > 0: - self._headers["Cookie"] = self._cookies.get_cookie_header_value() + self._headers["Cookie"] = ( + self._cookies.get_cookie_header_value(suppress_logs=True) + if self._suppress_oauth_response_logging + else self._cookies.get_cookie_header_value() + ) elif "Cookie" in self._headers: del self._headers["Cookie"] @@ -887,7 +895,7 @@ def __init__(self) -> None: self._content: dict[str, str] = {} self._reference = SimpleCookie() - def store_cookies(self, response: http.HTTPResponse): + def store_cookies(self, response: http.HTTPResponse, *, suppress_logs: bool = False): headers = filter(lambda h: h[0].lower() == "set-cookie", response.getheaders()) for header in headers: @@ -897,11 +905,13 @@ def store_cookies(self, response: http.HTTPResponse): self._keys.append(morsel.key) self._content[morsel.key] = morsel.value - logger.debug("CookieJar contains %d cookie(s).", len(self._keys)) + if not suppress_logs: + logger.debug("CookieJar contains %d cookie(s).", len(self._keys)) - def get_cookie_header_value(self): + def get_cookie_header_value(self, *, suppress_logs: bool = False): result = "; ".join([f"{key}={self._reference.value_encode(self._content[key])[1]}" for key in self._keys]) - logger.debug("Sending Cookie header with %d cookie(s).", len(self._keys)) + if not suppress_logs: + logger.debug("Sending Cookie header with %d cookie(s).", len(self._keys)) return result def as_dict(self): diff --git a/rsconnect/metadata.py b/rsconnect/metadata.py index 0b6506e66..24dd05a68 100644 --- a/rsconnect/metadata.py +++ b/rsconnect/metadata.py @@ -12,7 +12,6 @@ import shutil import stat import sys -import tempfile from contextlib import suppress from datetime import datetime, timezone from io import BufferedWriter @@ -302,26 +301,25 @@ def get_path(self): # noinspection PyShadowingBuiltins def save_to(self, path: str, data: bytes, open: Callable[..., BufferedWriter] = open): """ - Save our data to the specified file. Protected stores use an atomic write - with the default opener; supplied openers keep the direct destination-path - write behavior. + Save our data to the specified file. Protected stores set permissions on + the open file before writing; supplied openers keep direct write behavior. """ if self._chmod and open is _DEFAULT_OPEN: - descriptor, temporary_path = tempfile.mkstemp(prefix=".%s." % basename(path), dir=dirname(path) or ".") + # shortcut: writes remain non-atomic to preserve links; revisit when compatibility permits replacement. + flags = os.O_WRONLY | os.O_CREAT | getattr(os, "O_BINARY", 0) + descriptor = os.open(path, flags, 0o600) try: - with os.fdopen(descriptor, "wb") as stream: - descriptor = -1 - _chmod_private_file(stream.fileno(), temporary_path) + stream = os.fdopen(descriptor, "wb") + descriptor = -1 + with stream: + _chmod_private_file(stream.fileno(), path) + os.ftruncate(stream.fileno(), 0) stream.write(data) stream.flush() os.fsync(stream.fileno()) - os.replace(temporary_path, path) - temporary_path = "" finally: with suppress(OSError): os.close(descriptor) - with suppress(OSError): - os.unlink(temporary_path) else: with open(path, "wb") as f: f.write(data) @@ -338,9 +336,6 @@ def _already_holds(self, path: str, data: bytes, opener: Callable[..., BufferedW """ if self._real_path != path or not exists(path): return False - # Default protected writes must replace symlinks even when their target matches. - if self._chmod and opener is _DEFAULT_OPEN and os.path.islink(path): - return False try: with open(path, "rb") as f: return f.read() == data @@ -360,8 +355,7 @@ def save(self, open: Callable[..., BufferedWriter] = open): data = json.dumps(self._data, indent=4).encode("utf-8") if self._already_holds(self._primary_path, data, open): if self._chmod and open is _DEFAULT_OPEN: - metadata = os.lstat(self._primary_path) - # Avoid chmod following a symlink to an unrelated target. + metadata = os.stat(self._primary_path) if stat.S_ISREG(metadata.st_mode) and stat.S_IMODE(metadata.st_mode) != 0o600: os.chmod(self._primary_path, 0o600) return diff --git a/rsconnect/oauth.py b/rsconnect/oauth.py index 4623edc29..aef9ac708 100644 --- a/rsconnect/oauth.py +++ b/rsconnect/oauth.py @@ -129,8 +129,13 @@ def _post_oauth_form_request( ca_data: Optional[str | bytes] = None, request_timeout: Optional[float] = None, request_deadline: Optional[float] = None, + *, + suppress_response_logging: bool = False, ) -> Any: - """POST a form and return the raw response so device polling can inspect pending errors.""" + """POST a form and return the raw response so device polling can inspect pending errors. + + OAuth responses are always private; the suppression keyword remains for compatibility. + """ server = HTTPServer(base_url, disable_tls_check=insecure, ca_data=ca_data) server._suppress_oauth_response_logging = True if request_timeout is not None: @@ -152,6 +157,8 @@ def discover_oauth_metadata( ca_data: Optional[str | bytes] = None, request_timeout: Optional[float] = None, request_deadline: Optional[float] = None, + *, + suppress_response_logging: bool = False, ) -> dict[str, Any]: """Fetch OAuth 2.0 Authorization Server Metadata (RFC 8414). @@ -189,6 +196,8 @@ def register_client( ca_data: Optional[str | bytes] = None, request_timeout: Optional[float] = None, request_deadline: Optional[float] = None, + *, + suppress_response_logging: bool = False, ) -> str: """Register an OAuth client via Dynamic Client Registration (RFC 7591). @@ -499,6 +508,8 @@ def refresh_access_token( scope: Optional[str] = None, request_timeout: Optional[float] = None, request_deadline: Optional[float] = None, + *, + suppress_response_logging: bool = False, ) -> dict[str, Any]: """Refresh an OAuth access token using a refresh token. @@ -519,6 +530,8 @@ def refresh_access_token( request_options["request_timeout"] = request_timeout if request_deadline is not None: request_options["request_deadline"] = request_deadline + if suppress_response_logging: + request_options["suppress_response_logging"] = True data = _post_token_request(str(metadata["token_endpoint"]), params, insecure, ca_data, **request_options) if "access_token" not in data: raise RSConnectException("Token refresh returned an unexpected response.") @@ -562,11 +575,14 @@ def _post_token_request( ca_data: Optional[str | bytes] = None, request_timeout: Optional[float] = None, request_deadline: Optional[float] = None, + *, + suppress_response_logging: bool = False, ) -> dict[str, Any]: """POST a form-encoded request to an OAuth token endpoint and return the JSON body.""" parsed = urlparse(token_endpoint) base = f"{parsed.scheme}://{parsed.netloc}" + logging_options = {"suppress_response_logging": True} if suppress_response_logging else {} response = _post_oauth_form_request( base, parsed.path, @@ -575,6 +591,7 @@ def _post_token_request( ca_data, request_timeout, request_deadline, + **logging_options, ) return _unwrap_json_response(response) diff --git a/tests/test_connect_cloud.py b/tests/test_connect_cloud.py index a6de0cdaf..5686a101d 100644 --- a/tests/test_connect_cloud.py +++ b/tests/test_connect_cloud.py @@ -213,6 +213,15 @@ def test_refresh_requests_vivid_scope(self): self.assertEqual(kwargs["scope"], "vivid") self.assertEqual(kwargs["refresh_token"], "rt") + def test_refresh_preserves_logging_keyword_and_result(self): + with mock.patch("rsconnect.connect_cloud.refresh_access_token") as refresh: + refresh.return_value = {"access_token": "new"} + for suppress in (False, True): + with self.subTest(suppress_response_logging=suppress): + result = connect_cloud.refresh("rt", "production", suppress_response_logging=suppress) + self.assertEqual(result, {"access_token": "new"}) + self.assertEqual(refresh.call_args.kwargs.get("suppress_response_logging", False), suppress) + def test_refresh_can_limit_its_request_timeout(self): with mock.patch("rsconnect.connect_cloud.refresh_access_token") as refresh: refresh.return_value = {"access_token": "new"} diff --git a/tests/test_device_login.py b/tests/test_device_login.py index 5b308c57e..fdf7b26a6 100644 --- a/tests/test_device_login.py +++ b/tests/test_device_login.py @@ -1069,10 +1069,10 @@ def test_bytes_ca_is_persisted_and_reused_during_finish(login_env: Any, monkeypa ("lookup_results", "save_failures", "expected_error", "expected_message"), [ ( - [RSConnectException("temporary account lookup failure"), {"id": "team-id"}], + [RSConnectException("Rejected cloud-access-secret"), {"id": "team-id"}], [], RSConnectException, - "temporary account lookup failure", + "account lookup failed", ), ( [ @@ -1260,9 +1260,45 @@ def test_cloud_finish_refresh_uses_client_id_saved_at_start(login_env: Any, monk request_timeout=10.0, request_deadline=20.0, client_id_override="started-client", + suppress_response_logging=True, ) +def test_registration_deadline_is_reported_before_request(login_env: Any, monkeypatch: pytest.MonkeyPatch): + register = Mock() + monkeypatch.setattr(device_login, "register_client", register) + + with pytest.raises(RSConnectException, match="exceeded its 120-second limit"): + device_login._register_device_client(METADATA, SERVER, False, None, login_env.clock.monotonic()) + + register.assert_not_called() + assert not _state_path("connect", "work").exists() + + +def test_registration_error_is_private_and_can_be_retried(login_env: Any): + login_env.http.responses.extend( + [ + _response(200, METADATA), + _response(503, {"error": "server_error", "error_description": "device-code-secret"}), + ] + ) + + with pytest.raises(RSConnectException, match="client registration failed") as failed: + device_login.start_connect_login(SERVER, "work") + + assert "device-code-secret" not in str(failed.value) + assert not _state_path("connect", "work").exists() + assert login_env.http.instances[-1]._suppress_oauth_response_logging is True + login_env.http.responses.extend( + [ + _response(200, METADATA), + _response(201, {"client_id": "registered-client"}), + _response(200, DEVICE_RESPONSE), + ] + ) + assert device_login.start_connect_login(SERVER, "work")["status"] == "pending" + + def test_missing_cloud_account_removes_checkpoint_for_corrected_start(login_env: Any, monkeypatch: pytest.MonkeyPatch): monkeypatch.setenv(connect_cloud.ENVIRONMENT_ENV_VAR, "production") _queue_device(login_env) diff --git a/tests/test_http_support.py b/tests/test_http_support.py index b5c01b886..84feddf3b 100644 --- a/tests/test_http_support.py +++ b/tests/test_http_support.py @@ -316,6 +316,18 @@ def getheaders(self): class TestCookieJar(TestCase): + def test_suppressed_logs_preserve_cookie_storage_and_header(self): + from unittest.mock import patch + + jar = CookieJar() + with patch("rsconnect.http_support.logger.debug") as debug: + jar.store_cookies(FakeSetCookieResponse(["session=secret"]), suppress_logs=True) + header = jar.get_cookie_header_value(suppress_logs=True) + + self.assertEqual(header, "session=secret") + self.assertEqual(jar.as_dict(), {"keys": ["session"], "content": {"session": "secret"}}) + debug.assert_not_called() + def test_basic_stuff(self): jar = CookieJar() jar.store_cookies(FakeSetCookieResponse(["my-cookie=my-value", "my-2nd-cookie=my-other-value"])) @@ -374,6 +386,31 @@ def test_cookie_values_do_not_reach_the_debug_log(self): class TestDebugLogRedaction(TestCase): """Credential material must not reach the debug (-vv) log.""" + def test_initial_oauth_request_uri_is_private_without_changing_request(self): + from unittest.mock import Mock, patch + + target = "/oauth/opaque-secret?resume=another-secret" + reply = Mock() + reply.status = 200 + reply.reason = "OK" + reply.read.return_value = b"{}" + reply.getheaders.return_value = [] + reply.getheader.return_value = "application/json" + with HTTPServer("http://example.com") as server: + server._suppress_oauth_response_logging = True + transport = cast(Any, server._conn) + with self.assertLogs("rsconnect", level="DEBUG") as captured: + with patch.object(transport, "request") as send: + with patch.object(transport, "getresponse", return_value=reply): + response = cast(HTTPResponse, server.get(target)) + + log_text = "\n".join(captured.output) + self.assertNotIn("opaque-secret", log_text) + self.assertNotIn("another-secret", log_text) + self.assertIn("Request: GET ", log_text) + self.assertEqual(send.call_args.args[:2], ("GET", target)) + self.assertEqual(response.full_uri, target) + def test_form_encoded_credentials_are_redacted(self): from rsconnect.http_support import _redacted_body_for_log diff --git a/tests/test_metadata.py b/tests/test_metadata.py index a08a2143b..d66b29392 100644 --- a/tests/test_metadata.py +++ b/tests/test_metadata.py @@ -262,8 +262,8 @@ def test_remove_default_clears_it(self): def test_save_skips_rewrite_when_file_unchanged(self): with patch( - "rsconnect.metadata.tempfile.mkstemp", - side_effect=AssertionError("created a temporary file for unchanged data"), + "rsconnect.metadata.os.open", + side_effect=AssertionError("opened a descriptor for unchanged data"), ): self.server_store.save() @@ -279,16 +279,45 @@ def test_private_store_creates_mode_600_file_under_umask_022(self): base_dir = join(self.tempDir, "umask") path = join(base_dir, "servers.json") previous_umask = os.umask(0o022) + open_file = os.open + fchmod = os.fchmod + ftruncate = os.ftruncate + + def check_open(path_to_open, flags, mode, *args, **kwargs): + self.assertEqual(path_to_open, path) + self.assertFalse(flags & os.O_TRUNC) + self.assertEqual(mode, 0o600) + descriptor = open_file(path_to_open, flags, mode, *args, **kwargs) + metadata = os.fstat(descriptor) + self.assertEqual(stat.S_IMODE(metadata.st_mode), 0o600) + self.assertEqual(metadata.st_size, 0) + return descriptor + + def check_fchmod(descriptor, mode): + self.assertEqual(mode, 0o600) + self.assertEqual(os.fstat(descriptor).st_size, 0) + fchmod(descriptor, mode) + + def check_ftruncate(descriptor, length): + self.assertEqual(stat.S_IMODE(os.fstat(descriptor).st_mode), 0o600) + self.assertEqual(length, 0) + ftruncate(descriptor, length) + try: - store = ServerStore(base_dir=base_dir) - store.set("private", "https://connect.example.test", api_key="new-secret") + with patch("rsconnect.metadata.os.open", side_effect=check_open), patch( + "rsconnect.metadata.os.fchmod", side_effect=check_fchmod + ), patch("rsconnect.metadata.os.ftruncate", side_effect=check_ftruncate): + store = ServerStore(base_dir=base_dir) + store.set("private", "https://connect.example.test", api_key="new-secret") finally: os.umask(previous_umask) self.assertEqual(stat.S_IMODE(os.stat(path).st_mode), 0o600) self.assertEqual(store.get_path(), path) + with open(path, "rb") as metadata_file: + self.assertIn(b"new-secret", metadata_file.read()) - def test_private_store_keeps_new_secret_off_existing_file_until_replace(self): + def test_private_store_chmods_existing_file_before_truncating(self): if os.name != "posix": self.skipTest("POSIX file permissions are unavailable") @@ -296,72 +325,73 @@ def test_private_store_keeps_new_secret_off_existing_file_until_replace(self): os.chmod(path, 0o644) with open(path, "rb") as metadata_file: previous_data = metadata_file.read() + previous_inode = os.stat(path).st_ino new_secret = b"rotated-secret" self.server_store._data["foo"]["api_key"] = new_secret.decode("ascii") - fdopen = os.fdopen + open_file = os.open fchmod = os.fchmod - replace = os.replace - def check_private_fd(descriptor, mode): - self.assertEqual(stat.S_IMODE(os.fstat(descriptor).st_mode), 0o600) - return fdopen(descriptor, mode) + ftruncate = os.ftruncate + + def check_open(path_to_open, flags, mode, *args, **kwargs): + self.assertFalse(flags & os.O_TRUNC) + descriptor = open_file(path_to_open, flags, mode, *args, **kwargs) + with open(path, "rb") as metadata_file: + self.assertEqual(metadata_file.read(), previous_data) + self.assertEqual(stat.S_IMODE(os.fstat(descriptor).st_mode), 0o644) + return descriptor def check_fchmod(descriptor, mode): self.assertEqual(mode, 0o600) - self.assertEqual(stat.S_IMODE(os.fstat(descriptor).st_mode), 0o600) + with open(path, "rb") as metadata_file: + self.assertEqual(metadata_file.read(), previous_data) fchmod(descriptor, mode) + self.assertEqual(stat.S_IMODE(os.fstat(descriptor).st_mode), 0o600) - def check_atomic_replace(source, destination): - self.assertEqual(destination, path) - self.assertEqual(stat.S_IMODE(os.stat(source).st_mode), 0o600) - with open(source, "rb") as temporary_file: - self.assertIn(new_secret, temporary_file.read()) - with open(destination, "rb") as existing_file: - self.assertEqual(existing_file.read(), previous_data) - replace(source, destination) + def check_ftruncate(descriptor, length): + self.assertEqual(length, 0) + self.assertEqual(stat.S_IMODE(os.fstat(descriptor).st_mode), 0o600) + with open(path, "rb") as metadata_file: + self.assertEqual(metadata_file.read(), previous_data) + ftruncate(descriptor, length) - with patch("rsconnect.metadata.os.fdopen", side_effect=check_private_fd) as fdopen_mock, patch( + with patch("rsconnect.metadata.os.open", side_effect=check_open), patch( "rsconnect.metadata.os.fchmod", side_effect=check_fchmod - ) as fchmod_mock, patch("rsconnect.metadata.os.replace", side_effect=check_atomic_replace) as replace_mock: + ), patch("rsconnect.metadata.os.ftruncate", side_effect=check_ftruncate): self.server_store.save() - fdopen_mock.assert_called_once() - fchmod_mock.assert_called_once() - replace_mock.assert_called_once() - self.assertEqual(fchmod_mock.call_args[0][0], fdopen_mock.call_args[0][0]) self.assertEqual(stat.S_IMODE(os.stat(path).st_mode), 0o600) + self.assertEqual(os.stat(path).st_ino, previous_inode) with open(path, "rb") as metadata_file: self.assertIn(new_secret, metadata_file.read()) - def test_fdopen_failure_closes_descriptor_removes_temp_and_preserves_file(self): + def test_fdopen_failure_closes_descriptor_and_preserves_file(self): path = join(self.tempDir, "existing.json") with open(path, "wb") as metadata_file: metadata_file.write(b"old credentials") - previous_mode = stat.S_IMODE(os.stat(path).st_mode) store = DataStore(path, chmod=True, autoload=False) store._data = {"server": {"api_key": "new-secret"}} files_before = set(os.listdir(self.tempDir)) - mkstemp = tempfile.mkstemp - temporary = {} + open_file = os.open + descriptor = None - def record_temp(*args, **kwargs): - descriptor, temporary_path = mkstemp(*args, **kwargs) - temporary.update(descriptor=descriptor, path=temporary_path) - return descriptor, temporary_path + def record_open(*args, **kwargs): + nonlocal descriptor + descriptor = open_file(*args, **kwargs) + return descriptor - with patch("rsconnect.metadata.tempfile.mkstemp", side_effect=record_temp), patch( + with patch("rsconnect.metadata.os.open", side_effect=record_open), patch( "rsconnect.metadata.os.fdopen", side_effect=OSError("fdopen failed") ): with self.assertRaisesRegex(OSError, "fdopen failed"): store.save() + self.assertIsNotNone(descriptor) with self.assertRaises(OSError): - os.fstat(temporary["descriptor"]) - self.assertFalse(os.path.exists(temporary["path"])) + os.fstat(descriptor) self.assertEqual(set(os.listdir(self.tempDir)), files_before) with open(path, "rb") as metadata_file: self.assertEqual(metadata_file.read(), b"old credentials") - self.assertEqual(stat.S_IMODE(os.stat(path).st_mode), previous_mode) self.assertIsNone(store._real_path) def test_unchanged_private_store_repairs_mode_without_rewriting(self): @@ -375,8 +405,8 @@ def test_unchanged_private_store_repairs_mode_without_rewriting(self): previous_data = metadata_file.read() with patch( - "rsconnect.metadata.tempfile.mkstemp", - side_effect=AssertionError("rewrote unchanged credentials"), + "rsconnect.metadata.os.open", + side_effect=AssertionError("opened a descriptor for unchanged credentials"), ): self.server_store.save() @@ -386,38 +416,46 @@ def test_unchanged_private_store_repairs_mode_without_rewriting(self): with open(path, "rb") as metadata_file: self.assertEqual(metadata_file.read(), previous_data) - def test_private_store_failure_preserves_file_and_real_path(self): + def test_private_store_truncate_failure_closes_descriptor_and_preserves_file(self): path = join(self.tempDir, "existing.json") with open(path, "wb") as metadata_file: metadata_file.write(b"old credentials") - previous_mode = stat.S_IMODE(os.stat(path).st_mode) store = DataStore(path, chmod=True, autoload=False) store._data = {"server": {"api_key": "new-secret"}} - files_before = set(os.listdir(self.tempDir)) + open_file = os.open + descriptor = None + + def record_open(*args, **kwargs): + nonlocal descriptor + descriptor = open_file(*args, **kwargs) + return descriptor - with patch("rsconnect.metadata.os.fsync", side_effect=OSError("sync failed")): - with self.assertRaisesRegex(OSError, "sync failed"): + with patch("rsconnect.metadata.os.open", side_effect=record_open), patch( + "rsconnect.metadata.os.ftruncate", side_effect=OSError("truncate failed") + ): + with self.assertRaisesRegex(OSError, "truncate failed"): store.save() + self.assertIsNotNone(descriptor) + with self.assertRaises(OSError): + os.fstat(descriptor) with open(path, "rb") as metadata_file: self.assertEqual(metadata_file.read(), b"old credentials") - self.assertEqual(stat.S_IMODE(os.stat(path).st_mode), previous_mode) self.assertIsNone(store._real_path) - self.assertEqual(set(os.listdir(self.tempDir)), files_before) - def test_private_store_falls_back_after_atomic_replace_failure(self): + def test_private_store_falls_back_after_primary_open_failure(self): primary = join(self.tempDir, "local", "servers.json") secondary = join(self.tempDir, "config", "servers.json") store = DataStore(primary, secondary, chmod=True, autoload=False) store._data = {"server": {"api_key": "fallback-secret"}} - replace = os.replace + open_file = os.open - def fail_primary(source, destination): - if destination == primary: + def fail_primary(path, flags, mode, *args, **kwargs): + if path == primary: raise OSError("primary unavailable") - replace(source, destination) + return open_file(path, flags, mode, *args, **kwargs) - with patch("rsconnect.metadata.os.replace", side_effect=fail_primary): + with patch("rsconnect.metadata.os.open", side_effect=fail_primary): store.save() self.assertEqual(store.get_path(), secondary) @@ -426,50 +464,49 @@ def fail_primary(source, destination): with open(secondary, "r", encoding="utf-8") as metadata_file: self.assertEqual(json.load(metadata_file), store._data) - def test_private_store_replaces_symlink_without_writing_through(self): + def test_private_store_updates_symlink_target_in_place(self): if os.name != "posix": self.skipTest("POSIX symlinks are unavailable") path = self.server_store_path target = join(self.tempDir, "servers-target.json") - os.replace(path, target) + os.rename(path, target) os.chmod(target, 0o644) with open(target, "rb") as metadata_file: previous_data = metadata_file.read() + before = os.stat(target) try: os.symlink(target, path) except OSError: self.skipTest("symlinks are unavailable") - def fail_open(path_to_open, mode, *args, **kw): - self.fail("custom opener was called for unchanged symlink data") - - self.server_store.save(fail_open) + self.server_store.save() self.assertTrue(os.path.islink(path)) + self.assertEqual(os.readlink(path), target) + self.assertEqual(stat.S_IMODE(os.stat(target).st_mode), 0o600) + after = os.stat(target) + self.assertEqual(after.st_ino, before.st_ino) + self.assertEqual(after.st_mtime_ns, before.st_mtime_ns) with open(target, "rb") as metadata_file: self.assertEqual(metadata_file.read(), previous_data) - self.assertEqual(stat.S_IMODE(os.stat(target).st_mode), 0o644) - self.server_store.save() + def fail_open(*args, **kwargs): + self.fail("custom opener was called for unchanged symlink data") - self.assertFalse(os.path.islink(path)) - self.assertEqual(stat.S_IMODE(os.stat(path).st_mode), 0o600) - with open(path, "rb") as metadata_file: - self.assertEqual(metadata_file.read(), previous_data) - with open(target, "rb") as metadata_file: - self.assertEqual(metadata_file.read(), previous_data) - self.assertEqual(stat.S_IMODE(os.stat(target).st_mode), 0o644) + self.server_store.save(fail_open) self.server_store._data["foo"]["api_key"] = "symlink-secret" self.server_store.save() - self.assertFalse(os.path.islink(path)) + self.assertTrue(os.path.islink(path)) with open(target, "rb") as metadata_file: - self.assertEqual(metadata_file.read(), previous_data) - self.assertEqual(stat.S_IMODE(os.stat(target).st_mode), 0o644) - self.assertEqual(stat.S_IMODE(os.stat(path).st_mode), 0o600) + target_data = metadata_file.read() + self.assertIn(b"symlink-secret", target_data) + self.assertEqual(os.stat(path).st_ino, before.st_ino) + self.assertEqual(os.stat(target).st_ino, before.st_ino) + self.assertEqual(stat.S_IMODE(os.stat(target).st_mode), 0o600) - def test_private_store_does_not_update_other_hardlinks(self): + def test_private_store_updates_other_hardlinks_in_place(self): if os.name != "posix": self.skipTest("POSIX hard links are unavailable") @@ -480,21 +517,51 @@ def test_private_store_does_not_update_other_hardlinks(self): except OSError: self.skipTest("hard links are unavailable") os.chmod(path, 0o644) - with open(path, "rb") as metadata_file: - previous_data = metadata_file.read() - self.server_store.save() - self.assertEqual(stat.S_IMODE(os.stat(alias).st_mode), 0o600) + previous_inode = os.stat(path).st_ino self.server_store._data["foo"]["api_key"] = "hardlink-secret" self.server_store.save() with open(alias, "rb") as metadata_file: alias_data = metadata_file.read() - self.assertEqual(alias_data, previous_data) - self.assertNotIn(b"hardlink-secret", alias_data) + with open(path, "rb") as metadata_file: + path_data = metadata_file.read() + self.assertEqual(alias_data, path_data) + self.assertIn(b"hardlink-secret", alias_data) + self.assertEqual(os.stat(path).st_ino, previous_inode) + self.assertEqual(os.stat(alias).st_ino, previous_inode) self.assertEqual(stat.S_IMODE(os.stat(alias).st_mode), 0o600) self.assertEqual(stat.S_IMODE(os.stat(path).st_mode), 0o600) + def test_private_store_updates_existing_file_in_read_only_parent(self): + if os.name != "posix": + self.skipTest("POSIX directory permissions are unavailable") + if os.geteuid() == 0: + self.skipTest("root can write in read-only directories") + + parent = join(self.tempDir, "read-only") + os.makedirs(parent) + path = join(parent, "servers.json") + store = DataStore(path, chmod=True, autoload=False) + store._data = {"server": {"api_key": "read-only-parent-secret"}} + with open(path, "wb") as metadata_file: + metadata_file.write(b"old credentials") + os.chmod(path, 0o600) + previous_inode = os.stat(path).st_ino + files_before = set(os.listdir(parent)) + + os.chmod(parent, 0o555) + try: + store.save() + self.assertEqual(stat.S_IMODE(os.stat(parent).st_mode), 0o555) + finally: + os.chmod(parent, 0o700) + + self.assertEqual(os.stat(path).st_ino, previous_inode) + self.assertEqual(set(os.listdir(parent)), files_before) + with open(path, "r", encoding="utf-8") as metadata_file: + self.assertEqual(json.load(metadata_file), store._data) + def test_save_rewrites_when_data_changed(self): writes = [] diff --git a/tests/test_oauth.py b/tests/test_oauth.py index ebd8a1f9a..bab8ce33e 100644 --- a/tests/test_oauth.py +++ b/tests/test_oauth.py @@ -60,6 +60,48 @@ def mock_http_server(): yield mock_server +@pytest.mark.parametrize("suppress_response_logging", [False, True]) +@pytest.mark.parametrize( + ("function_name", "args", "data", "expected"), + [ + ("discover_oauth_metadata", (FAKE_URL,), FAKE_METADATA, FAKE_METADATA), + ("register_client", (FAKE_METADATA, FAKE_URL), {"client_id": "client"}, "client"), + ("_post_oauth_form_request", (FAKE_URL, "/token", {}), {"access_token": "token"}, None), + ( + "_post_token_request", + (FAKE_METADATA["token_endpoint"], {}), + {"access_token": "token"}, + {"access_token": "token"}, + ), + ( + "refresh_access_token", + (FAKE_METADATA, "client", "refresh"), + {"access_token": "token"}, + {"access_token": "token"}, + ), + ], +) +def test_logging_keyword_remains_accepted_and_oauth_responses_stay_private( + mock_http_server: MagicMock, + suppress_response_logging: bool, + function_name: str, + args: tuple[Any, ...], + data: dict[str, Any], + expected: Any, +): + from rsconnect import oauth + + response = _make_response(200, data) + mock_http_server.get.return_value = response + mock_http_server.post.return_value = response + mock_http_server.request.return_value = response + + result = getattr(oauth, function_name)(*args, suppress_response_logging=suppress_response_logging) + + assert result == (response if expected is None else expected) + assert mock_http_server._suppress_oauth_response_logging is True + + class TestPKCE: def test_generates_valid_pair(self): verifier, challenge = generate_pkce_pair() From 631a480bdf1e5e510661dd5c06ddf3a9dc07ed82 Mon Sep 17 00:00:00 2001 From: Matt Conflitti Date: Thu, 15 Oct 2026 12:06:02 -0400 Subject: [PATCH 3/5] fix(auth): store device checkpoints in keyring first --- docs/CHANGELOG.md | 4 + docs/commands/add.md | 9 +- docs/commands/login.md | 9 +- rsconnect/device_login.py | 96 +++++++++++++--- tests/test_device_login.py | 226 +++++++++++++++++++++++++++++++++++-- 5 files changed, 316 insertions(+), 28 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 9f6212af0..c8a5a23ea 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - OAuth clients now register with Posit Connect as "Posit CLI", the name shown on the consent screen. Previously registered clients keep their existing name. +- Resumable device login stores approved tokens in a separate system keyring + entry when available, keeping only a reference in pending state. Protected-file + fallback and retries from existing plaintext checkpoints remain supported. + Cloud retries preserve rotated tokens without replacing a saved login before account lookup completes. - Credential files written through the default file backend are created with private permissions, and existing files have their permissions repaired before new credentials are written. Unchanged files also have their permissions repaired. diff --git a/docs/commands/add.md b/docs/commands/add.md index 6359d9853..497c909a2 100644 --- a/docs/commands/add.md +++ b/docs/commands/add.md @@ -20,9 +20,14 @@ The command stores pending logins under its configuration directory. Files use o Resumable device login requires a POSIX system such as Linux or macOS. Use a private configuration directory. Existing blocking login remains available on Windows. -Pending state contains device codes and token checkpoints in plaintext, even when final credentials use a keyring. +Pending state contains device codes in plaintext. After approval, tokens are stored in a separate system keyring entry, +and the pending file keeps a reference to those tokens. If keyring storage is unavailable, tokens are saved in the owner-only +pending file instead. Existing plaintext token checkpoints remain readable. Owner-only permissions restrict ordinary access to your operating-system account; processes running as that account and backups can still read it. -Finish pending logins promptly. Abandoned state has no background cleanup, and deleting local state does not revoke an issued token. +Finish pending logins promptly. Completion removes the pending file and its temporary keyring entry. +Abandoned state has no background cleanup, and deleting local state does not revoke an issued token. +If a pending keyring entry cannot be read, unlock the same keyring and retry; the command will not redeem the device code again. +Recovery after an interrupted pending-file write also requires the same keyring backend. Use HTTPS with certificate verification and keep the configuration directory private and outside your application directory. Finish the login after the user approves: diff --git a/docs/commands/login.md b/docs/commands/login.md index 00eacfe51..69817f489 100644 --- a/docs/commands/login.md +++ b/docs/commands/login.md @@ -33,9 +33,14 @@ The command stores pending logins under its configuration directory. Files use o Resumable device login requires a POSIX system such as Linux or macOS. Use a private configuration directory. Existing blocking login remains available on Windows. -Pending state contains device codes and token checkpoints in plaintext, even when final credentials use a keyring. +Pending state contains device codes in plaintext. After approval, tokens are stored in a separate system keyring entry, +and the pending file keeps a reference to those tokens. If keyring storage is unavailable, tokens are saved in the owner-only +pending file instead. Existing plaintext token checkpoints remain readable. Owner-only permissions restrict ordinary access to your operating-system account; processes running as that account and backups can still read it. -Finish pending logins promptly. Abandoned state has no background cleanup, and deleting local state does not revoke an issued token. +Finish pending logins promptly. Completion removes the pending file and its temporary keyring entry. +Abandoned state has no background cleanup, and deleting local state does not revoke an issued token. +If a pending keyring entry cannot be read, unlock the same keyring and retry; the command will not redeem the device code again. +Recovery after an interrupted pending-file write also requires the same keyring backend. Use HTTPS with certificate verification and keep the configuration directory private and outside your application directory. ## Finish device authentication diff --git a/rsconnect/device_login.py b/rsconnect/device_login.py index acca42edf..b928d164f 100644 --- a/rsconnect/device_login.py +++ b/rsconnect/device_login.py @@ -26,7 +26,10 @@ _post_oauth_form_request, _unwrap_json_response, discover_oauth_metadata, + keyring_delete_values, + keyring_read_value, keyring_store_token, + keyring_store_values, register_client, ) from .validation import require_posix @@ -461,7 +464,12 @@ def _validate_state_polling(raw: dict[str, Any]) -> None: raise RSConnectException("Pending device login state is invalid.") if not _positive_finite_duration(raw["interval"]): raise RSConnectException("Pending device login state is invalid.") - _validate_tokens(raw["tokens"]) + tokens = raw["tokens"] + if isinstance(tokens, dict) and "keyring" in tokens: + if tokens != {"keyring": True} or tokens["keyring"] is not True: + raise RSConnectException("Pending device login state is invalid.") + else: + _validate_tokens(tokens) def _validate_state(kind: str, name: str, raw: Any) -> dict[str, Any]: @@ -489,7 +497,11 @@ def _read_state(kind: str, name: str) -> Optional[dict[str, Any]]: os.close(descriptor) except OSError: pass - return _validate_state(kind, name, raw) + state = _validate_state(kind, name, raw) + # Recover a keyring write even if the subsequent state-file write failed. + if state["tokens"] is None and _read_keyring_checkpoint(state) is not None: + state["tokens"] = {"keyring": True} + return state def _read_state_json(descriptor: int) -> Any: @@ -500,11 +512,20 @@ def _read_state_json(descriptor: int) -> Any: raise RSConnectException("Pending device login state is invalid.") from exc -def _remove_state(kind: str, name: str) -> None: +def _remove_state(state: dict[str, Any]) -> None: + tokens = None + if state["tokens"] == {"keyring": True}: + tokens = _read_keyring_checkpoint(state) + if not keyring_delete_values(_checkpoint_key(state), ("tokens",)): + raise RSConnectException("Could not remove pending login tokens from the keyring. Unlock it and retry.") try: - os.unlink(_state_path(kind, name)) + os.unlink(_state_path(state["kind"], state["name"])) except FileNotFoundError: pass + except OSError: + if tokens is not None: + _store_checkpoint(state, tokens) + raise def _ca_values(ca_data: Optional[str | bytes]) -> tuple[Optional[str], Optional[str]]: @@ -807,7 +828,7 @@ def _start_connect_login( _assert_nickname_target(store, "connect", name, server) state = _read_state("connect", name) if state and _device_code_expired(state): - _remove_state("connect", name) + _remove_state(state) state = None if state: _check_pending_target(state, "connect", server, None, client_id, insecure, ca_data) @@ -864,7 +885,7 @@ def _start_cloud_login( _assert_nickname_target(store, "cloud", name, server, account) state = _read_state("cloud", name) if state and _device_code_expired(state): - _remove_state("cloud", name) + _remove_state(state) state = None if state: _check_pending_target(state, "cloud", server, account, None, False, None) @@ -1015,12 +1036,54 @@ def _checkpoint(tokens: dict[str, Any]) -> dict[str, Any]: } +def _checkpoint_key(state: dict[str, Any]) -> str: + identity = [state[key] for key in ("kind", "name", "server", "client_id", "device_code")] + digest = hashlib.sha256(json.dumps(identity).encode("utf-8")).hexdigest() + return "device-login:" + digest + + +def _read_keyring_checkpoint(state: dict[str, Any]) -> Optional[dict[str, Any]]: + readable, value = keyring_read_value(_checkpoint_key(state), "tokens") + if not readable: + raise RSConnectException("Could not read pending login tokens from the keyring. Unlock it and retry.") + if value is None: + return None + try: + tokens = json.loads(value) + except (ValueError, TypeError) as exc: + raise RSConnectException("Pending login tokens in the keyring are invalid.") from exc + _validate_tokens(tokens) + if tokens is None: + raise RSConnectException("Pending login tokens in the keyring are invalid.") + return cast(Dict[str, Any], tokens) + + +def _approved_tokens(state: dict[str, Any]) -> dict[str, Any]: + if state["tokens"] != {"keyring": True}: + return state["tokens"] + tokens = _read_keyring_checkpoint(state) + if tokens is None: + raise RSConnectException( + "Pending login tokens are missing from the keyring. Restore the entry, or remove pending state " + "and start a new login." + ) + return tokens + + +def _store_checkpoint(state: dict[str, Any], tokens: dict[str, Any]) -> None: + # Keep file checkpoints on disk so a failed rotation write cannot revive older file tokens. + use_keyring = state["tokens"] is None or state["tokens"] == {"keyring": True} + in_keyring = use_keyring and keyring_store_values(_checkpoint_key(state), {"tokens": json.dumps(tokens)}) + state["tokens"] = {"keyring": True} if in_keyring else tokens + _write_state(state) + + def _pending(state: dict[str, Any]) -> dict[str, Any]: return {"status": "pending", "name": state["name"], "server": state["server"]} def _finalize_connect(state: dict[str, Any]) -> dict[str, Any]: - name, server, tokens = state["name"], state["server"], state["tokens"] + name, server, tokens = state["name"], state["server"], _approved_tokens(state) store = _store() _assert_nickname_target(store, "connect", name, server) saved = _saved_connect_entry(store, name, server) @@ -1053,7 +1116,7 @@ def _no_such_account(self, accounts: list[api.ConnectCloudAccount], not_found_me def _cloud_login_client( state: dict[str, Any], deadline: float ) -> tuple[api.ConnectCloudServer, api.ConnectCloudClient, dict[str, Any]]: - tokens = state["tokens"] + tokens = _approved_tokens(state) cloud_server = api.ConnectCloudServer( account_name=state["account"], access_token=tokens["access_token"], @@ -1102,12 +1165,12 @@ def _checkpoint_rotated_cloud_tokens( ) -> None: if cloud_server.access_token == tokens["access_token"] and cloud_server.refresh_token == tokens["refresh_token"]: return - state["tokens"] = { + rotated = { "access_token": cloud_server.access_token, "refresh_token": cloud_server.refresh_token, "expires_at": tokens["expires_at"], } - _write_state(state) + _store_checkpoint(state, rotated) def _save_cloud_account( @@ -1155,8 +1218,7 @@ def _finish_state(state: dict[str, Any], deadline: float) -> dict[str, Any]: tokens = _poll_for_token(state, deadline) if tokens is None: return _pending(state) - state["tokens"] = _checkpoint(tokens) - _write_state(state) + _store_checkpoint(state, _checkpoint(tokens)) if deadline <= time.monotonic(): return _pending(state) if state["kind"] == "cloud": @@ -1169,7 +1231,7 @@ def _load_pending(kind: str, name: str) -> dict[str, Any]: if state is None: raise RSConnectException('No pending "%s" login exists for nickname "%s".' % (kind, name)) if _device_code_expired(state): - _remove_state(kind, name) + _remove_state(state) raise _Expired() _assert_nickname_target(_store(), kind, name, state["server"], state["account"]) return state @@ -1192,19 +1254,19 @@ def finish_login(kind: str, name: str, timeout: int = 120) -> dict[str, Any]: with _state_lock(kind, name, deadline): _remove_temporary_state_files(kind, name) state = _load_pending(kind, name) - return _finish_and_cleanup(kind, name, state, deadline) + return _finish_and_cleanup(state, deadline) except _LockBusy: return {"status": "pending", "name": name, "server": None} -def _finish_and_cleanup(kind: str, name: str, state: dict[str, Any], deadline: float) -> dict[str, Any]: +def _finish_and_cleanup(state: dict[str, Any], deadline: float) -> dict[str, Any]: try: result = _finish_state(state, deadline) except _FinishDeadline: return _pending(state) except (_Denied, _Expired, InvalidClientError, InvalidGrantError, ConnectCloudAccountNotFoundError): - _remove_state(kind, name) + _remove_state(state) raise if result["status"] == "done": - _remove_state(kind, name) + _remove_state(state) return result diff --git a/tests/test_device_login.py b/tests/test_device_login.py index fdf7b26a6..7718e98b6 100644 --- a/tests/test_device_login.py +++ b/tests/test_device_login.py @@ -8,7 +8,7 @@ import time from contextlib import contextmanager from pathlib import Path -from types import SimpleNamespace +from types import ModuleType, SimpleNamespace from typing import Any from unittest.mock import Mock @@ -132,6 +132,27 @@ def _state_path(kind: str, name: str) -> Path: return Path(device_login._state_path(kind, name)) +@pytest.fixture +def memory_keyring(monkeypatch: pytest.MonkeyPatch) -> dict[tuple[str, str], str]: + values: dict[tuple[str, str], str] = {} + module = ModuleType("keyring") + errors = ModuleType("keyring.errors") + errors.PasswordDeleteError = type("PasswordDeleteError", (Exception,), {}) + errors.NoKeyringError = type("NoKeyringError", (Exception,), {}) + + def delete(service: str, username: str) -> None: + if values.pop((service, username), None) is None: + raise errors.PasswordDeleteError() + + module.errors = errors + module.set_password = lambda service, username, value: values.__setitem__((service, username), value) + module.get_password = lambda service, username: values.get((service, username)) + module.delete_password = delete + monkeypatch.setitem(sys.modules, "keyring", module) + monkeypatch.setitem(sys.modules, "keyring.errors", errors) + return values + + def test_connect_start_uses_oauth_helpers_with_one_bounded_deadline(login_env: Any, monkeypatch: pytest.MonkeyPatch): login_env.http.responses.extend( [ @@ -1025,6 +1046,171 @@ def fail_once(store: ServerStore, *args: Any, **kwargs: Any) -> Any: assert not _state_path("connect", "work").exists() +@pytest.mark.parametrize("interrupt_state_write", [False, True]) +def test_approved_keyring_tokens_resume_without_plaintext_checkpoint( + login_env: Any, monkeypatch: pytest.MonkeyPatch, memory_keyring: Any, interrupt_state_write: bool +): + _stub_connect_discovery(monkeypatch) + _queue_device(login_env) + device_login.start_connect_login(SERVER, "work") + login_env.clock.advance(5) + login_env.http.responses.append({"access_token": "approved-secret", "expires_in": 3600}) + post = device_login._post_oauth_form_request + + def approve_at_deadline(*args: Any, **kwargs: Any) -> Any: + response = post(*args, **kwargs) + login_env.clock.advance(5) + return response + + monkeypatch.setattr(device_login, "_post_oauth_form_request", approve_at_deadline) + replace = device_login.os.replace + if interrupt_state_write: + monkeypatch.setattr(device_login.os, "replace", Mock(side_effect=OSError("interrupted write"))) + with pytest.raises(RSConnectException, match="safely write"): + device_login.finish_login("connect", "work", timeout=5) + monkeypatch.setattr(device_login.os, "replace", replace) + else: + assert device_login.finish_login("connect", "work", timeout=5)["status"] == "pending" + token_expiry = login_env.clock.time() + 3600 + assert "approved-secret" not in _state_path("connect", "work").read_text() + assert stat.S_IMODE(_state_path("connect", "work").stat().st_mode) == 0o600 + + login_env.clock.advance(601) + device_login.start_connect_login(SERVER, "work") + assert device_login._read_state("connect", "work")["tokens"] == {"keyring": True} + assert device_login.finish_login("connect", "work")["status"] == "done" + assert oauth.keyring_get_tokens(SERVER) == ("approved-secret", None) + saved = device_login._store().get_by_name("work") + assert saved.get("oauth_access_token") is None + assert saved.get("oauth_refresh_token") is None + assert saved["oauth_token_expiry"] == token_expiry + assert len(login_env.http.instances) == 2 + assert not _state_path("connect", "work").exists() + assert not any("device-login:" in username for _, username in memory_keyring) + + +def test_failed_pending_keyring_write_uses_private_file_fallback( + login_env: Any, monkeypatch: pytest.MonkeyPatch, memory_keyring: Any +): + _stub_connect_discovery(monkeypatch) + _queue_device(login_env) + device_login.start_connect_login(SERVER, "work") + state = device_login._read_state("connect", "work") + monkeypatch.setattr(sys.modules["keyring"], "set_password", Mock(side_effect=RuntimeError("locked"))) + tokens = {"access_token": "approved-secret", "refresh_token": "refresh-secret", "expires_at": None} + device_login._store_checkpoint(state, tokens) + assert device_login._read_state("connect", "work")["tokens"] == tokens + assert stat.S_IMODE(_state_path("connect", "work").stat().st_mode) == 0o600 + assert not memory_keyring + login_env.clock.advance(601) + assert device_login.finish_login("connect", "work")["status"] == "done" + assert device_login._store().get_by_name("work")["oauth_refresh_token"] == "refresh-secret" + assert len(login_env.http.instances) == 1 + + +@pytest.mark.parametrize("failure", ["missing", "locked", "invalid", "malformed"]) +def test_unreadable_pending_keyring_does_not_redeem_code_again( + login_env: Any, monkeypatch: pytest.MonkeyPatch, memory_keyring: Any, failure: str +): + _stub_connect_discovery(monkeypatch) + _queue_device(login_env) + device_login.start_connect_login(SERVER, "work") + state = device_login._read_state("connect", "work") + device_login._store_checkpoint( + state, {"access_token": "approved-secret", "refresh_token": None, "expires_at": None} + ) + if failure == "missing": + memory_keyring.clear() + elif failure == "locked": + monkeypatch.setattr(sys.modules["keyring"], "get_password", Mock(side_effect=RuntimeError("locked"))) + else: + memory_keyring[("rsconnect-python", device_login._checkpoint_key(state) + ":tokens")] = ( + "not-json" if failure == "malformed" else "null" + ) + with pytest.raises(RSConnectException, match="keyring"): + device_login.finish_login("connect", "work") + assert len(login_env.http.instances) == 1 + assert _state_path("connect", "work").exists() + assert "approved-secret" not in _state_path("connect", "work").read_text() + + +def test_pending_keyring_cleanup_failure_can_be_retried( + login_env: Any, monkeypatch: pytest.MonkeyPatch, memory_keyring: Any +): + _stub_connect_discovery(monkeypatch) + _queue_device(login_env) + device_login.start_connect_login(SERVER, "work") + state = device_login._read_state("connect", "work") + tokens = {"access_token": "approved-secret", "refresh_token": "refresh-secret", "expires_at": None} + device_login._store_checkpoint(state, tokens) + module = sys.modules["keyring"] + delete = module.delete_password + monkeypatch.setattr(module, "delete_password", Mock(side_effect=RuntimeError("locked"))) + with pytest.raises(RSConnectException, match="remove pending login tokens"): + device_login.finish_login("connect", "work") + assert _state_path("connect", "work").exists() + assert device_login._approved_tokens(device_login._read_state("connect", "work")) == tokens + + monkeypatch.setattr(module, "delete_password", delete) + assert device_login.finish_login("connect", "work")["status"] == "done" + assert not _state_path("connect", "work").exists() + assert not any("device-login:" in username for _, username in memory_keyring) + assert oauth.keyring_get_tokens(SERVER) == ("approved-secret", "refresh-secret") + assert len(login_env.http.instances) == 1 + + +def test_legacy_file_checkpoint_keeps_its_backend_when_keyring_becomes_available( + login_env: Any, monkeypatch: pytest.MonkeyPatch, memory_keyring: Any +): + _stub_connect_discovery(monkeypatch) + _queue_device(login_env) + device_login.start_connect_login(SERVER, "work") + state = device_login._read_state("connect", "work") + state["tokens"] = {"access_token": "old-access", "refresh_token": "old-refresh", "expires_at": None} + device_login._write_state(state) + rotated = {"access_token": "new-access", "refresh_token": "new-refresh", "expires_at": None} + device_login._store_checkpoint(state, rotated) + assert device_login._read_state("connect", "work")["tokens"] == rotated + assert not memory_keyring + assert device_login.finish_login("connect", "work")["status"] == "done" + assert oauth.keyring_get_tokens(SERVER) == ("new-access", "new-refresh") + assert not _state_path("connect", "work").exists() + + +def test_failed_pending_file_removal_restores_keyring_checkpoint( + login_env: Any, monkeypatch: pytest.MonkeyPatch, memory_keyring: Any +): + _stub_connect_discovery(monkeypatch) + _queue_device(login_env) + device_login.start_connect_login(SERVER, "work") + state = device_login._read_state("connect", "work") + tokens = {"access_token": "approved-secret", "refresh_token": "refresh-secret", "expires_at": None} + device_login._store_checkpoint(state, tokens) + unlink = device_login.os.unlink + monkeypatch.setattr(device_login.os, "unlink", Mock(side_effect=OSError("cannot remove state"))) + with pytest.raises(OSError, match="cannot remove state"): + device_login._remove_state(state) + assert device_login._approved_tokens(device_login._read_state("connect", "work")) == tokens + assert "approved-secret" not in _state_path("connect", "work").read_text() + monkeypatch.setattr(device_login.os, "unlink", unlink) + assert device_login.finish_login("connect", "work")["status"] == "done" + assert not _state_path("connect", "work").exists() + assert not any("device-login:" in username for _, username in memory_keyring) + + +@pytest.mark.parametrize("reference", [{"keyring": False}, {"keyring": 1}, {"keyring": True, "access_token": "secret"}]) +def test_pending_keyring_reference_is_validated(login_env: Any, monkeypatch: pytest.MonkeyPatch, reference: Any): + _stub_connect_discovery(monkeypatch) + _queue_device(login_env) + device_login.start_connect_login(SERVER, "work") + state = device_login._read_state("connect", "work") + state["tokens"] = reference + device_login._write_state(state) + with pytest.raises(RSConnectException, match="invalid"): + device_login.finish_login("connect", "work") + assert len(login_env.http.instances) == 1 + + @pytest.mark.parametrize( "expires_in", [float("inf"), float("-inf"), float("nan"), True, False, "invalid", 10**400], @@ -1324,10 +1510,19 @@ def test_missing_cloud_account_removes_checkpoint_for_corrected_start(login_env: assert device_login._read_state("cloud", "cloud-login")["account"] == "correct-team" +@pytest.mark.parametrize("use_keyring", [False, True]) def test_cloud_account_deadline_returns_pending_and_checkpoints_rotated_tokens( - login_env: Any, monkeypatch: pytest.MonkeyPatch + login_env: Any, monkeypatch: pytest.MonkeyPatch, request: Any, use_keyring: bool ): monkeypatch.setenv(connect_cloud.ENVIRONMENT_ENV_VAR, "production") + if use_keyring: + request.getfixturevalue("memory_keyring") + server_url = connect_cloud.urls().api + store = device_login._store() + store.set("cloud-login", server_url, connect_cloud_account_name="team", connect_cloud_account_id="old-team-id") + original_saved = store.get_by_name("cloud-login").copy() + if use_keyring: + connect_cloud.store_credentials_in_keyring(server_url, "cloud-login", "old-access", "old-refresh", None) _queue_device(login_env) device_login.start_cloud_login("team", "cloud-login") login_env.clock.advance(5) @@ -1366,18 +1561,35 @@ def get_account_by_name(self, account_name: str) -> dict[str, str]: monkeypatch.setattr(device_login.api, "ConnectCloudServer", CloudServer) monkeypatch.setattr(device_login, "_DeviceLoginCloudClient", CloudClient) - monkeypatch.setattr(connect_cloud, "store_credentials_in_keyring", lambda *args: False) + if not use_keyring: + monkeypatch.setattr(connect_cloud, "store_credentials_in_keyring", lambda *args: False) result = device_login.finish_login("cloud", "cloud-login", timeout=5) checkpoint = device_login._read_state("cloud", "cloud-login") assert result == {"status": "pending", "name": "cloud-login", "server": connect_cloud.urls().api} - assert checkpoint["tokens"]["access_token"] == "rotated-access" - assert checkpoint["tokens"]["refresh_token"] == "rotated-refresh" + assert device_login._approved_tokens(checkpoint)["access_token"] == "rotated-access" + assert device_login._approved_tokens(checkpoint)["refresh_token"] == "rotated-refresh" + assert device_login._store().get_by_name("cloud-login") == original_saved + if use_keyring: + assert "rotated-access" not in _state_path("cloud", "cloud-login").read_text() + assert "initial-access" not in _state_path("cloud", "cloud-login").read_text() + assert oauth.keyring_get_tokens(connect_cloud.keyring_key(server_url, "cloud-login")) == ( + "old-access", + "old-refresh", + ) assert clients[0].request_deadline == 10.0 completed = device_login.finish_login("cloud", "cloud-login", timeout=5) saved = device_login._store().get_by_name("cloud-login") assert completed["status"] == "done" - assert saved["connect_cloud_access_token"] == "rotated-access" - assert saved["connect_cloud_refresh_token"] == "rotated-refresh" + if use_keyring: + assert oauth.keyring_get_tokens(connect_cloud.keyring_key(server_url, "cloud-login")) == ( + "rotated-access", + "rotated-refresh", + ) + assert saved.get("connect_cloud_access_token") is None + assert not any("device-login:" in username for _, username in request.getfixturevalue("memory_keyring")) + else: + assert saved["connect_cloud_access_token"] == "rotated-access" + assert saved["connect_cloud_refresh_token"] == "rotated-refresh" From 0e72ea6e8c13c07f7b5eb4542c6f345392881150 Mon Sep 17 00:00:00 2001 From: Matt Conflitti Date: Thu, 15 Oct 2026 16:13:07 -0400 Subject: [PATCH 4/5] fix(security): close credential filtering gaps --- rsconnect/bundle.py | 20 +++--- tests/test_bundle.py | 165 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 171 insertions(+), 14 deletions(-) diff --git a/rsconnect/bundle.py b/rsconnect/bundle.py index 7ca82db8b..7969dc754 100644 --- a/rsconnect/bundle.py +++ b/rsconnect/bundle.py @@ -100,26 +100,26 @@ def _is_config_path( config_dir: Path | None = None, ) -> bool: candidate = Path(path) + if base_dir is not None and not candidate.is_absolute(): + candidate = Path(base_dir) / candidate if _config_dir_names.intersection(candidate.parts): return True if config_dir is None: if base_dir is None and not candidate.is_absolute(): return False config_dir = _resolved_config_dir() - if base_dir is not None and not candidate.is_absolute(): - candidate = Path(base_dir) / candidate - return _path_is_within(candidate.resolve(), config_dir) + resolved = candidate.resolve() + return bool(_config_dir_names.intersection(resolved.parts)) or _path_is_within(resolved, config_dir) def _ensure_content_root_is_safe(path: str | Path, config_dir: Path) -> None: content_root = Path(path) if content_root.is_file(): content_root = content_root.parent - content_root = content_root.resolve() - if _path_is_within(content_root, config_dir): + if _is_config_path(content_root, config_dir=config_dir): raise RSConnectException( - f"Cannot bundle content from '{content_root}' because it is inside the " - f"rsconnect-python configuration directory '{config_dir}'. " + f"Cannot bundle content from '{content_root.resolve()}' because it is inside the " + f"rsconnect-python configuration directory '{config_dir}' or a reserved '.rsconnect-python' directory. " "Choose a content directory outside the configuration directory." ) @@ -2119,8 +2119,12 @@ def write_notebook_manifest_json( :return: whether or not the environment file (requirements.txt, environment.yml, etc.) that goes along with the manifest exists. """ - extra_files = validate_extra_files(dirname(entry_point_file), extra_files) directory = dirname(entry_point_file) + config_dir = _resolved_config_dir() + _ensure_content_root_is_safe(directory, config_dir) + if _is_config_path(entry_point_file, config_dir=config_dir): + raise RSConnectException("A credential configuration file cannot be bundled as notebook content.") + extra_files = _filter_config_paths(validate_extra_files(directory, extra_files), directory, config_dir) file_name = basename(entry_point_file) manifest_path = join(directory, "manifest.json") diff --git a/tests/test_bundle.py b/tests/test_bundle.py index 50ce6f813..e1f744189 100644 --- a/tests/test_bundle.py +++ b/tests/test_bundle.py @@ -52,6 +52,7 @@ write_api_manifest_json, write_environment_file, write_manifest, + write_notebook_manifest_json, ) from rsconnect.shiny_express import escape_to_var_name from rsconnect.environment_node import NodeEnvironment @@ -3329,6 +3330,42 @@ def test_node_bundle_excludes_config_paths_and_explicit_extras(tmp_path, monkeyp assert b"SECRET" not in b"".join(members.values()) +def test_node_bundle_filters_reserved_config_symlinks_and_keeps_safe_symlinks(tmp_path, monkeypatch): + project = tmp_path / "project" + project.mkdir() + config_dir = tmp_path / "active-config" + config_dir.mkdir() + monkeypatch.setattr(rsconnect.bundle.metadata, "config_dirname", lambda: str(config_dir)) + + (project / "app.js").write_text("// app") + (project / "package.json").write_text("{}") + (project / "safe.txt").write_text("safe") + (project / ".rsconnect-python" / "data").mkdir(parents=True) + (project / ".rsconnect-python" / "data" / "servers.json").write_text("SECRET") + + reserved_alias = project / "reserved-alias" + credential_alias = project / "credential-alias.json" + safe_alias = project / "safe-alias.txt" + try: + reserved_alias.symlink_to(project / ".rsconnect-python", target_is_directory=True) + credential_alias.symlink_to(project / ".rsconnect-python" / "data" / "servers.json") + safe_alias.symlink_to("safe.txt") + except (NotImplementedError, OSError): + pytest.skip("symlinks are unavailable") + + with make_nodejs_bundle( + str(project), "app.js", _make_node_env(), ["reserved-alias/data/servers.json"], [] + ) as bundle_file: + members, manifest = _bundle_snapshot(bundle_file) + + expected_files = {"app.js", "package.json", "safe.txt", "safe-alias.txt"} + assert set(manifest["files"]) == expected_files + assert set(members) == expected_files | {"manifest.json"} + assert "credential-alias.json" not in manifest["files"] + assert "reserved-alias/data/servers.json" not in manifest["files"] + assert b"SECRET" not in b"".join(members.values()) + + def test_manifest_bundle_removes_config_entries_only(tmp_path, monkeypatch): project = tmp_path / "project" project.mkdir() @@ -3443,7 +3480,7 @@ def test_notebook_html_bundle_rejects_config_paths_before_conversion(tmp_path, m elif path_kind == "reserved_path": config_dir = tmp_path / "user-config" notebook = project / ".rsconnect-python" / "private.ipynb" - error_match = "credential configuration file" + error_match = "inside the rsconnect-python configuration directory" else: config_dir = tmp_path / "user-config" notebook = project / "private.ipynb" @@ -3515,15 +3552,29 @@ def test_standalone_quarto_bundle_filters_config_extras(tmp_path, monkeypatch): assert b"SECRET" not in b"".join(members.values()) -def test_bundle_rejects_config_directory_as_content_root(tmp_path, monkeypatch): +@pytest.mark.parametrize("root_kind", ["active_config", "active_alias", "reserved_directory", "reserved_alias"]) +def test_bundle_rejects_config_directory_as_content_root(tmp_path, monkeypatch, root_kind): config_dir = tmp_path / "custom-config" config_dir.mkdir() monkeypatch.setattr(rsconnect.bundle.metadata, "config_dirname", lambda: str(config_dir)) - content_root = tmp_path / "config-alias" - try: - content_root.symlink_to(config_dir, target_is_directory=True) - except (NotImplementedError, OSError): + if root_kind == "active_config": content_root = config_dir + elif root_kind == "active_alias": + content_root = tmp_path / "config-alias" + try: + content_root.symlink_to(config_dir, target_is_directory=True) + except (NotImplementedError, OSError): + pytest.skip("symlinks are unavailable") + else: + reserved_dir = tmp_path / "project" / ".rsconnect-python" + reserved_dir.mkdir(parents=True) + content_root = reserved_dir + if root_kind == "reserved_alias": + content_root = tmp_path / "config-alias" + try: + content_root.symlink_to(reserved_dir, target_is_directory=True) + except (NotImplementedError, OSError): + pytest.skip("symlinks are unavailable") with pytest.raises(RSConnectException, match="inside the rsconnect-python configuration directory"): make_nodejs_bundle(str(content_root), "app.js", _make_node_env(), [], []) @@ -3572,6 +3623,108 @@ def test_write_api_manifest_excludes_current_home_config_extra(tmp_path, monkeyp assert b"SECRET" not in b"".join(members.values()) +def test_write_notebook_manifest_filters_protected_extras_before_hashing(tmp_path, monkeypatch): + project = tmp_path / "project" + project.mkdir() + config_dir = project / "custom-state" + config_dir.mkdir() + monkeypatch.setattr(rsconnect.bundle.metadata, "config_dirname", lambda: str(config_dir)) + + notebook = project / "notebook.ipynb" + notebook.write_text("{}") + safe_extra = project / "data.csv" + safe_extra.write_text("safe") + environment = _make_test_environment() + requirements_file = project / environment.filename + requirements_file.write_text(environment.contents) + + (config_dir / "servers.json").write_text("ACTIVE SECRET") + reserved_file = project / ".rsconnect-python" / "data" / "servers.json" + reserved_file.parent.mkdir(parents=True) + reserved_file.write_text("RESERVED SECRET") + reserved_alias = project / "reserved-alias" + alias_extra = "reserved-alias/data/servers.json" + try: + reserved_alias.symlink_to(reserved_file.parents[1], target_is_directory=True) + except (NotImplementedError, OSError): + alias_extra = "" + + original_checksum = rsconnect.bundle.file_checksum + hashed_paths = [] + + def record_checksum(path): + hashed_paths.append(Path(path).resolve()) + return original_checksum(path) + + monkeypatch.setattr(rsconnect.bundle, "file_checksum", record_checksum) + extra_files = [ + str(config_dir / "servers.json"), + str(reserved_file), + ] + if alias_extra: + extra_files.append(str(project / alias_extra)) + extra_files.append(str(safe_extra)) + + assert write_notebook_manifest_json( + str(notebook), + environment, + AppModes.JUPYTER_NOTEBOOK, + extra_files, + hide_all_input=False, + hide_tagged_input=False, + ) + + manifest = json.loads((project / "manifest.json").read_text()) + expected_files = { + "notebook.ipynb": {"checksum": original_checksum(notebook)}, + environment.filename: {"checksum": original_checksum(requirements_file)}, + "data.csv": {"checksum": original_checksum(safe_extra)}, + } + assert manifest["files"] == expected_files + assert hashed_paths == [notebook.resolve(), safe_extra.resolve()] + + +@pytest.mark.parametrize("entrypoint_kind", ["reserved_root", "active_root", "symlink_alias"]) +def test_write_notebook_manifest_rejects_protected_entrypoint_before_io(tmp_path, monkeypatch, entrypoint_kind): + project = tmp_path / "project" + project.mkdir() + config_dir = tmp_path / "active-config" + config_dir.mkdir() + monkeypatch.setattr(rsconnect.bundle.metadata, "config_dirname", lambda: str(config_dir)) + + if entrypoint_kind in ("reserved_root", "active_root"): + entrypoint_root = project / ".rsconnect-python" if entrypoint_kind == "reserved_root" else config_dir + notebook = entrypoint_root / "notebook.ipynb" + notebook.parent.mkdir(parents=True, exist_ok=True) + notebook.write_text("{}") + error_match = "inside the rsconnect-python configuration directory" + else: + protected_notebook = project / ".rsconnect-python" / "notebook.ipynb" + protected_notebook.parent.mkdir() + protected_notebook.write_text("{}") + notebook = project / "notebook.ipynb" + try: + notebook.symlink_to(protected_notebook) + except (NotImplementedError, OSError): + pytest.skip("symlinks are unavailable") + error_match = "A credential configuration file cannot be bundled" + + calls = [] + monkeypatch.setattr(rsconnect.bundle, "file_checksum", lambda path: calls.append(("checksum", path))) + monkeypatch.setattr(rsconnect.bundle, "write_manifest_json", lambda *args: calls.append(("write", args))) + + with pytest.raises(RSConnectException, match=error_match): + write_notebook_manifest_json( + str(notebook), + _make_test_environment(), + AppModes.JUPYTER_NOTEBOOK, + [], + hide_all_input=False, + hide_tagged_input=False, + ) + assert calls == [] + + class TestNodeJSManifest: def test_manifest_structure(self): env = _make_node_env() From 82e7867a7729e559f689c440f687eff7df002b22 Mon Sep 17 00:00:00 2001 From: Matt Conflitti Date: Fri, 16 Oct 2026 10:39:04 -0400 Subject: [PATCH 5/5] test: preserve requirements fixture bytes on Windows --- tests/test_bundle.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_bundle.py b/tests/test_bundle.py index e1f744189..e6eaf980f 100644 --- a/tests/test_bundle.py +++ b/tests/test_bundle.py @@ -3593,7 +3593,7 @@ def test_write_api_manifest_excludes_current_home_config_extra(tmp_path, monkeyp pending_state.write_text("SECRET") (project / "app.py").write_text("app = object()") (project / "data.csv").write_text("safe") - (project / "requirements.txt").write_text("flask==3.0.0\n") + (project / "requirements.txt").write_bytes(b"flask==3.0.0\n") extra_files = [str(pending_state), str(project / "data.csv")] environment = _make_test_environment() @@ -3636,7 +3636,7 @@ def test_write_notebook_manifest_filters_protected_extras_before_hashing(tmp_pat safe_extra.write_text("safe") environment = _make_test_environment() requirements_file = project / environment.filename - requirements_file.write_text(environment.contents) + requirements_file.write_bytes(to_bytes(environment.contents)) (config_dir / "servers.json").write_text("ACTIVE SECRET") reserved_file = project / ".rsconnect-python" / "data" / "servers.json"