Skip to content

Commit 977a8fb

Browse files
leliaclaude
andcommitted
fix(comments): require write access to ignore an alert
An @SocketSecurity ignore command suppresses a security finding, but the CLI honored one from any commenter. Comment.author_association was carried on the dataclass and never read, so nothing on the path from comment to suppressed alert asked whether the author could push to the repository. A drive-by ignore-all on an open pull request silenced every finding on it. Gate the ignore bucket in check_for_socket_comments, the one place every consumer goes through. A rejected command is logged with its author and is also absent from the ignore telemetry, which should record what was acted on. GitHub returns author_association with every comment, so the check is free and definitive: OWNER, MEMBER and COLLABORATOR only. GitLab notes carry no equivalent, so project membership is read once per run, and only when an ignore command is actually present. members/all is used rather than a per-user lookup because it answers non-membership with a 200 and an absent id -- CliClient collapses every HTTP error into APIFailure without a status code, so a per-user 404, exactly the outsider case, would be indistinguishable from a token that cannot read the endpoint and would have to fail open. When membership genuinely cannot be read -- a CI_JOB_TOKEN typically cannot -- the command is honored and a warning names the author, so this does not silently break pipelines already relying on ignore commands. Documented alongside the token requirement to get enforcement. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 63e938d commit 977a8fb

5 files changed

Lines changed: 294 additions & 4 deletions

File tree

