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..cf7bdb97 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,23 @@ 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. +// 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) + } + 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 +1334,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..2d0cb760 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,29 @@ 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 gets 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 +172,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..e2331e2b 100644 --- a/internal/server/handlers_user.go +++ b/internal/server/handlers_user.go @@ -119,6 +119,11 @@ func (s *Server) handleDeleteUser(w http.ResponseWriter, r *http.Request) { // Clean up files for _, d := range targetUser.Devices { + if !isSinglePathComponent(d.ID) { + // 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 + } 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 +134,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) + // 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 { + 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..a0b0bbae 100644 --- a/internal/server/helpers.go +++ b/internal/server/helpers.go @@ -372,6 +372,24 @@ 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 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 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, `/\`) +} + // 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 +593,10 @@ 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) { + // Each device gets its own subdirectory of webp. + 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..ac013b01 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: "Invalid username."})}, + }) + 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..83ed28a4 --- /dev/null +++ b/internal/server/path_validation_test.go @@ -0,0 +1,192 @@ +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) + } +} + +// 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() + + // 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)) + // 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") + 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, "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") +} + +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) +} + +// 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() + + 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 is kept after 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" },