diff --git a/docs/contracts/workspace-preprocessing-cli.md b/docs/contracts/workspace-preprocessing-cli.md index a21bc44c..6c28e90d 100644 --- a/docs/contracts/workspace-preprocessing-cli.md +++ b/docs/contracts/workspace-preprocessing-cli.md @@ -24,6 +24,6 @@ The bounded schema-v1 stdin envelope is exact: omitted fields are not equivalent * `check`: `resume` is required; `annotations` and `reviewedCandidates` are either both present or both absent. `annotations` is exactly `{basename, contentBase64, sha256}`. * `index-schema`: no additional fields. -No other fields, duplicate JSON value, host path, raw SQL, or raw annotation content are accepted. SQL and annotations use a logical basename, base64 bytes, and a declared `sha256:` digest. The complete request is at most 1 MiB and every supplied field/value must exactly match the command-derived envelope. Candidate responses may carry the internal `hostExport` (`mediaType`, `sha256`, `contentBase64`) only for `suggest-fks`; its object is strict (unknown fields rejected) and is always verified, even without `--output`: YAML media type (`application/yaml` or `text/yaml`), UTF-8, digest, and decoded size at most 700 KiB. It is removed from the public result and written exclusively only after result/run/identity validation. Output publication uses restrictive mode `0600` and refuses existing leaves, links, hardlinks, directories, replacement races, and reparse points. +No other fields, duplicate JSON value, host path, raw SQL, or raw annotation content are accepted. SQL and annotations use a logical basename, base64 bytes, and a declared `sha256:` digest. The request envelope is independently capped at 24 MiB (after JSON/base64 encoding), enough to carry the frozen 1 MiB-per-file/16 MiB aggregate raw ingress bounds; every supplied field/value must exactly match the command-derived envelope. Candidate responses may carry the internal `hostExport` (`mediaType`, `sha256`, `contentBase64`) only for `suggest-fks`; its object is strict (unknown fields rejected) and is always verified, even without `--output`: YAML media type (`application/yaml` or `text/yaml`), UTF-8, digest, and decoded size at most 700 KiB. It is removed from the public result and written exclusively only after result/run/identity validation. Output publication uses restrictive mode `0600` and refuses existing leaves, links, hardlinks, directories, replacement races, and reparse points. The public result has schema version 1 and only these fields: `status`, `code`, workspace/revision/descriptor/operation identities, optional run and child run IDs, completed stages, counts, artifact identities, and warnings. Revisions/descriptors are 40-hex; run IDs are 32-hex; artifact digests are `sha256:`. Allowed statuses are `succeeded`, `unchanged`, `dry_run`, `blocked`, and `failed`. Allowed codes are `ok`, `workspace_not_found`, `workspace_not_activatable`, `binding_missing`, `preprocessing_conflict`, `preprocessing_resume_mismatch`, `manual_review_required`, `evidence_materialization_required`, `effective_config_mismatch`, `semantic_index_incompatible`, `annotation_invalid`, `egress_policy_refused`, and `registry_bootstrap_recovery_conflict`. `succeeded`, `unchanged`, and `dry_run` require `ok` and child exit 0. `blocked` requires one of `manual_review_required`, `evidence_materialization_required`, `preprocessing_conflict`, `preprocessing_resume_mismatch`, or `registry_bootstrap_recovery_conflict`, and child exit 3. `failed` requires a non-`ok` operational code other than those blocked-only codes, and child exit 1. A nonzero child exit is never accepted for another status/code combination. Stdout is capped at 1 MiB and stderr at 64 KiB. Compose is invoked only as `compose run --rm --no-deps --no-TTY workspace-maintenance ...`; output never includes child stderr or secrets. diff --git a/tools/thothctl/cmd/thothctl/main_test.go b/tools/thothctl/cmd/thothctl/main_test.go index fa3b7b83..4f7046b5 100644 --- a/tools/thothctl/cmd/thothctl/main_test.go +++ b/tools/thothctl/cmd/thothctl/main_test.go @@ -105,6 +105,59 @@ func TestUsageDocumentsClosedConfigureUpdateSourcesAndMaintenanceRecovery(t *tes } } +func TestRunWorkspacePublicDispatchExitMatrix(t *testing.T) { + cases := []struct { + name, status, code string + exit int + }{ + {"succeeded", "succeeded", "ok", 0}, + {"blocked", "blocked", "manual_review_required", 3}, + {"failed", "failed", "workspace_not_found", 1}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + fixture := newCLIFixture(t, "") + fixture.setEnvironment(t) + t.Setenv("THOTHCTL_FAKE_WORKSPACE_RESULT", fmt.Sprintf(`{"schemaVersion":1,"status":%q,"code":%q,"workspaceId":"psd","workspaceRevision":"%s","descriptorBlob":"%s","operation":"inspect","completedStages":[]}`, tc.status, tc.code, strings.Repeat("0", 40), strings.Repeat("a", 40))) + t.Setenv("THOTHCTL_FAKE_WORKSPACE_EXIT", strconv.Itoa(tc.exit)) + var stdout, stderr bytes.Buffer + got := run(context.Background(), []string{"--installation", fixture.installationPath, "workspace", "inspect", "--workspace", "psd", "--json"}, &stdout, &stderr) + if got != tc.exit || stderr.Len() != 0 { + t.Fatalf("exit=%d stderr=%q; want exit %d", got, stderr.String(), tc.exit) + } + if len(fixture.invocations(t)) != 1 { + t.Fatalf("workspace dispatch invocations = %#v, want one", fixture.invocations(t)) + } + }) + } +} + +func TestRunWorkspaceOperationalFailureExitsOne(t *testing.T) { + fixture := newCLIFixture(t, "") + fixture.setEnvironment(t) + t.Setenv("THOTHCTL_FAKE_WORKSPACE_RESULT", "not-json") + var stdout, stderr bytes.Buffer + got := run(context.Background(), []string{"--installation", fixture.installationPath, "workspace", "inspect", "--workspace", "psd"}, &stdout, &stderr) + if got != 1 || !strings.Contains(stderr.String(), "invalid workspace result") { + t.Fatalf("exit=%d stderr=%q; want operational exit 1", got, stderr.String()) + } +} + +func TestRunWorkspaceUnsafeOutputDoesNotInvokeCompose(t *testing.T) { + fixture := newCLIFixture(t, "") + fixture.setEnvironment(t) + output := filepath.Join(fixture.root, "existing.yaml") + if err := os.WriteFile(output, []byte("existing"), 0o600); err != nil { + t.Fatal(err) + } + var stdout, stderr bytes.Buffer + got := run(context.Background(), []string{"--installation", fixture.installationPath, "workspace", "schema", "suggest-fks", "--workspace", "psd", "--output", output}, &stdout, &stderr) + if got != 2 || !strings.Contains(stderr.String(), "unsafe output file") { + t.Fatalf("exit=%d stderr=%q; want unsafe host failure", got, stderr.String()) + } + assertDockerNotInvoked(t, fixture) +} + func TestRunWorkspaceRejectsInvalidCommandBeforeDocker(t *testing.T) { fixture := newCLIFixture(t, "") fixture.setEnvironment(t) @@ -750,6 +803,9 @@ case " $* " in else printf '%s\n' '{"volumes":{"settings":{}},"services":{"core":{"image":"thothii-core:local","environment":{"THT_LLM_URL":"https://llm.example.invalid"}}}}' fi ;; + *" run --rm --no-deps --no-TTY workspace-maintenance "*) + printf '%s\n' "${THOTHCTL_FAKE_WORKSPACE_RESULT:-}" + exit "${THOTHCTL_FAKE_WORKSPACE_EXIT:-0}" ;; *" run --rm --no-deps --no-TTY session-migrate "*) if [ "${THOTHCTL_FAKE_MIGRATION_EXIT:-0}" -ne 0 ]; then printf '%s\n' "$THOTHCTL_FAKE_MIGRATION_FAILURE" >&2 @@ -805,6 +861,8 @@ func (f cliFixture) setEnvContents(t *testing.T, env string) { t.Setenv("THOTHCTL_FAKE_CONFIG", "") t.Setenv("THOTHCTL_FAKE_MIGRATION_FAILURE", "") t.Setenv("THOTHCTL_FAKE_MIGRATION_EXIT", "0") + t.Setenv("THOTHCTL_FAKE_WORKSPACE_RESULT", "") + t.Setenv("THOTHCTL_FAKE_WORKSPACE_EXIT", "0") } func (f cliFixture) setProfile(t *testing.T, profile string) { diff --git a/tools/thothctl/internal/compose/process_alive_unix_test.go b/tools/thothctl/internal/compose/process_alive_unix_test.go new file mode 100644 index 00000000..bcf36283 --- /dev/null +++ b/tools/thothctl/internal/compose/process_alive_unix_test.go @@ -0,0 +1,7 @@ +//go:build !windows + +package compose + +import "syscall" + +func processExists(pid int) bool { return syscall.Kill(pid, 0) == nil } diff --git a/tools/thothctl/internal/compose/process_alive_windows_test.go b/tools/thothctl/internal/compose/process_alive_windows_test.go new file mode 100644 index 00000000..c85b04ec --- /dev/null +++ b/tools/thothctl/internal/compose/process_alive_windows_test.go @@ -0,0 +1,5 @@ +//go:build windows + +package compose + +func processExists(int) bool { return true } diff --git a/tools/thothctl/internal/compose/process_unix.go b/tools/thothctl/internal/compose/process_unix.go index f69852d9..96370ad6 100644 --- a/tools/thothctl/internal/compose/process_unix.go +++ b/tools/thothctl/internal/compose/process_unix.go @@ -3,6 +3,8 @@ package compose import ( + "context" + "io" "os/exec" "syscall" ) @@ -16,3 +18,7 @@ func terminateOwnedProcess(c *exec.Cmd) { _ = c.Process.Kill() } } + +func runBoundedPlatform(context.Context, Runner, []string, io.Reader, CaptureLimits) (Result, bool, error) { + return Result{}, false, nil +} diff --git a/tools/thothctl/internal/compose/process_windows.go b/tools/thothctl/internal/compose/process_windows.go index 66e208df..061b6bdc 100644 --- a/tools/thothctl/internal/compose/process_windows.go +++ b/tools/thothctl/internal/compose/process_windows.go @@ -3,32 +3,57 @@ package compose import ( + "context" "errors" + "fmt" + "io" + "os" "os/exec" "sync" + "syscall" "unsafe" "golang.org/x/sys/windows" ) -var ownedJobs = struct { - sync.Mutex - m map[*exec.Cmd]windows.Handle -}{m: make(map[*exec.Cmd]windows.Handle)} +// PROC_THREAD_ATTRIBUTE_JOB_LIST is intentionally kept here because x/sys does +// not expose this newer SDK constant yet. Supplying it to CreateProcess makes +// job ownership atomic with process creation; CREATE_SUSPENDED plus a later +// AssignProcessToJobObject is not sufficient (the parent can die in between). +const procThreadAttributeJobList = 0x0002000d -// CREATE_SUSPENDED closes the registration race: no child code can create a -// descendant until the real process HANDLE has been assigned to the job. -func configureOwnedProcess(c *exec.Cmd) { - c.SysProcAttr = &windows.SysProcAttr{CreationFlags: windows.CREATE_NEW_PROCESS_GROUP | windows.CREATE_SUSPENDED} +// The generic exec.Cmd path is not used on Windows: runBoundedPlatform launches +// with CreateProcess and a creation-time job-list attribute. These hooks remain +// for the platform-neutral runner's compile-time shape only. +func configureOwnedProcess(*exec.Cmd) {} +func registerOwnedProcess(*exec.Cmd) error { + return errors.New("Windows owned processes require creation-time job ownership") +} +func releaseOwnedProcess(*exec.Cmd) {} +func terminateOwnedProcess(c *exec.Cmd) { + if c.Process != nil { + _ = c.Process.Kill() + } } -func registerOwnedProcess(c *exec.Cmd) error { - if c.Process == nil { - return errors.New("owned process has no process handle") +func runBoundedPlatform(ctx context.Context, runner Runner, args []string, stdin io.Reader, limits CaptureLimits) (Result, bool, error) { + if limits.StdoutBytes < 0 || limits.StderrBytes < 0 { + return Result{}, true, ErrOutputLimit } + result, err := runBoundedWindows(ctx, runner.binary, args, stdin, limits) + return result, true, err +} + +type windowsCollector struct { + buf []byte + n int64 + overflow bool +} + +func runBoundedWindows(ctx context.Context, binary string, args []string, stdin io.Reader, limits CaptureLimits) (Result, error) { job, err := windows.CreateJobObject(nil, nil) if err != nil { - return err + return Result{}, err } closeJob := true defer func() { @@ -36,72 +61,178 @@ func registerOwnedProcess(c *exec.Cmd) error { windows.CloseHandle(job) } }() - limits := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{} - limits.BasicLimitInformation.LimitFlags = windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE - if _, err = windows.SetInformationJobObject(job, windows.JobObjectExtendedLimitInformation, uintptr(unsafe.Pointer(&limits)), uint32(unsafe.Sizeof(limits))); err != nil { - return err + limitsInfo := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{} + limitsInfo.BasicLimitInformation.LimitFlags = windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE + if _, err = windows.SetInformationJobObject(job, windows.JobObjectExtendedLimitInformation, uintptr(unsafe.Pointer(&limitsInfo)), uint32(unsafe.Sizeof(limitsInfo))); err != nil { + return Result{}, err } - // c.Process.Pid is used only to obtain a genuine process HANDLE; a PID is - // never passed to AssignProcessToJobObject. - process, err := windows.OpenProcess(windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE|windows.PROCESS_QUERY_LIMITED_INFORMATION, false, uint32(c.Process.Pid)) - if err != nil { - return err - } - defer windows.CloseHandle(process) - if err = windows.AssignProcessToJobObject(job, process); err != nil { - return err - } - thread, err := suspendedPrimaryThread(uint32(c.Process.Pid)) - if err != nil { - _ = windows.TerminateJobObject(job, 1) - return err - } - _, resumeErr := windows.ResumeThread(thread) - windows.CloseHandle(thread) - if resumeErr != nil { - _ = windows.TerminateJobObject(job, 1) - return resumeErr - } - ownedJobs.Lock() - ownedJobs.m[c] = job - ownedJobs.Unlock() - closeJob = false - return nil -} -func suspendedPrimaryThread(pid uint32) (windows.Handle, error) { - snapshot, err := windows.CreateToolhelp32Snapshot(windows.TH32CS_SNAPTHREAD, 0) + stdinRead, stdinWrite, err := os.Pipe() if err != nil { - return 0, err + return Result{}, err } - defer windows.CloseHandle(snapshot) - entry := windows.ThreadEntry32{Size: uint32(unsafe.Sizeof(windows.ThreadEntry32{}))} - err = windows.Thread32First(snapshot, &entry) - for err == nil { - if entry.OwnerProcessID == pid { - return windows.OpenThread(windows.THREAD_SUSPEND_RESUME, false, entry.ThreadID) + stdoutRead, stdoutWrite, err := os.Pipe() + if err != nil { + _ = stdinRead.Close() + _ = stdinWrite.Close() + return Result{}, err + } + stderrRead, stderrWrite, err := os.Pipe() + if err != nil { + _ = stdinRead.Close() + _ = stdinWrite.Close() + _ = stdoutRead.Close() + _ = stdoutWrite.Close() + return Result{}, err + } + closeAll := func() { + _ = stdinRead.Close() + _ = stdinWrite.Close() + _ = stdoutRead.Close() + _ = stdoutWrite.Close() + _ = stderrRead.Close() + _ = stderrWrite.Close() + } + defer closeAll() + childHandles := []windows.Handle{windows.Handle(stdinRead.Fd()), windows.Handle(stdoutWrite.Fd()), windows.Handle(stderrWrite.Fd())} + for _, h := range childHandles { + if err := windows.SetHandleInformation(h, windows.HANDLE_FLAG_INHERIT, windows.HANDLE_FLAG_INHERIT); err != nil { + return Result{}, err } - err = windows.Thread32Next(snapshot, &entry) } - return 0, err -} + // The attribute list limits inheritance to exactly these standard handles. + attributes, err := windows.NewProcThreadAttributeList(2) + if err != nil { + return Result{}, err + } + defer attributes.Delete() + jobList := []windows.Handle{job} + if err := attributes.Update(procThreadAttributeJobList, unsafe.Pointer(&jobList[0]), unsafe.Sizeof(jobList[0])); err != nil { + return Result{}, err + } + if err := attributes.Update(windows.PROC_THREAD_ATTRIBUTE_HANDLE_LIST, unsafe.Pointer(&childHandles[0]), uintptr(len(childHandles))*unsafe.Sizeof(childHandles[0])); err != nil { + return Result{}, err + } -func releaseOwnedProcess(c *exec.Cmd) { - ownedJobs.Lock() - h := ownedJobs.m[c] - delete(ownedJobs.m, c) - ownedJobs.Unlock() - if h != 0 { - windows.CloseHandle(h) + command := syscall.EscapeArg(binary) + for _, arg := range args { + command += " " + syscall.EscapeArg(arg) } -} -func terminateOwnedProcess(c *exec.Cmd) { - ownedJobs.Lock() - h := ownedJobs.m[c] - ownedJobs.Unlock() - if h != 0 { - _ = windows.TerminateJobObject(h, 1) - } else if c.Process != nil { - _ = c.Process.Kill() + commandLine, err := windows.UTF16FromString(command) + if err != nil { + return Result{}, err } + startup := windows.StartupInfoEx{ + StartupInfo: windows.StartupInfo{ + Cb: uint32(unsafe.Sizeof(windows.StartupInfoEx{})), + Flags: windows.STARTF_USESTDHANDLES, + StdInput: childHandles[0], + StdOutput: childHandles[1], + StdErr: childHandles[2], + }, + ProcThreadAttributeList: attributes.List(), + } + var process windows.ProcessInformation + creationFlags := uint32(windows.CREATE_UNICODE_ENVIRONMENT | windows.CREATE_NEW_PROCESS_GROUP | windows.EXTENDED_STARTUPINFO_PRESENT) + if err := windows.CreateProcess(nil, &commandLine[0], nil, nil, true, creationFlags, nil, nil, (*windows.StartupInfo)(unsafe.Pointer(&startup)), &process); err != nil { + return Result{ExitCode: 127}, err + } + // The child owns duplicate pipe handles; parent copies use the opposite ends. + _ = stdinRead.Close() + _ = stdoutWrite.Close() + _ = stderrWrite.Close() + _ = windows.CloseHandle(process.Thread) + + var inputDone sync.WaitGroup + inputDone.Add(1) + go func() { + defer inputDone.Done() + defer stdinWrite.Close() + if stdin != nil { + _, _ = io.Copy(stdinWrite, stdin) + } + }() + out := windowsCollector{buf: make([]byte, limits.StdoutBytes)} + er := windowsCollector{buf: make([]byte, limits.StderrBytes)} + overflow := make(chan struct{}, 1) + var readers sync.WaitGroup + readers.Add(2) + read := func(file *os.File, maximum int64, collector *windowsCollector) { + defer readers.Done() + chunk := make([]byte, 32*1024) + for { + n, readErr := file.Read(chunk) + if n > 0 { + remain := maximum - collector.n + if remain > 0 { + take := int64(n) + if take > remain { + take = remain + } + copy(collector.buf[collector.n:collector.n+take], chunk[:take]) + collector.n += take + } + if int64(n) > remain { + collector.overflow = true + select { + case overflow <- struct{}{}: + default: + } + return + } + } + if readErr != nil { + return + } + } + } + go read(stdoutRead, limits.StdoutBytes, &out) + go read(stderrRead, limits.StderrBytes, &er) + waited := make(chan uint32, 1) + go func() { + _, waitErr := windows.WaitForSingleObject(process.Process, windows.INFINITE) + if waitErr != nil { + waited <- uint32(0x103) + } else { + var code uint32 + if windows.GetExitCodeProcess(process.Process, &code) != nil { + code = 1 + } + waited <- code + } + }() + var exitCode uint32 + var cancelled error + select { + case exitCode = <-waited: + // Closing the job handle kills any descendants before waiting on pipe readers. + _ = windows.CloseHandle(job) + closeJob = false + case <-overflow: + _ = windows.TerminateJobObject(job, 1) + exitCode = <-waited + _ = windows.CloseHandle(job) + closeJob = false + case <-ctx.Done(): + cancelled = ctx.Err() + _ = windows.TerminateJobObject(job, 1) + exitCode = <-waited + _ = windows.CloseHandle(job) + closeJob = false + } + _ = windows.CloseHandle(process.Process) + _ = stdinWrite.Close() + readers.Wait() + inputDone.Wait() + result := Result{Stdout: string(out.buf[:out.n]), Stderr: string(er.buf[:er.n]), ExitCode: int(exitCode)} + if out.overflow || er.overflow { + return result, ErrOutputLimit + } + if cancelled != nil { + return result, cancelled + } + if exitCode == 0 { + return result, nil + } + return result, fmt.Errorf("child exited with code %d", exitCode) } diff --git a/tools/thothctl/internal/compose/process_windows_test.go b/tools/thothctl/internal/compose/process_windows_test.go index bdf050cd..c3b081e0 100644 --- a/tools/thothctl/internal/compose/process_windows_test.go +++ b/tools/thothctl/internal/compose/process_windows_test.go @@ -8,11 +8,14 @@ import ( "testing" ) -func TestOwnedProcessConfigurationStartsSuspended(t *testing.T) { +func TestOwnedProcessUsesCreationTimeJobAttribute(t *testing.T) { + if procThreadAttributeJobList == 0 { + t.Fatal("owned Windows process must carry a creation-time job-list attribute") + } cmd := exec.Command("cmd.exe") configureOwnedProcess(cmd) - if cmd.SysProcAttr == nil || cmd.SysProcAttr.CreationFlags&windows.CREATE_SUSPENDED == 0 { - t.Fatal("owned Windows process must start suspended until job assignment") + if cmd.SysProcAttr != nil && cmd.SysProcAttr.CreationFlags&windows.CREATE_SUSPENDED != 0 { + t.Fatal("owned Windows process must not rely on a post-start assignment race") } } diff --git a/tools/thothctl/internal/compose/runner.go b/tools/thothctl/internal/compose/runner.go index f915a4a1..aef12700 100644 --- a/tools/thothctl/internal/compose/runner.go +++ b/tools/thothctl/internal/compose/runner.go @@ -57,6 +57,9 @@ func (r Runner) Run(ctx context.Context, args []string, stdin io.Reader) (Result // RunBounded streams each pipe into a preallocated fixed-capacity collector. It owns the // process group and tears it down on the first overflow or cancellation. func (r Runner) RunBounded(ctx context.Context, args []string, stdin io.Reader, limits CaptureLimits) (Result, error) { + if result, handled, err := runBoundedPlatform(ctx, r, args, stdin, limits); handled { + return result, err + } if limits.StdoutBytes < 0 || limits.StderrBytes < 0 { return Result{}, ErrOutputLimit } diff --git a/tools/thothctl/internal/compose/runner_test.go b/tools/thothctl/internal/compose/runner_test.go index fd630064..beb885d7 100644 --- a/tools/thothctl/internal/compose/runner_test.go +++ b/tools/thothctl/internal/compose/runner_test.go @@ -7,6 +7,7 @@ import ( "os/exec" "path/filepath" "runtime" + "strconv" "strings" "testing" "time" @@ -123,6 +124,35 @@ func TestRunnerBoundedCancellationWinsOverChildKill(t *testing.T) { } } +func TestRunnerBoundedKillsGrandchildProcess(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("shell process-group fixture is Unix-specific") + } + pidFile := filepath.Join(t.TempDir(), "grandchild.pid") + script := "#!/bin/sh\n(sleep 30) &\nprintf '%s' $! > '" + pidFile + "'\nwhile :; do printf x; done\n" + runner := NewRunner(writeExecutable(t, script)) + _, err := runner.RunBounded(context.Background(), nil, nil, CaptureLimits{StdoutBytes: 64, StderrBytes: 64}) + if !errors.Is(err, ErrOutputLimit) { + t.Fatalf("err = %v", err) + } + pidBytes, readErr := os.ReadFile(pidFile) + if readErr != nil { + t.Fatal(readErr) + } + pid, err := strconv.Atoi(strings.TrimSpace(string(pidBytes))) + if err != nil { + t.Fatal(err) + } + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + if !processExists(pid) { + return + } + time.Sleep(20 * time.Millisecond) + } + t.Fatalf("grandchild pid %d remained alive after owned-process termination", pid) +} + func TestRunnerBoundedTerminatesGrandchildHoldingPipe(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("shell process-group fixture is Unix-specific") diff --git a/tools/thothctl/internal/safeio/files_test.go b/tools/thothctl/internal/safeio/files_test.go index 2b34c253..53f5e6b6 100644 --- a/tools/thothctl/internal/safeio/files_test.go +++ b/tools/thothctl/internal/safeio/files_test.go @@ -75,3 +75,51 @@ func TestWriteCanonicalExclusiveRejectsExistingAndCreatesPrivateFile(t *testing. t.Fatalf("replacement = %v", err) } } + +func TestReadCanonicalRegularRejectsHardlinkAndDirectory(t *testing.T) { + root, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + original := filepath.Join(root, "original.sql") + if err := os.WriteFile(original, []byte("select 1"), 0o600); err != nil { + t.Fatal(err) + } + hardlink := filepath.Join(root, "hardlink.sql") + if err := os.Link(original, hardlink); err != nil { + t.Fatal(err) + } + if _, err := ReadCanonicalRegular(hardlink, 1024); !errors.Is(err, ErrUnsafeFile) { + t.Fatalf("hardlink error = %v, want ErrUnsafeFile", err) + } + directory := filepath.Join(root, "directory.sql") + if err := os.Mkdir(directory, 0o700); err != nil { + t.Fatal(err) + } + if _, err := ReadCanonicalRegular(directory, 1024); !errors.Is(err, ErrUnsafeFile) { + t.Fatalf("directory error = %v, want ErrUnsafeFile", err) + } +} + +func TestValidateCanonicalOutputPathRejectsExistingDirectoryAndSymlink(t *testing.T) { + root, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + directory := filepath.Join(root, "existing.yaml") + if err := os.Mkdir(directory, 0o700); err != nil { + t.Fatal(err) + } + if err := ValidateCanonicalOutputPath(directory); !errors.Is(err, ErrUnsafeFile) { + t.Fatalf("existing directory error = %v, want ErrUnsafeFile", err) + } + target := filepath.Join(root, "target.yaml") + if err := os.WriteFile(target, []byte("target"), 0o600); err != nil { + t.Fatal(err) + } + link := filepath.Join(root, "link.yaml") + testsupport.SymlinkOrSkip(t, target, link) + if err := ValidateCanonicalOutputPath(link); !errors.Is(err, ErrUnsafeFile) { + t.Fatalf("output symlink error = %v, want ErrUnsafeFile", err) + } +} diff --git a/tools/thothctl/internal/safeio/files_unix.go b/tools/thothctl/internal/safeio/files_unix.go index cb2d5470..1a257a30 100644 --- a/tools/thothctl/internal/safeio/files_unix.go +++ b/tools/thothctl/internal/safeio/files_unix.go @@ -79,7 +79,7 @@ func closeUnixDescriptors(descriptors []int) { } func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) error { - if err := ValidateCanonicalPath(path); err != nil || len(contents) > 16<<20 || mode.Perm() == 0 || mode.Perm()&0o077 != 0 { + if err := ValidateCanonicalPath(path); err != nil || len(contents) > 16<<20 || mode.Perm() != 0o600 { return ErrUnsafeFile } components := strings.Split(strings.TrimPrefix(path, string(os.PathSeparator)), string(os.PathSeparator)) diff --git a/tools/thothctl/internal/safeio/files_windows.go b/tools/thothctl/internal/safeio/files_windows.go index d6322902..d7094ae8 100644 --- a/tools/thothctl/internal/safeio/files_windows.go +++ b/tools/thothctl/internal/safeio/files_windows.go @@ -6,8 +6,11 @@ import ( "io/fs" "os" "path/filepath" + "runtime" "strings" + "unsafe" + "golang.org/x/sys/windows" ) @@ -119,7 +122,7 @@ func closeWindowsHandles(handles []windows.Handle) { } func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) error { - if err := ValidateCanonicalPath(path); err != nil || len(contents) > 16<<20 || mode.Perm() == 0 || mode.Perm()&0o077 != 0 { + if err := ValidateCanonicalPath(path); err != nil || len(contents) > 16<<20 || mode.Perm() != 0o600 { return ErrUnsafeFile } parent, retainedParents, err := openWindowsParents(path) @@ -129,7 +132,11 @@ func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) err defer closeWindowsHandles(retainedParents) // Parent handles stay open with delete sharing denied until publication and // identity recheck complete; this is the Windows equivalent of retained dirfds. - h, err := windows.CreateFile(windows.StringToUTF16Ptr(filepath.Join(parent, filepath.Base(path))), windows.GENERIC_WRITE, 0, nil, windows.CREATE_NEW, windows.FILE_ATTRIBUTE_NORMAL|windows.FILE_FLAG_OPEN_REPARSE_POINT, 0) + securityDescriptor, securityAttributes, err := ownerOnlySecurityAttributes() + if err != nil { + return ErrUnsafeFile + } + h, err := windows.CreateFile(windows.StringToUTF16Ptr(filepath.Join(parent, filepath.Base(path))), windows.GENERIC_WRITE, 0, securityAttributes, windows.CREATE_NEW, windows.FILE_ATTRIBUTE_NORMAL|windows.FILE_FLAG_OPEN_REPARSE_POINT, 0) if err != nil { return ErrUnsafeFile } @@ -139,7 +146,11 @@ func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) err return ErrUnsafeFile } defer f.Close() - if err := f.Chmod(mode); err != nil { + // Go's Windows Chmod only toggles the read-only attribute; it cannot enforce + // owner-only permissions. The restrictive DACL is installed at creation time + // through SECURITY_ATTRIBUTES above. + _ = securityDescriptor + if mode.Perm() != 0o600 { return ErrUnsafeFile } if _, err := f.Write(contents); err != nil { @@ -202,3 +213,47 @@ func validateCanonicalOutputPath(path string) error { func sameWindowsFile(a, b windows.ByHandleFileInformation) bool { return a.VolumeSerialNumber == b.VolumeSerialNumber && a.FileIndexHigh == b.FileIndexHigh && a.FileIndexLow == b.FileIndexLow } + +// ownerOnlySecurityAttributes builds a non-inheriting DACL granting the current +// token's user read/write access and no access to inherited or other trustees. +// The descriptor is retained by the returned Go value for the duration of CreateFile. +func ownerOnlySecurityAttributes() (*windows.SECURITY_DESCRIPTOR, *windows.SecurityAttributes, error) { + user, err := windows.GetCurrentProcessToken().GetTokenUser() + if err != nil || user == nil || user.User.Sid == nil { + return nil, nil, ErrUnsafeFile + } + var pinner runtime.Pinner + pinner.Pin(user.User.Sid) + defer pinner.Unpin() + trustee := windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_USER, + TrusteeValue: windows.TrusteeValueFromSID(user.User.Sid), + } + entries := []windows.EXPLICIT_ACCESS{{ + AccessPermissions: windows.FILE_GENERIC_READ | windows.FILE_GENERIC_WRITE, + AccessMode: windows.SET_ACCESS, + Inheritance: windows.NO_INHERITANCE, + Trustee: trustee, + }} + descriptor, err := windows.BuildSecurityDescriptor(nil, nil, entries, nil, nil) + if err != nil || descriptor == nil { + return nil, nil, ErrUnsafeFile + } + acl, _, err := descriptor.DACL() + if err != nil || acl == nil { + return nil, nil, ErrUnsafeFile + } + if err := descriptor.SetControl(windows.SE_DACL_PROTECTED, windows.SE_DACL_PROTECTED); err != nil { + return nil, nil, ErrUnsafeFile + } + // BuildSecurityDescriptor returns a self-relative descriptor. Re-using its + // DACL as the creation descriptor is valid, and the explicit DACL has no + // inheritable ACEs; the protected flag is applied by the kernel on creation. + _ = acl + attrs := &windows.SecurityAttributes{ + Length: uint32(unsafe.Sizeof(windows.SecurityAttributes{})), + SecurityDescriptor: descriptor, + } + return descriptor, attrs, nil +} diff --git a/tools/thothctl/internal/safeio/files_windows_test.go b/tools/thothctl/internal/safeio/files_windows_test.go index 5a1da6f4..154fb25f 100644 --- a/tools/thothctl/internal/safeio/files_windows_test.go +++ b/tools/thothctl/internal/safeio/files_windows_test.go @@ -72,3 +72,29 @@ func TestWriteCanonicalExclusiveRequiresRestrictiveMode(t *testing.T) { t.Fatal("accepted non-restrictive output mode") } } + +func TestWriteCanonicalExclusiveCreatesProtectedOwnerOnlyDACL(t *testing.T) { + root, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + path := filepath.Join(root, "candidate.yaml") + if err := writeCanonicalExclusive(path, []byte("x"), 0o600); err != nil { + t.Fatal(err) + } + sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Fatal(err) + } + control, _, err := sd.Control() + if err != nil { + t.Fatal(err) + } + if control&windows.SE_DACL_PROTECTED == 0 { + t.Fatalf("output DACL control = %#x, want protected", control) + } + acl, _, err := sd.DACL() + if err != nil || acl == nil || acl.AceCount != 1 { + t.Fatalf("output DACL = %#v, err=%v; want one owner ACE", acl, err) + } +} diff --git a/tools/thothctl/internal/workspaceops/operations.go b/tools/thothctl/internal/workspaceops/operations.go index 572c19a7..81d2fa23 100644 --- a/tools/thothctl/internal/workspaceops/operations.go +++ b/tools/thothctl/internal/workspaceops/operations.go @@ -29,8 +29,11 @@ const ( maxAssumptions = 256 maxAssumptionBytes = 256 maxResult = 1 << 20 - maxCandidate = 700 << 10 - maxAnnotations = 16 << 20 + // Requests carry up to 16 MiB of raw SQL or annotation bytes encoded as base64. + // This is deliberately independent from maxResult, which bounds child stdout. + maxRequest = 24 << 20 + maxCandidate = 700 << 10 + maxAnnotations = 16 << 20 ) var workspaceIDPattern = regexp.MustCompile(`^[a-z][a-z0-9-]{2,62}$`) @@ -392,7 +395,7 @@ func makeInput(c Command) (inputEnvelope, string, error) { if e != nil { return env, "", e } - if len(payload) > maxResult { + if len(payload) > maxRequest { return env, "", errors.New("request exceeds limit") } return env, string(payload), nil @@ -461,8 +464,8 @@ func Run(ctx context.Context, installation config.Installation, runner compose.R } payload := []byte(generated) if stdin != nil { - payload, e = io.ReadAll(io.LimitReader(stdin, maxResult+1)) - if e != nil || len(payload) > maxResult { + payload, e = io.ReadAll(io.LimitReader(stdin, maxRequest+1)) + if e != nil || len(payload) > maxRequest { return Result{}, errors.New("request exceeds limit") } if e = validateIngress(payload, env); e != nil { diff --git a/tools/thothctl/internal/workspaceops/operations_test.go b/tools/thothctl/internal/workspaceops/operations_test.go index 2f8149f0..dcdc5643 100644 --- a/tools/thothctl/internal/workspaceops/operations_test.go +++ b/tools/thothctl/internal/workspaceops/operations_test.go @@ -83,9 +83,10 @@ func TestRunUsesBase64BasenameIngressAndNoTTY(t *testing.T) { if err := os.WriteFile(sqlPath, []byte("select 1"), 0o600); err != nil { t.Fatal(err) } - out := filepath.Join(root, "candidate.yaml") + stdinCapture := filepath.Join(root, "stdin.json") + argsCapture := filepath.Join(root, "args.txt") resultJSON := `{"schemaVersion":1,"status":"succeeded","code":"ok","workspaceId":"psd","workspaceRevision":"0123456789012345678901234567890123456789","descriptorBlob":"abcdefabcdefabcdefabcdefabcdefabcdefabcd","operation":"suggest-fks","runId":"0123456789abcdef0123456789abcdef","completedStages":[]}` - script := "#!/bin/sh\ncat >/dev/null\nprintf '%s' '" + resultJSON + "'\n" + script := "#!/bin/sh\nprintf '%s\n' \"$@\" > '" + argsCapture + "'\ncat > '" + stdinCapture + "'\nprintf '%s' '" + resultJSON + "'\n" fake := filepath.Join(root, "docker") if err := os.WriteFile(fake, []byte(script), 0o700); err != nil { t.Fatal(err) @@ -101,7 +102,20 @@ func TestRunUsesBase64BasenameIngressAndNoTTY(t *testing.T) { if got.Code != "ok" { t.Fatalf("result = %#v", got) } - _ = out + stdinBytes, err := os.ReadFile(stdinCapture) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(stdinBytes), sqlPath) || !strings.Contains(string(stdinBytes), "contentBase64") || !strings.Contains(string(stdinBytes), "schema.sql") { + t.Fatalf("stdin envelope = %q; want basename/base64 without host path", stdinBytes) + } + argsBytes, err := os.ReadFile(argsCapture) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(argsBytes), sqlPath) || !strings.Contains(string(argsBytes), "--operation\nsuggest-fks") || !strings.Contains(string(argsBytes), "--workspace\npsd") { + t.Fatalf("child args = %q; want closed operation/workspace args", argsBytes) + } } func TestRunPublishesOnlyVerifiedCandidateExport(t *testing.T) { @@ -295,3 +309,43 @@ func TestParseWorkspaceAcceptsAssumptionAndEnforcesLimits(t *testing.T) { t.Fatal("accepted 33 SQL files") } } + +func TestMakeInputAcceptsMaximumSingleSQLFile(t *testing.T) { + root, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + path := filepath.Join(root, "schema.sql") + if err := os.WriteFile(path, bytes.Repeat([]byte("x"), maxSQLFile), 0o600); err != nil { + t.Fatal(err) + } + command := SuggestFksRequest{WorkspaceID: "psd", FromSQL: []string{path}} + if _, payload, err := makeInput(command); err != nil { + t.Fatalf("makeInput() error = %v", err) + } else if len(payload) <= maxResult { + t.Fatalf("payload length = %d, want larger than result cap %d", len(payload), maxResult) + } +} + +func TestMakeInputAcceptsMaximumAnnotationWithReviewDigestPair(t *testing.T) { + root, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + path := filepath.Join(root, "annotations.md") + contents := bytes.Repeat([]byte("a"), maxAnnotations) + if err := os.WriteFile(path, contents, 0o600); err != nil { + t.Fatal(err) + } + command := CheckSchemaRequest{WorkspaceID: "psd", Resume: strings.Repeat("0", 32), Annotations: path, ReviewedCandidates: DigestBytes([]byte("reviewed"))} + env, payload, err := makeInput(command) + if err != nil { + t.Fatal(err) + } + if env.Annotations == nil || env.ReviewedCandidates == "" || len(payload) <= maxResult { + t.Fatalf("annotation envelope = %#v payload=%d; want pair and request larger than result cap", env, len(payload)) + } + if err := validateIngress([]byte(payload), env); err != nil { + t.Fatalf("validateIngress() = %v", err) + } +}