diff --git a/ingestion/internal/session/id_test.go b/ingestion/internal/session/id_test.go index d40c605..fe06f21 100644 --- a/ingestion/internal/session/id_test.go +++ b/ingestion/internal/session/id_test.go @@ -1,9 +1,12 @@ package session import ( + "errors" "os" "path/filepath" + "syscall" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -47,3 +50,43 @@ func TestReadRefusesSessionIDsThatAreNotPlainNames(t *testing.T) { _, err := Read(filepath.Join(root, "sessions"), "../x") assert.Error(t, err) } + +// A name that starts plainly and then climbs out with ".." is exactly what the +// ban on "/" in the id's tail stops: filepath.Join cleans "a/../../x" to a +// path beside the sessions dir without needing sessions/a to exist. The +// assertion is on the refusal itself (ErrInvalidID), not on any error. +func TestAppendRefusesAnIDThatClimbsOutAfterAPlainPrefix(t *testing.T) { + root := t.TempDir() + dir := filepath.Join(root, "sessions") + require.NoError(t, os.MkdirAll(dir, 0o755)) + for _, id := range []string{"a/../../escape", "a/../../../escape", "a/../b"} { + err := Append(dir, id, Entry{SessionID: id}) + assert.ErrorIs(t, err, ErrInvalidID, "%q", id) + } + _, err := os.Lstat(filepath.Join(root, "escape.jsonl")) + assert.True(t, errors.Is(err, os.ErrNotExist), "nothing created beside the sessions dir") + _, err = os.Lstat(filepath.Join(dir, "b.jsonl")) + assert.True(t, errors.Is(err, os.ErrNotExist), "no in-dir alias created either") +} + +// Append does no Lstat type check, so O_NONBLOCK is the only thing that keeps +// a FIFO named .jsonl with no reader from blocking session_log forever. +func TestAppendReturnsPromptlyOnAFIFOLogWithNoReader(t *testing.T) { + dir := filepath.Join(t.TempDir(), "sessions") + require.NoError(t, os.MkdirAll(dir, 0o755)) + fifo := filepath.Join(dir, "s.jsonl") + require.NoError(t, syscall.Mkfifo(fifo, 0o644)) + done := make(chan error, 1) + go func() { done <- Append(dir, "s", Entry{SessionID: "s"}) }() + select { + case err := <-done: + assert.Error(t, err, "a FIFO with no reader is refused, not written") + case <-time.After(3 * time.Second): + // Unblock the stuck open so the goroutine and TempDir cleanup finish. + if r, err := os.OpenFile(fifo, os.O_RDONLY|syscall.O_NONBLOCK, 0); err == nil { + defer r.Close() //nolint:errcheck + } + <-done + t.Fatal("Append blocked on a FIFO log with no reader") + } +} diff --git a/ingestion/internal/session/session.go b/ingestion/internal/session/session.go index 095441b..878d425 100644 --- a/ingestion/internal/session/session.go +++ b/ingestion/internal/session/session.go @@ -49,9 +49,12 @@ type Attempt struct { // write (hyperguild#169). var idPattern = regexp.MustCompile(`^[A-Za-z0-9_][A-Za-z0-9._-]{0,127}$`) +// ErrInvalidID is returned when a session id is not a plain file name. +var ErrInvalidID = errors.New("session id is not a plain name") + func checkID(sessionID string) error { if !idPattern.MatchString(sessionID) { - return fmt.Errorf("session id %q is not a plain name", sessionID) + return fmt.Errorf("%w: %q", ErrInvalidID, sessionID) } return nil }