Skip to content

Commit 63eefef

Browse files
fix: resolve 15 critical and high severity bugs across CLI (#18)
Critical fixes: - Fix region mapping collision causing silent data loss on round-trip (Koyeb/Railway regions now use disambiguated codes like koyeb_fra, railway_us-west2) - Fix global koanf instance causing state pollution between ReadConfig/ReadOpenStatus calls - Fix partial failure in ApplyChanges leaving lock map in inconsistent state - Fix spinner leak when statusToSDK fails in status-report list/add-update - Fix TCP monitor creation accepting zero/invalid port values High severity fixes: - Fix strconv.Atoi error silently ignored in httpMonitorToLocal/tcpMonitorToLocal - Fix DNS monitor info returning opaque "unknown monitor type" error - Fix ResolveToken silently discarding file read errors (e.g. permission denied) - Fix confirmation prompt blocking in non-interactive environments (added TTY check) - Fix delete commands showing confirmation with empty ID before validation - Fix --component-ids "" silently clearing components on status-report update - Fix monitor trigger printing plain text in --json mode - Fix http.DefaultClient used without timeout (added api.DefaultHTTPClient with 30s) - Fix unknown jobType aborting entire result set in run command - Fix monitor import truncating output file before API call succeeds
1 parent 093b72c commit 63eefef

29 files changed

Lines changed: 473 additions & 125 deletions

internal/api/client.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package api
33
import (
44
"context"
55
"fmt"
6+
"net/http"
67
"os"
78
"time"
89

@@ -14,6 +15,10 @@ const APIBaseURL = "https://api.openstatus.dev/v1"
1415

1516
const ConnectBaseURL = "https://api.openstatus.dev/rpc"
1617

18+
var DefaultHTTPClient = &http.Client{
19+
Timeout: 30 * time.Second,
20+
}
21+
1722
func NewAuthInterceptor(apiKey string) connect.UnaryInterceptorFunc {
1823
return func(next connect.UnaryFunc) connect.UnaryFunc {
1924
return func(ctx context.Context, req connect.AnyRequest) (connect.AnyResponse, error) {

internal/auth/auth.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ func ResolveToken(flagValue string) (string, error) {
2727
if token != "" {
2828
return token, nil
2929
}
30+
} else if !os.IsNotExist(readErr) {
31+
return "", fmt.Errorf("failed to read token file %s: %w", tokenPath, readErr)
3032
}
3133
}
3234

internal/cli/confirmation.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,20 @@ import (
55
"fmt"
66
"os"
77
"strings"
8+
9+
"github.com/mattn/go-isatty"
810
)
911

12+
var isInteractiveStdin = func() bool {
13+
return isatty.IsTerminal(os.Stdin.Fd()) || isatty.IsCygwinTerminal(os.Stdin.Fd())
14+
}
15+
1016
func AskForConfirmation(s string) (bool, error) {
17+
if !isInteractiveStdin() {
18+
return false, fmt.Errorf("confirmation required but stdin is not a terminal (use --auto-accept / -y to skip)")
19+
}
1120
reader := bufio.NewReader(os.Stdin)
12-
fmt.Printf("%s [y/N]: ", s)
21+
fmt.Fprintf(os.Stderr, "%s [y/N]: ", s)
1322
response, err := reader.ReadString('\n')
1423
if err != nil {
1524
return false, fmt.Errorf("failed to read user input: %w", err)

internal/cli/confirmation_test.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@ import (
88
)
99

1010
func Test_AskForConfirmation(t *testing.T) {
11+
restore := cli.SetInteractiveStdin(func() bool { return true })
12+
t.Cleanup(restore)
13+
1114
t.Run("Returns true for 'y' input", func(t *testing.T) {
1215
// Create a pipe to simulate stdin
1316
r, w, err := os.Pipe()

internal/cli/export_test.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
package cli
2+
3+
func SetInteractiveStdin(f func() bool) func() {
4+
old := isInteractiveStdin
5+
isInteractiveStdin = f
6+
return func() { isInteractiveStdin = old }
7+
}

internal/config/config.go

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,9 @@ package config
33
import (
44
"github.com/knadh/koanf/parsers/yaml"
55
"github.com/knadh/koanf/providers/file"
6-
76
"github.com/knadh/koanf/v2"
87
)
98

10-
var k = koanf.New(".")
11-
129
type TestsConfig struct {
1310
Ids []int `koanf:"ids"`
1411
}
@@ -18,22 +15,17 @@ type Config struct {
1815
}
1916

2017
func ReadConfig(path string) (*Config, error) {
18+
k := koanf.New(".")
2119

22-
file := file.Provider(path)
23-
24-
err := k.Load(file, yaml.Parser())
25-
26-
if err != nil {
20+
f := file.Provider(path)
21+
if err := k.Load(f, yaml.Parser()); err != nil {
2722
return nil, err
2823
}
2924

3025
var out Config
31-
32-
err = k.Unmarshal("", &out)
33-
if err != nil {
26+
if err := k.Unmarshal("", &out); err != nil {
3427
return nil, err
3528
}
3629

3730
return &out, nil
38-
3931
}

internal/config/config_test.go

Lines changed: 33 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package config_test
22

33
import (
44
"os"
5+
"path/filepath"
56
"testing"
67

78
"github.com/google/go-cmp/cmp"
@@ -18,11 +19,10 @@ tests:
1819

1920
func Test_ReadConfig(t *testing.T) {
2021
t.Run("Read valid config file", func(t *testing.T) {
21-
f, err := os.CreateTemp(".", "config*.yaml")
22+
f, err := os.CreateTemp(t.TempDir(), "config*.yaml")
2223
if err != nil {
2324
t.Fatal(err)
2425
}
25-
defer os.Remove(f.Name())
2626

2727
if _, err := f.Write([]byte(configFile)); err != nil {
2828
t.Fatal(err)
@@ -55,11 +55,10 @@ func Test_ReadConfig(t *testing.T) {
5555
})
5656

5757
t.Run("Invalid YAML content", func(t *testing.T) {
58-
f, err := os.CreateTemp(".", "invalid*.yaml")
58+
f, err := os.CreateTemp(t.TempDir(), "invalid*.yaml")
5959
if err != nil {
6060
t.Fatal(err)
6161
}
62-
defer os.Remove(f.Name())
6362

6463
if _, err := f.Write([]byte("invalid: yaml: content: [")); err != nil {
6564
t.Fatal(err)
@@ -74,3 +73,33 @@ func Test_ReadConfig(t *testing.T) {
7473
}
7574
})
7675
}
76+
77+
func Test_ReadConfig_NoStatePollution(t *testing.T) {
78+
dir := t.TempDir()
79+
80+
file1 := filepath.Join(dir, "config1.yaml")
81+
if err := os.WriteFile(file1, []byte("tests:\n ids:\n - 1\n - 2\n - 3\n"), 0600); err != nil {
82+
t.Fatal(err)
83+
}
84+
85+
file2 := filepath.Join(dir, "config2.yaml")
86+
if err := os.WriteFile(file2, []byte("tests:\n ids:\n - 4\n - 5\n - 6\n"), 0600); err != nil {
87+
t.Fatal(err)
88+
}
89+
90+
out1, err := config.ReadConfig(file1)
91+
if err != nil {
92+
t.Fatal(err)
93+
}
94+
if !cmp.Equal(out1.Tests.Ids, []int{1, 2, 3}) {
95+
t.Errorf("First read: expected [1,2,3], got %v", out1.Tests.Ids)
96+
}
97+
98+
out2, err := config.ReadConfig(file2)
99+
if err != nil {
100+
t.Fatal(err)
101+
}
102+
if !cmp.Equal(out2.Tests.Ids, []int{4, 5, 6}) {
103+
t.Errorf("Second read: expected [4,5,6], got %v", out2.Tests.Ids)
104+
}
105+
}

internal/config/monitor.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,18 @@ const (
142142
Waw Region = "waw"
143143
Yul Region = "yul"
144144
Yyz Region = "yyz"
145+
// Koyeb regions (disambiguated from Fly.io regions with same airport code)
146+
KoyebFra Region = "koyeb_fra"
147+
KoyebPar Region = "koyeb_par"
148+
KoyebSfo Region = "koyeb_sfo"
149+
KoyebSin Region = "koyeb_sin"
150+
KoyebTyo Region = "koyeb_tyo"
151+
KoyebWas Region = "koyeb_was"
152+
// Railway regions
153+
RailwayUsWest2 Region = "railway_us-west2"
154+
RailwayUsEast4 Region = "railway_us-east4-eqdc4a"
155+
RailwayEuropeWest4 Region = "railway_europe-west4-drams3a"
156+
RailwayAsiaSoutheast1 Region = "railway_asia-southeast1-eqsg3a"
145157
)
146158

147159
type Method string

internal/config/openstatus.go

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,24 +3,21 @@ package config
33
import (
44
"github.com/knadh/koanf/parsers/yaml"
55
"github.com/knadh/koanf/providers/file"
6+
"github.com/knadh/koanf/v2"
67
)
78

89
type Monitors map[string]Monitor
910

1011
func ReadOpenStatus(path string) (Monitors, error) {
11-
f := file.Provider(path)
12-
13-
err := k.Load(f, yaml.Parser())
12+
k := koanf.New(".")
1413

15-
if err != nil {
14+
f := file.Provider(path)
15+
if err := k.Load(f, yaml.Parser()); err != nil {
1616
return nil, err
1717
}
1818

1919
var out Monitors
20-
21-
err = k.Unmarshal("", &out)
22-
23-
if err != nil {
20+
if err := k.Unmarshal("", &out); err != nil {
2421
return nil, err
2522
}
2623

internal/config/openstatus_test.go

Lines changed: 65 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package config_test
22

33
import (
44
"os"
5+
"path/filepath"
56
"testing"
67

78
"github.com/openstatusHQ/cli/internal/config"
@@ -31,11 +32,10 @@ var openstatusConfig = `
3132

3233
func Test_ReadOpenStatus(t *testing.T) {
3334
t.Run("Read valid openstatus config", func(t *testing.T) {
34-
f, err := os.CreateTemp(".", "openstatus*.yaml")
35+
f, err := os.CreateTemp(t.TempDir(), "openstatus*.yaml")
3536
if err != nil {
3637
t.Fatal(err)
3738
}
38-
defer os.Remove(f.Name())
3939

4040
if _, err := f.Write([]byte(openstatusConfig)); err != nil {
4141
t.Fatal(err)
@@ -49,9 +49,6 @@ func Test_ReadOpenStatus(t *testing.T) {
4949
t.Fatal(err)
5050
}
5151

52-
// Check that the monitor was read correctly
53-
// Note: We check for the specific monitor because the global koanf instance
54-
// may have accumulated state from previous tests
5552
monitor, exists := out["test-monitor"]
5653
if !exists {
5754
t.Fatal("Expected 'test-monitor' to exist in output")
@@ -112,11 +109,10 @@ func Test_ReadOpenStatus_FollowRedirects(t *testing.T) {
112109
url: https://example.com
113110
followRedirects: false
114111
`
115-
f, err := os.CreateTemp(".", "openstatus*.yaml")
112+
f, err := os.CreateTemp(t.TempDir(), "openstatus*.yaml")
116113
if err != nil {
117114
t.Fatal(err)
118115
}
119-
defer os.Remove(f.Name())
120116

121117
if _, err := f.Write([]byte(yaml)); err != nil {
122118
t.Fatal(err)
@@ -156,11 +152,10 @@ func Test_ReadOpenStatus_FollowRedirects(t *testing.T) {
156152
method: GET
157153
url: https://example.com
158154
`
159-
f, err := os.CreateTemp(".", "openstatus*.yaml")
155+
f, err := os.CreateTemp(t.TempDir(), "openstatus*.yaml")
160156
if err != nil {
161157
t.Fatal(err)
162158
}
163-
defer os.Remove(f.Name())
164159

165160
if _, err := f.Write([]byte(yaml)); err != nil {
166161
t.Fatal(err)
@@ -261,3 +256,64 @@ func Test_ParseConfigMonitorsToMonitor(t *testing.T) {
261256
}
262257
})
263258
}
259+
260+
func Test_ReadOpenStatus_NoStatePollution(t *testing.T) {
261+
dir := t.TempDir()
262+
263+
file1 := filepath.Join(dir, "config1.yaml")
264+
if err := os.WriteFile(file1, []byte(`
265+
"monitor-a":
266+
active: true
267+
frequency: 10m
268+
kind: http
269+
name: Monitor A
270+
regions:
271+
- iad
272+
request:
273+
method: GET
274+
url: https://a.example.com
275+
`), 0600); err != nil {
276+
t.Fatal(err)
277+
}
278+
279+
file2 := filepath.Join(dir, "config2.yaml")
280+
if err := os.WriteFile(file2, []byte(`
281+
"monitor-b":
282+
active: true
283+
frequency: 5m
284+
kind: http
285+
name: Monitor B
286+
regions:
287+
- ams
288+
request:
289+
method: POST
290+
url: https://b.example.com
291+
`), 0600); err != nil {
292+
t.Fatal(err)
293+
}
294+
295+
out1, err := config.ReadOpenStatus(file1)
296+
if err != nil {
297+
t.Fatal(err)
298+
}
299+
if _, exists := out1["monitor-a"]; !exists {
300+
t.Error("First read: expected 'monitor-a' to exist")
301+
}
302+
if len(out1) != 1 {
303+
t.Errorf("First read: expected 1 monitor, got %d", len(out1))
304+
}
305+
306+
out2, err := config.ReadOpenStatus(file2)
307+
if err != nil {
308+
t.Fatal(err)
309+
}
310+
if _, exists := out2["monitor-b"]; !exists {
311+
t.Error("Second read: expected 'monitor-b' to exist")
312+
}
313+
if _, exists := out2["monitor-a"]; exists {
314+
t.Error("Second read: 'monitor-a' should not be present (state pollution)")
315+
}
316+
if len(out2) != 1 {
317+
t.Errorf("Second read: expected 1 monitor, got %d", len(out2))
318+
}
319+
}

0 commit comments

Comments
 (0)