Skip to content

Commit 8ee3bad

Browse files
leliaclaude
andcommitted
Keep credentials out of log output
CodeQL reported py/clear-text-logging-sensitive-data at output.py:127, the `logger.info(json.dumps(...))` in output_console_json. That line is a false positive -- neither build_json_report nor build_fossa_report_payload copies a credential into the payload; both pick named fields. CodeQL taints anything derived from self.config because CliConfig declares api_token. Chasing it did turn up three real leaks: - socketcli logged `config.to_dict()` at debug level, and to_dict() is asdict(), so `--debug` printed the Socket API token in clear text. In CI that lands in the job log, which is retained, pasted into support tickets and world-readable for public repositories. - The Slack plugin logged the full webhook URL twice, once unconditionally at debug level. A webhook URL is a bearer credential -- anyone holding it can post into the customer's channel. - output.py logged the configured webhook URL in its Slack debug block. The Slack sites matter more than the token one: they run inside the StreamingLogs context, whose upload handler has no level filter and whose loggers are forced to DEBUG, so those records are shipped to Socket. The config line runs before streaming attaches, so it stayed local. Adds socketsecurity/redaction.py: redact_mapping masks values whose field name looks credential-bearing, matching on the name so a field added later is covered without anyone remembering. redact_url keeps a webhook's scheme and host -- which is what the debug line was for -- and drops the secret path and any userinfo. Unset values are left alone, since "no token configured" is useful and is not a secret. CliConfig.to_dict() still returns real values; to_redacted_dict() is the logging view. A serialiser that silently dropped the token would be its own bug. The six actions/cache-poisoning alerts are inert: nothing in this repository uses actions/cache or a `cache:` input, so there is no cache to poison. They are not fixed, they are documented -- pr-preview.yml's build job now says why caching must never be added there, since under workflow_dispatch it executes untrusted PR code in the default branch's cache scope. Recommend dismissing them rather than leaving them open. TRY400/TRY401 move from "not selected" into `ignore` so the decision survives a future family-wide selection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 4886a34 commit 8ee3bad

9 files changed

Lines changed: 253 additions & 13 deletions

File tree

.github/workflows/pr-preview.yml

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,18 @@ jobs:
9393
core.setOutput('head_sha', pullRequest.head.sha);
9494
9595
build:
96+
# This job checks out and executes untrusted pull request code. Under
97+
# workflow_dispatch it runs in the default branch's context, which means its
98+
# Actions cache scope is main's. Do not add caching here -- no `actions/cache`
99+
# step, and no `cache:` input on setup-python -- or a preview build of a
100+
# malicious branch could plant an entry that every workflow on main then
101+
# restores. CodeQL's actions/cache-poisoning/poisonable-step alerts point at
102+
# these steps for exactly that reason; they are inert only while nothing in
103+
# this job writes a cache.
104+
#
105+
# The privilege split is what keeps this safe: this job holds `contents: read`
106+
# and no secrets, and publish-package (which holds `id-token: write`) never
107+
# checks out code -- it only downloads the built artifact.
96108
needs: context
97109
runs-on: ubuntu-latest
98110
timeout-minutes: 10

CONTRIBUTING.md

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,9 +47,21 @@ commits made with `--no-verify` or without hooks installed.
4747
whitespace, so the linter does not duplicate those checks: `E501` and `W291`/
4848
`W293` are deliberately not selected. Everything the formatter cannot reflow is
4949
a string literal -- argparse help text, log messages, the Markdown used to build
50-
pull request comments -- where rewrapping risks silently changing user-visible
51-
text. The PR-comment markup in particular relies on trailing double-spaces as
52-
Markdown hard line breaks.
50+
pull request comments -- where rewrapping risks silently changing text that
51+
customers read.
52+
53+
The pull request comment markup is the clearest case. It uses trailing
54+
double-spaces as Markdown hard line breaks, so stripping them takes the rendered
55+
comment from two lines to one and runs the "Caution" banner into the body text:
56+
57+
```
58+
> **Caution**··
59+
> **Review the following alerts detected in dependencies.**··
60+
```
61+
62+
Rendered with those two trailing spaces the banner sits on its own line. Without
63+
them both lines collapse into a single paragraph. Whitespace inside a string is
64+
content, and the formatter is right to leave it alone.
5365

5466
### One trap worth knowing
5567

