From ecf25deee444e5454858b5d749b6f5957f1db105 Mon Sep 17 00:00:00 2001 From: Amy Lin Date: Wed, 29 Jul 2026 11:19:46 -0500 Subject: [PATCH 1/8] feat(package-vulnerability-scanner): scan as the signed-in viewer Connect API calls now run as the signed-in viewer via their per-request session token, instead of the deploying publisher, so each person sees the content they published and their own name. Requires a Connect Visitor API Key integration (added to requiredFeatures). Every Connect call also moves off the event loop (asyncio.to_thread) and gets a bounded timeout with retry-on-transient-failure: the SDK sets no request timeout of its own, so an unresponsive server would otherwise hang the calling task indefinitely. A timeout is retried the same as a 5xx before giving up. Endpoint shapes are unchanged in this PR (packages is still one request per content item); that rework is a separate PR. --- .../package-vulnerability-scanner.yml | 8 + .../package-vulnerability-scanner/main.py | 147 ++++++- .../manifest.json | 7 +- .../pyproject.toml | 6 + .../test_main.py | 388 ++++++++++++++++++ 5 files changed, 541 insertions(+), 15 deletions(-) create mode 100644 extensions/package-vulnerability-scanner/test_main.py diff --git a/.github/workflows/package-vulnerability-scanner.yml b/.github/workflows/package-vulnerability-scanner.yml index f4c760c7..d5f1aa73 100644 --- a/.github/workflows/package-vulnerability-scanner.yml +++ b/.github/workflows/package-vulnerability-scanner.yml @@ -36,6 +36,14 @@ jobs: - run: npm ci - run: npm run build + # Run the Python backend tests. + - uses: astral-sh/setup-uv@v9 + with: + pyproject-file: ./extensions/${{ env.EXTENSION_NAME }}/pyproject.toml + + - name: Run backend tests + run: uv run pytest + # Now that the extension is built we need to upload an artifact to pass # to the package-extension action that contains the files we want to be # included in the extension diff --git a/extensions/package-vulnerability-scanner/main.py b/extensions/package-vulnerability-scanner/main.py index f7982f46..cd59a14e 100644 --- a/extensions/package-vulnerability-scanner/main.py +++ b/extensions/package-vulnerability-scanner/main.py @@ -1,16 +1,70 @@ import asyncio import json +import os import httpx -from fastapi import FastAPI, HTTPException +from fastapi import FastAPI, HTTPException, Request from fastapi.staticfiles import StaticFiles from posit import connect +from posit.connect.errors import ClientError from pydantic import BaseModel app = FastAPI() client = connect.Client() + +# Connect sets one of these to "CONNECT" for deployed content. Check both because +# a missed detection here would fall back to the owner's client for an anonymous +# viewer (see get_visitor_client), so err toward "on Connect". +def _running_on_connect() -> bool: + return "CONNECT" in (os.getenv("POSIT_PRODUCT"), os.getenv("RSTUDIO_PRODUCT")) + + +# Build a Connect client scoped to the signed-in viewer by exchanging their +# per-request session token, so API calls return that viewer's own content and +# identity. +def get_visitor_client(request: Request) -> connect.Client: + token = request.headers.get("posit-connect-user-session-token") + if token: + return client.with_user_session_token(token) + # On Connect, no token means the viewer's session can't be read (they aren't + # signed in, or OAuth integrations are disabled on the server). Falling back to + # the owner's default client would scan the owner's content as if it were the + # viewer's, so require the visitor session instead. Off Connect (local + # development), the default client is the intended one. + if _running_on_connect(): + raise HTTPException( + status_code=424, + detail="Couldn't read your Connect session, so the scan can't run as " + "you. Make sure you're signed in to Connect. If you are, your " + "administrator may need to enable OAuth integrations on this server.", + ) + return client + + +# Connect raises ClientError code 212 when the Visitor API Key integration that +# viewer-scoped calls depend on has not been added to this content. Surface it as +# a distinct 424 so the UI can show setup instructions. +def _setup_required(exc: ClientError) -> HTTPException: + if exc.error_code == 212: + return HTTPException( + status_code=424, + detail="In the content settings, on the Access tab, add a Connect " + "Visitor API Key integration under Integrations, to scan your content.", + ) + return HTTPException( + status_code=502, detail=f"Connect API error: {exc.error_message}" + ) + + +# Log the underlying error for the publisher's server logs, but show the viewer a +# short, curated message rather than a raw SDK/httpx string. +def _upstream_error(message: str, exc: Exception) -> HTTPException: + print(f"{message} {exc}") + return HTTPException(status_code=502, detail=message) + + # The public Package Manager is always current. To scan against your own # instance instead, point this at "https://your-ppm/__api__/filter/packages". PPM_URL = "https://packagemanager.posit.co/__api__/filter/packages" @@ -19,25 +73,84 @@ # deployment doesn't produce an unwieldy payload. PPM_QUERY_CHUNK = 100 +# How many times to try a Connect API call before giving up, so a transient 5xx +# (e.g. a 504 while Connect is under load) doesn't fail the whole scan. +CONTENT_MAX_ATTEMPTS = 3 + +# Give up on a single Connect API call. The SDK sets no request timeout of its +# own, so an unresponsive Connect server would otherwise hang the calling task +# indefinitely. A timeout is treated the same as a transient 5xx: retried up to +# CONTENT_MAX_ATTEMPTS before giving up. This bounds how long the app waits, not +# how long the underlying thread runs: asyncio.to_thread can't interrupt a call +# already in flight, so the abandoned thread still runs until Connect (or the OS) +# eventually gives up on its end. +CONNECT_API_TIMEOUT_SECONDS = 30 + + +# A 5xx from Connect or its gateway (e.g. a 504 while it is under scan load), or a +# call that timed out outright, is worth retrying; a 4xx or a missing item is not. +# A ClientError carries http_status; a lower-level requests error carries a +# response with status_code. +def _is_transient(exc: Exception) -> bool: + if isinstance(exc, asyncio.TimeoutError): + return True + status = getattr(exc, "http_status", None) or getattr( + getattr(exc, "response", None), "status_code", None + ) + return status is not None and status >= 500 + + +# Run a synchronous SDK call off the event loop, retrying a transient 5xx (e.g. a +# 504 while Connect is under load) or an outright timeout with a short backoff +# before giving up. +async def _fetch_with_retry(fn): + for attempt in range(CONTENT_MAX_ATTEMPTS): + try: + return await asyncio.wait_for( + asyncio.to_thread(fn), CONNECT_API_TIMEOUT_SECONDS + ) + except Exception as e: + if _is_transient(e) and attempt < CONTENT_MAX_ATTEMPTS - 1: + await asyncio.sleep(0.5 * (attempt + 1)) + continue + raise + @app.get("/api/content") -async def search_content(show_all: bool = False): - if show_all: - return client.content.find() - return client.me.content.find() +async def search_content(request: Request, show_all: bool = False): + try: + visitor = get_visitor_client(request) + if show_all: + return await _fetch_with_retry(lambda: visitor.content.find()) + return await _fetch_with_retry(lambda: visitor.me.content.find()) + except ClientError as e: + raise _setup_required(e) + except HTTPException: + raise # e.g. the 424 from get_visitor_client; don't re-wrap as a 502 + except Exception as e: + raise _upstream_error("Couldn't load your content from Connect.", e) @app.get("/api/packages/{guid}") -async def get_packages(guid: str): +async def get_packages(guid: str, request: Request): try: - content = client.content.get(guid) - packages = list(content.packages) - return packages - except Exception as e: + visitor = get_visitor_client(request) + content = await _fetch_with_retry(lambda: visitor.content.get(guid)) + return list(content.packages) + except ClientError as e: + if e.error_code == 212: + raise _setup_required(e) + # Preserve the existing "not found" shape for other Connect errors (e.g. + # an invalid guid), rather than folding every ClientError into the + # missing-integration case. raise HTTPException( status_code=404, - detail=f"Content not found or error fetching packages: {str(e)}", + detail=f"Content not found or error fetching packages: {e.error_message}", ) + except HTTPException: + raise # e.g. the 424 from get_visitor_client; don't re-wrap as a 502 + except Exception as e: + raise _upstream_error("Couldn't fetch packages from Connect.", e) # The installed packages to scan, as "name==version" specifiers grouped by the @@ -89,8 +202,16 @@ async def fetch_repo_vulns(repo, specifiers): @app.get("/api/user") -async def get_current_user(): - return client.me +async def get_current_user(request: Request): + try: + visitor = get_visitor_client(request) + return await _fetch_with_retry(lambda: visitor.me) + except ClientError as e: + raise _setup_required(e) + except HTTPException: + raise # e.g. the 424 from get_visitor_client; don't re-wrap as a 502 + except Exception as e: + raise _upstream_error("Couldn't load your account from Connect.", e) app.mount("/", StaticFiles(directory="dist", html=True), name="static") diff --git a/extensions/package-vulnerability-scanner/manifest.json b/extensions/package-vulnerability-scanner/manifest.json index 55466caa..9fb037df 100644 --- a/extensions/package-vulnerability-scanner/manifest.json +++ b/extensions/package-vulnerability-scanner/manifest.json @@ -30,7 +30,7 @@ "checksum": "07435c1b16a3ab78d62c07501cc2e32d" }, "main.py": { - "checksum": "b4d98ffdae1449709c3bc679f49262d6" + "checksum": "331f2474111a6280734062de89152bb6" }, "requirements.txt": { "checksum": "9fc5cc5fb559eded1f323793b73e82c5" @@ -43,7 +43,10 @@ "homepage": "https://github.com/posit-dev/connect-extensions/tree/main/extensions/package-vulnerability-scanner", "category": "extension", "minimumConnectVersion": "2025.04.0", - "requiredFeatures": ["API Publishing"], + "requiredFeatures": [ + "API Publishing", + "OAuth Integrations" + ], "version": "3.0.6" } } diff --git a/extensions/package-vulnerability-scanner/pyproject.toml b/extensions/package-vulnerability-scanner/pyproject.toml index 115e3da2..f2eca760 100644 --- a/extensions/package-vulnerability-scanner/pyproject.toml +++ b/extensions/package-vulnerability-scanner/pyproject.toml @@ -10,3 +10,9 @@ dependencies = [ "httpx>=0.28.1", "posit-sdk>=0.10.0", ] + +# Test-only dependencies; not bundled into the extension (requirements.txt is). +[dependency-groups] +dev = [ + "pytest>=8", +] diff --git a/extensions/package-vulnerability-scanner/test_main.py b/extensions/package-vulnerability-scanner/test_main.py new file mode 100644 index 00000000..f7dd936f --- /dev/null +++ b/extensions/package-vulnerability-scanner/test_main.py @@ -0,0 +1,388 @@ +# Backend tests for the scanner API. +# +# main.py builds a Connect client at import time, which reads CONNECT_SERVER and +# CONNECT_API_KEY from the environment, so set placeholders before importing it. +# The client makes no network call at construction; every test replaces `client` +# or `get_visitor_client` with a mock, so no request ever leaves the process. +import asyncio +import os +from unittest.mock import MagicMock, PropertyMock + +import pytest +from fastapi import HTTPException +from fastapi.testclient import TestClient +from posit.connect.errors import ClientError + +os.environ.setdefault("CONNECT_SERVER", "https://connect.example.com") +os.environ.setdefault("CONNECT_API_KEY", "placeholder-not-used") + +import main # noqa: E402 + + +def make_client_error(code, http_status=400, message="Server error"): + return ClientError(code, message, http_status, "Connect returned an error") + + +async def noop_sleep(*args, **kwargs): + pass + + +@pytest.fixture +def api(): + return TestClient(main.app) + + +# --- _is_transient --------------------------------------------------------- + + +def test_is_transient_true_for_5xx_client_error(): + assert main._is_transient(make_client_error(999, http_status=500)) is True + assert main._is_transient(make_client_error(999, http_status=503)) is True + + +def test_is_transient_false_for_4xx_client_error(): + assert main._is_transient(make_client_error(212, http_status=404)) is False + + +def test_is_transient_true_for_response_5xx(): + err = Exception("network") + err.response = MagicMock(status_code=502) + assert main._is_transient(err) is True + + +def test_is_transient_false_when_no_status(): + assert main._is_transient(ValueError("nope")) is False + + +def test_is_transient_true_for_timeout(): + # A call that never responds is worth retrying just like a 5xx: it may be a + # transient blip rather than a truly dead server. + assert main._is_transient(asyncio.TimeoutError()) is True + + +# --- _setup_required ------------------------------------------------------- + + +def test_setup_required_maps_212_to_424(): + exc = main._setup_required(make_client_error(212)) + assert isinstance(exc, HTTPException) + assert exc.status_code == 424 + assert "Visitor API Key" in exc.detail + + +def test_setup_required_maps_other_to_502(): + # A non-212 error (e.g. a 403 permission denial when the content owner lacks + # publisher access) surfaces the human-readable Connect message, not raw JSON. + exc = main._setup_required( + make_client_error(5, message="You do not have permission to access this content") + ) + assert exc.status_code == 502 + assert ( + exc.detail + == "Connect API error: You do not have permission to access this content" + ) + + +# --- _fetch_with_retry ----------------------------------------------------- + + +def test_fetch_with_retry_returns_value(): + calls = [] + + def fn(): + calls.append(1) + return "ok" + + assert asyncio.run(main._fetch_with_retry(fn)) == "ok" + assert len(calls) == 1 + + +def test_fetch_with_retry_retries_transient_then_succeeds(monkeypatch): + monkeypatch.setattr(main.asyncio, "sleep", noop_sleep) + calls = [] + + def fn(): + calls.append(1) + if len(calls) == 1: + raise make_client_error(999, http_status=503) + return "ok" + + assert asyncio.run(main._fetch_with_retry(fn)) == "ok" + assert len(calls) == 2 + + +def test_fetch_with_retry_gives_up_after_max_attempts(monkeypatch): + monkeypatch.setattr(main.asyncio, "sleep", noop_sleep) + calls = [] + + def fn(): + calls.append(1) + raise make_client_error(999, http_status=500) + + with pytest.raises(ClientError): + asyncio.run(main._fetch_with_retry(fn)) + assert len(calls) == main.CONTENT_MAX_ATTEMPTS + + +def test_fetch_with_retry_does_not_retry_non_transient(): + calls = [] + + def fn(): + calls.append(1) + raise ValueError("permanent") + + with pytest.raises(ValueError): + asyncio.run(main._fetch_with_retry(fn)) + assert len(calls) == 1 + + +def test_fetch_with_retry_retries_a_timeout_then_succeeds(monkeypatch): + monkeypatch.setattr(main.asyncio, "sleep", noop_sleep) + monkeypatch.setattr(main, "CONNECT_API_TIMEOUT_SECONDS", 0.01) + calls = [] + + def fn(): + calls.append(1) + if len(calls) == 1: + import time + + time.sleep(0.05) # longer than the patched timeout: this call times out + return "ok" + + assert asyncio.run(main._fetch_with_retry(fn)) == "ok" + assert len(calls) == 2 + + +def test_fetch_with_retry_gives_up_after_repeated_timeouts(monkeypatch): + monkeypatch.setattr(main.asyncio, "sleep", noop_sleep) + monkeypatch.setattr(main, "CONNECT_API_TIMEOUT_SECONDS", 0.01) + + def fn(): + import time + + time.sleep(0.05) + return "ok" + + with pytest.raises(asyncio.TimeoutError): + asyncio.run(main._fetch_with_retry(fn)) + + +# --- get_visitor_client ---------------------------------------------------- + + +def test_get_visitor_client_uses_token_when_present(monkeypatch): + mock_client = MagicMock() + monkeypatch.setattr(main, "client", mock_client) + request = MagicMock() + request.headers = {"posit-connect-user-session-token": "tok-123"} + + result = main.get_visitor_client(request) + + mock_client.with_user_session_token.assert_called_once_with("tok-123") + assert result is mock_client.with_user_session_token.return_value + + +def test_get_visitor_client_falls_back_without_token(monkeypatch): + # Off Connect (local development), no token falls back to the default client. + monkeypatch.delenv("POSIT_PRODUCT", raising=False) + monkeypatch.delenv("RSTUDIO_PRODUCT", raising=False) + mock_client = MagicMock() + monkeypatch.setattr(main, "client", mock_client) + request = MagicMock() + request.headers = {} + + assert main.get_visitor_client(request) is mock_client + mock_client.with_user_session_token.assert_not_called() + + +def test_get_visitor_client_falls_back_on_empty_token(monkeypatch): + # An empty header value is falsy, so off Connect it must fall back rather than + # attempt an exchange with a blank token. + monkeypatch.delenv("POSIT_PRODUCT", raising=False) + monkeypatch.delenv("RSTUDIO_PRODUCT", raising=False) + mock_client = MagicMock() + monkeypatch.setattr(main, "client", mock_client) + request = MagicMock() + request.headers = {"posit-connect-user-session-token": ""} + + assert main.get_visitor_client(request) is mock_client + mock_client.with_user_session_token.assert_not_called() + + +def test_get_visitor_client_requires_session_on_connect(monkeypatch): + # On Connect with no session token, don't fall back to the owner's client; + # require the visitor session so an anonymous viewer can't read the owner's + # content. + monkeypatch.setenv("POSIT_PRODUCT", "CONNECT") + mock_client = MagicMock() + monkeypatch.setattr(main, "client", mock_client) + request = MagicMock() + request.headers = {} + + with pytest.raises(HTTPException) as excinfo: + main.get_visitor_client(request) + assert excinfo.value.status_code == 424 + mock_client.with_user_session_token.assert_not_called() + + +# --- /api/user ------------------------------------------------------------- + + +def test_user_endpoint_ok(monkeypatch, api): + visitor = MagicMock() + visitor.me = {"guid": "u1", "username": "alice"} + monkeypatch.setattr(main, "get_visitor_client", lambda request: visitor) + + resp = api.get("/api/user") + + assert resp.status_code == 200 + assert resp.json()["username"] == "alice" + + +def test_user_endpoint_setup_required(monkeypatch, api): + # A missing Visitor API Key integration raises ClientError 212 while building + # the visitor client; the handler must turn it into a 424 for the setup screen. + def raise_setup_error(request): + raise make_client_error(212) + + monkeypatch.setattr(main, "get_visitor_client", raise_setup_error) + + resp = api.get("/api/user") + + assert resp.status_code == 424 + assert "Visitor API Key" in resp.json()["detail"] + + +def test_user_endpoint_other_client_error_is_502(monkeypatch, api): + def raise_client_error(request): + raise make_client_error(5) + + monkeypatch.setattr(main, "get_visitor_client", raise_client_error) + + assert api.get("/api/user").status_code == 502 + + +def test_user_endpoint_non_client_error_is_502(monkeypatch, api): + # A non-Connect failure (e.g. the server is unreachable) returns a descriptive + # 502 rather than a bare 500. + visitor = MagicMock() + type(visitor).me = PropertyMock(side_effect=RuntimeError("network down")) + monkeypatch.setattr(main, "get_visitor_client", lambda request: visitor) + + resp = api.get("/api/user") + + assert resp.status_code == 502 + assert resp.json()["detail"] == "Couldn't load your account from Connect." + + +# --- /api/content ---------------------------------------------------------- + + +def test_content_endpoint_setup_required(monkeypatch, api): + def raise_setup_error(request): + raise make_client_error(212) + + monkeypatch.setattr(main, "get_visitor_client", raise_setup_error) + + assert api.get("/api/content").status_code == 424 + + +def test_content_endpoint_requires_session_on_connect(monkeypatch, api): + # On Connect with no session token, the real get_visitor_client raises a 424, + # which must reach the client rather than being re-wrapped as a 502 by the + # endpoint's broad exception handler. + monkeypatch.setenv("POSIT_PRODUCT", "CONNECT") + + assert api.get("/api/content").status_code == 424 + + +def test_content_endpoint_returns_my_content_by_default(monkeypatch, api): + visitor = MagicMock() + visitor.me.content.find.return_value = [{"guid": "mine"}] + visitor.content.find.return_value = [{"guid": "all"}] + monkeypatch.setattr(main, "get_visitor_client", lambda request: visitor) + + resp = api.get("/api/content") + + assert resp.status_code == 200 + assert resp.json() == [{"guid": "mine"}] + visitor.me.content.find.assert_called_once() + visitor.content.find.assert_not_called() + + +def test_content_endpoint_show_all_returns_all_content(monkeypatch, api): + visitor = MagicMock() + visitor.me.content.find.return_value = [{"guid": "mine"}] + visitor.content.find.return_value = [{"guid": "all"}] + monkeypatch.setattr(main, "get_visitor_client", lambda request: visitor) + + resp = api.get("/api/content", params={"show_all": "true"}) + + assert resp.status_code == 200 + assert resp.json() == [{"guid": "all"}] + visitor.content.find.assert_called_once() + visitor.me.content.find.assert_not_called() + + +def test_content_endpoint_non_client_error_is_502(monkeypatch, api): + # An infrastructure failure (not a Connect ClientError, e.g. the server is + # unreachable) returns a descriptive 502 rather than a bare 500. + visitor = MagicMock() + visitor.me.content.find.side_effect = RuntimeError("network down") + monkeypatch.setattr(main, "get_visitor_client", lambda request: visitor) + + resp = api.get("/api/content") + + assert resp.status_code == 502 + assert resp.json()["detail"] == "Couldn't load your content from Connect." + + +# --- /api/packages/{guid} --------------------------------------------------- + + +def test_packages_endpoint_ok(monkeypatch, api): + visitor = MagicMock() + visitor.content.get.return_value.packages = [{"name": "requests"}] + monkeypatch.setattr(main, "get_visitor_client", lambda request: visitor) + + resp = api.get("/api/packages/g1") + + assert resp.status_code == 200 + assert resp.json() == [{"name": "requests"}] + visitor.content.get.assert_called_once_with("g1") + + +def test_packages_endpoint_setup_required(monkeypatch, api): + def raise_setup_error(request): + raise make_client_error(212) + + monkeypatch.setattr(main, "get_visitor_client", raise_setup_error) + + resp = api.get("/api/packages/g1") + + assert resp.status_code == 424 + assert "Visitor API Key" in resp.json()["detail"] + + +def test_packages_endpoint_other_client_error_is_404(monkeypatch, api): + # An invalid guid, or any other Connect error, keeps the existing "not found" + # shape rather than being folded into the missing-integration case. + visitor = MagicMock() + visitor.content.get.side_effect = make_client_error(5, message="No such content") + monkeypatch.setattr(main, "get_visitor_client", lambda request: visitor) + + resp = api.get("/api/packages/g1") + + assert resp.status_code == 404 + assert "No such content" in resp.json()["detail"] + + +def test_packages_endpoint_non_client_error_is_502(monkeypatch, api): + visitor = MagicMock() + visitor.content.get.side_effect = RuntimeError("network down") + monkeypatch.setattr(main, "get_visitor_client", lambda request: visitor) + + resp = api.get("/api/packages/g1") + + assert resp.status_code == 502 + assert resp.json()["detail"] == "Couldn't fetch packages from Connect." From ff0e564043806392dbf73fcfc7283916c70116d4 Mon Sep 17 00:00:00 2001 From: Amy Lin Date: Wed, 29 Jul 2026 12:45:52 -0500 Subject: [PATCH 2/8] ci(package-vulnerability-scanner): pin setup-uv to the exact release tag astral-sh/setup-uv doesn't publish a bare "v9" major-version alias (only "v9.0.0" exists), so @v9 failed to resolve and every job needing it never started. --- .github/workflows/package-vulnerability-scanner.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/package-vulnerability-scanner.yml b/.github/workflows/package-vulnerability-scanner.yml index d5f1aa73..6da7b970 100644 --- a/.github/workflows/package-vulnerability-scanner.yml +++ b/.github/workflows/package-vulnerability-scanner.yml @@ -37,7 +37,7 @@ jobs: - run: npm run build # Run the Python backend tests. - - uses: astral-sh/setup-uv@v9 + - uses: astral-sh/setup-uv@v9.0.0 with: pyproject-file: ./extensions/${{ env.EXTENSION_NAME }}/pyproject.toml From 25145ac463f5d2980ceccf8cec43e518974fbddc Mon Sep 17 00:00:00 2001 From: Amy Lin Date: Fri, 31 Jul 2026 20:22:32 -0500 Subject: [PATCH 3/8] fix(package-vulnerability-scanner): bound the packages iteration by the retry wrapper too --- extensions/package-vulnerability-scanner/main.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/extensions/package-vulnerability-scanner/main.py b/extensions/package-vulnerability-scanner/main.py index cd59a14e..c2242143 100644 --- a/extensions/package-vulnerability-scanner/main.py +++ b/extensions/package-vulnerability-scanner/main.py @@ -135,8 +135,11 @@ async def search_content(request: Request, show_all: bool = False): async def get_packages(guid: str, request: Request): try: visitor = get_visitor_client(request) - content = await _fetch_with_retry(lambda: visitor.content.get(guid)) - return list(content.packages) + # Iterating .packages makes its own blocking call, so it has to run + # inside the same retried thread as the content fetch, not after it. + return await _fetch_with_retry( + lambda: list(visitor.content.get(guid).packages) + ) except ClientError as e: if e.error_code == 212: raise _setup_required(e) From 79f9a69d10f598d469898d25107f8cded15589bf Mon Sep 17 00:00:00 2001 From: Amy Lin Date: Fri, 31 Jul 2026 20:46:13 -0500 Subject: [PATCH 4/8] fix(package-vulnerability-scanner): run the session-token exchange off the event loop, preserve 4xx status codes --- .../package-vulnerability-scanner/main.py | 21 ++++++-- .../test_main.py | 53 ++++++++++++++++--- 2 files changed, 63 insertions(+), 11 deletions(-) diff --git a/extensions/package-vulnerability-scanner/main.py b/extensions/package-vulnerability-scanner/main.py index c2242143..375ec0c3 100644 --- a/extensions/package-vulnerability-scanner/main.py +++ b/extensions/package-vulnerability-scanner/main.py @@ -53,6 +53,15 @@ def _setup_required(exc: ClientError) -> HTTPException: detail="In the content settings, on the Access tab, add a Connect " "Visitor API Key integration under Integrations, to scan your content.", ) + # A 4xx here (e.g. a 403 permission denial, or a 404 for a bad guid) is a + # legitimate client error, not an upstream failure; pass its real status + # through instead of folding it into a 502. A 5xx reaching this point has + # already exhausted _fetch_with_retry's retries, so 502 is the right call. + if exc.http_status is not None and 400 <= exc.http_status < 500: + return HTTPException( + status_code=exc.http_status, + detail=f"Connect API error: {exc.error_message}", + ) return HTTPException( status_code=502, detail=f"Connect API error: {exc.error_message}" ) @@ -119,7 +128,9 @@ async def _fetch_with_retry(fn): @app.get("/api/content") async def search_content(request: Request, show_all: bool = False): try: - visitor = get_visitor_client(request) + # A session-token exchange makes a real Connect API call, so it has to + # run off the event loop like every other blocking SDK call here. + visitor = await _fetch_with_retry(lambda: get_visitor_client(request)) if show_all: return await _fetch_with_retry(lambda: visitor.content.find()) return await _fetch_with_retry(lambda: visitor.me.content.find()) @@ -134,7 +145,9 @@ async def search_content(request: Request, show_all: bool = False): @app.get("/api/packages/{guid}") async def get_packages(guid: str, request: Request): try: - visitor = get_visitor_client(request) + # A session-token exchange makes a real Connect API call, so it has to + # run off the event loop like every other blocking SDK call here. + visitor = await _fetch_with_retry(lambda: get_visitor_client(request)) # Iterating .packages makes its own blocking call, so it has to run # inside the same retried thread as the content fetch, not after it. return await _fetch_with_retry( @@ -207,7 +220,9 @@ async def fetch_repo_vulns(repo, specifiers): @app.get("/api/user") async def get_current_user(request: Request): try: - visitor = get_visitor_client(request) + # A session-token exchange makes a real Connect API call, so it has to + # run off the event loop like every other blocking SDK call here. + visitor = await _fetch_with_retry(lambda: get_visitor_client(request)) return await _fetch_with_retry(lambda: visitor.me) except ClientError as e: raise _setup_required(e) diff --git a/extensions/package-vulnerability-scanner/test_main.py b/extensions/package-vulnerability-scanner/test_main.py index f7dd936f..22941a48 100644 --- a/extensions/package-vulnerability-scanner/test_main.py +++ b/extensions/package-vulnerability-scanner/test_main.py @@ -70,19 +70,34 @@ def test_setup_required_maps_212_to_424(): assert "Visitor API Key" in exc.detail -def test_setup_required_maps_other_to_502(): - # A non-212 error (e.g. a 403 permission denial when the content owner lacks - # publisher access) surfaces the human-readable Connect message, not raw JSON. +def test_setup_required_passes_through_4xx(): + # A non-212 4xx (e.g. a 403 permission denial when the content owner lacks + # publisher access) is a legitimate client error, not an upstream failure, so + # its real status is passed through rather than folded into a 502. exc = main._setup_required( - make_client_error(5, message="You do not have permission to access this content") + make_client_error( + 5, + http_status=403, + message="You do not have permission to access this content", + ) ) - assert exc.status_code == 502 + assert exc.status_code == 403 assert ( exc.detail == "Connect API error: You do not have permission to access this content" ) +def test_setup_required_maps_5xx_to_502(): + # A 5xx reaching here has already exhausted _fetch_with_retry's retries, so a + # 502 (upstream failure) is the right call, unlike a legitimate 4xx above. + exc = main._setup_required( + make_client_error(5, http_status=500, message="Server error") + ) + assert exc.status_code == 502 + assert exc.detail == "Connect API error: Server error" + + # --- _fetch_with_retry ----------------------------------------------------- @@ -239,6 +254,28 @@ def test_user_endpoint_ok(monkeypatch, api): assert resp.json()["username"] == "alice" +def test_user_endpoint_retries_transient_session_exchange_failure(monkeypatch, api): + # The session-token exchange makes a real Connect API call, so a transient + # 5xx there is retried like any other blocking SDK call, not raised outright. + monkeypatch.setattr(main.asyncio, "sleep", noop_sleep) + visitor = MagicMock() + visitor.me = {"guid": "u1", "username": "alice"} + calls = [] + + def flaky_get_visitor_client(request): + calls.append(1) + if len(calls) == 1: + raise make_client_error(999, http_status=503) + return visitor + + monkeypatch.setattr(main, "get_visitor_client", flaky_get_visitor_client) + + resp = api.get("/api/user") + + assert resp.status_code == 200 + assert len(calls) == 2 + + def test_user_endpoint_setup_required(monkeypatch, api): # A missing Visitor API Key integration raises ClientError 212 while building # the visitor client; the handler must turn it into a 424 for the setup screen. @@ -253,13 +290,13 @@ def raise_setup_error(request): assert "Visitor API Key" in resp.json()["detail"] -def test_user_endpoint_other_client_error_is_502(monkeypatch, api): +def test_user_endpoint_other_client_error_passes_through_status(monkeypatch, api): def raise_client_error(request): - raise make_client_error(5) + raise make_client_error(5, http_status=403) monkeypatch.setattr(main, "get_visitor_client", raise_client_error) - assert api.get("/api/user").status_code == 502 + assert api.get("/api/user").status_code == 403 def test_user_endpoint_non_client_error_is_502(monkeypatch, api): From 62e5d7f06a9204082b0d60e2a0464ede5b146256 Mon Sep 17 00:00:00 2001 From: Amy Lin Date: Fri, 31 Jul 2026 22:33:57 -0500 Subject: [PATCH 5/8] fix(package-vulnerability-scanner): pass through 4xx from a bare HTTPError, not just ClientError --- .../package-vulnerability-scanner/main.py | 8 +++++ .../test_main.py | 35 +++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/extensions/package-vulnerability-scanner/main.py b/extensions/package-vulnerability-scanner/main.py index 375ec0c3..9204d373 100644 --- a/extensions/package-vulnerability-scanner/main.py +++ b/extensions/package-vulnerability-scanner/main.py @@ -71,6 +71,14 @@ def _setup_required(exc: ClientError) -> HTTPException: # short, curated message rather than a raw SDK/httpx string. def _upstream_error(message: str, exc: Exception) -> HTTPException: print(f"{message} {exc}") + # Connect returning a 4xx with no JSON body (rare, but the SDK's error hook + # falls back to a bare requests.HTTPError in that case) is still a + # legitimate client error, not an upstream failure; pass its real status + # through instead of folding it into a 502, same as _setup_required does + # for a ClientError. + status = getattr(getattr(exc, "response", None), "status_code", None) + if status is not None and 400 <= status < 500: + return HTTPException(status_code=status, detail=message) return HTTPException(status_code=502, detail=message) diff --git a/extensions/package-vulnerability-scanner/test_main.py b/extensions/package-vulnerability-scanner/test_main.py index 22941a48..af1b417e 100644 --- a/extensions/package-vulnerability-scanner/test_main.py +++ b/extensions/package-vulnerability-scanner/test_main.py @@ -8,6 +8,7 @@ import os from unittest.mock import MagicMock, PropertyMock +import httpx import pytest from fastapi import HTTPException from fastapi.testclient import TestClient @@ -98,6 +99,40 @@ def test_setup_required_maps_5xx_to_502(): assert exc.detail == "Connect API error: Server error" +# --- _upstream_error --------------------------------------------------------- + + +def test_upstream_error_passes_through_4xx_from_bare_http_error(): + # Connect returning a 4xx with no JSON body (rare) makes the SDK fall back to + # a bare requests.HTTPError instead of a ClientError; its real status is + # still passed through rather than folded into a 502. + err = httpx.HTTPStatusError( + "404 Client Error", + request=httpx.Request("GET", "https://connect.example.com"), + response=httpx.Response(404, request=httpx.Request("GET", "https://x")), + ) + exc = main._upstream_error("Couldn't load your content from Connect.", err) + assert exc.status_code == 404 + assert exc.detail == "Couldn't load your content from Connect." + + +def test_upstream_error_maps_5xx_to_502(): + err = httpx.HTTPStatusError( + "503 Server Error", + request=httpx.Request("GET", "https://connect.example.com"), + response=httpx.Response(503, request=httpx.Request("GET", "https://x")), + ) + exc = main._upstream_error("Couldn't load your content from Connect.", err) + assert exc.status_code == 502 + + +def test_upstream_error_maps_502_when_no_response_status(): + exc = main._upstream_error( + "Couldn't load your content from Connect.", ValueError("boom") + ) + assert exc.status_code == 502 + + # --- _fetch_with_retry ----------------------------------------------------- From cfdd478deb68d2ce1c86f389809996483ffddfea Mon Sep 17 00:00:00 2001 From: Amy Lin Date: Fri, 31 Jul 2026 23:35:19 -0500 Subject: [PATCH 6/8] style(package-vulnerability-scanner): format manifest.json with prettier Co-Authored-By: Claude Opus 5 (1M context) --- extensions/package-vulnerability-scanner/manifest.json | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/extensions/package-vulnerability-scanner/manifest.json b/extensions/package-vulnerability-scanner/manifest.json index 9fb037df..896934ae 100644 --- a/extensions/package-vulnerability-scanner/manifest.json +++ b/extensions/package-vulnerability-scanner/manifest.json @@ -43,10 +43,7 @@ "homepage": "https://github.com/posit-dev/connect-extensions/tree/main/extensions/package-vulnerability-scanner", "category": "extension", "minimumConnectVersion": "2025.04.0", - "requiredFeatures": [ - "API Publishing", - "OAuth Integrations" - ], + "requiredFeatures": ["API Publishing", "OAuth Integrations"], "version": "3.0.6" } } From 7db15516b37513d58334e7b9b02ac479dbe00ca7 Mon Sep 17 00:00:00 2001 From: Amy Lin Date: Fri, 31 Jul 2026 23:51:24 -0500 Subject: [PATCH 7/8] build(package-vulnerability-scanner): keep prettier out of the pytest cache Running the new backend suite leaves a .pytest_cache that git ignores via its own generated file but prettier still walks, so check-format failed after pytest. Listed alongside .venv, which is ignored for the same reason. Co-Authored-By: Claude Opus 5 (1M context) --- extensions/package-vulnerability-scanner/.prettierignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/extensions/package-vulnerability-scanner/.prettierignore b/extensions/package-vulnerability-scanner/.prettierignore index 14d44d84..101dd13b 100644 --- a/extensions/package-vulnerability-scanner/.prettierignore +++ b/extensions/package-vulnerability-scanner/.prettierignore @@ -5,3 +5,6 @@ coverage # Ignore virtual environments: .venv + +# Ignore test caches: +.pytest_cache From 0751bb00a954c91f18fa81e5e7d7e82887bd7934 Mon Sep 17 00:00:00 2001 From: Amy Lin Date: Sat, 1 Aug 2026 00:37:13 -0500 Subject: [PATCH 8/8] fix(package-vulnerability-scanner): give every Connect request a deadline The SDK sets no timeout on its session, so a Connect server that accepts a connection and then goes quiet hangs the calling thread forever. asyncio can stop waiting on those calls but cannot interrupt them, so the threads pile up and eventually starve the pool. A session adapter supplies a default read timeout through requests' own extension point, and a request that hits it is retried like any other transient failure. Co-Authored-By: Claude Opus 5 (1M context) --- .../package-vulnerability-scanner/main.py | 52 +++++++++++++++---- .../pyproject.toml | 1 + .../requirements.txt | 1 + .../test_main.py | 47 ++++++++++++++++- 4 files changed, 90 insertions(+), 11 deletions(-) diff --git a/extensions/package-vulnerability-scanner/main.py b/extensions/package-vulnerability-scanner/main.py index 9204d373..60319a1e 100644 --- a/extensions/package-vulnerability-scanner/main.py +++ b/extensions/package-vulnerability-scanner/main.py @@ -3,6 +3,7 @@ import os import httpx +import requests from fastapi import FastAPI, HTTPException, Request from fastapi.staticfiles import StaticFiles from posit import connect @@ -11,7 +12,34 @@ app = FastAPI() -client = connect.Client() +# How long a single Connect HTTP request may stall. This is requests' read +# timeout, i.e. the gap between bytes, so a slow but progressing response is not +# cut off; only a genuinely stuck one is. +CONNECT_REQUEST_TIMEOUT_SECONDS = 60 + + +# The SDK ships its session with no timeout of its own, so a Connect server that +# accepts a connection and then goes quiet would hang the calling thread forever. +# The endpoints run these calls through asyncio.to_thread, which can stop waiting +# on a stuck call but cannot interrupt it, so without a deadline here those +# threads accumulate and eventually starve the pool. Supplying the default +# through an adapter uses requests' own extension point rather than reaching into +# the SDK. CONNECT_API_TIMEOUT_SECONDS is the separate, shorter bound on how long +# a caller waits; this one is what eventually frees the thread. +class _TimeoutAdapter(requests.adapters.HTTPAdapter): + def send(self, request, **kwargs): + if kwargs.get("timeout") is None: + kwargs["timeout"] = CONNECT_REQUEST_TIMEOUT_SECONDS + return super().send(request, **kwargs) + + +def _with_request_timeout(c: connect.Client) -> connect.Client: + for prefix in ("http://", "https://"): + c.session.mount(prefix, _TimeoutAdapter()) + return c + + +client = _with_request_timeout(connect.Client()) # Connect sets one of these to "CONNECT" for deployed content. Check both because @@ -27,7 +55,9 @@ def _running_on_connect() -> bool: def get_visitor_client(request: Request) -> connect.Client: token = request.headers.get("posit-connect-user-session-token") if token: - return client.with_user_session_token(token) + # The exchange builds a fresh client with its own session, so the + # timeout has to be applied to that one too. + return _with_request_timeout(client.with_user_session_token(token)) # On Connect, no token means the viewer's session can't be read (they aren't # signed in, or OAuth integrations are disabled on the server). Falling back to # the owner's default client would scan the owner's content as if it were the @@ -94,13 +124,13 @@ def _upstream_error(message: str, exc: Exception) -> HTTPException: # (e.g. a 504 while Connect is under load) doesn't fail the whole scan. CONTENT_MAX_ATTEMPTS = 3 -# Give up on a single Connect API call. The SDK sets no request timeout of its -# own, so an unresponsive Connect server would otherwise hang the calling task -# indefinitely. A timeout is treated the same as a transient 5xx: retried up to -# CONTENT_MAX_ATTEMPTS before giving up. This bounds how long the app waits, not -# how long the underlying thread runs: asyncio.to_thread can't interrupt a call -# already in flight, so the abandoned thread still runs until Connect (or the OS) -# eventually gives up on its end. +# Stop waiting on a single Connect API call. A timeout is treated the same as a +# transient 5xx: retried up to CONTENT_MAX_ATTEMPTS before giving up. +# +# Deliberately shorter than CONNECT_REQUEST_TIMEOUT_SECONDS, because the two +# bound different things. This one caps how long a caller waits, so a slow call +# is retried promptly; asyncio.to_thread can't interrupt the call it abandons, so +# that thread lives on until the request-level deadline frees it. CONNECT_API_TIMEOUT_SECONDS = 30 @@ -109,7 +139,9 @@ def _upstream_error(message: str, exc: Exception) -> HTTPException: # A ClientError carries http_status; a lower-level requests error carries a # response with status_code. def _is_transient(exc: Exception) -> bool: - if isinstance(exc, asyncio.TimeoutError): + # Either kind of timeout: we stopped waiting on the call, or the request + # itself hit the deadline the session adapter sets. + if isinstance(exc, (asyncio.TimeoutError, requests.exceptions.Timeout)): return True status = getattr(exc, "http_status", None) or getattr( getattr(exc, "response", None), "status_code", None diff --git a/extensions/package-vulnerability-scanner/pyproject.toml b/extensions/package-vulnerability-scanner/pyproject.toml index f2eca760..d20fa48a 100644 --- a/extensions/package-vulnerability-scanner/pyproject.toml +++ b/extensions/package-vulnerability-scanner/pyproject.toml @@ -9,6 +9,7 @@ dependencies = [ "starlette>=0.47.2", "httpx>=0.28.1", "posit-sdk>=0.10.0", + "requests>=2.31.0", ] # Test-only dependencies; not bundled into the extension (requirements.txt is). diff --git a/extensions/package-vulnerability-scanner/requirements.txt b/extensions/package-vulnerability-scanner/requirements.txt index 34fc68e6..aaccf761 100644 --- a/extensions/package-vulnerability-scanner/requirements.txt +++ b/extensions/package-vulnerability-scanner/requirements.txt @@ -2,3 +2,4 @@ httpx fastapi starlette>=0.47.2 posit-sdk +requests>=2.31.0 diff --git a/extensions/package-vulnerability-scanner/test_main.py b/extensions/package-vulnerability-scanner/test_main.py index af1b417e..72a5ce84 100644 --- a/extensions/package-vulnerability-scanner/test_main.py +++ b/extensions/package-vulnerability-scanner/test_main.py @@ -57,8 +57,11 @@ def test_is_transient_false_when_no_status(): def test_is_transient_true_for_timeout(): # A call that never responds is worth retrying just like a 5xx: it may be a - # transient blip rather than a truly dead server. + # transient blip rather than a truly dead server. Both flavours count: giving + # up on the wait, and the request hitting the adapter's own deadline. assert main._is_transient(asyncio.TimeoutError()) is True + assert main._is_transient(main.requests.exceptions.ReadTimeout()) is True + assert main._is_transient(main.requests.exceptions.ConnectTimeout()) is True # --- _setup_required ------------------------------------------------------- @@ -217,6 +220,48 @@ def fn(): asyncio.run(main._fetch_with_retry(fn)) +# --- _TimeoutAdapter ------------------------------------------------------- + + +def test_timeout_adapter_supplies_a_default(monkeypatch): + seen = {} + + def fake_send(self, request, **kwargs): + seen.update(kwargs) + return "sent" + + monkeypatch.setattr(main.requests.adapters.HTTPAdapter, "send", fake_send) + + main._TimeoutAdapter().send(None, timeout=None) + assert seen["timeout"] == main.CONNECT_REQUEST_TIMEOUT_SECONDS + + +def test_timeout_adapter_leaves_an_explicit_timeout_alone(monkeypatch): + seen = {} + + def fake_send(self, request, **kwargs): + seen.update(kwargs) + return "sent" + + monkeypatch.setattr(main.requests.adapters.HTTPAdapter, "send", fake_send) + + main._TimeoutAdapter().send(None, timeout=5) + assert seen["timeout"] == 5 + + +def test_client_sessions_carry_the_timeout_adapter(): + # Both the owner client built at import and a per-request visitor client + # need the deadline; the SDK builds a fresh session for the latter. + for prefix in ("http://", "https://"): + assert isinstance(main.client.session.adapters[prefix], main._TimeoutAdapter) + + visitor = main._with_request_timeout( + main.connect.Client("https://connect.example.com") + ) + for prefix in ("http://", "https://"): + assert isinstance(visitor.session.adapters[prefix], main._TimeoutAdapter) + + # --- get_visitor_client ----------------------------------------------------