Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/launchpad/artifact_processor.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
from launchpad.tracing import request_context
from launchpad.utils.file_utils import IdPrefix, id_from_bytes
from launchpad.utils.logging import get_logger
from launchpad.utils.objectstore import create_objectstore_client
from launchpad.utils.statsd import StatsdInterface, get_statsd

logger = get_logger(__name__)
Expand Down Expand Up @@ -93,9 +94,7 @@ def process_message(
statsd = get_statsd()
if artifact_processor is None:
sentry_client = SentryClient(base_url=service_config.sentry_base_url)
objectstore_client = None
if service_config.objectstore_url is not None:
objectstore_client = ObjectstoreClient(service_config.objectstore_url)
objectstore_client = create_objectstore_client(service_config.objectstore_config)
artifact_processor = ArtifactProcessor(sentry_client, statsd, objectstore_client)

if service_config and project_id in service_config.projects_to_skip:
Expand Down
25 changes: 22 additions & 3 deletions src/launchpad/service.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@

from dataclasses import dataclass

from objectstore_client import Permission as ObjectstorePermission
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import ObjectstorePermission in service module

Low Severity

Permission as ObjectstorePermission is imported in service.py but never referenced anywhere in the file. The Permission class is only used in utils/objectstore.py, where it's imported separately. This is dead code left over from development.

Fix in Cursor Fix in Web


from launchpad.sentry_client import SentryClient
from launchpad.utils.logging import get_logger
from launchpad.utils.statsd import NullStatsd, StatsdInterface, get_statsd
Expand Down Expand Up @@ -121,29 +123,46 @@ def _shutdown_server(self) -> None:
self._server_thread.join(timeout=10)


@dataclass
class ObjectstoreConfig:
"""Objectstore client configuration data."""

objectstore_url: str | None
key_id: str | None = None
key_file: str | None = None
token_expiry_seconds: int = 60


@dataclass
class ServiceConfig:
"""Service configuration data."""

sentry_base_url: str
projects_to_skip: list[str]
objectstore_url: str | None
objectstore_config: ObjectstoreConfig
Comment on lines 137 to +142
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Instantiations of the ServiceConfig dataclass in tests use the old objectstore_url parameter, which was removed, causing a TypeError when tests run.
Severity: HIGH

Suggested Fix

Update all test instantiations of ServiceConfig to pass an ObjectstoreConfig instance to the new objectstore_config parameter instead of passing a string to the removed objectstore_url parameter.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/launchpad/service.py#L137-L142

Potential issue: The `ServiceConfig` dataclass was refactored to replace the
`objectstore_url` string parameter with a structured `objectstore_config:
ObjectstoreConfig` parameter. However, multiple test files
(`test_artifact_processor.py`, `test_kafka_service.py`) were not updated to reflect this
change. These tests continue to instantiate `ServiceConfig` using the old
`objectstore_url` keyword argument. Since this argument no longer exists on the
dataclass's `__init__` method, a `TypeError` will be raised at runtime when these tests
are executed, causing the test suite to fail.

Did we get this right? 👍 / 👎 to inform future reviews.



def get_service_config() -> ServiceConfig:
"""Get service configuration from environment."""
sentry_base_url = os.getenv("SENTRY_BASE_URL")
projects_to_skip_str = os.getenv("PROJECT_IDS_TO_SKIP")
projects_to_skip = projects_to_skip_str.split(",") if projects_to_skip_str else []
objectstore_url = os.getenv("OBJECTSTORE_URL")

objectstore_config = ObjectstoreConfig(
objectstore_url=os.getenv("OBJECTSTORE_URL"),
key_id=os.getenv("OBJECTSTORE_SIGNING_KEY_ID"),
key_file=os.getenv("OBJECTSTORE_SIGNING_KEY_FILE"),
)
if expiry_seconds := os.getenv("OBJECTSTORE_TOKEN_EXPIRY_SECONDS"):
objectstore_config.token_expiry_seconds = int(expiry_seconds)

if sentry_base_url is None:
sentry_base_url = "http://getsentry.default"

return ServiceConfig(
sentry_base_url=sentry_base_url,
projects_to_skip=projects_to_skip,
objectstore_url=objectstore_url,
objectstore_config=objectstore_config,
)


Expand Down
50 changes: 50 additions & 0 deletions src/launchpad/utils/objectstore.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
from objectstore_client import (
Client as ObjectstoreClient,
)
from objectstore_client import (
Permission,
TokenGenerator,
)

import launchpad

from launchpad.utils.logging import get_logger

logger = get_logger(__name__)

_cached_keyfile: str | None = None

TOKEN_PERMISSIONS: list[Permission] = [
Permission.OBJECT_READ,
Permission.OBJECT_WRITE,
Permission.OBJECT_DELETE,
]


def _read_keyfile(path: str) -> str | None:
global _cached_keyfile
if not _cached_keyfile:
try:
with open(path) as f:
_cached_keyfile = f.read()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Key file content not stripped of trailing whitespace

Medium Severity

_read_keyfile passes the raw result of f.read() to TokenGenerator without stripping whitespace. The PR description states the signing key is mounted as a file via GCP Secret Manager CSI driver. File-mounted secrets commonly include a trailing newline, which would become part of the key material passed to TokenGenerator, likely causing token signing failures or invalid auth tokens in production.

Fix in Cursor Fix in Web

except Exception:
logger.exception(f"Failed to load objectstore keyfile at {path}")

return _cached_keyfile
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Global keyfile cache ignores path parameter changes

Low Severity

_read_keyfile uses a single module-level _cached_keyfile global that ignores the path parameter after the first successful read. If ever called with a different path (e.g., key rotation or tests), the stale cached content from the first call is returned. The cache is not keyed by path, making the function's contract misleading.

Fix in Cursor Fix in Web



def create_objectstore_client(config: "launchpad.service.ObjectstoreConfig") -> ObjectstoreClient | None:
if not config.objectstore_url:
return None

token_generator = None
if config.key_id and config.key_file:
if secret_key := _read_keyfile(config.key_file):
token_generator = TokenGenerator(
config.key_id,
secret_key,
config.token_expiry_seconds,
TOKEN_PERMISSIONS,
)

return ObjectstoreClient(config.objectstore_url, token_generator=token_generator)
Loading