pyproject.toml

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -172,11 +172,6 @@ select = [
172172
"PLR1730", # if-statement that should be min()/max()
173173
"PLR5501", # else-if that should be elif
174174
"TRY201", # `raise` instead of re-raising the bound name
175-
# TRY400 (logging.error -> logging.exception in an except block) is
176-
# deliberately absent. This CLI reports expected failures -- a missing
177-
# config file, an APIFailure -- by catching them and logging a readable
178-
# message. Promoting those to .exception() dumps a traceback into the
179-
# user's CI log for conditions that are not crashes, which reads as one.
180175

181176
# --- Keeping the suppressions honest --------------------------------
182177
"PGH", # no blanket `# noqa` / `# type: ignore` -- codes required
@@ -185,7 +180,14 @@ select = [
185180

186181
ignore = [
187182
# Rules whose fix makes this codebase worse. Each was evaluated against the
188-
# actual call sites, not waved off.
183+
# actual call sites, not waved off. Listed here rather than merely left out
184+
# of `select` so the decision survives a future family-wide selection.
185+
"TRY400", # logging.error -> logging.exception inside an except block. This
186+
# CLI reports expected failures -- a missing config file, an
187+
# APIFailure -- by catching them and logging a readable message.
188+
# Promoting those to .exception() dumps a traceback into the
189+
# customer's CI log for conditions that are not crashes.
190+
"TRY401", # only meaningful alongside TRY400.
189191
"SIM108", # if/else -> ternary: the branches here carry explanatory
190192
# comments that a ternary has nowhere to put.
191193
"PERF401", # loop -> comprehension: the loop bodies build multi-line dict

socketsecurity/config.py

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
from socketdev import INTEGRATION_TYPES, IntegrationType
1010

1111
from socketsecurity import __version__
12+
from socketsecurity.redaction import redact_mapping
1213

1314
log = logging.getLogger("socketcli")
1415

@@ -465,6 +466,15 @@ def from_args(cls, args_list: list[str] | None = None) -> "CliConfig": # noqa:
465466
def to_dict(self) -> dict:
466467
return asdict(self)
467468

469+
def to_redacted_dict(self) -> dict:
470+
"""The config as `to_dict`, with credential-bearing fields masked.
471+
472+
Use this for anything that gets logged. `to_dict` still returns the real
473+
values, because a serialiser that silently drops the API token would be
474+
its own kind of bug.
475+
"""
476+
return redact_mapping(asdict(self))
477+
468478

469479
def create_argument_parser() -> argparse.ArgumentParser:
470480
parser = argparse.ArgumentParser(

socketsecurity/output.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
from .core.classes import Diff, Issue
1818
from .core.messages import Messages
1919
from .fossa_compat import build_fossa_report_payload
20+
from .redaction import redact_url
2021

2122

2223
class OutputHandler:
@@ -77,7 +78,7 @@ def handle_output(self, diff_report: Diff) -> None: # noqa: C901
7778
self.logger.debug(f"Slack Mode: {slack_mode}")
7879
self.logger.debug(f"SOCKET_SLACK_ENABLED environment variable: {slack_enabled_env}")
7980
self.logger.debug(f"SOCKET_SLACK_CONFIG_JSON environment variable: {slack_config_env}")
80-
self.logger.debug(f"Slack Webhook URL: {slack_url}")
81+
self.logger.debug(f"Slack Webhook URL: {redact_url(slack_url)}")
8182
self.logger.debug(f"SOCKET_SLACK_BOT_TOKEN: {bot_token_status}")
8283
self.logger.debug(f"Slack Alert Levels: {self.config.slack_plugin.levels}")
8384
if self.config.reach:

socketsecurity/plugins/slack.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
)
1818
from socketsecurity.core.messages import Messages
1919
from socketsecurity.plugins.formatters.slack import format_socket_facts_for_slack
20+
from socketsecurity.redaction import redact_url
2021

2122
from .base import REQUEST_TIMEOUT_SECONDS, Plugin
2223

@@ -112,10 +113,10 @@ def _send_webhook_alerts(self, diff, config: CliConfig): # noqa: C901
112113

113114
message = self.create_slack_blocks_from_diff(filtered_diff, config)
114115

115-
logger.debug(f"Sending diff alerts message to {name} ({url})")
116+
logger.debug(f"Sending diff alerts message to {name} ({redact_url(url)})")
116117

117118
if config.enable_debug:
118-
logger.debug(f"Slack webhook URL: {url}")
119+
logger.debug(f"Slack webhook URL: {redact_url(url)}")
119120
logger.debug(f"Slack webhook name: {name}")
120121
logger.debug(
121122
f"Total diff alerts: {len(diff_alert_source)}, Filtered alerts: {len(filtered_alerts)}"

socketsecurity/redaction.py

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
"""Helpers for keeping credentials out of anything that reaches a log line.
2+
3+
The CLI runs inside other people's pipelines. Its stdout is captured into CI job
4+
logs that are retained, pasted into support tickets, and world-readable for
5+
public repositories. Some of those records are also shipped to Socket by the log
6+
streamer in `core/streaming.py`, whose upload handler has no level filter and
7+
runs with its loggers forced to DEBUG -- so a debug line emitted while streaming
8+
is active leaves the machine entirely.
9+
10+
Nothing here tries to be a general-purpose scrubber. It covers the two shapes
11+
the CLI actually holds: a config mapping with credential-ish field names, and a
12+
webhook URL whose secret lives in the path.
13+
"""
14+
15+
from typing import Any
16+
from urllib.parse import urlsplit
17+
18+
REDACTED = "***redacted***"
19+
20+
# Substrings that mark a field name as carrying a credential. Matching on the
21+
# name rather than an explicit allow-list means a field added later -- a
22+
# `github_token`, say -- is covered without anyone remembering to come back here.
23+
_SENSITIVE_NAME_MARKERS = (
24+
"apikey",
25+
"api_key",
26+
"auth",
27+
"credential",
28+
"passwd",
29+
"password",
30+
"secret",
31+
"token",
32+
"webhook",
33+
)
34+
35+
36+
def is_sensitive_name(name: str) -> bool:
37+
"""Whether a field name looks like it holds a credential."""
38+
lowered = name.lower()
39+
return any(marker in lowered for marker in _SENSITIVE_NAME_MARKERS)
40+
41+
42+
def redact_url(value: Any) -> Any:
43+
"""Reduce a URL to scheme and host, dropping the parts that carry secrets.
44+
45+
A Slack webhook URL is a bearer credential: the secret is the path, and
46+
anyone holding it can post into the customer's channel. Keeping the host
47+
preserves what the debug line was for -- seeing *which* endpoint is
48+
configured -- without printing the credential.
49+
50+
Values that are not absolute URLs are returned unchanged, so placeholders
51+
such as "Not configured" stay readable.
52+
"""
53+
if not isinstance(value, str) or not value:
54+
return value
55+
try:
56+
parts = urlsplit(value)
57+
except ValueError:
58+
return REDACTED
59+
if not parts.scheme or not parts.hostname:
60+
return value
61+
host = parts.hostname
62+
if parts.port:
63+
host = f"{host}:{parts.port}"
64+
return f"{parts.scheme}://{host}/{REDACTED}"
65+
66+
67+
def redact_mapping(data: dict[str, Any]) -> dict[str, Any]:
68+
"""Copy a mapping with credential-bearing values masked.
69+
70+
Empty and unset values are left alone: "no token configured" is useful
71+
debugging information and is not a secret.
72+
"""
73+
redacted: dict[str, Any] = {}
74+
for key, value in data.items():
75+
if not is_sensitive_name(key) or not value:
76+
redacted[key] = value
77+
elif isinstance(value, str) and urlsplit(value).scheme and urlsplit(value).hostname:
78+
redacted[key] = redact_url(value)
79+
else:
80+
redacted[key] = REDACTED
81+
return redacted

socketsecurity/socketcli.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ def cli():
168168
def main_code(): # noqa: C901
169169
config = CliConfig.from_args()
170170
log.info(f"Starting Socket Security CLI version {config.version}")
171-
log.debug(f"config: {config.to_dict()}")
171+
log.debug(f"config: {config.to_redacted_dict()}")
172172

173173
# Warn if strict-blocking is used with disable-blocking
174174
if config.strict_blocking and config.disable_blocking:

tests/unit/test_redaction.py

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
"""Tests for credential redaction in log output.
2+
3+
The CLI runs in customer CI. Its stdout lands in job logs that are retained,
4+
shared in support tickets and public for public repositories, and the log
5+
streamer uploads records to Socket with no level filter. Credentials must not
6+
reach any of that.
7+
"""
8+
9+
import dataclasses
10+
import logging
11+
12+
import pytest
13+
14+
from socketsecurity.config import CliConfig
15+
from socketsecurity.redaction import REDACTED, is_sensitive_name, redact_mapping, redact_url
16+
17+
TOKEN = "sk-not-a-real-token-abc123"
18+
# CliConfig.from_args reads these before falling back to --api-token, and
19+
# socketcli calls load_dotenv() on import, so a developer's .env leaks in as
20+
# soon as another test module imports it. Clear them so these tests are
21+
# hermetic regardless of collection order.
22+
_TOKEN_ENV_VARS = (
23+
"SOCKET_SECURITY_API_KEY",
24+
"SOCKET_SECURITY_API_TOKEN",
25+
"SOCKET_API_KEY",
26+
"SOCKET_API_TOKEN",
27+
)
28+
29+
30+
@pytest.fixture(autouse=True)
31+
def _clear_token_env(monkeypatch):
32+
for name in _TOKEN_ENV_VARS:
33+
monkeypatch.delenv(name, raising=False)
34+
35+
36+
WEBHOOK = "https://hooks.slack.com/services/T00000/B00000/XXXXXXXXsecretXXXXXXXX"
37+
38+
39+
@pytest.mark.parametrize(
40+
"name",
41+
["api_token", "API_TOKEN", "github_token", "slack_webhook", "client_secret", "password", "auth_header", "api_key"],
42+
)
43+
def test_credential_field_names_are_recognised(name):
44+
assert is_sensitive_name(name)
45+
46+
47+
@pytest.mark.parametrize("name", ["repo", "branch", "commit_sha", "target_path", "scm", "enable_debug"])
48+
def test_ordinary_field_names_are_not_recognised(name):
49+
assert not is_sensitive_name(name)
50+
51+
52+
def test_webhook_url_keeps_the_host_and_drops_the_secret_path():
53+
redacted = redact_url(WEBHOOK)
54+
assert redacted == "https://hooks.slack.com/***redacted***"
55+
assert "secret" not in redacted
56+
57+
58+
def test_redact_url_strips_userinfo():
59+
assert redact_url("https://user:hunter2@example.com:8443/path?q=1") == "https://example.com:8443/***redacted***"
60+
assert "hunter2" not in redact_url("https://user:hunter2@example.com/x")
61+
62+
63+
@pytest.mark.parametrize("value", ["Not configured", "", None, "not-a-url"])
64+
def test_non_urls_pass_through_so_placeholders_stay_readable(value):
65+
assert redact_url(value) == value
66+
67+
68+
def test_redact_mapping_masks_secrets_and_keeps_everything_else():
69+
out = redact_mapping({"api_token": TOKEN, "slack_webhook": WEBHOOK, "repo": "acme/widgets", "enable_debug": True})
70+
assert out["api_token"] == REDACTED
71+
assert out["slack_webhook"] == "https://hooks.slack.com/***redacted***"
72+
assert out["repo"] == "acme/widgets"
73+
assert out["enable_debug"] is True
74+
75+
76+
@pytest.mark.parametrize("empty", ["", None])
77+
def test_unset_secrets_are_left_alone(empty):
78+
""" "No token configured" is useful debugging information, not a secret."""
79+
assert redact_mapping({"api_token": empty})["api_token"] == empty
80+
81+
82+
def test_config_to_dict_is_still_a_faithful_serialiser():
83+
config = CliConfig.from_args(["--api-token", TOKEN, "--repo", "acme/widgets"])
84+
assert config.to_dict()["api_token"] == TOKEN
85+
86+
87+
def test_config_to_redacted_dict_masks_the_api_token():
88+
config = CliConfig.from_args(["--api-token", TOKEN, "--repo", "acme/widgets"])
89+
assert config.to_redacted_dict()["api_token"] == REDACTED
90+
assert TOKEN not in str(config.to_redacted_dict())
91+
92+
93+
def test_every_credential_field_on_cliconfig_is_redacted():
94+
"""Guard against a future secret field being added and quietly logged.
95+
96+
Sets every credential-named field to a sentinel and asserts none of them
97+
survive into the redacted view, so `github_token` or similar is covered
98+
without anyone editing this test.
99+
"""
100+
config = CliConfig.from_args(["--api-token", TOKEN, "--repo", "acme/widgets"])
101+
sentinels = {}
102+
for field in dataclasses.fields(CliConfig):
103+
if is_sensitive_name(field.name):
104+
sentinel = f"SENTINEL-{field.name}-value"
105+
setattr(config, field.name, sentinel)
106+
sentinels[field.name] = sentinel
107+
108+
assert sentinels, "expected CliConfig to declare at least one credential field"
109+
rendered = str(config.to_redacted_dict())
110+
for name, sentinel in sentinels.items():
111+
assert sentinel not in rendered, f"{name} leaked into the redacted config"
112+
113+
114+
def test_the_config_debug_line_does_not_emit_the_token(caplog):
115+
"""End-to-end guard on the line CodeQL's sibling alert pointed at."""
116+
config = CliConfig.from_args(["--api-token", TOKEN, "--repo", "acme/widgets"])
117+
log = logging.getLogger("socketcli")
118+
with caplog.at_level(logging.DEBUG, logger="socketcli"):
119+
log.debug(f"config: {config.to_redacted_dict()}")
120+
assert TOKEN not in caplog.text
121+
assert REDACTED in caplog.text

0 commit comments

Comments
 (0)