From 3eb3cd93dbb1f7869b7bcebd0e35897b8757affc Mon Sep 17 00:00:00 2001 From: Ilai Goldschmidt <117302862+ilaigold@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:25:10 +0300 Subject: [PATCH 1/2] fix(tm3): skip umask-masked modes and report permissive umask values The TM3 "Unsafe Defaults" permission pattern `(?:mode|permission|umask)\s*=\s*(?:0?o?777|0?o?666)` reported `mode = 0o666 & ~umask`, which is the umask-respecting default (#644, item B6). It also read umask values backwards: `umask = 0o777`, the most restrictive value, was reported, while `umask 000` and `os.umask(0)`, which make new files world-writable, were not. - mode/permission: a 777/666 mode followed only by `& ~umask` (or `& ~.umask(...)`) up to the end of the expression is skipped. Bits added after the mask (`& ~umask | 0o777`) and masks that are not the umask (`& ~0`) are still reported. - umask assignment or shell command: report a value whose last octal digit leaves the world-write bit unmasked (0, 000, 0000, 004). - umask(...) call: report a zero or leading-0 octal argument with the same rule, except when the result is assigned (`old = os.umask(0)`), which is how Python reads the current umask before restoring it. Signed-off-by: Ilai Goldschmidt <117302862+ilaigold@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 --- .../analyzers/static_patterns_tool_misuse.py | 16 ++- tests/unit/test_patterns_new.py | 102 ++++++++++++++++++ 2 files changed, 116 insertions(+), 2 deletions(-) diff --git a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py index fd2cbf162..763f682d7 100644 --- a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py +++ b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py @@ -355,8 +355,20 @@ # Overly permissive CORS / access (r"(?:CORS|cors)[^=]*=\s*['\"]?\*['\"]?", 0.65), (r"(?:allow|access)[_-]?(?:origin|hosts?)\s*=\s*['\"]?\*['\"]?", 0.7), - # Unsafe permissions - (r"(?:mode|permission|umask)\s*=\s*(?:0?o?777|0?o?666)", 0.8), + # Unsafe permissions. A mode masked with the umask (0o666 & ~umask) respects it, + # like a plain open(), unless more bits are added after the mask. + ( + r"(?:mode|permission)\s*=\s*(?:0?o?777|0?o?666)" + r"(?![ \t]*&[ \t]*~[ \t]*[\w.]*umask\b(?:\([^()\n]*\))?[ \t]*(?:[),;\]}#]|//|$))", + 0.8, + ), + # A umask whose last octal digit leaves the world-write bit (0o002) unmasked, such + # as 000, makes new files world-writable. A umask such as 0o777 is restrictive. + (r"umask(?:\s*=\s*|[ \t]+)(?:0o)?[0-7]*[0145]\b", 0.8), + # umask(0) as a statement or inside an expression. In a call, a number without a + # leading 0 is decimal. `old = os.umask(0)` is how Python reads the umask before + # restoring it, so a call whose result is assigned is not reported. + (r"(? None: def test_tm3_detected(self, content: str, filename: str, filetype: str) -> None: assert any(f.rule_id == "TM3" for f in tm_mod.analyze(content, filename, filetype)) + @pytest.mark.parametrize( + "content,filename,filetype,expected", + [ + pytest.param("mode = 0o777", "fs.py", "python", "mode = 0o777", id="mode_777"), + pytest.param( + "permission = 0o666", "fs.py", "python", "permission = 0o666", id="permission_666" + ), + pytest.param( + "mode = 0o777 & ~umask | 0o777", + "fs.py", + "python", + "mode = 0o777", + id="bits_added_after_umask_mask", + ), + pytest.param( + "mode = 0o777 & ~0", "fs.py", "python", "mode = 0o777", id="mask_is_not_the_umask" + ), + pytest.param("umask 000", "run.sh", "shell", "umask 000", id="shell_umask_000"), + pytest.param("UMASK=0000", "env.sh", "shell", "UMASK=0000", id="env_umask_0000"), + pytest.param("umask = 0o000", "cfg.py", "python", "umask = 0o000", id="assign_0o000"), + pytest.param("umask 004", "run.sh", "shell", "umask 004", id="world_write_unmasked"), + pytest.param( + "umask=0o7770", "cfg.py", "python", "umask=0o7770", id="long_value_ending_in_0" + ), + pytest.param("os.umask(0)", "fs.py", "python", "os.umask(0)", id="py_umask_call"), + pytest.param( + "process.umask(0o000);", "fs.js", "javascript", "process.umask(0o000)", id="js_call" + ), + pytest.param("umask(0);", "main.c", "c", "umask(0)", id="c_umask_call"), + pytest.param( + "fd = os.open(p, flags, mode=0o666 & ~os.umask(0))", + "fs.py", + "python", + "os.umask(0)", + id="umask_zero_inside_mask_expression", + ), + ], + ) + def test_tm3_world_writable_mode_or_umask_detected( + self, content: str, filename: str, filetype: str, expected: str + ) -> None: + """A world-writable mode, or a umask that leaves world-write unmasked, is TM3.""" + findings = tm_mod.analyze(content, filename, filetype) + assert [f.matched_text for f in findings if f.rule_id == "TM3"] == [expected] + + @pytest.mark.parametrize( + "content,filename,filetype", + [ + pytest.param("mode = 0o666 & ~umask", "fs.py", "python", id="mode_masked_by_umask"), + pytest.param( + "os.chmod(path, mode=0o777 & ~current_umask)", + "fs.py", + "python", + id="mode_masked_in_call", + ), + pytest.param( + "const mode = 0o666 & ~process.umask();", + "fs.js", + "javascript", + id="mode_masked_by_process_umask", + ), + pytest.param("umask = 0o777", "cfg.py", "python", id="most_restrictive_umask"), + pytest.param("UMASK=0777", "env.sh", "shell", id="restrictive_env_umask"), + pytest.param("umask 077", "run.sh", "shell", id="umask_077"), + pytest.param("umask 022", "run.sh", "shell", id="umask_022"), + pytest.param("os.umask(0o022)", "fs.py", "python", id="umask_call_022"), + pytest.param("os.umask(18)", "fs.py", "python", id="decimal_umask_call_022"), + pytest.param( + "umask = os.umask(0)\nos.umask(umask)\nos.chmod(tmp, 0o666 & ~umask)", + "fs.py", + "python", + id="read_and_restore_umask", + ), + pytest.param( + "const previous = process.umask(0);", "fs.js", "javascript", id="js_read_umask" + ), + ], + ) + def test_tm3_umask_masked_mode_or_restrictive_umask_not_flagged( + self, content: str, filename: str, filetype: str + ) -> None: + """A mode masked with the umask and a restrictive umask are not unsafe defaults.""" + findings = tm_mod.analyze(content, filename, filetype) + assert not [f.matched_text for f in findings if f.rule_id == "TM3"] + + @pytest.mark.parametrize( + "content", + [ + pytest.param("mode=0o777 & ~" + "a" * 50_000, id="long_mask_operand"), + pytest.param("mode=0o777 &" + " " * 50_000 + "~umask |", id="long_gap_before_mask"), + pytest.param("mode=0o777 & ~os.umask(" + "0" * 50_000, id="unclosed_mask_call"), + pytest.param("umask " + "0" * 50_000 + "2", id="long_umask_value"), + pytest.param("umask(0" + "0" * 50_000, id="unclosed_umask_call"), + pytest.param("a." * 25_000 + "umask(", id="long_dotted_name"), + pytest.param("umask umask " * 4_000, id="repeated_umask_words"), + ], + ) + def test_tm3_permission_patterns_are_linear(self, content: str) -> None: + started = time.perf_counter() + tm_mod.analyze(content, "fs.py", "python") + assert time.perf_counter() - started < 1.0 + @pytest.mark.parametrize( "content,filename,filetype", [ From 94547bd38dc4fd39c2b796115ae193dbd812cd21 Mon Sep 17 00:00:00 2001 From: Ilai Goldschmidt <117302862+ilaigold@users.noreply.github.com> Date: Wed, 7 Oct 2026 09:19:54 -0400 Subject: [PATCH 2/2] 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 <117302862+ilaigold@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 --- .../analyzers/static_patterns_tool_misuse.py | 63 +++++++++++-- tests/unit/test_patterns_new.py | 92 +++++++++++++++++++ 2 files changed, 145 insertions(+), 10 deletions(-) diff --git a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py index 763f682d7..12ba15f4e 100644 --- a/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py +++ b/src/skillspector/nodes/analyzers/static_patterns_tool_misuse.py @@ -341,6 +341,29 @@ TM2_PATTERNS = TM2_CODE_PATTERNS + TM2_PROSE_PATTERNS # TM3: Unsafe Defaults — overly permissive default settings +# +# A umask leaves new files world-writable when its world-write bit (0o002) is unset. +# In octal digits that is a last digit of 0, 1, 4 or 5. +_UMASK_OCTAL_WORLD_WRITE = r"[0-7]*[0145]\b" +# Python, JavaScript and C read a number as octal only with a 0o prefix or a leading +# 0, so a bare 14 is 0o016 (world-write masked) and a bare 8 is 0o010 (unmasked). A +# decimal N leaves the bit unset when N % 4 is 0 or 1, which its last two digits decide. +_UMASK_NUMBER_WORLD_WRITE = ( + rf"(?:0(?:o?{_UMASK_OCTAL_WORLD_WRITE})?\b" + r"|[1-9]\d*+(?<=[02468][014589]|[13579][2367]|\D[14589])\b)" +) +# umask 000, or an assigned value such as umask = 0o000 or umask = 8. +_TM3_UMASK = ( + rf"umask(?:[ \t]+(?:0o)?{_UMASK_OCTAL_WORLD_WRITE}|\s*=\s*{_UMASK_NUMBER_WORLD_WRITE})", + 0.8, +) +# Shell reads every umask value as octal, including a variable such as UMASK=14 that +# is later passed to the umask command. +_TM3_SHELL_UMASK = (rf"umask(?:\s*=\s*|[ \t]+)(?:0o)?{_UMASK_OCTAL_WORLD_WRITE}", 0.8) +# umask(0) as a statement or inside an expression. `old = os.umask(0)` is how Python +# reads the umask before restoring it, so analyze() skips a call whose result is +# assigned (see _is_assigned_value). +_TM3_UMASK_CALL = (rf"(?=?:.]|(?:and|or)\b))))", 0.8, ), - # A umask whose last octal digit leaves the world-write bit (0o002) unmasked, such - # as 000, makes new files world-writable. A umask such as 0o777 is restrictive. - (r"umask(?:\s*=\s*|[ \t]+)(?:0o)?[0-7]*[0145]\b", 0.8), - # umask(0) as a statement or inside an expression. In a call, a number without a - # leading 0 is decimal. `old = os.umask(0)` is how Python reads the umask before - # restoring it, so a call whose result is assigned is not reported. - (r"(? str: return content[line_start:line_end] +def _is_assigned_value(content: str, start: int) -> bool: + """Return whether the expression at *start* is the value of an assignment. + + Whitespace, line continuations and opening parentheses after the ``=`` are + skipped, so ``old = (os.umask(0))`` reads like ``old = os.umask(0)``. + ``==``, ``!=``, ``<=`` and ``>=`` compare rather than assign. + """ + index = start + while index and content[index - 1] in " \t\r\n\\(": + index -= 1 + if not index or content[index - 1] != "=": + return False + return index < 2 or content[index - 2] not in "=!<>" + + def analyze(content: str, file_path: str, file_type: str) -> list[AnalyzerFinding]: """Analyze content for tool misuse patterns (TM1–TM3).""" findings: list[AnalyzerFinding] = [] @@ -4148,13 +4188,16 @@ def ctx(start: int) -> str: complete_match=match.group(0), ) ) - for pattern, confidence in TM3_PATTERNS: + for pattern, confidence in _TM3_SHELL_PATTERNS if file_type == "shell" else TM3_PATTERNS: matches = ( static_runner.iter_paragraph_matches if (pattern, confidence) in TM3_PROSE_PATTERNS else re.finditer ) + skip_assigned = (pattern, confidence) == _TM3_UMASK_CALL for match in matches(pattern, content, re.IGNORECASE | re.MULTILINE): + if skip_assigned and _is_assigned_value(content, match.start()): + continue line_num = get_line_number(content, match.start()) findings.append( AnalyzerFinding( diff --git a/tests/unit/test_patterns_new.py b/tests/unit/test_patterns_new.py index e59752dac..b6a5df4c5 100644 --- a/tests/unit/test_patterns_new.py +++ b/tests/unit/test_patterns_new.py @@ -1826,6 +1826,52 @@ def test_tm3_detected(self, content: str, filename: str, filetype: str) -> None: "os.umask(0)", id="umask_zero_inside_mask_expression", ), + pytest.param( + "mode = 0o666 & ~current_umask // 512", + "fs.py", + "python", + "mode = 0o666", + id="floor_division_after_umask_mask", + ), + pytest.param( + "os.chmod(path, mode=0o666 & ~umask\n | 0o666)", + "fs.py", + "python", + "mode=0o666", + id="bits_added_on_continued_line", + ), + pytest.param( + "os.chmod(path, mode=0o666 & ~umask\r\n | 0o666)", + "fs.py", + "python", + "mode=0o666", + id="bits_added_on_continued_crlf_line", + ), + pytest.param( + "os.chmod(path, mode=0o666 & ~umask # keep the umask\n | 0o666)", + "fs.py", + "python", + "mode=0o666", + id="bits_added_after_comment_line", + ), + pytest.param( + "os.chmod(path, mode=0o666 & ~umask\n or 0o777)", + "fs.py", + "python", + "mode=0o666", + id="or_on_continued_line", + ), + pytest.param( + "changed = old == os.umask(0)", + "fs.py", + "python", + "os.umask(0)", + id="umask_call_compared_not_assigned", + ), + pytest.param("umask = 8", "cfg.py", "python", "umask = 8", id="decimal_8_is_0o010"), + pytest.param("os.umask(8)", "fs.py", "python", "os.umask(8)", id="decimal_call_0o010"), + pytest.param("umask 14", "run.sh", "shell", "umask 14", id="shell_umask_is_octal"), + pytest.param("UMASK=14", "env.sh", "shell", "UMASK=14", id="shell_variable_is_octal"), ], ) def test_tm3_world_writable_mode_or_umask_detected( @@ -1866,6 +1912,43 @@ def test_tm3_world_writable_mode_or_umask_detected( pytest.param( "const previous = process.umask(0);", "fs.js", "javascript", id="js_read_umask" ), + pytest.param( + "mode = 0o666 & ~umask\r\nfd = os.open(path, flags, mode)\r\n", + "fs.py", + "python", + id="mode_masked_by_umask_crlf", + ), + pytest.param( + "mode = 0o666 & ~umask # respect the umask\nfd = os.open(path, flags, mode)\n", + "fs.py", + "python", + id="mode_masked_by_umask_with_comment", + ), + pytest.param( + "mode = 0o666 & ~umask # respect the umask\r\nif exists:\r\n pass\r\n", + "fs.py", + "python", + id="mode_masked_by_umask_with_comment_crlf", + ), + pytest.param( + "os.chmod(path, mode=0o666 & ~umask\n )", + "fs.py", + "python", + id="mask_ends_before_closing_line", + ), + pytest.param( + "old = os.umask(0)\nos.umask(old)", "fs.py", "python", id="read_umask_two_spaces" + ), + pytest.param( + "old =\tos.umask(0)\nos.umask(old)", "fs.py", "python", id="read_umask_tab" + ), + pytest.param( + "old = (os.umask(0))\nos.umask(old)", + "fs.py", + "python", + id="read_umask_parenthesized", + ), + pytest.param("umask = 14", "cfg.py", "python", id="decimal_14_is_0o016"), ], ) 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( pytest.param("umask(0" + "0" * 50_000, id="unclosed_umask_call"), pytest.param("a." * 25_000 + "umask(", id="long_dotted_name"), pytest.param("umask umask " * 4_000, id="repeated_umask_words"), + pytest.param("old =" + " " * 50_000 + "os.umask(0)", id="long_gap_after_assignment"), + pytest.param("(" * 50_000 + "os.umask(0)", id="long_paren_run_before_call"), + pytest.param("old = (os.umask(0))\n" * 10_000, id="repeated_assigned_umask_calls"), + pytest.param("umask = 1" + "0" * 50_000 + "4", id="long_decimal_umask"), + pytest.param( + "mode=0o777 & ~umask\n" + " " * 50_000 + "|", id="long_indent_on_continued_line" + ), + pytest.param("mode=0o777 & ~umask #" + "c" * 50_000, id="long_comment_after_mask"), + pytest.param("mode=0o777 & ~umask # c\n" * 5_000, id="repeated_commented_masks"), ], ) def test_tm3_permission_patterns_are_linear(self, content: str) -> None: