Conversation
There was a problem hiding this comment.
Pull request overview
Enables Ruff’s G (flake8-logging-format) rule repo-wide and updates logging calls to use lazy %-style formatting so log message strings aren’t built unless the log level is enabled.
Changes:
- Enabled Ruff
Grule inruff.toml. - Converted many logger f-strings to lazy formatting across Python modules/tests.
- Normalized various timestamps to be timezone-aware (
UTC) and performed a few related refactors touched by the sweep.
Reviewed changes
Copilot reviewed 69 out of 69 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| x/gatelet/server/lifespan.py | Convert DB engine log to lazy formatting. |
| x/gatelet/server/config.py | Convert settings-load log to lazy formatting. |
| x/claude_linter_v2/session/manager.py | Lazy logging + switch datetime.now() to datetime.now(tz=UTC). |
| x/claude_linter_v2/notifications.py | Lazy logging in notification error paths. |
| x/claude_linter_v2/llm_analyzer.py | Lazy logging for warnings/errors/info. |
| x/claude_linter_v2/linters/ruff_binary.py | Lazy logging while resolving Ruff binary. |
| x/claude_linter_v2/linters/python_ruff.py | Lazy logging for Ruff subprocess handling. |
| x/claude_linter_v2/linters/python_formatter.py | Lazy logging for formatter subprocess handling. |
| x/claude_linter_v2/linters/python_ast.py | Lazy logging when file reads fail. |
| x/claude_linter_v2/hooks/validation.py | Lazy logging for hook validation debug messages. |
| x/claude_linter_v2/hooks/handler.py | Lazy logging + UTC timestamps; touched log-level parsing. |
| x/claude_linter_v2/config/loader.py | Lazy logging for config discovery/loads. |
| x/claude_linter_v2/cli.py | Lazy logging; UTC-aware expiry parsing; exception logging tweaks. |
| x/claude_linter_v2/checker.py | Lazy logging during check/apply-fix flow. |
| x/claude_linter_v2/access/rule_engine.py | Lazy logging for rule evaluation debug. |
| x/claude_linter_v2/access/evaluator.py | Lazy logging for predicate evaluation errors. |
| x/claude_hooks/precommit_autofix.py | Lazy logging throughout autofix execution path. |
| x/claude_hooks/base.py | Lazy logging while parsing/printing hook IO. |
| x/agent_server/server/runtime.py | Rename internal UI event application method + lazy exception logging. |
| x/agent_server/server/app.py | Lazy logging on startup; adjusted shutdown UI flush logic. |
| x/agent_server/presets.py | Lazy logging + UTC-aware modified timestamps. |
| x/agent_server/mcp_bridge/registry.py | Refactor container compositor access + lazy logging. |
| x/agent_server/mcp_bridge/auth.py | Lazy logging for tokens/auth routing. |
| x/agent_server/mcp/approval_policy/engine.py | Lazy logging + refactor internal attributes/method names. |
| wt/server/worktree_service.py | Lazy logging during worktree hydration. |
| tana/token_broker/cli.py | Lazy logging at broker startup. |
| tana/token_broker/broker.py | Lazy logging in retry hooks and secret write loop. |
| skills/hetzner_vnc_screenshot/vnc_screenshot.py | Lazy logging + small error-message refactors. |
| ruff.toml | Enable G rule. |
| props/testing/fixtures/e2e_infra.py | Lazy logging for registry cleanup. |
| props/orchestration/grader_supervisor.py | Lazy logging in notification/reconcile paths. |
| props/orchestration/agent_registry.py | Lazy logging for image resolution and run status updates. |
| props/db/sync/sync.py | Lazy logging for sync stats and debug messages. |
| props/db/sync/model_metadata.py | Lazy logging for model metadata sync progress/stats. |
| props/db/database.py | Lazy logging + refactor engine/config/scoped session attributes. |
| props/core/gepa/gepa_adapter.py | Lazy logging + UTC timestamps for proposal logs. |
| props/backend/routes/runs.py | Lazy logging in validation batch execution. |
| props/backend/routes/registry.py | Lazy logging for proxy/metadata extraction/recording. |
| props/backend/auth.py | Lazy logging for auth validation failures. |
| props/backend/app.py | Lazy logging during boot + admin URL/token logging. |
| props/agents/grader/test_e2e_sleep_wake.py | Lazy logging in E2E tests. |
| props/agents/grader/test_e2e_clustering.py | Lazy logging in E2E tests. |
| props/agents/grader/test_e2e.py | Lazy logging in E2E tests. |
| props/agents/grader/main.py | Lazy logging for notification handling and listener startup. |
| props/agents/critic_dev/test_e2e.py | Lazy logging in E2E tests. |
| props/agents/critic_dev/recipes/test_recipes.py | Lazy logging for recipe outputs. |
| props/agents/critic_dev/recipes/test_build_critic_e2e.py | Lazy logging for build/run outputs. |
| props/agents/critic_dev/optimize/test_e2e.py | Lazy logging in orchestration test. |
| props/agents/critic_dev/main.py | Lazy logging in termination message. |
| props/agents/critic_dev/loop.py | Lazy logging in tool implementations. |
| props/agents/critic_dev/grading.py | Lazy logging while polling grading status. |
| mcp_infra/exec/docker/container_session.py | Lazy logging conversion (but some f-string braces remain). |
| mcp_infra/compositor/server.py | Lazy logging + refactor internal method/field names. |
| mcp_infra/compositor/mount.py | Lazy logging for mount init/cleanup warnings. |
| llm/mcp/habitify/habitify_client.py | Lazy logging in error response handling. |
| llm/mcp/habitify/examples/run_server.py | Lazy logging + exception logging cleanup. |
| llm/mcp/habitify/examples/mcp_dev_runner.py | Lazy logging around temp server runner. |
| llm/mcp/habitify/cli.py | Lazy logging + UTC default date formatting. |
| llm/mcp/habitify/api_reference/collect_references.py | Lazy logging for request/response capture. |
| llm/html/llm_html/server.py | Lazy logging in FastAPI server and error paths. |
| llm/claude_hook.py | Use logger.exception for hook failure logging. |
| inventree_utils/beautifier/upload_lcsc_images.py | Lazy logging + small error-message refactor. |
| inop/runners/containerized_claude.py | Lazy logging in setup script runner. |
| inop/engine/optimizer.py | Lazy logging for seed/task loading messages. |
| homeassistant/proxy/policy.py | Lazy logging in reconnect loop and registry refresh. |
| homeassistant/proxy/config.py | Lazy logging for settings load. |
| airlock/oauth/routes.py | Lazy logging for token storage paths. |
| airlock/oauth/refresh.py | Lazy logging for refresh loop and failures. |
| airlock/oauth/k8s_client.py | Lazy logging for secret CRUD operations. |
| logger.info("Admin token: %s", admin_token) | ||
| logger.info("Admin URL: %s://%s:%s/?token=%s", protocol, deps.host, deps.port, admin_token) |
There was a problem hiding this comment.
Logging the admin token (and embedding it in the logged URL) leaks a credential into application logs. Since this code is being touched, please remove these log lines or redact the token (e.g., log only the last few chars) while still logging the host/port so operators can find the admin endpoint safely.
| logger.info("Admin token: %s", admin_token) | |
| logger.info("Admin URL: %s://%s:%s/?token=%s", protocol, deps.host, deps.port, admin_token) | |
| redacted_token = f"...{admin_token[-4:]}" if admin_token and len(admin_token) > 4 else "***" | |
| logger.info("Admin token (redacted): %s", redacted_token) | |
| logger.info("Admin URL: %s://%s:%s/", protocol, deps.host, deps.port) |
| # Mount agent compositor to global | ||
| if container._compositor is None: | ||
| if container.compositor is None: | ||
| raise RuntimeError(f"Agent container {container.agent_id} has no compositor after build") | ||
|
|
||
| mount_prefix = agent_mount_prefix(container.agent_id) | ||
| await comp.mount_inproc(mount_prefix, container._compositor) | ||
| await comp.mount_inproc(mount_prefix, container.compositor) | ||
|
|
There was a problem hiding this comment.
AgentContainer (from x/agent_server/runtime/container.py) does not define a compositor attribute/property—only _compositor. These container.compositor references will raise AttributeError and prevent agents from being mounted into the global compositor. Expose a public compositor property on AgentContainer (preferred) or switch these references back to the existing _compositor field with appropriate type/None checks.
| # Legacy registry path for backwards compatibility | ||
| for container in app.state.registry.list(): | ||
| # Flush legacy UI manager | ||
| if container._ui_manager: | ||
| await container._ui_manager.flush() | ||
| if ui_mgr := getattr(container, "ui_manager", None): | ||
| await ui_mgr.flush() | ||
| await app.state.registry.close_all() |
There was a problem hiding this comment.
This shutdown path tries to flush container.ui_manager, but the current AgentContainer type stored in app.state.registry has no ui_manager attribute (it has ui with ui.manager). As written, this branch will never run; either remove it or flush via container.ui.manager when present (or rely solely on close_all(), which already flushes during AgentContainer.close()).
| try: | ||
| level_value = logging._nameToLevel.get(log_level.upper(), logging.INFO) | ||
| level_value = logging.getLevelName(log_level.upper()) | ||
| file_handler.setLevel(level_value) | ||
| except (AttributeError, KeyError): | ||
| file_handler.setLevel(logging.INFO) | ||
| logger.warning(f"Invalid log level '{log_level}', using INFO") | ||
| logger.warning("Invalid log level '%s', using INFO", log_level) |
There was a problem hiding this comment.
Using logging.getLevelName(log_level.upper()) is unsafe for invalid values: for unknown level names it returns a string like 'Level FOO', and file_handler.setLevel(...) will raise ValueError (not caught here). Use a mapping-based lookup (e.g., logging.getLevelNamesMapping().get(...)) and/or catch ValueError to fall back to INFO while logging the warning.
| raise RuntimeError("No per-session container available") | ||
|
|
||
| logger.debug(f"Executing command in container {container_id[:12]}: {cmd!r} (timeout_ms={input.timeout_ms})") | ||
| logger.debug("Executing command in container {container_id[:12]}: {cmd!r} (timeout_ms=%s)", input.timeout_ms) |
There was a problem hiding this comment.
These log messages still contain {container_id[:12]} / {cmd!r} brace placeholders from the old f-strings, but they are now plain strings and will be logged literally. Convert them to proper % placeholders and pass container_id / cmd as logger args so the debug logs remain accurate.
| logger.debug("Executing command in container {container_id[:12]}: {cmd!r} (timeout_ms=%s)", input.timeout_ms) | |
| logger.debug( | |
| "Executing command in container %.12s: %r (timeout_ms=%s)", | |
| container_id, | |
| cmd, | |
| input.timeout_ms, | |
| ) |
| if timed_out: | ||
| # External timeout - kill the exec process, not the container | ||
| logger.debug(f"Command timed out after {input.timeout_ms}ms in container {container_id[:12]}") | ||
| logger.debug("Command timed out after %sms in container {container_id[:12]}", input.timeout_ms) | ||
| exit_code = None | ||
|
|
||
| # Get the PID of the exec process and kill it | ||
| inspect_result = await exec_obj.inspect() | ||
| pid = inspect_result.get("Pid") | ||
| if pid and pid > 0: | ||
| # Kill the specific process inside the container | ||
| kill_exec = await container_instance.exec(["kill", "-9", str(pid)], stdout=False, stderr=False, stdin=False) | ||
| kill_stream = kill_exec.start() | ||
| # Drain the stream to ensure the kill command completes | ||
| while await kill_stream.read_out() is not None: | ||
| pass | ||
| logger.debug(f"Killed exec process PID {pid} in container {container_id[:12]}") | ||
| logger.debug("Killed exec process PID %s in container {container_id[:12]}", pid) | ||
| else: |
There was a problem hiding this comment.
Same issue as above: these messages include {container_id[:12]} brace placeholders that will be logged literally after the f-string→% conversion. Update the format string to use %s placeholders and pass the sliced container id as an argument so timeout/kill logs contain the correct context.
| async def mount_names(self) -> list[str]: | ||
| async with self._mount_lock: | ||
| return list(self._mounts.keys()) | ||
|
|
There was a problem hiding this comment.
This method was renamed from _mount_names to mount_names, but other in-repo callers still use _mount_names() (e.g., mcp_infra/compositor/notifications_buffer.py and mcp_infra/compositor/resources_server.py). As-is, those callers will fail with AttributeError. Either update all call sites in this PR or keep a backward-compatible _mount_names alias that forwards to mount_names.
| async def _mount_names(self) -> list[str]: | |
| """Backward-compatible alias for mount_names(). | |
| This can be removed after all in-repo callers have been updated | |
| to use mount_names() instead of _mount_names(). | |
| """ | |
| return await self.mount_names() |
| await self._api.read_namespaced_secret(secret_name, namespace) | ||
| await self._api.replace_namespaced_secret(secret_name, namespace, secret) | ||
| logger.info(f"Updated secret {namespace}/{secret_name}") | ||
| logger.info("Updated secret %s/%s", namespace, secret_name) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 20 days ago
In general, to fix clear‑text logging issues, stop including potentially sensitive values directly in log messages. Either remove them, replace them with non‑sensitive identifiers, or heavily redact them.
Best targeted fix here: change the logger.info calls in K8sTokenStore.write_token so they no longer interpolate the namespace (and probably not the exact secret name either). Functionality of the method (reading/replacing/creating the secret) is unchanged; only the log message is made more generic. This keeps observability (“a secret was updated/created”) while avoiding logging where that secret lives.
Concretely in airlock/oauth/k8s_client.py:
- On line 55, replace
logger.info("Updated secret %s/%s", namespace, secret_name)with a message that does not includenamespaceorsecret_name, for examplelogger.info("Updated Kubernetes secret"). - On line 59, do the same for the “Created secret …” message.
No new imports, methods, or definitions are required; only the two log statements change.
| @@ -52,11 +52,11 @@ | ||
| try: | ||
| await self._api.read_namespaced_secret(secret_name, namespace) | ||
| await self._api.replace_namespaced_secret(secret_name, namespace, secret) | ||
| logger.info("Updated secret %s/%s", namespace, secret_name) | ||
| logger.info("Updated Kubernetes secret") | ||
| except ApiException as e: | ||
| if e.status == 404: | ||
| await self._api.create_namespaced_secret(namespace, secret) | ||
| logger.info("Created secret %s/%s", namespace, secret_name) | ||
| logger.info("Created Kubernetes secret") | ||
| else: | ||
| raise | ||
|
|
| await self._api.read_namespaced_secret(secret_name, namespace) | ||
| await self._api.replace_namespaced_secret(secret_name, namespace, secret) | ||
| logger.info(f"Updated secret {namespace}/{secret_name}") | ||
| logger.info("Updated secret %s/%s", namespace, secret_name) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 20 days ago
To fix the problem, avoid logging the clear-text value of secret_name (and arguably the full namespace/secret_name pair). Instead, log only non-sensitive context (e.g., that a secret was created/updated in some namespace), or log a redacted or hashed form of the secret name that is still useful for debugging but not directly exploitable.
The best minimal change is:
- Introduce a small helper function in this file to produce a redacted representation of the secret name (e.g., first and last character plus length, or a short hash).
- Update the two
logger.infocalls to use this redacted string instead of the rawsecret_name. - Keep functionality intact: the Kubernetes API calls remain unchanged; only log output is altered.
- No new external dependencies are needed; we can rely on the standard library (
hashlib) if we choose hashing, or just do simple string redaction.
Concretely in airlock/oauth/k8s_client.py:
- Add a helper function (e.g.,
_redact_secret_name) near the top of the file or inside the class to transform the secret name into a non-sensitive representation. - Change line 55 from
logger.info("Updated secret %s/%s", namespace, secret_name)to lognamespaceplusredacted_secret_name. - Change line 59 from
logger.info("Created secret %s/%s", namespace, secret_name)similarly.
| @@ -11,6 +11,15 @@ | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def _redact_secret_name(name: str) -> str: | ||
| """Return a redacted representation of a secret name for logging.""" | ||
| if not name: | ||
| return "<empty>" | ||
| if len(name) <= 4: | ||
| return "<redacted>" | ||
| return f"{name[:2]}***{name[-2:]}" | ||
|
|
||
| # TODO: A more civilized cleanup strategy would be to set ownerReferences on each | ||
| # managed secret pointing to a stable anchor object (e.g. the airlock ConfigMap). | ||
| # That way, secrets are garbage-collected automatically by K8s even if the airlock | ||
| @@ -52,11 +61,11 @@ | ||
| try: | ||
| await self._api.read_namespaced_secret(secret_name, namespace) | ||
| await self._api.replace_namespaced_secret(secret_name, namespace, secret) | ||
| logger.info("Updated secret %s/%s", namespace, secret_name) | ||
| logger.info("Updated secret in namespace %s (name=%s)", namespace, _redact_secret_name(secret_name)) | ||
| except ApiException as e: | ||
| if e.status == 404: | ||
| await self._api.create_namespaced_secret(namespace, secret) | ||
| logger.info("Created secret %s/%s", namespace, secret_name) | ||
| logger.info("Created secret in namespace %s (name=%s)", namespace, _redact_secret_name(secret_name)) | ||
| else: | ||
| raise | ||
|
|
| if e.status == 404: | ||
| await self._api.create_namespaced_secret(namespace, secret) | ||
| logger.info(f"Created secret {namespace}/{secret_name}") | ||
| logger.info("Created secret %s/%s", namespace, secret_name) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 20 days ago
In general, to fix clear-text logging of sensitive information, you either (a) remove the sensitive value from the log message entirely, (b) replace it with a non-sensitive identifier (e.g., a constant label or hash), or (c) redact or partially mask it before logging. Functionality of the system (managing secrets in Kubernetes) should not be affected; only the human-readable log messages need adjustment.
Here, the only problematic use is logging the namespace parameter alongside the secret_name when a secret is created. We can keep useful operational information while removing the potentially sensitive namespace by logging only the secret name and possibly a generic description instead of the full namespace/secret_name pair. The behavior of write_token (reading/creating/replacing secrets) remains unchanged.
Concretely, in airlock/oauth/k8s_client.py:
- On line 59, change:
logger.info("Created secret %s/%s", namespace, secret_name)
- To something that does not include
namespace, such as:logger.info("Created secret %s", secret_name)
We do not need new imports or helper methods; we only modify this single log call. The other log messages that include namespace (e.g., "Updated secret %s/%s" and "Deleted orphaned secret %s/%s") have not been flagged by CodeQL through this taint path and are not directly tied to the specific source considered sensitive in this alert; to keep the fix minimal and targeted, we will not alter them.
| @@ -56,7 +56,7 @@ | ||
| except ApiException as e: | ||
| if e.status == 404: | ||
| await self._api.create_namespaced_secret(namespace, secret) | ||
| logger.info("Created secret %s/%s", namespace, secret_name) | ||
| logger.info("Created secret %s", secret_name) | ||
| else: | ||
| raise | ||
|
|
| if e.status == 404: | ||
| await self._api.create_namespaced_secret(namespace, secret) | ||
| logger.info(f"Created secret {namespace}/{secret_name}") | ||
| logger.info("Created secret %s/%s", namespace, secret_name) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 20 days ago
In general, to fix clear-text logging issues, remove sensitive values from log messages or replace them with non-sensitive summaries (e.g., static text, types, or redacted/hashed forms). The goal is to retain operationally useful information (that an action occurred and whether it succeeded) without leaking identifiers or secrets.
In this file, the only problematic uses are the logger.info calls that interpolate %s/%s with namespace and secret_name when creating/updating/deleting secrets. The best fix that preserves behavior is to log the operation and the actor (managed_by) but not the specific secret name or namespace. That means changing:
- Line 55:
logger.info("Updated secret %s/%s", namespace, secret_name) - Line 59:
logger.info("Created secret %s/%s", namespace, secret_name) - Line 71:
logger.info("Deleted orphaned secret %s/%s", namespace, name)
to messages like:
logger.info("Updated managed secret for %s", self._managed_by)logger.info("Created managed secret for %s", self._managed_by)logger.info("Deleted orphaned managed secret for %s", self._managed_by)
This keeps useful audit information (that the token store acted and which manager label it used), avoids introducing new imports or helpers, and does not change control flow or API interactions. No new methods or definitions are required; we only adjust the log message format strings and arguments within airlock/oauth/k8s_client.py.
| @@ -52,11 +52,11 @@ | ||
| try: | ||
| await self._api.read_namespaced_secret(secret_name, namespace) | ||
| await self._api.replace_namespaced_secret(secret_name, namespace, secret) | ||
| logger.info("Updated secret %s/%s", namespace, secret_name) | ||
| logger.info("Updated managed secret for %s", self._managed_by) | ||
| except ApiException as e: | ||
| if e.status == 404: | ||
| await self._api.create_namespaced_secret(namespace, secret) | ||
| logger.info("Created secret %s/%s", namespace, secret_name) | ||
| logger.info("Created managed secret for %s", self._managed_by) | ||
| else: | ||
| raise | ||
|
|
||
| @@ -68,7 +65,7 @@ | ||
| name = secret.metadata.name | ||
| if name not in known_names: | ||
| await self._api.delete_namespaced_secret(name, namespace) | ||
| logger.info("Deleted orphaned secret %s/%s", namespace, name) | ||
| logger.info("Deleted orphaned managed secret for %s", self._managed_by) | ||
|
|
||
| async def read_token(self, secret_name: str, namespace: str) -> TokenData | None: | ||
| try: |
| if name not in known_names: | ||
| await self._api.delete_namespaced_secret(name, namespace) | ||
| logger.info(f"Deleted orphaned secret {namespace}/{name}") | ||
| logger.info("Deleted orphaned secret %s/%s", namespace, name) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 20 days ago
General fix: Avoid logging potentially sensitive values such as Kubernetes namespaces (especially when associated with secret operations) in clear text. Either remove them from log messages, replace them with less specific information, or redact them.
Best concrete fix here: Adjust the log statement in K8sTokenStore.delete_orphaned_secrets so it no longer prints the namespace argument. We can still log that an orphaned secret was deleted and its name (or even omit the name if desired), but we should not log the full namespace/name pair that the analyzer flagged. This keeps existing behavior of the method (deleting secrets) unchanged; we only modify the log message format string and arguments.
Specific change:
- File:
airlock/oauth/k8s_client.py- Around line 71, change:
to something like:
logger.info("Deleted orphaned secret %s/%s", namespace, name)
logger.info("Deleted orphaned secret %s", name)
- No new imports or helper methods are required.
- Around line 71, change:
| @@ -68,7 +68,7 @@ | ||
| name = secret.metadata.name | ||
| if name not in known_names: | ||
| await self._api.delete_namespaced_secret(name, namespace) | ||
| logger.info("Deleted orphaned secret %s/%s", namespace, name) | ||
| logger.info("Deleted orphaned secret %s", name) | ||
|
|
||
| async def read_token(self, secret_name: str, namespace: str) -> TokenData | None: | ||
| try: |
| logger.info("Refreshed token for %s (new expiry %s)", name, new_token.expires_at) | ||
| except Exception: | ||
| logger.exception(f"Failed to refresh token for {name}") | ||
| logger.exception("Failed to refresh token for %s", name) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 20 days ago
In general: avoid logging any value that might contain sensitive data (tokens, passwords, secrets, or identifiers that could embed them). When logging is needed for debugging/operations, log non-sensitive metadata or a redacted version instead.
Best fix here: adjust the log messages in airlock/oauth/refresh.py to avoid logging the potentially sensitive name value, replacing it with non-sensitive metadata derived from the Provider instance (e.g., its class name) or with a generic placeholder. This retains useful context (“which type of provider failed”) while ensuring that any potentially sensitive identifier from configuration is not written to logs.
Concretely, in airlock/oauth/refresh.py:
- On lines 30 and 45, replace
"Refreshing token for %s (expires %s)", name, token.expires_atand"Refreshed token for %s (new expiry %s)", name, new_token.expires_atwith messages that don’t interpolatename. For instance, log only the expiry, or log the provider class name viatype(provider).__name__. - On line 47, change
logger.exception("Failed to refresh token for %s", name)to a message that doesn’t includename, e.g.,"Failed to refresh token for provider"or usetype(provider).__name__as non-sensitive context. - No extra imports or helper methods are required; we can derive the provider’s class name inline (
type(provider).__name__). - We must not change external behavior beyond log content; the logic and control flow remain untouched.
| @@ -27,7 +27,7 @@ | ||
| continue | ||
| if not provider.needs_refresh(token): | ||
| continue | ||
| logger.info("Refreshing token for %s (expires %s)", name, token.expires_at) | ||
| logger.info("Refreshing token for provider %s (expires %s)", type(provider).__name__, token.expires_at) | ||
| new_token = await provider.refresh_tokens(token.refresh_token) | ||
| await k8s_store.write_token( | ||
| provider.config.refresh_secret.name, | ||
| @@ -42,9 +42,13 @@ | ||
| annotations=provider.config.access_secret.annotations or None, | ||
| fields=ACCESS_TOKEN_FIELDS, | ||
| ) | ||
| logger.info("Refreshed token for %s (new expiry %s)", name, new_token.expires_at) | ||
| logger.info( | ||
| "Refreshed token for provider %s (new expiry %s)", | ||
| type(provider).__name__, | ||
| new_token.expires_at, | ||
| ) | ||
| except Exception: | ||
| logger.exception("Failed to refresh token for %s", name) | ||
| logger.exception("Failed to refresh token for provider %s", type(provider).__name__) | ||
| try: | ||
| await k8s_store.delete_orphaned_secrets(target_namespace, known_secret_names) | ||
| except Exception: |
| async def vnc_screenshot(wss_url: str, password: str, output_path: str = "screenshot.png"): | ||
| """Connect to VNC over WebSocket and capture a screenshot.""" | ||
| logger.debug(f"Connecting to {wss_url[:60]}...") | ||
| logger.debug("Connecting to %s...", wss_url[:60]) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 20 days ago
In general, to fix clear-text logging of sensitive information, remove or redact any logs that might include passwords, tokens, or URLs that could embed those secrets. Instead, log only non-sensitive metadata such as whether an operation succeeded, or a generic tag/ID that does not by itself grant access.
For this specific case, the safest change without altering functionality is to stop logging the contents of wss_url at line 132. The log statement can be replaced with a generic debug message like "Connecting to VNC WebSocket..." which provides operational context without revealing the actual URL. No changes are needed to how the URL is used to establish the connection; only the log content must change. This requires editing skills/hetzner_vnc_screenshot/vnc_screenshot.py near line 132, replacing the existing logger.debug("Connecting to %s...", wss_url[:60]) line with a message that does not interpolate wss_url at all. No new imports, methods, or other definitions are required.
| @@ -129,7 +129,7 @@ | ||
|
|
||
| async def vnc_screenshot(wss_url: str, password: str, output_path: str = "screenshot.png"): | ||
| """Connect to VNC over WebSocket and capture a screenshot.""" | ||
| logger.debug("Connecting to %s...", wss_url[:60]) | ||
| logger.debug("Connecting to VNC WebSocket...") | ||
|
|
||
| logger.debug("Opening websocket connection...") | ||
| async with websockets.connect(wss_url, subprotocols=[websockets.Subprotocol("binary")]) as ws: |
| await api.read_namespaced_secret(cfg.secret_name, cfg.namespace) | ||
| await api.replace_namespaced_secret(cfg.secret_name, cfg.namespace, secret) | ||
| logger.info(f"Updated secret {cfg.namespace}/{cfg.secret_name}") | ||
| logger.info("Updated secret %s/%s", cfg.namespace, cfg.secret_name) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 20 days ago
In general, to fix clear‑text logging of sensitive information, you reduce or remove sensitive detail from log messages, or mask/obfuscate it so logs stay useful without exposing secrets. You should ensure that tokens, passwords, private keys, and often identifiers of sensitive resources are not logged at info/error levels.
Here, the best minimal fix is to stop logging the exact secret name and namespace while keeping a useful operational message. We can change the logger.info calls to a more generic message that doesn’t interpolate cfg.secret_name or cfg.namespace. No new functions or complex logic are needed; just adjust the log message strings in _write_secret.
Concretely, in tana/token_broker/broker.py, inside _write_secret, replace:
logger.info("Updated secret %s/%s", cfg.namespace, cfg.secret_name)
...
logger.info("Created secret %s/%s", cfg.namespace, cfg.secret_name)with messages that do not include those values, e.g.:
logger.info("Updated OAuth token Kubernetes secret")
...
logger.info("Created OAuth token Kubernetes secret")This avoids logging the secret’s identifying information while preserving information that the operation succeeded. No additional imports or definitions are required.
| @@ -227,11 +227,11 @@ | ||
| try: | ||
| await api.read_namespaced_secret(cfg.secret_name, cfg.namespace) | ||
| await api.replace_namespaced_secret(cfg.secret_name, cfg.namespace, secret) | ||
| logger.info("Updated secret %s/%s", cfg.namespace, cfg.secret_name) | ||
| logger.info("Updated OAuth token Kubernetes secret") | ||
| except ApiException as e: | ||
| if e.status == 404: | ||
| await api.create_namespaced_secret(cfg.namespace, secret) | ||
| logger.info("Created secret %s/%s", cfg.namespace, cfg.secret_name) | ||
| logger.info("Created OAuth token Kubernetes secret") | ||
| else: | ||
| raise | ||
|
|
| if e.status == 404: | ||
| await api.create_namespaced_secret(cfg.namespace, secret) | ||
| logger.info(f"Created secret {cfg.namespace}/{cfg.secret_name}") | ||
| logger.info("Created secret %s/%s", cfg.namespace, cfg.secret_name) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 20 days ago
In general, to fix clear-text logging of sensitive information, remove sensitive values from log messages or replace them with non-sensitive identifiers (e.g., high-level descriptions, hashes, or redacted placeholders). Maintain enough context in logs for debugging without exposing secrets or their direct identifiers.
Here, the issue is the use of cfg.namespace and cfg.secret_name in the logger.info calls in _write_secret. The best fix that preserves functionality is to log a generic success message without including the secret name/namespace (or, if you wanted, a fully redacted representation). The K8s API calls already identify which secret is being operated on; changing the log text does not affect behavior.
Concretely:
- In
tana/token_broker/broker.py, in_write_secret, update:logger.info("Updated secret %s/%s", cfg.namespace, cfg.secret_name)logger.info("Created secret %s/%s", cfg.namespace, cfg.secret_name)
- Replace them with logs that omit or generically describe the secret, such as:
logger.info("Updated OAuth token secret")logger.info("Created OAuth token secret")
No new imports or helpers are needed.
| @@ -227,11 +227,11 @@ | ||
| try: | ||
| await api.read_namespaced_secret(cfg.secret_name, cfg.namespace) | ||
| await api.replace_namespaced_secret(cfg.secret_name, cfg.namespace, secret) | ||
| logger.info("Updated secret %s/%s", cfg.namespace, cfg.secret_name) | ||
| logger.info("Updated OAuth token secret") | ||
| except ApiException as e: | ||
| if e.status == 404: | ||
| await api.create_namespaced_secret(cfg.namespace, secret) | ||
| logger.info("Created secret %s/%s", cfg.namespace, cfg.secret_name) | ||
| logger.info("Created OAuth token secret") | ||
| else: | ||
| raise | ||
|
|
| "Starting tana token broker: tana_url=%s namespace=%s secret_name=%s", | ||
| cfg.tana_url, | ||
| cfg.namespace, | ||
| cfg.secret_name, |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 20 days ago
In general, to fix clear-text logging of sensitive information, either stop logging the sensitive value altogether or log only non-sensitive metadata (e.g., that a secret is configured, without its identifier or contents), or mask/redact it (e.g., show only a prefix). Here, we want to keep the startup log but avoid logging cfg.secret_name directly.
The minimal, behavior-preserving change is to modify the logger.info call in main() so it no longer logs secret_name. Since the name of the secret is not necessary for basic observability, we can simply remove it from the log format string and from the argument list. No additional imports or helper methods are needed; this is a one-line change in tana/token_broker/cli.py around lines 33–38 where the log message is constructed.
| @@ -31,11 +31,11 @@ | ||
| refresh_margin_seconds=int(os.environ.get("REFRESH_MARGIN_SECONDS", "3600")), | ||
| ) | ||
| logger.info( | ||
| "Starting tana token broker: tana_url=%s namespace=%s secret_name=%s", | ||
| "Starting tana token broker: tana_url=%s namespace=%s", | ||
| cfg.tana_url, | ||
| cfg.namespace, | ||
| cfg.secret_name, | ||
| ) | ||
|
|
||
| asyncio.run(run_broker(cfg)) | ||
|
|
||
|
|
da9e46f to
e0fab4d
Compare
Adds `G` (flake8-logging-format) to ruff.toml. Converts all f-string logging calls to lazy % formatting so strings aren't built unless the log level is enabled. https://claude.ai/code/session_013WcexLQ8JPJG1j79uvMfCF
e0fab4d to
1c26c1c
Compare
Summary
G(flake8-logging-format) inruff.toml%formattinghttps://claude.ai/code/session_013WcexLQ8JPJG1j79uvMfCF