fix(tm3): skip umask-masked modes and report permissive umask values

[ilaigold]

Refs #644 (item B6)

I was going through false positives from a --no-llm scan of my skills and looked at the TM3 permission pattern for #644 B6, where mode = 0o666 & ~umask gets flagged even though that's the umask-respecting default a plain open() uses. The same pattern also has umask backwards. A umask lists the bits to remove, so umask = 0o777 is the most restrictive value, and it gets flagged. Meanwhile umask 000 and os.umask(0), which make new files world-writable, don't.

Repro with skillspector scan <dir> --no-llm --format json: put mode = 0o666 & ~CURRENT_UMASK, umask = 0o777 and os.umask(0) in a Python script and umask 000 in a shell script. main @ 5485cda flags the first two, this branch flags the last two.

The one pattern is now three, with the same confidence and severity as before:

• mode/permission 777 or 666: skipped when the rest of the expression is only & ~<name ending in umask> or a call like os.umask(...). Bits added after the mask (& ~umask | 0o777) or a mask that isn't the umask (& ~0) still get flagged.
• umask assignment or shell command: flagged when the last octal digit leaves world-write unmasked (0, 1, 4 or 5). So 000 is flagged and 022, 077 and 0777 aren't.
• umask(...) call: same rule for 0 or a leading-0 octal. A bare non-zero number in a call is decimal, so os.umask(18) (0o022) isn't flagged.

The one you'll probably ask about: an assigned call like old = os.umask(0) isn't flagged, because that's the standard way to read the current umask before restoring it (anthropics/claude-plugins-official does exactly this in plugins/code-modernization/scripts/compare.py). Code that creates files between that call and the restore would slip through, but main doesn't catch that either.

Detections: nothing world-writable that main catches is lost. The only lines that stop matching are 777/666 modes masked by the umask and umasks that already mask world-write. On 20,000 generated <key> = <value><suffix> lines, 7,392 lose TM3 and all of them fall into those two cases.

Tests: paired cases in tests/unit/test_patterns_new.py::TestToolMisuse, 13 malicious with exact matched_text and 11 benign (masked modes, restrictive umasks, the read-and-restore idiom in Python and JS).

ReDoS: no nested quantifiers over overlapping text, the lookbehinds are fixed-width, and the mask lookahead stays on one line. test_tm3_permission_patterns_are_linear runs 7 adversarial ~50k-character inputs with a 1 s limit each. Outside the suite, the new regexes took at most 0.01 s on 10 adversarial ~50k-character inputs and grew linearly up to 800k characters.

Checked on main @ 5485cda:

• uv run pytest -m "not integration and not provider" tests/ -q: 10336 passed, 0 failed
• uv run pytest -m "not integration and not provider" tests/unit/test_patterns_new.py -k "tm3 or TM3" -q: 38 passed, and 14 of them fail with only static_patterns_tool_misuse.py reverted to main
• uv run make lint and uv run make format-check pass

🤖 Generated with Claude Code

[yashrajp22]

The usual masked-mode and restrictive-umask cases now behave as intended, but the four inline cases still need attention. The unsafe-expression and assignment-formatting cases regress detection, while CRLF leaves the B6 false positive unresolved.

Validation: 159 selected tests pass against HEAD in both source and fresh wheel installs. Completed 48 corpus scans and 40 focused scans across BASE b359c66 and HEAD 3eb3cd9, with source/wheel results agreeing. The expected 14 BASE failures confirm the intended changes. Main-only differences were assessed separately from the authored diff at 025c65e. Greptile feedback was adjudicated; the decimal case and additional multiline trigger are source-confirmed. These checks cover offline behavior, with existing partial-analysis limitations retained.

[ilaigold]

Thanks for the careful review, all four were real. Fixed in a new commit on top of 3eb3cd9.

1. Whole expression: // no longer ends the masked expression, so 0o666 & ~current_umask // 512 is reported again. A line end (also after a # comment) only ends it when the next line doesn't start with an operator, and or or, so your continued | 0o666 call is reported, while a mask followed by a closing ) on the next line is still skipped. Tests: floor_division_after_umask_mask, bits_added_on_continued_line, bits_added_after_comment_line, or_on_continued_line, mask_ends_before_closing_line.

