diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 92afe09..5d80e47 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,6 +1,6 @@ # Configuration Discovery Review Findings -Status: findings 1–3 complete; findings 4–5 remain open. +Status: findings 1–4 complete; finding 5 remains open. Reviewed commit: `e50fee2`, including the discovery implementation commits after `424c897`. The review compared the implementation with the @@ -142,7 +142,14 @@ release procedure; do not introduce native macOS validation as a gate. ## 4. Use Platform-Aware Windows Component Validation -Priority: P3. Status: open. Algorithmic audit: correctness finding. +Priority: P3. Status: complete. Algorithmic audit: correctness finding. + +**Completed:** Windows validation now delegates reserved-name decisions to +`filepath.IsLocal` while preserving the selected component/character checks. The +manual device table was removed. Tests cover console names, platform-dependent +extension behavior, and invalid later search components; Unix tests retain their +own filename rules. Focused tests and `./scripts/check.sh` passed on Linux. The +Windows engine test binary cross-compiled successfully; it was not run natively. **Location:** [discovery.go](../../internal/engine/discovery.go), `validWindowsFilenameComponent`, lines 266–281. @@ -249,8 +256,8 @@ ordinary file acquisition. It did not benchmark or reopen those choices. No unplanned quadratic algorithm or other actionable asymptotic regression was identified. Findings 4 and 5 record the additional ordinary correctness issues -found while examining those algorithms; findings 1–3 remain open from the earlier -review. Existing configuration-tree algorithms outside the discovery changes +found while examining those algorithms; findings 1–3 were carried forward from +the earlier review. Existing configuration-tree algorithms outside the discovery changes were not re-audited as new feature work. ## Review Validation @@ -261,7 +268,7 @@ were not re-audited as new feature work. - An external temporary Go program reproduced findings 1 and 2 through the public API, comparing explicit file loading with discovery. No production or test files were modified for these reproductions. -- Findings 1–3 remain open despite the existing suites passing. +- Findings 1–3 remained open at the original review despite the suites passing. The broader discovery architecture is implemented: immutable root-owned descriptions, incremental ordered selection during shared source acquisition, concrete selected-source propagation, and shared absent-input handling before diff --git a/internal/engine/discovery.go b/internal/engine/discovery.go index b6498e7..80800e4 100644 --- a/internal/engine/discovery.go +++ b/internal/engine/discovery.go @@ -258,27 +258,16 @@ func validFilenameComponent(value string) bool { return false } if runtime.GOOS == "windows" { - return validWindowsFilenameComponent(value) - } - return true -} - -func validWindowsFilenameComponent(value string) bool { - if strings.ContainsAny(value, `<>:"|?*`) || strings.HasSuffix(value, ".") || strings.HasSuffix(value, " ") { - return false - } - for _, character := range value { - if character < 32 { + if !filepath.IsLocal(value) || strings.ContainsAny(value, `<>:"|?*`) || strings.HasSuffix(value, ".") || strings.HasSuffix(value, " ") { return false } + for _, character := range value { + if character < 32 { + return false + } + } } - base := strings.ToUpper(strings.TrimSuffix(value, filepath.Ext(value))) - switch base { - case "CON", "PRN", "AUX", "NUL", "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9", "COM¹", "COM²", "COM³", "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9", "LPT¹", "LPT²", "LPT³": - return false - default: - return true - } + return true } var errUnixSystemPresetUnavailable = errors.New("Unix system directories are unavailable on this platform") diff --git a/internal/engine/discovery_test.go b/internal/engine/discovery_test.go index 65d8988..e1ca4de 100644 --- a/internal/engine/discovery_test.go +++ b/internal/engine/discovery_test.go @@ -129,31 +129,33 @@ func TestFilenameComponentValidation(t *testing.T) { } } if runtime.GOOS == "windows" { - for _, value := range []string{"CON", "nul.txt", "name.", "bad:name"} { + for _, value := range []string{"CON", "CONIN$", "CONOUT$", "name.", "bad:name", "bad\x01name"} { if validFilenameComponent(value) { t.Errorf("Windows component %q unexpectedly accepted", value) } } - } else { - for _, value := range []string{"name with spaces", "naïve"} { - if !validFilenameComponent(value) { - t.Errorf("host-valid component %q rejected", value) + // Device names with extensions depend on the Windows version. Match + // Go's platform-aware decision instead of imposing a blanket ban. + for _, value := range []string{"nul.txt", "COM¹.log", "LPT³.txt", "CON.extra.yml"} { + if got, want := validFilenameComponent(value), filepath.IsLocal(value); got != want { + t.Errorf("Windows component %q validity = %v, want Go's %v", value, got, want) } } + } else { + for _, value := range []string{"CONIN$", "CONOUT$", "nul.txt"} { + if !validFilenameComponent(value) { + t.Errorf("Windows component restriction applied to this host: %q", value) + } + } + } + for _, value := range []string{"name with spaces", "naïve"} { + if !validFilenameComponent(value) { + t.Errorf("host-valid component %q rejected", value) + } } if diagnostics := validateSearch(SearchDirectory("configs", DirectorySearchOptions{Filenames: []FilenameGroup{CustomFilenames("bad/name")}}), ""); len(diagnostics) == 0 { t.Fatal("invalid custom filename was not retained for validation") } - for _, value := range []string{"CON", "nul.txt", "COM¹.log", "LPT³.txt", "name.", "bad:name", "bad\x01name"} { - if validWindowsFilenameComponent(value) { - t.Errorf("Windows component %q unexpectedly accepted", value) - } - } - for _, value := range []string{"name with spaces", "naïve"} { - if !validWindowsFilenameComponent(value) { - t.Errorf("valid Windows component %q rejected", value) - } - } } func TestSearchCandidatesPreserveDirectoryAndGroupOrder(t *testing.T) { diff --git a/internal/engine/discovery_windows_test.go b/internal/engine/discovery_windows_test.go new file mode 100644 index 0000000..c1addaf --- /dev/null +++ b/internal/engine/discovery_windows_test.go @@ -0,0 +1,32 @@ +//go:build windows + +package engine + +import ( + "errors" + "testing" +) + +func TestWindowsSearchRejectsReservedComponentsBeforeAcquisition(t *testing.T) { + t.Parallel() + + root := t.TempDir() + first := writeImportFile(t, root, "first.yml", "value: first\n") + for _, name := range []string{"CONIN$", "CONOUT$"} { + for _, source := range []Source{ + SearchConfig(name, Searches(Paths(first), WorkingDirectory())), + SearchConfig("", Searches(Paths(first), SearchDirectory(root, DirectorySearchOptions{ + Filenames: []FilenameGroup{CustomFilenames(name)}, + }))), + } { + result, err := testLoad[discoveryLoadConfig](source, Options[discoveryLoadConfig]{}) + var failure *LoadError + if result != nil || !errors.As(err, &failure) || len(failure.Diagnostics()) != 1 || failure.Diagnostics()[0].Code != CodeOptionsInvalid { + t.Fatalf("reserved component %q accepted: %#v, %v", name, result, err) + } + if len(failure.Sources()) != 0 { + t.Fatalf("reserved component %q was checked after acquisition: %#v", name, failure.Sources()) + } + } + } +}