Skip to content

Commit 94547bd

Browse files
ilaigoldclaude
ilaigold
and
claude
committed
fix(tm3): end the umask mask at the expression end and read umask values by base
Review follow-ups for the TM3 umask patterns: - mode/permission: `//` no longer ends the masked expression, because in Python it is floor division (`0o666 & ~current_umask // 512` is 0o666). The end of a line, also after a # comment, ends it only when the next line does not start with an operator, `and` or `or`, so a call continued with `| 0o666` on the next line is reported. The comment is matched up to 240 characters to keep the check linear; a longer one is reported as before. A CRLF line ending is accepted the same way as LF, so `mode = 0o666 & ~umask` in a CRLF file is no longer reported. - umask(...) call: the read-and-restore exemption skips spaces, tabs, line breaks and opening parentheses back to the `=`, so `old = os.umask(0)` and `old = (os.umask(0))` behave like `old = os.umask(0)`. `==`, `!=`, `<=` and `>=` are comparisons and are still reported. - umask values: outside shell files a number is octal only with a 0o prefix or a leading 0. A bare number is decimal, so `umask = 14` (0o016) is not reported and `umask = 8` (0o010) is, in assignments and calls alike. Shell files and the `umask NNN` command still read every value as octal. Signed-off-by: Ilai Goldschmidt <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
1 parent 3eb3cd9 commit 94547bd

2 files changed

Lines changed: 145 additions & 10 deletions

File tree

‎src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py‎

Lines changed: 53 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -341,6 +341,29 @@
341341
TM2_PATTERNS = TM2_CODE_PATTERNS + TM2_PROSE_PATTERNS
342342

343343
# TM3: Unsafe Defaults — overly permissive default settings
344+
#
345+
# A umask leaves new files world-writable when its world-write bit (0o002) is unset.
346+
# In octal digits that is a last digit of 0, 1, 4 or 5.
347+
_UMASK_OCTAL_WORLD_WRITE = r"[0-7]*[0145]\b"
348+
# Python, JavaScript and C read a number as octal only with a 0o prefix or a leading
349+
# 0, so a bare 14 is 0o016 (world-write masked) and a bare 8 is 0o010 (unmasked). A
350+
# decimal N leaves the bit unset when N % 4 is 0 or 1, which its last two digits decide.
351+
_UMASK_NUMBER_WORLD_WRITE = (
352+
rf"(?:0(?:o?{_UMASK_OCTAL_WORLD_WRITE})?\b"
353+
r"|[1-9]\d*+(?<=[02468][014589]|[13579][2367]|\D[14589])\b)"
354+
)
355+
# umask 000, or an assigned value such as umask = 0o000 or umask = 8.
356+
_TM3_UMASK = (
357+
rf"umask(?:[ \t]+(?:0o)?{_UMASK_OCTAL_WORLD_WRITE}|\s*=\s*{_UMASK_NUMBER_WORLD_WRITE})",
358+
0.8,
359+
)
360+
# Shell reads every umask value as octal, including a variable such as UMASK=14 that
361+
# is later passed to the umask command.
362+
_TM3_SHELL_UMASK = (rf"umask(?:\s*=\s*|[ \t]+)(?:0o)?{_UMASK_OCTAL_WORLD_WRITE}", 0.8)
363+
# umask(0) as a statement or inside an expression. `old = os.umask(0)` is how Python
364+
# reads the umask before restoring it, so analyze() skips a call whose result is
365+
# assigned (see _is_assigned_value).
366+
_TM3_UMASK_CALL = (rf"(?<![\w.])(?:\w+\.)*umask\(\s*{_UMASK_NUMBER_WORLD_WRITE}\s*\)", 0.8)
344367
TM3_CODE_PATTERNS = [
345368
# TLS/SSL verification disabled
346369
(r"verify\s*=\s*False", 0.75),
@@ -356,19 +379,20 @@
356379
(r"(?:CORS|cors)[^=]*=\s*['\"]?\*['\"]?", 0.65),
357380
(r"(?:allow|access)[_-]?(?:origin|hosts?)\s*=\s*['\"]?\*['\"]?", 0.7),
358381
# Unsafe permissions. A mode masked with the umask (0o666 & ~umask) respects it,
359-
# like a plain open(), unless more bits are added after the mask.
382+
# like a plain open(), when the mask ends the expression: a closing bracket, a
383+
# separator, or the end of a line (LF or CRLF, after an optional # comment) whose
384+
# next line does not continue it with an operator, `and` or `or`. Any other
385+
# operator, such as | 0o777 or the floor division // 512, can change the result
386+
# and is still reported. The comment is bounded so the check stays linear.
360387
(
361388
r"(?:mode|permission)\s*=\s*(?:0?o?777|0?o?666)"
362-
r"(?![ \t]*&[ \t]*~[ \t]*[\w.]*umask\b(?:\([^()\n]*\))?[ \t]*(?:[),;\]}#]|//|$))",
389+
r"(?![ \t]*&[ \t]*~[ \t]*[\w.]*umask\b(?:\([^()\n]*\))?[ \t]*"
390+
r"(?:[),;\]}]|(?:#[^\n]{0,240}|\r)?$"
391+
r"(?!\n[ \t]*(?:[-+*/%|&^<>=?:.]|(?:and|or)\b))))",
363392
0.8,
364393
),
365-
# A umask whose last octal digit leaves the world-write bit (0o002) unmasked, such
366-
# as 000, makes new files world-writable. A umask such as 0o777 is restrictive.
367-
(r"umask(?:\s*=\s*|[ \t]+)(?:0o)?[0-7]*[0145]\b", 0.8),
368-
# umask(0) as a statement or inside an expression. In a call, a number without a
369-
# leading 0 is decimal. `old = os.umask(0)` is how Python reads the umask before
370-
# restoring it, so a call whose result is assigned is not reported.
371-
(r"(?<![\w.])(?<!=)(?<!= )(?:\w+\.)*umask\(\s*0(?:o?[0-7]*[0145])?\s*\)", 0.8),
394+
_TM3_UMASK,
395+
_TM3_UMASK_CALL,
372396
(r"world[_-]?(?:readable|writable|executable)", 0.7),
373397
# Debug/dev mode in production
374398
(r"(?:debug|dev|development)[_-]?mode\s*=\s*(?:True|true|1|on|yes|enable)", 0.6),
@@ -393,6 +417,7 @@
393417
),
394418
]
395419
TM3_PATTERNS = TM3_CODE_PATTERNS + TM3_PROSE_PATTERNS
420+
_TM3_SHELL_PATTERNS = [_TM3_SHELL_UMASK if entry == _TM3_UMASK else entry for entry in TM3_PATTERNS]
396421

