Skip to content

fix(input): expand 8.3 short DOS names before the opened-handle check - #484

Open
kevin9327 wants to merge 1 commit into
NVIDIA:mainfrom
kevin9327:fix/windows-short-dos-name
Open

fix(input): expand 8.3 short DOS names before the opened-handle check#484
kevin9327 wants to merge 1 commit into
NVIDIA:mainfrom
kevin9327:fix/windows-short-dos-name

Conversation

@kevin9327

Copy link
Copy Markdown

Fixes #481.

Problem

On Windows, every file in a scan is rejected when the scanned path is spelled with an 8.3 short
component, so execution_successful is false, the ledger fills with missing_file_cache, and the
risk score means nothing regardless of what the skill contains.

_open_regular_file_from_windows_handle guards against a reparse-point swap by comparing the
canonical path of the handle it opened against the path it was asked to open.
GetFinalPathNameByHandleW always answers with long components, while the requested path can carry
short ones, and _windows_normalized_path only stripped the \\?\ prefix and normcased. The two
spellings never matched, so the guard rejected the file it had just safely opened.

This is not an unusual setup. Windows keeps an 8.3 alias for any directory whose name holds a space,
so a profile directory like C:\Users\Hoang Pham reaches the scanner as C:\Users\HOANGP~1 through
%TEMP%. Because skillspector scan <url> clones into a temp directory, URL scans are broken by
default on any machine with a spaced account name.

Fix

_windows_long_path_name expands short components through GetLongPathNameW, and
_windows_normalized_path runs the path through it before comparing.

The guard keeps its meaning. An 8.3 name is an alias the filesystem holds for one directory entry,
so expanding it names that same entry; GetLongPathNameW does not resolve symlinks or junctions, so
a reparse point that appeared mid-open still shows up as a different final path and is still
rejected. A path that no longer resolves comes back unchanged, which leaves the comparison
fail-closed.

Tests

Two tests in tests/unit/test_input_handler.py, using the existing _mock_windows_secure_open
harness so they run on any platform. The mock gains a GetLongPathNameW stub driven by a
long_names mapping.

  • test_windows_no_follow_open_accepts_a_short_dos_name — a requested short spelling and a long
    final path open the same entry. Before the fix:

    _UnsafeFileError: Could not safely open file: ...\test_windows_no_follow_open_ac0\SHORTN~1.MD
    src\skillspector\input_handler.py:414: _UnsafeFileError
    
  • test_windows_no_follow_open_rejects_an_unresolvable_short_name — a short name that does not
    expand is still refused, so the fail-closed half is pinned rather than assumed.

How I tested

Windows 11, Python 3.12.10, 8.3 name creation enabled on the volume. Built a two-file skill under
C:\Users\swsz9\claudeworkspace\Skill Space Test, whose short name is SKILLS~3, and scanned both
spellings with --no-llm --format json:

scan target execution_successful status components scanned ledger exceptions
SKILLS~3 before the fix false failed 0 / 0 5
SKILLS~3 after the fix true complete 2 / 2 0
Skill Space Test (control) true complete 2 / 2 0

The first ledger exception in the failing run is the one from the report:

{"outcome": "failed", "phase": "static", "reason_code": "missing_file_cache",
 "message": "Applicable analyzer could not obtain file content.", "path": "SKILL.md",
 "fatal": true, "analyzers": ["static_yara"]}

ruff check src/ tests/ and ruff format --check src/ tests/ are clean.

pytest -m "not integration and not provider" tests/ adds no failures: 23 failed / 3943 passed
before, 23 failed / 3945 passed after, the two extra passes being the tests above.

That suite is not green on Windows to begin with, and none of the 23 involve this code path:
12 in tests/nodes/test_build_context.py (symlink fixtures, which need elevation or developer mode
on Windows), 5 in test_create_github_release.py, 4 in test_compare_scan_accuracy.py, 1 in
test_static_yara.py, and test_input_handler.py::test_resolve_file_open_failure_does_not_create_temp_dir.
I ran tests/nodes/test_build_context.py on its own with and without this change to be sure: 12
failed either way. Happy to file those separately if they are not already known.

Signed-off-by: kevin9327 <kevin9327@users.noreply.github.com>
@kevin9327

Copy link
Copy Markdown
Author

The test-unit failure looks unrelated to this change, and I do not have rerun rights. The evidence, in case it saves someone a look:

The failing assertion is tests/nodes/test_security_end_to_end.py::test_nine_case_contract_across_public_surfaces on analysis_completeness.is_complete is True. That test writes an oversized multi-window payload and runs a full graph scan; the file took about five minutes on that runner, and completeness degrades when a scan deadline is reached.

This diff only touches _windows_normalized_path and the new _windows_long_path_name beside it. Both are reached only from _open_regular_file_from_windows_handle, which _open_regular_file_no_follow calls only when _HAS_SECURE_DIR_FD is false. On Linux that flag is true (os.open in os.supports_dir_fd and hasattr(os, "O_NOFOLLOW")), so the changed code does not execute on the CI runner at all.

On Windows, where it does execute, that whole file passes: pytest tests/nodes/test_security_end_to_end.py gives 43 passed.

Happy to rebase or add anything that would help; a rerun is the only thing I cannot do myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Windows: every scan fails with missing_file_cache / execution_successful: false when the skill path resolves to an 8.3 short DOS name

1 participant