Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion internal/autostart/autostart_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@
package autostart

import (
"errors"
"fmt"
"io/fs"
"os"
"path/filepath"
"strings"
Expand Down Expand Up @@ -42,7 +44,13 @@ func Enable(skrogExe string) error {
"(a console binary at logon would flash a console window): %w", err)
}

k, err := registry.OpenKey(registry.CURRENT_USER, runKeyPath, registry.SET_VALUE)
// CreateKey, not OpenKey: the Run key is created on demand by Windows and
// is NOT guaranteed to exist (#444). A profile that has never had a logon
// entry has no key, and OpenKey then fails with "The system cannot find
// the file specified" — which reads as a missing FILE and sends the user
// looking for skrogw.exe. CreateKey opens an existing key unchanged, so
// this costs nothing on the machines that already have one.
k, _, err := registry.CreateKey(registry.CURRENT_USER, runKeyPath, registry.SET_VALUE)
if err != nil {
return fmt.Errorf("opening the Run key: %w", err)
}
Expand All @@ -59,6 +67,13 @@ func Enable(skrogExe string) error {
// success: the user asked for a state, not an action.
func Disable() error {
k, err := registry.OpenKey(registry.CURRENT_USER, runKeyPath, registry.SET_VALUE)
if errors.Is(err, registry.ErrNotExist) || errors.Is(err, fs.ErrNotExist) {
// No Run key at all, so nothing is registered — which is the state
// the caller asked for. The comment above already said a missing
// VALUE is success; a missing KEY is the same answer one level up,
// and failing here contradicted it (#444).
return nil
}
if err != nil {
return fmt.Errorf("opening the Run key: %w", err)
}
Expand Down Expand Up @@ -106,6 +121,13 @@ func DisableIfOwned(installDir string) (bool, error) {
// Status returns whether autostart is registered, and the command if so.
func Status() (enabled bool, command string, err error) {
k, err := registry.OpenKey(registry.CURRENT_USER, runKeyPath, registry.QUERY_VALUE)
if errors.Is(err, registry.ErrNotExist) || errors.Is(err, fs.ErrNotExist) {
// No key means nothing is registered, which is an answer and not a
// failure (#444). Reporting an error here made `skrog status` and
// `doctor` fail on a profile that had simply never autostarted
// anything.
return false, "", nil
}
if err != nil {
return false, "", fmt.Errorf("opening the Run key: %w", err)
}
Expand Down
82 changes: 82 additions & 0 deletions internal/autostart/autostart_windows_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -194,3 +194,85 @@ func TestDisableIfOwnedMatchesAResolvedDir(t *testing.T) {
t.Fatal("the owner could not remove its own entry when passing a resolved directory")
}
}

// useAbsentScratchKey points the package at a key that does NOT exist, and
// makes sure it stays that way until the code under test creates it.
//
// This is the case the existing helper cannot cover, because it creates the
// key first — with a comment saying it must exist for OpenKey(SET_VALUE) to
// succeed. That comment is the bug report: the product assumed a precondition
// the test then supplied for it (#444).
func useAbsentScratchKey(t *testing.T) {
t.Helper()
orig := runKeyPath
runKeyPath = `Software\SkrogTest\AbsentRun`

// Make sure a previous run did not leave it behind.
registry.DeleteKey(registry.CURRENT_USER, runKeyPath)
if k, err := registry.OpenKey(registry.CURRENT_USER, runKeyPath, registry.QUERY_VALUE); err == nil {
k.Close()
t.Fatalf("%s exists; this test is about the case where it does not", runKeyPath)
}

t.Cleanup(func() {
registry.DeleteKey(registry.CURRENT_USER, runKeyPath)
registry.DeleteKey(registry.CURRENT_USER, `Software\SkrogTest`)
runKeyPath = orig
})
}

// A profile with no Run key at all must still be able to enable autostart.
//
// Windows creates HKCU\...\CurrentVersion\Run on demand, so a profile that has
// never registered a logon entry does not have one. OpenKey then fails with
// "The system cannot find the file specified", which `skrog install --config`
// surfaced as:
//
// skrog: applying config: opening the Run key: The system cannot find the file specified.
//
// — a message that reads as a missing FILE and sends the user looking for
// skrogw.exe. Found by the nightly acceptance run on a clean hosted runner.
func TestEnableCreatesTheRunKeyWhenAbsent(t *testing.T) {
useAbsentScratchKey(t)
exe := fakeInstall(t, true)

if err := Enable(exe); err != nil {
t.Fatalf("Enable with no Run key: %v", err)
}
enabled, cmd, err := Status()
if err != nil {
t.Fatalf("Status: %v", err)
}
if !enabled {
t.Error("autostart not registered after Enable created the key")
}
if !strings.Contains(cmd, "skrogw.exe") {
t.Errorf("Run entry = %q, want the skrogw launcher", cmd)
}
}

// Status on a profile with no Run key is "not enabled", not an error. It used
// to fail, which broke `skrog status` and `doctor` on such a profile.
func TestStatusWithNoRunKeyIsNotAnError(t *testing.T) {
useAbsentScratchKey(t)

enabled, cmd, err := Status()
if err != nil {
t.Fatalf("Status with no Run key returned an error: %v", err)
}
if enabled || cmd != "" {
t.Errorf("Status = (%v, %q), want (false, \"\")", enabled, cmd)
}
}

// Disable's own doc comment says removing an entry that does not exist is
// success — "the user asked for a state, not an action". That held for a
// missing VALUE and not for a missing KEY, which is the same answer one level
// up.
func TestDisableWithNoRunKeyIsSuccess(t *testing.T) {
useAbsentScratchKey(t)

if err := Disable(); err != nil {
t.Errorf("Disable with no Run key: %v", err)
}
}
Loading