From 293835618b38d3af4485482d590a1f7c5e2e8138 Mon Sep 17 00:00:00 2001 From: Jason Shelton Date: Mon, 5 Oct 2026 10:31:29 -0700 Subject: [PATCH 1/5] fix: validate names that are used as directory names Usernames, generated device IDs and uploaded app names are all joined onto paths under the data directory. Tighten how each one is accepted: - Validate usernames on registration and OIDC auto-create (letters, digits and . _ @ + -, starting with a letter or digit), and skip the per-user directory cleanup for stored names that are not a single path component. - When a device ID is generated from the device name, fall back to a random ID if the name has no letters or digits, and add a numeric suffix if the ID is already taken. - Reject empty, "." and ".." device IDs in ensureDeviceImageDir. - Reject uploaded app names that are empty or start with a dot. Co-Authored-By: Claude Opus 5.5 --- internal/server/auth.go | 6 + internal/server/handlers_app.go | 26 +++- internal/server/handlers_device.go | 34 ++++- internal/server/handlers_user.go | 17 ++- internal/server/helpers.go | 24 +++ internal/server/oidc.go | 8 + internal/server/path_validation_test.go | 193 ++++++++++++++++++++++++ web/i18n/de.json | 3 + web/i18n/en.json | 3 + 9 files changed, 306 insertions(+), 8 deletions(-) create mode 100644 internal/server/path_validation_test.go diff --git a/internal/server/auth.go b/internal/server/auth.go index afc8eeda..3ce161fa 100644 --- a/internal/server/auth.go +++ b/internal/server/auth.go @@ -274,6 +274,12 @@ func (s *Server) handleRegisterPost(w http.ResponseWriter, r *http.Request) { return } + if !isValidUsername(username) { + slog.Warn("Registration rejected: invalid username", "username", username) + s.renderTemplate(w, r, "register", TemplateData{Flashes: []string{localizer.MustLocalize(&i18n.LocalizeConfig{MessageID: "Invalid username."})}}) + return + } + if _, err := gorm.G[data.User](s.DB).Where("username = ?", username).First(r.Context()); err == nil { s.renderTemplate(w, r, "register", TemplateData{Flashes: []string{localizer.MustLocalize(&i18n.LocalizeConfig{MessageID: "Username already exists"})}}) return diff --git a/internal/server/handlers_app.go b/internal/server/handlers_app.go index 432d3178..9fde4293 100644 --- a/internal/server/handlers_app.go +++ b/internal/server/handlers_app.go @@ -1183,9 +1183,9 @@ func (s *Server) handleUploadAppPost(w http.ResponseWriter, r *http.Request) { appName := strings.TrimSuffix(filename, ext) userAppsDir := filepath.Join(s.DataDir, "users", user.Username, "apps") - appDir, err := securejoin.SecureJoin(userAppsDir, appName) + appDir, err := userAppDir(userAppsDir, appName) if err != nil { - slog.Warn("Path traversal attempt blocked", "error", err) + slog.Warn("Rejected upload with invalid app name", "filename", filename, "error", err) http.Error(w, "Invalid app name", http.StatusBadRequest) return } @@ -1243,6 +1243,24 @@ func (s *Server) handleUploadAppPost(w http.ResponseWriter, r *http.Request) { http.Redirect(w, r, fmt.Sprintf("/devices/%s/addapp", device.ID), http.StatusSeeOther) } +// userAppDir returns the directory for one uploaded app inside userAppsDir. +// A file named ".zip" or "..zip" gives an app name of "" or ".", which would +// resolve to userAppsDir itself; the zip handler deletes appDir before +// extracting, so that would wipe every custom app the user has. +func userAppDir(userAppsDir, appName string) (string, error) { + if strings.TrimSpace(appName) == "" || strings.HasPrefix(appName, ".") { + return "", fmt.Errorf("invalid app name %q", appName) + } + appDir, err := securejoin.SecureJoin(userAppsDir, appName) + if err != nil { + return "", err + } + if appDir == filepath.Clean(userAppsDir) { + return "", fmt.Errorf("app name %q resolves to the apps directory", appName) + } + return appDir, nil +} + func (s *Server) parseManifest(tempExtractDir string) (string, error) { manifestPath := filepath.Join(tempExtractDir, "manifest.yaml") data, err := os.ReadFile(manifestPath) @@ -1317,9 +1335,9 @@ func (s *Server) handleZipUpload(w http.ResponseWriter, r *http.Request, user *d } // Re-calculate appDir with potentially new appName - appDir, err := securejoin.SecureJoin(userAppsDir, appName) + appDir, err := userAppDir(userAppsDir, appName) if err != nil { - slog.Warn("Path traversal attempt blocked", "error", err) + slog.Warn("Rejected zip upload with invalid app name", "app_name", appName, "error", err) return err } diff --git a/internal/server/handlers_device.go b/internal/server/handlers_device.go index 7594ba8a..057cd142 100644 --- a/internal/server/handlers_device.go +++ b/internal/server/handlers_device.go @@ -1,6 +1,7 @@ package server import ( + "context" "encoding/json" "fmt" "log/slog" @@ -30,6 +31,31 @@ func slugifyDeviceName(name string) string { return s } +// uniqueDeviceIDFromName builds a device ID from the device name. Device IDs +// are global, so if the slug is taken (possibly by another user's device) a +// numeric suffix is added. A name with no letters or digits slugifies to "", +// which would point at the shared webp directory, so it falls back to a +// random ID. +func (s *Server) uniqueDeviceIDFromName(ctx context.Context, name string) (string, error) { + base := slugifyDeviceName(name) + if base == "" { + return generateSecureToken(8) + } + + candidate := base + for i := 2; i <= 100; i++ { + count, err := gorm.G[data.Device](s.DB).Where("id = ?", candidate).Count(ctx, "*") + if err != nil { + return "", err + } + if count == 0 { + return candidate, nil + } + candidate = fmt.Sprintf("%s-%d", base, i) + } + return generateSecureToken(8) +} + func (s *Server) handleCreateDeviceGet(w http.ResponseWriter, r *http.Request) { user := GetUser(r) @@ -148,7 +174,13 @@ func (s *Server) handleCreateDevicePost(w http.ResponseWriter, r *http.Request) } else { switch formData.DeviceIDMode { case "from_name": - deviceID = slugifyDeviceName(formData.Name) + var err error + deviceID, err = s.uniqueDeviceIDFromName(r.Context(), formData.Name) + if err != nil { + slog.Error("Failed to generate device ID", "error", err) + http.Error(w, "Internal Server Error", http.StatusInternalServerError) + return + } case "hex16": var err error deviceID, err = generateSecureToken(16) diff --git a/internal/server/handlers_user.go b/internal/server/handlers_user.go index 9c34169a..68dbe234 100644 --- a/internal/server/handlers_user.go +++ b/internal/server/handlers_user.go @@ -119,6 +119,12 @@ func (s *Server) handleDeleteUser(w http.ResponseWriter, r *http.Request) { // Clean up files for _, d := range targetUser.Devices { + if !isSinglePathComponent(d.ID) { + // Older versions could create a device with an empty ID; its + // image dir would be the shared webp root, so leave files alone. + slog.Warn("Skipping webp cleanup for device with unsafe ID", "device_id", d.ID) + continue + } deviceWebpDir, err := s.ensureDeviceImageDir(d.ID) if err != nil { slog.Error("Failed to get device webp directory for deletion", "device_id", d.ID, "error", err) @@ -129,9 +135,14 @@ func (s *Server) handleDeleteUser(w http.ResponseWriter, r *http.Request) { slog.Error("Failed to remove device webp directory", "device_id", d.ID, "error", err) } } - userAppsDir := filepath.Join(s.DataDir, "users", targetUsername) - if err := os.RemoveAll(userAppsDir); err != nil { - slog.Error("Failed to remove user apps directory", "username", targetUsername, "error", err) + // Never let a username like ".." turn this into a delete of the whole data dir. + if isSinglePathComponent(targetUsername) { + userAppsDir := filepath.Join(s.DataDir, "users", targetUsername) + if err := os.RemoveAll(userAppsDir); err != nil { + slog.Error("Failed to remove user apps directory", "username", targetUsername, "error", err) + } + } else { + slog.Warn("Skipping removal of user directory for unsafe username", "username", targetUsername) } err = s.DB.Transaction(func(tx *gorm.DB) error { diff --git a/internal/server/helpers.go b/internal/server/helpers.go index 84dad00f..fa9b0304 100644 --- a/internal/server/helpers.go +++ b/internal/server/helpers.go @@ -372,6 +372,25 @@ func generateSecureToken(length int) (string, error) { return hex.EncodeToString(b)[:length], nil // Take only requested length } +// validUsernameRe allows letters, digits and the punctuation found in email +// addresses (OIDC usernames are often emails). The first character must be a +// letter or digit, which rules out "." and "..". +var validUsernameRe = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._@+-]{0,127}$`) + +// isValidUsername reports whether a username is safe to create. Usernames are +// used as directory names under DataDir/users, so they must not be able to +// escape that directory. +func isValidUsername(username string) bool { + return validUsernameRe.MatchString(username) +} + +// isSinglePathComponent reports whether name can be joined onto a directory +// without leaving it. It is used as a last line of defense for usernames that +// were stored before isValidUsername existed. +func isSinglePathComponent(name string) bool { + return name != "" && name != "." && name != ".." && !strings.ContainsAny(name, `/\`) +} + // flashAndRedirect adds a flash message and redirects to the specified URL. func (s *Server) flashAndRedirect(w http.ResponseWriter, r *http.Request, messageID string, redirectURL string, status int) { localizer := s.getLocalizer(r) @@ -575,6 +594,11 @@ func (s *Server) saveSession(w http.ResponseWriter, r *http.Request, session *se // ensureDeviceImageDir is a helper to get and ensure the device webp directory exists. func (s *Server) ensureDeviceImageDir(deviceID string) (string, error) { + // An empty ID or ".." would resolve to the shared webp directory itself, + // and callers RemoveAll this path when a device is deleted. + if !isSinglePathComponent(deviceID) { + return "", fmt.Errorf("invalid device ID for webp directory: %q", deviceID) + } path, err := securejoin.SecureJoin(filepath.Join(s.DataDir, "webp"), deviceID) if err != nil { return "", fmt.Errorf("failed to securejoin path for device webp directory %s: %w", deviceID, err) diff --git a/internal/server/oidc.go b/internal/server/oidc.go index 1d5ca738..ad9c92cf 100644 --- a/internal/server/oidc.go +++ b/internal/server/oidc.go @@ -496,6 +496,14 @@ func (s *Server) handleOIDCNewIdentity(w http.ResponseWriter, r *http.Request, u // handleOIDCCreateUser creates a new user from OIDC and logs them in. func (s *Server) handleOIDCCreateUser(w http.ResponseWriter, r *http.Request, username, email string, claims map[string]any, prov *OIDCProvider, localizer *i18n.Localizer) { ctx := r.Context() + if !isValidUsername(username) { + slog.Warn("OIDC login: refusing to auto-create user with invalid username", "username", username, "subject", claims["sub"]) + s.renderTemplate(w, r, "login", TemplateData{ + Flashes: []string{localizer.MustLocalize(&i18n.LocalizeConfig{MessageID: "OIDCErrorNoAccount"})}, + }) + return + } + apiKey, err := generateSecureToken(32) if err != nil { slog.Error("Failed to generate API key for new user", "error", err) diff --git a/internal/server/path_validation_test.go b/internal/server/path_validation_test.go new file mode 100644 index 00000000..587b4e50 --- /dev/null +++ b/internal/server/path_validation_test.go @@ -0,0 +1,193 @@ +package server + +import ( + "bytes" + "context" + "mime/multipart" + "net/http" + "net/http/httptest" + "net/url" + "os" + "path/filepath" + "strings" + "testing" + + "tronbyt-server/internal/data" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/gorm" +) + +func TestIsValidUsername(t *testing.T) { + for _, name := range []string{"admin", "Alice", "user1", "jane.doe", "jane_doe", "jane-doe", "jane+tag@example.com", "1"} { + assert.True(t, isValidUsername(name), "expected %q to be valid", name) + } + for _, name := range []string{"", ".", "..", "../admin", "a/b", `a\b`, ".hidden", "-dash", "has space", "colon:name", strings.Repeat("a", 129)} { + assert.False(t, isValidUsername(name), "expected %q to be invalid", name) + } +} + +func TestHandleRegisterPost_RejectsInvalidUsername(t *testing.T) { + s := newTestServer(t) + ctx := context.Background() + + for _, username := range []string{"..", "../users/alice", "a/b"} { + form := url.Values{} + form.Add("username", username) + form.Add("password", "password123") + + req, _ := http.NewRequest(http.MethodPost, "/auth/register", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + rr := httptest.NewRecorder() + http.HandlerFunc(s.handleRegisterPost).ServeHTTP(rr, req) + + assert.Equal(t, http.StatusOK, rr.Code, "username %q", username) + count, err := gorm.G[data.User](s.DB).Where("username = ?", username).Count(ctx, "*") + require.NoError(t, err) + assert.Zero(t, count, "user %q should not have been created", username) + } +} + +// A user stored before username validation existed must not be able to turn +// "delete user" into a delete of the whole data directory. +func TestHandleDeleteUser_UnsafeUsernameKeepsDataDir(t *testing.T) { + s := newTestServer(t) + ctx := context.Background() + + // api_key is unique, so each user needs its own. + admin := data.User{Username: "admin", IsAdmin: true, APIKey: "admin-key"} + require.NoError(t, gorm.G[data.User](s.DB).Create(ctx, &admin)) + bad := data.User{Username: "..", APIKey: "bad-key"} + require.NoError(t, gorm.G[data.User](s.DB).Create(ctx, &bad)) + // An empty device ID maps to the shared webp directory. + require.NoError(t, gorm.G[data.Device](s.DB).Create(ctx, &data.Device{ID: "", Username: ".."})) + + sentinel := filepath.Join(s.DataDir, "keep-me") + require.NoError(t, os.WriteFile(sentinel, []byte("x"), 0644)) + otherImage := filepath.Join(s.DataDir, "webp", "otherdevice", "img.webp") + require.NoError(t, os.MkdirAll(filepath.Dir(otherImage), 0755)) + require.NoError(t, os.WriteFile(otherImage, []byte("x"), 0644)) + + req, _ := http.NewRequest(http.MethodPost, "/admin/users/../delete", nil) + req.SetPathValue("username", "..") + req = req.WithContext(context.WithValue(req.Context(), userContextKey, &admin)) + + rr := httptest.NewRecorder() + http.HandlerFunc(s.handleDeleteUser).ServeHTTP(rr, req) + + assert.FileExists(t, sentinel, "data directory must not be wiped") + assert.FileExists(t, otherImage, "other devices' images must not be wiped") + count, err := gorm.G[data.User](s.DB).Where("username = ?", "..").Count(ctx, "*") + require.NoError(t, err) + assert.Zero(t, count, "user should still be deleted from the database") +} + +func TestEnsureDeviceImageDir_RejectsUnsafeIDs(t *testing.T) { + s := newTestServer(t) + for _, id := range []string{"", ".", ".."} { + _, err := s.ensureDeviceImageDir(id) + assert.Error(t, err, "device ID %q", id) + } +} + +func TestUniqueDeviceIDFromName(t *testing.T) { + s := newTestServer(t) + ctx := context.Background() + + id, err := s.uniqueDeviceIDFromName(ctx, "Kitchen") + require.NoError(t, err) + assert.Equal(t, "kitchen", id) + + // Device IDs are global: another user's "Kitchen" must not collide. + require.NoError(t, gorm.G[data.Device](s.DB).Create(ctx, &data.Device{ID: "kitchen", Username: "someone-else"})) + id, err = s.uniqueDeviceIDFromName(ctx, "Kitchen") + require.NoError(t, err) + assert.Equal(t, "kitchen-2", id) + + // A name with no letters or digits must not produce an empty ID. + for _, name := range []string{"!!!", "🎉", "---"} { + id, err := s.uniqueDeviceIDFromName(ctx, name) + require.NoError(t, err) + assert.Len(t, id, 8, "name %q", name) + assert.Regexp(t, validDeviceIDRe, id) + } +} + +func TestHandleCreateDevicePost_FromNameWithoutAlphanumerics(t *testing.T) { + s := newTestServer(t) + ctx := context.Background() + + user := data.User{Username: "testuser"} + require.NoError(t, gorm.G[data.User](s.DB).Create(ctx, &user)) + + form := url.Values{} + form.Add("name", "🎉🎉") + form.Add("device_type", "tidbyt_gen1") + form.Add("brightness", "2") + form.Add("device_id_mode", "from_name") + + req, _ := http.NewRequest(http.MethodPost, "/devices/create", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req = req.WithContext(context.WithValue(req.Context(), userContextKey, &user)) + + rr := httptest.NewRecorder() + http.HandlerFunc(s.handleCreateDevicePost).ServeHTTP(rr, req) + require.Equal(t, http.StatusSeeOther, rr.Code, "body: %s", rr.Body.String()) + + device, err := gorm.G[data.Device](s.DB).Where("name = ?", "🎉🎉").First(ctx) + require.NoError(t, err) + assert.NotEmpty(t, device.ID) +} + +// Uploading a file called ".zip" or "..zip" used to resolve to the user's +// whole apps directory, which the zip handler deletes before extracting. +func TestHandleUploadAppPost_DotNamesDoNotWipeApps(t *testing.T) { + s := newTestServer(t) + ctx := context.Background() + + user := data.User{Username: "testuser"} + require.NoError(t, gorm.G[data.User](s.DB).Create(ctx, &user)) + device := data.Device{ID: "testdevice", Username: "testuser"} + require.NoError(t, gorm.G[data.Device](s.DB).Create(ctx, &device)) + + existing := filepath.Join(s.DataDir, "users", "testuser", "apps", "myapp", "myapp.star") + require.NoError(t, os.MkdirAll(filepath.Dir(existing), 0755)) + require.NoError(t, os.WriteFile(existing, []byte("x"), 0644)) + + for _, filename := range []string{".zip", "..zip", ".star"} { + body := &bytes.Buffer{} + writer := multipart.NewWriter(body) + part, err := writer.CreateFormFile("file", filename) + require.NoError(t, err) + _, err = part.Write([]byte("not really a zip")) + require.NoError(t, err) + require.NoError(t, writer.Close()) + + req, _ := http.NewRequest(http.MethodPost, "/devices/testdevice/uploadapp", body) + req.Header.Set("Content-Type", writer.FormDataContentType()) + reqCtx := context.WithValue(req.Context(), userContextKey, &user) + reqCtx = context.WithValue(reqCtx, deviceContextKey, &device) + req = req.WithContext(reqCtx) + + rr := httptest.NewRecorder() + http.HandlerFunc(s.handleUploadAppPost).ServeHTTP(rr, req) + + assert.Equal(t, http.StatusBadRequest, rr.Code, "filename %q", filename) + assert.FileExists(t, existing, "existing app must survive upload of %q", filename) + } +} + +func TestUserAppDir(t *testing.T) { + root := t.TempDir() + + dir, err := userAppDir(root, "myapp") + require.NoError(t, err) + assert.Equal(t, filepath.Join(root, "myapp"), dir) + + for _, name := range []string{"", " ", ".", "..", ".hidden", "../other"} { + _, err := userAppDir(root, name) + assert.Error(t, err, "app name %q", name) + } +} diff --git a/web/i18n/de.json b/web/i18n/de.json index c9c466d6..f9703105 100644 --- a/web/i18n/de.json +++ b/web/i18n/de.json @@ -1787,6 +1787,9 @@ "Username already exists": { "other": "Benutzername existiert bereits." }, + "Invalid username.": { + "other": "Ungültiger Benutzername. Nur Buchstaben, Ziffern und . _ @ + - sind erlaubt; das erste Zeichen muss ein Buchstabe oder eine Ziffer sein." + }, "Invalid username or password": { "other": "Ungültiger Benutzername oder Passwort." }, diff --git a/web/i18n/en.json b/web/i18n/en.json index 77fe32e4..5d99c9d9 100644 --- a/web/i18n/en.json +++ b/web/i18n/en.json @@ -1790,6 +1790,9 @@ "Username already exists": { "other": "Username already exists" }, + "Invalid username.": { + "other": "Invalid username. Use letters, numbers and . _ @ + - only, starting with a letter or number." + }, "Invalid username or password": { "other": "Invalid username or password" }, From 8e6230e9cc6faa9a5ab51cfe5d7c6f747e9700d1 Mon Sep 17 00:00:00 2001 From: Jason Shelton Date: Mon, 5 Oct 2026 10:35:53 -0700 Subject: [PATCH 2/5] chore: tidy comments and test names for name validation Co-Authored-By: Claude Opus 5.5 --- internal/server/handlers_app.go | 5 ++--- internal/server/handlers_device.go | 4 +--- internal/server/handlers_user.go | 5 ++--- internal/server/helpers.go | 14 ++++++-------- internal/server/path_validation_test.go | 19 +++++++++---------- 5 files changed, 20 insertions(+), 27 deletions(-) diff --git a/internal/server/handlers_app.go b/internal/server/handlers_app.go index 9fde4293..cf7bdb97 100644 --- a/internal/server/handlers_app.go +++ b/internal/server/handlers_app.go @@ -1244,9 +1244,8 @@ func (s *Server) handleUploadAppPost(w http.ResponseWriter, r *http.Request) { } // userAppDir returns the directory for one uploaded app inside userAppsDir. -// A file named ".zip" or "..zip" gives an app name of "" or ".", which would -// resolve to userAppsDir itself; the zip handler deletes appDir before -// extracting, so that would wipe every custom app the user has. +// The app name must be non-empty, must not start with a dot, and must +// resolve to a subdirectory of userAppsDir. func userAppDir(userAppsDir, appName string) (string, error) { if strings.TrimSpace(appName) == "" || strings.HasPrefix(appName, ".") { return "", fmt.Errorf("invalid app name %q", appName) diff --git a/internal/server/handlers_device.go b/internal/server/handlers_device.go index 057cd142..2d0cb760 100644 --- a/internal/server/handlers_device.go +++ b/internal/server/handlers_device.go @@ -33,9 +33,7 @@ func slugifyDeviceName(name string) string { // uniqueDeviceIDFromName builds a device ID from the device name. Device IDs // are global, so if the slug is taken (possibly by another user's device) a -// numeric suffix is added. A name with no letters or digits slugifies to "", -// which would point at the shared webp directory, so it falls back to a -// random ID. +// numeric suffix is added. A name with no letters or digits gets a random ID. func (s *Server) uniqueDeviceIDFromName(ctx context.Context, name string) (string, error) { base := slugifyDeviceName(name) if base == "" { diff --git a/internal/server/handlers_user.go b/internal/server/handlers_user.go index 68dbe234..e2331e2b 100644 --- a/internal/server/handlers_user.go +++ b/internal/server/handlers_user.go @@ -120,8 +120,7 @@ func (s *Server) handleDeleteUser(w http.ResponseWriter, r *http.Request) { // Clean up files for _, d := range targetUser.Devices { if !isSinglePathComponent(d.ID) { - // Older versions could create a device with an empty ID; its - // image dir would be the shared webp root, so leave files alone. + // Devices created by older versions may have an empty ID. slog.Warn("Skipping webp cleanup for device with unsafe ID", "device_id", d.ID) continue } @@ -135,7 +134,7 @@ func (s *Server) handleDeleteUser(w http.ResponseWriter, r *http.Request) { slog.Error("Failed to remove device webp directory", "device_id", d.ID, "error", err) } } - // Never let a username like ".." turn this into a delete of the whole data dir. + // Only remove the directory when the stored username is a single path component. if isSinglePathComponent(targetUsername) { userAppsDir := filepath.Join(s.DataDir, "users", targetUsername) if err := os.RemoveAll(userAppsDir); err != nil { diff --git a/internal/server/helpers.go b/internal/server/helpers.go index fa9b0304..a0b0bbae 100644 --- a/internal/server/helpers.go +++ b/internal/server/helpers.go @@ -377,16 +377,15 @@ func generateSecureToken(length int) (string, error) { // letter or digit, which rules out "." and "..". var validUsernameRe = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._@+-]{0,127}$`) -// isValidUsername reports whether a username is safe to create. Usernames are -// used as directory names under DataDir/users, so they must not be able to -// escape that directory. +// isValidUsername reports whether a username can be created. Usernames are +// used as directory names under DataDir/users. func isValidUsername(username string) bool { return validUsernameRe.MatchString(username) } -// isSinglePathComponent reports whether name can be joined onto a directory -// without leaving it. It is used as a last line of defense for usernames that -// were stored before isValidUsername existed. +// isSinglePathComponent reports whether name is a single, non-empty path +// component other than "." or "..". It covers usernames stored before +// isValidUsername existed. func isSinglePathComponent(name string) bool { return name != "" && name != "." && name != ".." && !strings.ContainsAny(name, `/\`) } @@ -594,8 +593,7 @@ func (s *Server) saveSession(w http.ResponseWriter, r *http.Request, session *se // ensureDeviceImageDir is a helper to get and ensure the device webp directory exists. func (s *Server) ensureDeviceImageDir(deviceID string) (string, error) { - // An empty ID or ".." would resolve to the shared webp directory itself, - // and callers RemoveAll this path when a device is deleted. + // Each device gets its own subdirectory of webp. if !isSinglePathComponent(deviceID) { return "", fmt.Errorf("invalid device ID for webp directory: %q", deviceID) } diff --git a/internal/server/path_validation_test.go b/internal/server/path_validation_test.go index 587b4e50..83ed28a4 100644 --- a/internal/server/path_validation_test.go +++ b/internal/server/path_validation_test.go @@ -50,9 +50,9 @@ func TestHandleRegisterPost_RejectsInvalidUsername(t *testing.T) { } } -// A user stored before username validation existed must not be able to turn -// "delete user" into a delete of the whole data directory. -func TestHandleDeleteUser_UnsafeUsernameKeepsDataDir(t *testing.T) { +// Deleting a user whose stored name predates validation only removes that +// user's own files. +func TestHandleDeleteUser_LegacyUsername(t *testing.T) { s := newTestServer(t) ctx := context.Background() @@ -61,7 +61,7 @@ func TestHandleDeleteUser_UnsafeUsernameKeepsDataDir(t *testing.T) { require.NoError(t, gorm.G[data.User](s.DB).Create(ctx, &admin)) bad := data.User{Username: "..", APIKey: "bad-key"} require.NoError(t, gorm.G[data.User](s.DB).Create(ctx, &bad)) - // An empty device ID maps to the shared webp directory. + // Older versions could create a device with an empty ID. require.NoError(t, gorm.G[data.Device](s.DB).Create(ctx, &data.Device{ID: "", Username: ".."})) sentinel := filepath.Join(s.DataDir, "keep-me") @@ -77,8 +77,8 @@ func TestHandleDeleteUser_UnsafeUsernameKeepsDataDir(t *testing.T) { rr := httptest.NewRecorder() http.HandlerFunc(s.handleDeleteUser).ServeHTTP(rr, req) - assert.FileExists(t, sentinel, "data directory must not be wiped") - assert.FileExists(t, otherImage, "other devices' images must not be wiped") + assert.FileExists(t, sentinel, "files outside the user's directory are kept") + assert.FileExists(t, otherImage, "other devices' images are kept") count, err := gorm.G[data.User](s.DB).Where("username = ?", "..").Count(ctx, "*") require.NoError(t, err) assert.Zero(t, count, "user should still be deleted from the database") @@ -141,9 +141,8 @@ func TestHandleCreateDevicePost_FromNameWithoutAlphanumerics(t *testing.T) { assert.NotEmpty(t, device.ID) } -// Uploading a file called ".zip" or "..zip" used to resolve to the user's -// whole apps directory, which the zip handler deletes before extracting. -func TestHandleUploadAppPost_DotNamesDoNotWipeApps(t *testing.T) { +// Uploads whose app name is empty or starts with a dot are rejected. +func TestHandleUploadAppPost_RejectsDotNames(t *testing.T) { s := newTestServer(t) ctx := context.Background() @@ -175,7 +174,7 @@ func TestHandleUploadAppPost_DotNamesDoNotWipeApps(t *testing.T) { http.HandlerFunc(s.handleUploadAppPost).ServeHTTP(rr, req) assert.Equal(t, http.StatusBadRequest, rr.Code, "filename %q", filename) - assert.FileExists(t, existing, "existing app must survive upload of %q", filename) + assert.FileExists(t, existing, "existing app is kept after upload of %q", filename) } } From a3c8beb092e6d8e819477564fc74bc790cf33fb6 Mon Sep 17 00:00:00 2001 From: Jason Shelton Date: Mon, 5 Oct 2026 11:09:44 -0700 Subject: [PATCH 3/5] claude --- .github/workflows/claude.yml | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 .github/workflows/claude.yml diff --git a/.github/workflows/claude.yml b/.github/workflows/claude.yml new file mode 100644 index 00000000..9007b884 --- /dev/null +++ b/.github/workflows/claude.yml @@ -0,0 +1,25 @@ +name: Claude +on: + issue_comment: + types: [created] + pull_request_review_comment: + types: [created] + issues: + types: [opened, assigned] + pull_request_review: + types: [submitted] + +jobs: + claude: + if: contains(github.event.comment.body, '@claude') || contains(github.event.review.body, '@claude') || contains(github.event.issue.body, '@claude') + runs-on: ubuntu-latest + permissions: + contents: write + pull-requests: write + issues: write + id-token: write + steps: + - uses: actions/checkout@v4 + - uses: anthropics/claude-code-action@v1 + with: + anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }} \ No newline at end of file From da820148a480de9646d8fb521345e00fe02fb84c Mon Sep 17 00:00:00 2001 From: Jason Shelton Date: Mon, 5 Oct 2026 11:38:16 -0700 Subject: [PATCH 4/5] fix: show the invalid-username message when OIDC auto-create rejects a name Co-Authored-By: Claude Opus 5.5 --- internal/server/oidc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/server/oidc.go b/internal/server/oidc.go index ad9c92cf..ac013b01 100644 --- a/internal/server/oidc.go +++ b/internal/server/oidc.go @@ -499,7 +499,7 @@ func (s *Server) handleOIDCCreateUser(w http.ResponseWriter, r *http.Request, us if !isValidUsername(username) { slog.Warn("OIDC login: refusing to auto-create user with invalid username", "username", username, "subject", claims["sub"]) s.renderTemplate(w, r, "login", TemplateData{ - Flashes: []string{localizer.MustLocalize(&i18n.LocalizeConfig{MessageID: "OIDCErrorNoAccount"})}, + Flashes: []string{localizer.MustLocalize(&i18n.LocalizeConfig{MessageID: "Invalid username."})}, }) return } From 55e2468c4fadde717c58b3235d6a44b04803f8b3 Mon Sep 17 00:00:00 2001 From: Jason Shelton Date: Mon, 5 Oct 2026 11:43:17 -0700 Subject: [PATCH 5/5] chore: drop Claude workflow from this PR It was added to this branch by mistake and is unrelated to the fix. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/claude.yml | 25 ------------------------- 1 file changed, 25 deletions(-) delete mode 100644 .github/workflows/claude.yml diff --git a/.github/workflows/claude.yml b/.github/workflows/claude.yml deleted file mode 100644 index 9007b884..00000000 --- a/.github/workflows/claude.yml +++ /dev/null @@ -1,25 +0,0 @@ -name: Claude -on: - issue_comment: - types: [created] - pull_request_review_comment: - types: [created] - issues: - types: [opened, assigned] - pull_request_review: - types: [submitted] - -jobs: - claude: - if: contains(github.event.comment.body, '@claude') || contains(github.event.review.body, '@claude') || contains(github.event.issue.body, '@claude') - runs-on: ubuntu-latest - permissions: - contents: write - pull-requests: write - issues: write - id-token: write - steps: - - uses: actions/checkout@v4 - - uses: anthropics/claude-code-action@v1 - with: - anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }} \ No newline at end of file