2. Read and restore: I replaced the fixed-width lookbehinds with a small helper that steps back over spaces, tabs, line breaks, \ and ( to the =, so the two-space, tab and parenthesized forms behave like the one-space one. ==, !=, <= and >= are comparisons and are still reported. Tests: read_umask_two_spaces, read_umask_tab, read_umask_parenthesized, umask_call_compared_not_assigned.

3. CRLF: the line end now accepts \r\n, so mode = 0o666 & ~umask in a CRLF file is skipped like the LF one, and a CRLF line continued with | 0o666 is still reported. Tests: mode_masked_by_umask_crlf, mode_masked_by_umask_with_comment_crlf, bits_added_on_continued_crlf_line.

4. Decimal: outside shell files a number is octal only with a 0o prefix or a leading 0, and a bare number is read as decimal. So umask = 14 (0o016) is no longer reported, while umask = 8 and os.umask(8) (0o010) are. .sh files and the umask NNN command still read every value as octal. Tests: decimal_14_is_0o016, decimal_8_is_0o010, decimal_call_0o010, shell_umask_is_octal, shell_variable_is_octal.

The new paths have linear-time cases next to the existing ones, including a long comment after the mask. The comment check stops at 240 characters, so a longer comment is reported like before.

Two things I left as they are. A JavaScript line like mode = 0o666 & ~process.umask() // note without a semicolon is reported again, because the same // is floor division in Python and the pattern can't tell the languages apart. With a semicolon it's still skipped. And a conditional continued on the next line (if safe else 0o777) still reads as the end of the mask, since line by line it looks like a following if statement, and reporting that would bring the B6 false positive back for ordinary code.

[rng1995]

[SkillSpector Review]

@ilaigold thank you for fixing TM3's umask handling from first principles: you read the value by base and test only the world-write bit. Thank you also for answering each of yashrajp22's four cases with a targeted test.

Value and readiness

This fixes #644 B6 and a real inversion on main. Main flags mode = 0o666 & ~umask and the most restrictive umask = 0o777, but not umask 000 or os.umask(0). Severity and confidence stay MEDIUM/0.8. I rechecked yashrajp22's four inline cases (raised at 3eb3cd9) against head 960b4da, and each one is addressed. The PR is ready for yashrajp22's re-review and final maintainer review.

Material findings

None blocking. yashrajp22's cases, checked by reading the current patterns:

1. Whole expression. In the mode pattern (static_patterns_tool_misuse.py:394-399), the exemption lookahead accepts only ), ,, ;, ], }, or a line end that is not followed by an operator, and or or. So // 512 and a continued | 0o666 line are reported again. Resolved.
2. Read-and-restore formatting. _is_assigned_value (5018-5031) steps back over spaces, tabs, line breaks, \ and ( to reach the =, and it excludes ==, !=, <= and >=. Resolved.
3. CRLF. (?:#[^\n]{0,240}|\r)?$ accepts \r\n, and the continuation check still applies afterwards. Resolved.
4. Decimal. _UMASK_NUMBER_WORLD_WRITE treats a bare number as decimal and tests N mod 4 through its last two digits. .sh files and the umask NNN command stay octal. Resolved.

• Optional: the description says "nothing world-writable that main catches is lost". Your follow-up comment names one exception: a conditional continued on the next line inside a call (mode=0o666 & ~umask followed by if safe else 0o777 on the next line). Main flagged it, and it is now exempt. Please correct the description so the merge record is accurate.
• Optional: the comment at line 355 says C reads a number as octal with a 0o prefix, but C only uses a leading 0. This is cosmetic.

PIC tradeoffs

• New TM3 findings. umask 000/umask 0 will now be reported, including in Markdown prose and Dockerfiles, and so will unassigned umask(0) calls. That is intended, but users will see new MEDIUM findings.
• Read idiom. Any assigned umask(0) (old = os.umask(0)) is silent, even without a restore. Main never detected umask(0), so this is not a regression.
• Kept FN and FP. The continued-conditional FN and the JS // note FP are both deliberate, and both are disclosed in your follow-up comment.

Verification and gaps

• Read the full diff, the TM3 loop in analyze(), and all of the new detection, benign and linear-time cases. I traced each pattern by hand: the decimal lookbehind, the octal last digit, the mask lookahead and _is_assigned_value. The lookbehinds are fixed-width, and the possessive \d*+ is supported by the required Python 3.12+.
• CI is green on 960b4da (6/6). static_patterns_tool_misuse.py on main has not changed since this head's merge base, so the result is current, and the PR merges cleanly with main.
• Overlap: open #790 conflicts with this PR at the TM3 loop head, where both add a line before line_num = .... Keep both lines. #741 also touches these loop heads.
• I did not execute the PR's regexes or tests, so the hand traces above are my evidence.