Skip to content

Commit 0558622

Browse files
teggenclaude
andcommitted
Fix security vulnerabilities and session reliability from code review
- Decouple gateway bind from launcher public mode so -public no longer exposes the gateway port on 0.0.0.0 (review finding sipeed#3) - Return HTTP 500 for real session directory read errors instead of silently returning an empty list (review finding sipeed#4) - Sort sessions by time.Time instead of RFC3339 strings to handle mixed timezone offsets correctly (review finding sipeed#5) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
1 parent cd3cb06 commit 0558622

5 files changed

Lines changed: 202 additions & 60 deletions

File tree

web/backend/api/gateway.go

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -389,9 +389,6 @@ func (h *Handler) startGatewayLocked(initialStatus string, existingPid int) (int
389389
if h.configPath != "" {
390390
cmd.Env = append(cmd.Env, config.EnvConfig+"="+h.configPath)
391391
}
392-
if host := h.gatewayHostOverride(); host != "" {
393-
cmd.Env = append(cmd.Env, config.EnvGatewayHost+"="+host)
394-
}
395392

396393
stdoutPipe, err := cmd.StdoutPipe()
397394
if err != nil {

web/backend/api/gateway_host.go

Lines changed: 0 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -10,30 +10,7 @@ import (
1010
"github.com/sipeed/picoclaw/pkg/config"
1111
)
1212

13-
func (h *Handler) effectiveLauncherPublic() bool {
14-
if h.serverPublicExplicit {
15-
return h.serverPublic
16-
}
17-
18-
cfg, err := h.loadLauncherConfig()
19-
if err == nil {
20-
return cfg.Public
21-
}
22-
23-
return h.serverPublic
24-
}
25-
26-
func (h *Handler) gatewayHostOverride() string {
27-
if h.effectiveLauncherPublic() {
28-
return "0.0.0.0"
29-
}
30-
return ""
31-
}
32-
3313
func (h *Handler) effectiveGatewayBindHost(cfg *config.Config) string {
34-
if override := h.gatewayHostOverride(); override != "" {
35-
return override
36-
}
3714
if cfg == nil {
3815
return ""
3916
}

web/backend/api/gateway_host_test.go

Lines changed: 32 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -10,42 +10,32 @@ import (
1010
"time"
1111

1212
"github.com/sipeed/picoclaw/pkg/config"
13-
"github.com/sipeed/picoclaw/web/backend/launcherconfig"
1413
)
1514

16-
func TestGatewayHostOverrideUsesExplicitRuntimePublic(t *testing.T) {
15+
func TestPublicLauncherDoesNotOverrideGatewayBind(t *testing.T) {
1716
configPath := filepath.Join(t.TempDir(), "config.json")
18-
launcherPath := launcherconfig.PathForAppConfig(configPath)
19-
if err := launcherconfig.Save(launcherPath, launcherconfig.Config{
20-
Port: 18800,
21-
Public: false,
22-
}); err != nil {
23-
t.Fatalf("launcherconfig.Save() error = %v", err)
17+
cfg := config.DefaultConfig()
18+
cfg.Gateway.Host = "127.0.0.1"
19+
cfg.Gateway.Port = 18790
20+
if err := config.SaveConfig(configPath, cfg); err != nil {
21+
t.Fatalf("SaveConfig() error = %v", err)
2422
}
2523

2624
h := NewHandler(configPath)
2725
h.SetServerOptions(18800, true, true, nil)
2826

29-
if got := h.gatewayHostOverride(); got != "0.0.0.0" {
30-
t.Fatalf("gatewayHostOverride() = %q, want %q", got, "0.0.0.0")
27+
if got := h.effectiveGatewayBindHost(cfg); got != "127.0.0.1" {
28+
t.Fatalf("effectiveGatewayBindHost() = %q, want %q (public mode should not override)", got, "127.0.0.1")
3129
}
3230
}
3331

34-
func TestBuildWsURLUsesRequestHostWhenLauncherPublicSaved(t *testing.T) {
32+
func TestBuildWsURLUsesRequestHostWhenGatewayBindIsWildcard(t *testing.T) {
3533
configPath := filepath.Join(t.TempDir(), "config.json")
36-
launcherPath := launcherconfig.PathForAppConfig(configPath)
37-
if err := launcherconfig.Save(launcherPath, launcherconfig.Config{
38-
Port: 18800,
39-
Public: true,
40-
}); err != nil {
41-
t.Fatalf("launcherconfig.Save() error = %v", err)
42-
}
43-
4434
h := NewHandler(configPath)
4535
h.SetServerOptions(18800, false, false, nil)
4636

4737
cfg := config.DefaultConfig()
48-
cfg.Gateway.Host = "127.0.0.1"
38+
cfg.Gateway.Host = "" // empty = wildcard, should use request host
4939
cfg.Gateway.Port = 18790
5040

5141
req := httptest.NewRequest("GET", "http://launcher.local/api/pico/token", nil)
@@ -56,6 +46,23 @@ func TestBuildWsURLUsesRequestHostWhenLauncherPublicSaved(t *testing.T) {
5646
}
5747
}
5848

49+
func TestBuildWsURLUsesConfigHostWhenSet(t *testing.T) {
50+
configPath := filepath.Join(t.TempDir(), "config.json")
51+
h := NewHandler(configPath)
52+
h.SetServerOptions(18800, false, false, nil)
53+
54+
cfg := config.DefaultConfig()
55+
cfg.Gateway.Host = "10.0.0.5"
56+
cfg.Gateway.Port = 18790
57+
58+
req := httptest.NewRequest("GET", "http://launcher.local/api/pico/token", nil)
59+
req.Host = "192.168.1.9:18800"
60+
61+
if got := h.buildWsURL(req, cfg); got != "ws://10.0.0.5:18800/pico/ws" {
62+
t.Fatalf("buildWsURL() = %q, want %q", got, "ws://10.0.0.5:18800/pico/ws")
63+
}
64+
}
65+
5966
func TestGatewayProbeHostUsesLoopbackForWildcardBind(t *testing.T) {
6067
if got := gatewayProbeHost("0.0.0.0"); got != "127.0.0.1" {
6168
t.Fatalf("gatewayProbeHost() = %q, want %q", got, "127.0.0.1")
@@ -106,13 +113,13 @@ func TestGetGatewayHealthUsesConfiguredHost(t *testing.T) {
106113
}
107114
}
108115

109-
func TestGetGatewayHealthUsesProbeHostForPublicLauncher(t *testing.T) {
116+
func TestGetGatewayHealthUsesConfigHostForPublicLauncher(t *testing.T) {
110117
configPath := filepath.Join(t.TempDir(), "config.json")
111118
h := NewHandler(configPath)
112119
h.SetServerOptions(18800, true, true, nil)
113120

114121
cfg := config.DefaultConfig()
115-
cfg.Gateway.Host = "127.0.0.1"
122+
cfg.Gateway.Host = "192.168.1.10"
116123
cfg.Gateway.Port = 18791
117124

118125
originalHealthGet := gatewayHealthGet
@@ -130,8 +137,9 @@ func TestGetGatewayHealthUsesProbeHostForPublicLauncher(t *testing.T) {
130137
_ = statusCode
131138
_ = err
132139

133-
if requestedURL != "http://127.0.0.1:18791/health" {
134-
t.Fatalf("health url = %q, want %q", requestedURL, "http://127.0.0.1:18791/health")
140+
// Public mode no longer overrides the gateway host — config host is used directly.
141+
if requestedURL != "http://192.168.1.10:18791/health" {
142+
t.Fatalf("health url = %q, want %q", requestedURL, "http://192.168.1.10:18791/health")
135143
}
136144
}
137145

web/backend/api/session.go

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -34,12 +34,13 @@ type sessionFile struct {
3434

3535
// sessionListItem is a lightweight summary returned by GET /api/sessions.
3636
type sessionListItem struct {
37-
ID string `json:"id"`
38-
Title string `json:"title"`
39-
Preview string `json:"preview"`
40-
MessageCount int `json:"message_count"`
41-
Created string `json:"created"`
42-
Updated string `json:"updated"`
37+
ID string `json:"id"`
38+
Title string `json:"title"`
39+
Preview string `json:"preview"`
40+
MessageCount int `json:"message_count"`
41+
Created string `json:"created"`
42+
Updated string `json:"updated"`
43+
updatedTime time.Time // unexported; used for sorting, omitted from JSON
4344
}
4445

4546
type sessionMetaFile struct {
@@ -229,6 +230,7 @@ func buildSessionListItem(sessionID string, sess sessionFile) sessionListItem {
229230
MessageCount: validMessageCount,
230231
Created: sess.Created.Format(time.RFC3339),
231232
Updated: sess.Updated.Format(time.RFC3339),
233+
updatedTime: sess.Updated,
232234
}
233235
}
234236

@@ -286,9 +288,13 @@ func (h *Handler) handleListSessions(w http.ResponseWriter, r *http.Request) {
286288

287289
entries, err := os.ReadDir(dir)
288290
if err != nil {
289-
// Directory doesn't exist yet = no sessions
290-
w.Header().Set("Content-Type", "application/json")
291-
json.NewEncoder(w).Encode([]sessionListItem{})
291+
if errors.Is(err, os.ErrNotExist) {
292+
// Directory doesn't exist yet = no sessions
293+
w.Header().Set("Content-Type", "application/json")
294+
json.NewEncoder(w).Encode([]sessionListItem{})
295+
return
296+
}
297+
http.Error(w, "failed to read sessions directory", http.StatusInternalServerError)
292298
return
293299
}
294300

@@ -367,7 +373,7 @@ func (h *Handler) handleListSessions(w http.ResponseWriter, r *http.Request) {
367373

368374
// Sort by updated descending (most recent first)
369375
sort.Slice(items, func(i, j int) bool {
370-
return items[i].Updated > items[j].Updated
376+
return items[i].updatedTime.After(items[j].updatedTime)
371377
})
372378

373379
// Pagination parameters

web/backend/api/session_test.go

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -320,3 +320,157 @@ func TestHandleSessions_FiltersEmptyJSONLFiles(t *testing.T) {
320320
t.Fatalf("detail status = %d, want %d, body=%s", detailRec.Code, http.StatusNotFound, detailRec.Body.String())
321321
}
322322
}
323+
324+
func TestHandleListSessions_MissingDirectory(t *testing.T) {
325+
configPath, cleanup := setupOAuthTestEnv(t)
326+
defer cleanup()
327+
328+
// Don't create the sessions dir — it should return empty list with 200.
329+
h := NewHandler(configPath)
330+
mux := http.NewServeMux()
331+
h.RegisterRoutes(mux)
332+
333+
rec := httptest.NewRecorder()
334+
req := httptest.NewRequest(http.MethodGet, "/api/sessions", nil)
335+
mux.ServeHTTP(rec, req)
336+
337+
if rec.Code != http.StatusOK {
338+
t.Fatalf("status = %d, want %d, body=%s", rec.Code, http.StatusOK, rec.Body.String())
339+
}
340+
341+
var items []sessionListItem
342+
if err := json.Unmarshal(rec.Body.Bytes(), &items); err != nil {
343+
t.Fatalf("Unmarshal() error = %v", err)
344+
}
345+
if len(items) != 0 {
346+
t.Fatalf("len(items) = %d, want 0", len(items))
347+
}
348+
}
349+
350+
func TestHandleListSessions_UnreadableDirectory(t *testing.T) {
351+
configPath, cleanup := setupOAuthTestEnv(t)
352+
defer cleanup()
353+
354+
dir := sessionsTestDir(t, configPath)
355+
356+
// Remove all permissions so ReadDir fails with a permission error.
357+
if err := os.Chmod(dir, 0o000); err != nil {
358+
t.Fatalf("Chmod() error = %v", err)
359+
}
360+
t.Cleanup(func() { os.Chmod(dir, 0o755) })
361+
362+
h := NewHandler(configPath)
363+
mux := http.NewServeMux()
364+
h.RegisterRoutes(mux)
365+
366+
rec := httptest.NewRecorder()
367+
req := httptest.NewRequest(http.MethodGet, "/api/sessions", nil)
368+
mux.ServeHTTP(rec, req)
369+
370+
if rec.Code != http.StatusInternalServerError {
371+
t.Fatalf("status = %d, want %d, body=%s", rec.Code, http.StatusInternalServerError, rec.Body.String())
372+
}
373+
}
374+
375+
func TestHandleListSessions_MalformedSessionFileSkipped(t *testing.T) {
376+
configPath, cleanup := setupOAuthTestEnv(t)
377+
defer cleanup()
378+
379+
dir := sessionsTestDir(t, configPath)
380+
381+
// Write a valid session.
382+
store, err := memory.NewJSONLStore(dir)
383+
if err != nil {
384+
t.Fatalf("NewJSONLStore() error = %v", err)
385+
}
386+
validKey := picoSessionPrefix + "valid-session"
387+
if err := store.AddFullMessage(nil, validKey, providers.Message{
388+
Role: "user",
389+
Content: "hello",
390+
}); err != nil {
391+
t.Fatalf("AddFullMessage() error = %v", err)
392+
}
393+
394+
// Write a malformed legacy JSON session alongside it.
395+
corruptName := sanitizeSessionKey(picoSessionPrefix+"corrupt-session") + ".json"
396+
if err := os.WriteFile(filepath.Join(dir, corruptName), []byte("{invalid json"), 0o644); err != nil {
397+
t.Fatalf("WriteFile() error = %v", err)
398+
}
399+
400+
h := NewHandler(configPath)
401+
mux := http.NewServeMux()
402+
h.RegisterRoutes(mux)
403+
404+
rec := httptest.NewRecorder()
405+
req := httptest.NewRequest(http.MethodGet, "/api/sessions", nil)
406+
mux.ServeHTTP(rec, req)
407+
408+
if rec.Code != http.StatusOK {
409+
t.Fatalf("status = %d, want %d, body=%s", rec.Code, http.StatusOK, rec.Body.String())
410+
}
411+
412+
var items []sessionListItem
413+
if err := json.Unmarshal(rec.Body.Bytes(), &items); err != nil {
414+
t.Fatalf("Unmarshal() error = %v", err)
415+
}
416+
if len(items) != 1 {
417+
t.Fatalf("len(items) = %d, want 1 (valid session only)", len(items))
418+
}
419+
if items[0].ID != "valid-session" {
420+
t.Fatalf("items[0].ID = %q, want %q", items[0].ID, "valid-session")
421+
}
422+
}
423+
424+
func TestHandleListSessions_SortsByTimeNotString(t *testing.T) {
425+
configPath, cleanup := setupOAuthTestEnv(t)
426+
defer cleanup()
427+
428+
dir := sessionsTestDir(t, configPath)
429+
store, err := memory.NewJSONLStore(dir)
430+
if err != nil {
431+
t.Fatalf("NewJSONLStore() error = %v", err)
432+
}
433+
434+
// Create "older" session first, then "newer" session.
435+
// The newer session should appear first in results.
436+
olderKey := picoSessionPrefix + "older"
437+
if err := store.AddFullMessage(nil, olderKey, providers.Message{
438+
Role: "user", Content: "old message",
439+
}); err != nil {
440+
t.Fatalf("AddFullMessage(older) error = %v", err)
441+
}
442+
443+
newerKey := picoSessionPrefix + "newer"
444+
if err := store.AddFullMessage(nil, newerKey, providers.Message{
445+
Role: "user", Content: "new message",
446+
}); err != nil {
447+
t.Fatalf("AddFullMessage(newer) error = %v", err)
448+
}
449+
450+
h := NewHandler(configPath)
451+
mux := http.NewServeMux()
452+
h.RegisterRoutes(mux)
453+
454+
rec := httptest.NewRecorder()
455+
req := httptest.NewRequest(http.MethodGet, "/api/sessions", nil)
456+
mux.ServeHTTP(rec, req)
457+
458+
if rec.Code != http.StatusOK {
459+
t.Fatalf("status = %d, want %d", rec.Code, http.StatusOK)
460+
}
461+
462+
var items []sessionListItem
463+
if err := json.Unmarshal(rec.Body.Bytes(), &items); err != nil {
464+
t.Fatalf("Unmarshal() error = %v", err)
465+
}
466+
if len(items) != 2 {
467+
t.Fatalf("len(items) = %d, want 2", len(items))
468+
}
469+
// The session created last should sort first (most recent).
470+
if items[0].ID != "newer" {
471+
t.Fatalf("items[0].ID = %q, want %q (most recent first)", items[0].ID, "newer")
472+
}
473+
if items[1].ID != "older" {
474+
t.Fatalf("items[1].ID = %q, want %q", items[1].ID, "older")
475+
}
476+
}

0 commit comments

Comments
 (0)