397422
# TM4: Privileged Kubernetes Workload — manifest/CLI primitives that grant
398423
# node/host takeover (the cluster-scale counterpart of a privileged container).
@@ -4048,6 +4073,21 @@ def _line_containing(content: str, start: int, end: int) -> str:
40484073
return content[line_start:line_end]
40494074

40504075

4076+
def _is_assigned_value(content: str, start: int) -> bool:
4077+
"""Return whether the expression at *start* is the value of an assignment.
4078+
4079+
Whitespace, line continuations and opening parentheses after the ``=`` are
4080+
skipped, so ``old = (os.umask(0))`` reads like ``old = os.umask(0)``.
4081+
``==``, ``!=``, ``<=`` and ``>=`` compare rather than assign.
4082+
"""
4083+
index = start
4084+
while index and content[index - 1] in " \t\r\n\\(":
4085+
index -= 1
4086+
if not index or content[index - 1] != "=":
4087+
return False
4088+
return index < 2 or content[index - 2] not in "=!<>"
4089+
4090+
40514091
def analyze(content: str, file_path: str, file_type: str) -> list[AnalyzerFinding]:
40524092
"""Analyze content for tool misuse patterns (TM1–TM3)."""
40534093
findings: list[AnalyzerFinding] = []
@@ -4148,13 +4188,16 @@ def ctx(start: int) -> str:
41484188
complete_match=match.group(0),
41494189
)
41504190
)
4151-
for pattern, confidence in TM3_PATTERNS:
4191+
for pattern, confidence in _TM3_SHELL_PATTERNS if file_type == "shell" else TM3_PATTERNS:
41524192
matches = (
41534193
static_runner.iter_paragraph_matches
41544194
if (pattern, confidence) in TM3_PROSE_PATTERNS
41554195
else re.finditer
41564196
)
4197+
skip_assigned = (pattern, confidence) == _TM3_UMASK_CALL
41574198
for match in matches(pattern, content, re.IGNORECASE | re.MULTILINE):
4199+
if skip_assigned and _is_assigned_value(content, match.start()):
4200+
continue
41584201
line_num = get_line_number(content, match.start())
41594202
findings.append(
41604203
AnalyzerFinding(

‎tests/unit/test_patterns_new.py‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1826,6 +1826,52 @@ def test_tm3_detected(self, content: str, filename: str, filetype: str) -> None:
18261826
"os.umask(0)",
18271827
id="umask_zero_inside_mask_expression",
18281828
),
1829+
pytest.param(
1830+
"mode = 0o666 & ~current_umask // 512",
1831+
"fs.py",
1832+
"python",
1833+
"mode = 0o666",
1834+
id="floor_division_after_umask_mask",
1835+
),
1836+
pytest.param(
1837+
"os.chmod(path, mode=0o666 & ~umask\n | 0o666)",
1838+
"fs.py",
1839+
"python",
1840+
"mode=0o666",
1841+
id="bits_added_on_continued_line",
1842+
),
1843+
pytest.param(
1844+
"os.chmod(path, mode=0o666 & ~umask\r\n | 0o666)",
1845+
"fs.py",
1846+
"python",
1847+
"mode=0o666",
1848+
id="bits_added_on_continued_crlf_line",
1849+
),
1850+
pytest.param(
1851+
"os.chmod(path, mode=0o666 & ~umask # keep the umask\n | 0o666)",
1852+
"fs.py",
1853+
"python",
1854+
"mode=0o666",
1855+
id="bits_added_after_comment_line",
1856+
),
1857+
pytest.param(
1858+
"os.chmod(path, mode=0o666 & ~umask\n or 0o777)",
1859+
"fs.py",
1860+
"python",
1861+
"mode=0o666",
1862+
id="or_on_continued_line",
1863+
),
1864+
pytest.param(
1865+
"changed = old == os.umask(0)",
1866+
"fs.py",
1867+
"python",
1868+
"os.umask(0)",
1869+
id="umask_call_compared_not_assigned",
1870+
),
1871+
pytest.param("umask = 8", "cfg.py", "python", "umask = 8", id="decimal_8_is_0o010"),
1872+
pytest.param("os.umask(8)", "fs.py", "python", "os.umask(8)", id="decimal_call_0o010"),
1873+
pytest.param("umask 14", "run.sh", "shell", "umask 14", id="shell_umask_is_octal"),
1874+
pytest.param("UMASK=14", "env.sh", "shell", "UMASK=14", id="shell_variable_is_octal"),
18291875
],
18301876
)
18311877
def test_tm3_world_writable_mode_or_umask_detected(
@@ -1866,6 +1912,43 @@ def test_tm3_world_writable_mode_or_umask_detected(
18661912
pytest.param(
18671913
"const previous = process.umask(0);", "fs.js", "javascript", id="js_read_umask"
18681914
),
1915+
pytest.param(
1916+
"mode = 0o666 & ~umask\r\nfd = os.open(path, flags, mode)\r\n",
1917+
"fs.py",
1918+
"python",
1919+
id="mode_masked_by_umask_crlf",
1920+
),
1921+
pytest.param(
1922+
"mode = 0o666 & ~umask # respect the umask\nfd = os.open(path, flags, mode)\n",
1923+
"fs.py",
1924+
"python",
1925+
id="mode_masked_by_umask_with_comment",
1926+
),
1927+
pytest.param(
1928+
"mode = 0o666 & ~umask # respect the umask\r\nif exists:\r\n pass\r\n",
1929+
"fs.py",
1930+
"python",
1931+
id="mode_masked_by_umask_with_comment_crlf",
1932+
),
1933+
pytest.param(
1934+
"os.chmod(path, mode=0o666 & ~umask\n )",
1935+
"fs.py",
1936+
"python",
1937+
id="mask_ends_before_closing_line",
1938+
),
1939+
pytest.param(
1940+
"old = os.umask(0)\nos.umask(old)", "fs.py", "python", id="read_umask_two_spaces"
1941+
),
1942+
pytest.param(
1943+
"old =\tos.umask(0)\nos.umask(old)", "fs.py", "python", id="read_umask_tab"
1944+
),
1945+
pytest.param(
1946+
"old = (os.umask(0))\nos.umask(old)",
1947+
"fs.py",
1948+
"python",
1949+
id="read_umask_parenthesized",
1950+
),
1951+
pytest.param("umask = 14", "cfg.py", "python", id="decimal_14_is_0o016"),
18691952
],
18701953
)
18711954
def test_tm3_umask_masked_mode_or_restrictive_umask_not_flagged(
@@ -1885,6 +1968,15 @@ def test_tm3_umask_masked_mode_or_restrictive_umask_not_flagged(
18851968
pytest.param("umask(0" + "0" * 50_000, id="unclosed_umask_call"),
18861969
pytest.param("a." * 25_000 + "umask(", id="long_dotted_name"),
18871970
pytest.param("umask umask " * 4_000, id="repeated_umask_words"),
1971+
pytest.param("old =" + " " * 50_000 + "os.umask(0)", id="long_gap_after_assignment"),
1972+
pytest.param("(" * 50_000 + "os.umask(0)", id="long_paren_run_before_call"),
1973+
pytest.param("old = (os.umask(0))\n" * 10_000, id="repeated_assigned_umask_calls"),
1974+
pytest.param("umask = 1" + "0" * 50_000 + "4", id="long_decimal_umask"),
1975+
pytest.param(
1976+
"mode=0o777 & ~umask\n" + " " * 50_000 + "|", id="long_indent_on_continued_line"
1977+
),
1978+
pytest.param("mode=0o777 & ~umask #" + "c" * 50_000, id="long_comment_after_mask"),
1979+
pytest.param("mode=0o777 & ~umask # c\n" * 5_000, id="repeated_commented_masks"),
18881980
],
18891981
)
18901982
def test_tm3_permission_patterns_are_linear(self, content: str) -> None:

0 commit comments

Comments
 (0)