‎docs/cli-reference.md‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -431,7 +431,7 @@ The launcher can be tuned via the `SOCKET_CLI_COANA_LAUNCHER` environment variab
431431
|:-------------------------|:---------|:--------|:----------------------------------------------------------------------|
432432
| `--ignore-commit-files` | False | False | Ignore commit files |
433433
| `--disable-blocking` | False | False | Non-blocking CI mode: the CLI always exits **0**, even when blocking alerts are present (including with `--strict-blocking`). Also exits 0 on uncaught runtime errors and Socket API failures, so the job is treated as successful while findings and errors are still logged. Takes precedence over `--strict-blocking`. |
434-
| `--disable-ignore` | False | False | Disable support for `@SocketSecurity ignore` commands in PR comments. When set, alerts cannot be suppressed via comments and ignore instructions are hidden from comment output. |
434+
| `--disable-ignore` | False | False | Disable support for `@SocketSecurity ignore` commands in PR comments. When set, alerts cannot be suppressed via comments and ignore instructions are hidden from comment output. See [Who can ignore an alert](#who-can-ignore-an-alert). |
435435
| `--strict-blocking` | False | False | Fail on ANY security policy violations (blocking severity), not just new ones. Only works in diff mode. See [Strict Blocking Mode](#strict-blocking-mode) for details. |
436436
| `--enable-diff` | False | False | Enable diff mode even when using `--integration api` (forces diff mode without SCM integration) |
437437
| `--scm` | False | api | Source control management type |
@@ -690,6 +690,25 @@ The CLI uses intelligent default branch detection with the following priority:
690690
691691
Both `--default-branch` and `--pending-head` parameters are automatically synchronized to ensure consistent behavior.
692692
693+
## Who can ignore an alert
694+
695+
`@SocketSecurity ignore <ecosystem>/<package>@<version>` and
696+
`@SocketSecurity ignore-all` suppress security findings, so the CLI honors them
697+
only from a commenter with write access to the repository. A command from anyone
698+
else is skipped, logged with the author's name, and the alerts it named stay
699+
reported. `--disable-ignore` turns the feature off entirely.
700+
701+
| Provider | How access is determined | If it cannot be determined |
702+
|:---------|:-------------------------|:---------------------------|
703+
| GitHub | The `author_association` returned with each comment. `OWNER`, `MEMBER` and `COLLABORATOR` are honored. | Treated as unauthorized. |
704+
| GitLab | Project membership, read once per run when an ignore command is present. Developer (30) or above is honored. | The command is honored and a warning is logged. |
705+
706+
GitLab notes carry no permission field, so the check needs a `GITLAB_TOKEN` that
707+
can read `GET /projects/:id/members/all`. A `CI_JOB_TOKEN` generally cannot, and
708+
in that case the CLI logs a warning and still honors the command rather than
709+
breaking a pipeline that was already relying on it. Use a personal or group access
710+
token with API read access to get enforcement.
711+
693712
## GitLab Token Configuration
694713
695714
GitLab token/auth behavior and CI examples are documented in [`ci-cd.md`](ci-cd.md).

‎socketsecurity/core/scm/github.py‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,17 @@ def get_comments_for_pr(self) -> dict:
224224
else:
225225
log.error(raw_comments)
226226

227-
return Comments.check_for_socket_comments(comments)
227+
return Comments.check_for_socket_comments(comments, self.is_ignore_authorized)
228+
229+
def is_ignore_authorized(self, comment: Comment) -> bool:
230+
"""Whether a commenter may suppress alerts with @SocketSecurity ignore.
231+
232+
GitHub returns the author's relationship to the repository on every issue
233+
comment, so this costs no extra request and is definitive. A missing value
234+
is treated as unauthorized rather than trusted.
235+
"""
236+
association = (getattr(comment, "author_association", "") or "").upper()
237+
return association in Comments.WRITE_ACCESS_ASSOCIATIONS
228238

229239
def add_socket_comments(
230240
self,

‎socketsecurity/core/scm/gitlab.py‎

Lines changed: 86 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,9 +126,21 @@ def _get_auth_headers(token: str) -> dict:
126126
}
127127

128128
class Gitlab:
129+
# GitLab access levels: 30 Developer, 40 Maintainer, 50 Owner. Reporter (20)
130+
# and Guest (10) cannot push, so they cannot suppress an alert either.
131+
MIN_IGNORE_ACCESS_LEVEL = 30
132+
# Bounded so a project with a very large membership cannot stall a scan. Past
133+
# the cap the answer is "undetermined", handled the same as a failed lookup.
134+
MEMBER_PAGE_SIZE = 100
135+
MEMBER_PAGE_LIMIT = 10
136+
129137
def __init__(self, client: CliClient, config: Optional[GitlabConfig] = None):
130138
self.config = config or GitlabConfig.from_env()
131139
self.client = client
140+
# None until the first ignore comment forces a lookup; stays None when the
141+
# members API cannot be read, which is the "undetermined" state.
142+
self._member_access: Optional[dict] = None
143+
self._member_lookup_attempted = False
132144

133145
def _request_with_fallback(self, **kwargs):
134146
"""
@@ -256,7 +268,80 @@ def get_comments_for_pr(self) -> dict:
256268
comment.body_list = comment.body.split("\n")
257269
else:
258270
log.error(raw_comments)
259-
return Comments.check_for_socket_comments(comments)
271+
return Comments.check_for_socket_comments(comments, self.is_ignore_authorized)
272+
273+
def _load_member_access(self) -> Optional[dict]:
274+
"""Map project member user id -> access level, or None if unreadable.
275+
276+
``members/all`` is used rather than a per-user lookup because it answers
277+
non-membership with a 200 and an absent id. CliClient collapses every HTTP
278+
error into APIFailure without a status code, so a per-user 404 -- exactly
279+
the outsider case this guards against -- would be indistinguishable from a
280+
token that cannot read the endpoint, and would have to fail open.
281+
"""
282+
if self._member_lookup_attempted:
283+
return self._member_access
284+
self._member_lookup_attempted = True
285+
if not self.config.mr_project_id:
286+
return None
287+
288+
access: dict = {}
289+
for page in range(1, Gitlab.MEMBER_PAGE_LIMIT + 1):
290+
path = (
291+
f"projects/{self.config.mr_project_id}/members/all"
292+
f"?per_page={Gitlab.MEMBER_PAGE_SIZE}&page={page}"
293+
)
294+
try:
295+
response = self._request_with_fallback(
296+
path=path,
297+
headers=self.config.headers,
298+
base_url=self.config.api_url
299+
)
300+
members = response.json()
301+
except Exception as error:
302+
log.warning(f"Could not read GitLab project members: {error}")
303+
return None
304+
if not isinstance(members, list):
305+
log.warning("Unexpected GitLab project members response")
306+
return None
307+
for member in members:
308+
if isinstance(member, dict) and member.get("id") is not None:
309+
access[member["id"]] = member.get("access_level") or 0
310+
if len(members) < Gitlab.MEMBER_PAGE_SIZE:
311+
self._member_access = access
312+
return access
313+
314+
log.warning(
315+
f"GitLab project has more than {Gitlab.MEMBER_PAGE_SIZE * Gitlab.MEMBER_PAGE_LIMIT} "
316+
"members; cannot confirm ignore-command authorization"
317+
)
318+
return None
319+
320+
def is_ignore_authorized(self, comment: Comment) -> bool:
321+
"""Whether a commenter may suppress alerts with @SocketSecurity ignore.
322+
323+
GitLab notes carry no permission field, so this costs one members lookup
324+
per run (cached, and only when an ignore command is actually present).
325+
326+
When membership can be read the answer is definitive. When it cannot -- a
327+
CI_JOB_TOKEN generally cannot read the members API -- the command is
328+
honored and a warning is logged, so turning this on does not silently break
329+
pipelines that were already relying on ignore commands. Set a token with
330+
API read access to get enforcement.
331+
"""
332+
access = self._load_member_access()
333+
if access is None:
334+
log.warning(
335+
"Honoring @SocketSecurity ignore from "
336+
f"{Comments.comment_author_name(comment)} without verifying write "
337+
"access: GitLab project membership could not be read. Use a token "
338+
"with API read access to enforce this."
339+
)
340+
return True
341+
342+
author = getattr(comment, "author", None) or {}
343+
user_id = author.get("id")
344+
return access.get(user_id, 0) >= Gitlab.MIN_IGNORE_ACCESS_LEVEL
260345

261346
def add_socket_comments(
262347
self,

‎socketsecurity/core/scm_comments.py‎

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import json
22
import re
3+
from typing import Callable, Optional
34

45
from requests import Response
56

@@ -11,6 +12,17 @@
1112
class Comments:
1213
VIEW_REPORT_PATTERN = re.compile(r"\[View full report\]\(([^)\s]+)\)")
1314

15+
# GitHub stamps every issue comment with the author's relationship to the
16+
# repository. Only these three imply write access; CONTRIBUTOR, MANNEQUIN,
17+
# MENTIONEE, FIRST_TIMER, FIRST_TIME_CONTRIBUTOR and NONE do not.
18+
WRITE_ACCESS_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"})
19+
20+
@staticmethod
21+
def comment_author_name(comment: Comment) -> str:
22+
"""Best-effort display name for a comment author, across providers."""
23+
user = getattr(comment, "user", None) or getattr(comment, "author", None) or {}
24+
return user.get("login") or user.get("username") or "an unknown user"
25+
1426
@staticmethod
1527
def process_response(response: Response) -> dict:
1628
output = {}
@@ -279,7 +291,20 @@ def extract_alert_details_from_row(row: str, ignore_all: bool, ignore_commands:
279291

280292

281293
@staticmethod
282-
def check_for_socket_comments(comments: dict):
294+
def check_for_socket_comments(
295+
comments: dict,
296+
is_authorized: Optional[Callable[[Comment], bool]] = None
297+
):
298+
"""Bucket a pull request's comments into the ones the CLI acts on.
299+
300+
``is_authorized`` gates the ignore bucket, and is the only place that gate
301+
exists: an ``@SocketSecurity ignore`` command suppresses a security alert,
302+
so it is honored only from someone with write access to the repository.
303+
Filtering here rather than at each consumer means the rejected command is
304+
also absent from the ignore telemetry, which should record what was acted
305+
on. Both SCM adapters supply a predicate; omitting it trusts every
306+
commenter and is only appropriate in tests.
307+
"""
283308
socket_comments = {}
284309
for comment_id in comments:
285310
comment = comments[comment_id]
@@ -289,6 +314,13 @@ def check_for_socket_comments(comments: dict):
289314
elif "socket-overview-comment-actions" in comment.body:
290315
socket_comments["overview"] = comment
291316
elif "SocketSecurity ignore".lower() in comment.body_list[0].lower():
317+
if is_authorized is not None and not is_authorized(comment):
318+
log.warning(
319+
"Skipping @SocketSecurity ignore command from "
320+
f"{Comments.comment_author_name(comment)}: no write access "
321+
"to this repository. Alerts remain reported."
322+
)
323+
continue
292324
if "ignore" not in socket_comments:
293325
socket_comments["ignore"] = []
294326
socket_comments["ignore"].append(comment)
Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
"""Who is allowed to suppress an alert with @SocketSecurity ignore.
2+
3+
An ignore command silences a security finding, so it is honored only from someone
4+
with write access to the repository. The gate lives in check_for_socket_comments,
5+
so a rejected command never reaches the ignore parser, the alert filter, or the
6+
ignore telemetry.
7+
"""
8+
from types import SimpleNamespace
9+
10+
import pytest
11+
12+
from socketsecurity.core.classes import Comment
13+
from socketsecurity.core.scm.github import Github
14+
from socketsecurity.core.scm.gitlab import Gitlab
15+
from socketsecurity.core.scm_comments import Comments
16+
17+
18+
def _comment(body="@SocketSecurity ignore npm/lodash@4.17.21", **fields):
19+
return Comment(id=1, body=body, body_list=body.split("\n"), **fields)
20+
21+
22+
# --- GitHub: author_association ships with the comment, no extra request -----
23+
24+
25+
@pytest.mark.parametrize("association", ["OWNER", "MEMBER", "COLLABORATOR"])
26+
def test_github_write_access_may_ignore(association):
27+
github = Github.__new__(Github)
28+
assert github.is_ignore_authorized(_comment(author_association=association)) is True
29+
30+
31+
@pytest.mark.parametrize(
32+
"association",
33+
["CONTRIBUTOR", "FIRST_TIME_CONTRIBUTOR", "FIRST_TIMER", "MANNEQUIN", "NONE", ""],
34+
)
35+
def test_github_without_write_access_may_not_ignore(association):
36+
github = Github.__new__(Github)
37+
assert github.is_ignore_authorized(_comment(author_association=association)) is False
38+
39+
40+
def test_github_missing_association_is_not_trusted():
41+
"""Absent field means unverified, which is not the same as authorized."""
42+
github = Github.__new__(Github)
43+
assert github.is_ignore_authorized(_comment()) is False
44+
45+
46+
def test_unauthorized_command_never_reaches_the_ignore_bucket():
47+
github = Github.__new__(Github)
48+
outsider = _comment(author_association="NONE")
49+
50+
bucketed = Comments.check_for_socket_comments(
51+
{outsider.id: outsider}, github.is_ignore_authorized
52+
)
53+
54+
assert "ignore" not in bucketed
55+
# ...so the alert it named survives.
56+
alert = SimpleNamespace(
57+
pkg_name="lodash", pkg_version="4.17.21", pkg_type="npm", type="malware"
58+
)
59+
assert Comments.remove_alerts(bucketed, [alert]) == [alert]
60+
61+
62+
def test_ignore_all_from_an_outsider_is_rejected_too():
63+
"""ignore-all is the more powerful command; it goes through the same gate."""
64+
github = Github.__new__(Github)
65+
outsider = _comment(body="@SocketSecurity ignore-all", author_association="NONE")
66+
67+
assert "ignore" not in Comments.check_for_socket_comments(
68+
{outsider.id: outsider}, github.is_ignore_authorized
69+
)
70+
71+
72+
# --- GitLab: notes carry no permission field, so membership is looked up -----
73+
74+
75+
def _gitlab(members_pages=None, raises=None):
76+
gitlab = Gitlab.__new__(Gitlab)
77+
gitlab.config = SimpleNamespace(mr_project_id="42", headers={}, api_url="https://gl/api/v4")
78+
gitlab._member_access = None
79+
gitlab._member_lookup_attempted = False
80+
81+
calls = []
82+
83+
def fake_request(**kwargs):
84+
calls.append(kwargs["path"])
85+
if raises:
86+
raise raises
87+
return SimpleNamespace(json=lambda: members_pages.pop(0))
88+
89+
gitlab._request_with_fallback = fake_request
90+
gitlab.calls = calls
91+
return gitlab
92+
93+
94+
@pytest.mark.parametrize("access_level,expected", [(50, True), (40, True), (30, True), (20, False), (10, False)])
95+
def test_gitlab_requires_developer_access(access_level, expected):
96+
gitlab = _gitlab([[{"id": 7, "access_level": access_level}]])
97+
comment = _comment(author={"id": 7, "username": "someone"})
98+
99+
assert gitlab.is_ignore_authorized(comment) is expected
100+
101+
102+
def test_gitlab_non_member_may_not_ignore():
103+
"""The outsider case: a 200 listing that simply does not contain them."""
104+
gitlab = _gitlab([[{"id": 7, "access_level": 40}]])
105+
comment = _comment(author={"id": 999, "username": "outsider"})
106+
107+
assert gitlab.is_ignore_authorized(comment) is False
108+
109+
110+
def test_gitlab_membership_is_fetched_once_per_run():
111+
gitlab = _gitlab([[{"id": 7, "access_level": 40}]])
112+
113+
gitlab.is_ignore_authorized(_comment(author={"id": 7}))
114+
gitlab.is_ignore_authorized(_comment(author={"id": 8}))
115+
116+
assert len(gitlab.calls) == 1
117+
118+
119+
def test_gitlab_paginates_until_a_short_page():
120+
first = [{"id": i, "access_level": 30} for i in range(Gitlab.MEMBER_PAGE_SIZE)]
121+
gitlab = _gitlab([first, [{"id": 999, "access_level": 40}]])
122+
123+
assert gitlab.is_ignore_authorized(_comment(author={"id": 999})) is True
124+
assert len(gitlab.calls) == 2
125+
126+
127+
def test_gitlab_unreadable_membership_honors_the_command_with_a_warning(caplog):
128+
"""A CI_JOB_TOKEN usually cannot read members; that must not break pipelines."""
129+
gitlab = _gitlab(raises=Exception("403 Forbidden"))
130+
131+
with caplog.at_level("WARNING", logger="socketcli"):
132+
allowed = gitlab.is_ignore_authorized(_comment(author={"id": 7, "username": "dev"}))
133+
134+
assert allowed is True
135+
assert "without verifying write access" in caplog.text
136+
137+
138+
def test_gitlab_oversized_membership_is_undetermined():
139+
full = [{"id": i, "access_level": 30} for i in range(Gitlab.MEMBER_PAGE_SIZE)]
140+
gitlab = _gitlab([list(full) for _ in range(Gitlab.MEMBER_PAGE_LIMIT)])
141+
142+
# Undetermined falls back to honoring the command, same as an API failure.
143+
assert gitlab.is_ignore_authorized(_comment(author={"id": 999})) is True
144+
assert len(gitlab.calls) == Gitlab.MEMBER_PAGE_LIMIT

0 commit comments

Comments
 (0)