diff --git a/TODO.md b/TODO.md index 3570099..3c4703d 100644 --- a/TODO.md +++ b/TODO.md @@ -20,6 +20,13 @@ regress. # Completed Steps +- 2026-10-02: App names may contain dots, such as `sneak.berlin`: lowercase + letters and numbers joined by single dots or by hyphens, 2 to 63 characters. + Docker accepts every such name in the image name `upaas-`; a dot needs a + letter or number on both sides because Docker requires it. The new and edit + app forms check the same rule; browsers ignored their old pattern, which was + not a valid regular expression there (#260). + - 2026-10-02: `.dockerignore` leaves out `.git/config`, so a remote URL there that carries a credential no longer goes into the Docker build; the image still shows the commit it was built from (#269). diff --git a/go.mod b/go.mod index c85cc4d..b2de996 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.25 require ( github.com/99designs/basicauth-go v0.0.0-20230316000542-bf6f9cbbf0f8 + github.com/distribution/reference v0.6.0 github.com/docker/docker v27.3.1+incompatible github.com/docker/go-connections v0.6.0 github.com/go-chi/chi/v5 v5.2.3 @@ -39,7 +40,6 @@ require ( github.com/containerd/ttrpc v1.2.5 // indirect github.com/containerd/typeurl/v2 v2.2.0 // indirect github.com/davecgh/go-spew v1.1.1 // indirect - github.com/distribution/reference v0.6.0 // indirect github.com/docker/go-units v0.5.0 // indirect github.com/felixge/httpsnoop v1.0.4 // indirect github.com/fsnotify/fsnotify v1.9.0 // indirect diff --git a/internal/handlers/app_name_validation.go b/internal/handlers/app_name_validation.go index d1d1abb..f6bf1e9 100644 --- a/internal/handlers/app_name_validation.go +++ b/internal/handlers/app_name_validation.go @@ -13,12 +13,15 @@ const ( appNameMaxLength = 63 ) -// validAppNameRe matches names containing only lowercase alphanumeric characters and -// hyphens, starting and ending with an alphanumeric character. -var validAppNameRe = regexp.MustCompile(`^[a-z0-9][a-z0-9-]*[a-z0-9]$`) +// validAppNameRe matches runs of lowercase letters and digits joined by +// single dots or by hyphens, such as "my-app" or "sneak.berlin". Docker +// accepts every name it allows as the app's image name, upaas-; a +// dot needs a letter or digit on both sides because Docker requires it. +// It also keeps the name from being "." or ".." or starting or ending +// with a dot, so it is safe as a directory and file name. The pattern +// attribute of the name field on the new and edit app forms is the same. +var validAppNameRe = regexp.MustCompile(`^[a-z0-9]+((\.|-+)[a-z0-9]+)*$`) -// validateAppName checks that the given app name is safe for use in Docker -// container names, image tags, and file system paths. var ( errAppNameLength = errors.New( "app name must be between " + @@ -26,11 +29,14 @@ var ( strconv.Itoa(appNameMaxLength) + " characters", ) errAppNamePattern = errors.New( - "app name must contain only lowercase letters, numbers, " + - "and hyphens, and must start and end with a letter or number", + "app name must contain only lowercase letters, numbers, hyphens, " + + "and dots, must start and end with a letter or number, " + + "and must have a letter or number on both sides of each dot", ) ) +// validateAppName checks that the given app name is safe for use in Docker +// container names, image tags, and file system paths. func validateAppName(name string) error { if len(name) < appNameMinLength || len(name) > appNameMaxLength { return errAppNameLength diff --git a/internal/handlers/app_name_validation_test.go b/internal/handlers/app_name_validation_test.go index b4120fb..f5e13a3 100644 --- a/internal/handlers/app_name_validation_test.go +++ b/internal/handlers/app_name_validation_test.go @@ -1,7 +1,16 @@ package handlers //nolint:testpackage // testing unexported validateAppName import ( + "bytes" + "strconv" + "strings" "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "sneak.berlin/go/upaas/internal/models" + "sneak.berlin/go/upaas/templates" ) func TestValidateAppName(t *testing.T) { @@ -18,6 +27,10 @@ func TestValidateAppName(t *testing.T) { {"valid two chars", "ab", false}, {"valid complex", "my-cool-app-v2", false}, {"valid all numbers", "123", false}, + {"valid double hyphen", "my--app", false}, + {"valid domain", "sneak.berlin", false}, + {"valid two dots", "www.sneak.berlin", false}, + {"valid dot and hyphen", "my-app.example.com", false}, {"empty", "", true}, {"single char", "a", true}, {"too long", "a" + string(make([]byte, 63)), true}, @@ -36,7 +49,12 @@ func TestValidateAppName(t *testing.T) { {"starts with hyphen", "-myapp", true}, {"ends with hyphen", "myapp-", true}, {"underscore", "my_app", true}, - {"dot", "my.app", true}, + {"two dots in a row", "a..b", true}, + {"starts with dot", ".a", true}, + {"ends with dot", "a.", true}, + {"only dots", "..", true}, + {"hyphen before dot", "a-.b", true}, + {"hyphen after dot", "a.-b", true}, {"slash", "my/app", true}, {"path traversal", "../etc/passwd", true}, {"special chars", "app@name!", true}, @@ -54,3 +72,42 @@ func TestValidateAppName(t *testing.T) { }) } } + +func TestValidateAppNameErrorMentionsDots(t *testing.T) { + t.Parallel() + + err := validateAppName("a..b") + + require.ErrorIs(t, err, errAppNamePattern) + assert.Contains(t, err.Error(), "dots") +} + +// TestAppFormsCheckAppNameLikeServer checks that the name field on the new +// and edit app forms has the server's pattern and length limits, and that +// its hint mentions dots. +func TestAppFormsCheckAppNameLikeServer(t *testing.T) { + t.Parallel() + + pattern := strings.TrimSuffix(strings.TrimPrefix(validAppNameRe.String(), "^"), "$") + + pages := map[string]map[string]any{ + "app_new.html": {}, + "app_edit.html": {dataKeyApp: &models.App{}}, + } + + for page, data := range pages { + var out bytes.Buffer + + require.NoError(t, templates.GetParsed().ExecuteTemplate(&out, page, data)) + + // The name field and its hint, up to the end of their div. + _, field, found := strings.Cut(out.String(), `id="name"`) + require.True(t, found, page) + + field, _, _ = strings.Cut(field, "") + assert.Contains(t, field, `pattern="`+pattern+`"`, page) + assert.Contains(t, field, `minlength="`+strconv.Itoa(appNameMinLength)+`"`, page) + assert.Contains(t, field, `maxlength="`+strconv.Itoa(appNameMaxLength)+`"`, page) + assert.Contains(t, field, "hyphens, and dots", page) + } +} diff --git a/internal/service/deploy/deploy_app_name_test.go b/internal/service/deploy/deploy_app_name_test.go new file mode 100644 index 0000000..3890c95 --- /dev/null +++ b/internal/service/deploy/deploy_app_name_test.go @@ -0,0 +1,121 @@ +package deploy_test + +import ( + "context" + "log/slog" + "net/http" + "net/http/httptest" + "net/url" + "os" + "slices" + "strings" + "testing" + + "github.com/distribution/reference" + "github.com/docker/docker/daemon/names" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "sneak.berlin/go/upaas/internal/database" + "sneak.berlin/go/upaas/internal/globals" + "sneak.berlin/go/upaas/internal/handlers" + "sneak.berlin/go/upaas/internal/logger" + "sneak.berlin/go/upaas/internal/models" + "sneak.berlin/go/upaas/internal/service/app" +) + +// TestDeployAppWithDotInName creates an app named sneak.berlin through the +// new app form, as a user does, then builds and deploys it against a fake +// Docker API and checks that Docker accepts the names of the image it +// builds and of the container it runs, using Docker's own rules for each. +func TestDeployAppWithDotInName(t *testing.T) { + t.Parallel() + + api := &fakeImageAPI{ + images: map[string][]string{}, + shortSHA: "abc1234", + nextID: "sha256:built", + } + svc, db := newImageTestService(t, api) + ctx := context.Background() + + createdApp := createAppWithForm(t, db, "sneak.berlin") + + deployment := models.NewDeployment(db) + deployment.AppID = createdApp.ID + require.NoError(t, deployment.Save(ctx)) + + imageID, err := svc.BuildImage(ctx, createdApp, deployment) + require.NoError(t, err) + require.NoError(t, svc.DeployContainer(ctx, createdApp, deployment, imageID)) + + images, _ := api.state() + + api.mu.Lock() + containers := slices.Clone(api.containers) + api.mu.Unlock() + + require.Equal(t, map[string][]string{ + "sha256:built": {"upaas-sneak.berlin:abc1234"}, + }, images) + + _, err = reference.ParseNormalizedNamed("upaas-sneak.berlin:abc1234") + require.NoError(t, err) + + require.Equal(t, []string{"upaas-sneak.berlin"}, containers) + assert.Regexp(t, names.RestrictedNamePattern, containers[0]) + + assert.DirExists(t, svc.GetBuildDirExported(createdApp.Name)) +} + +// createAppWithForm posts the new app form with the given name, which +// checks the name as it does for a user, and returns the app it created. +func createAppWithForm( + t *testing.T, + db *database.Database, + name string, +) *models.App { + t.Helper() + + log := logger.NewForTest(slog.New(slog.NewTextHandler(os.Stderr, nil))) + + appSvc, err := app.New(nil, app.ServiceParams{Logger: log, Database: db}) + require.NoError(t, err) + + globalInstance, err := globals.New(nil) + require.NoError(t, err) + + handlersInstance, err := handlers.New(nil, handlers.Params{ + Logger: log, + Globals: globalInstance, + Database: db, + App: appSvc, + }) + require.NoError(t, err) + + form := url.Values{ + "name": {name}, + "repo_url": {"git@example.com:sneak/" + name + ".git"}, + } + request := httptest.NewRequestWithContext( + t.Context(), http.MethodPost, "/apps", strings.NewReader(form.Encode()), + ) + request.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + recorder := httptest.NewRecorder() + + handlersInstance.HandleAppCreate().ServeHTTP(recorder, request) + + // The form redirects to the new app's page; it shows the form again, + // with the error, when it refuses the name. + require.Equal(t, http.StatusSeeOther, recorder.Code, recorder.Body.String()) + + appID, found := strings.CutPrefix(recorder.Header().Get("Location"), "/apps/") + require.True(t, found) + + createdApp, err := models.FindApp(t.Context(), db, appID) + require.NoError(t, err) + require.NotNil(t, createdApp) + + return createdApp +} diff --git a/internal/service/deploy/deploy_images_test.go b/internal/service/deploy/deploy_images_test.go index 7e93849..7141c25 100644 --- a/internal/service/deploy/deploy_images_test.go +++ b/internal/service/deploy/deploy_images_test.go @@ -35,12 +35,13 @@ import ( // an image's last tag, or an untagged image by its ID, deletes the image. // It also answers the steps of a git clone that reports shortSHA. type fakeImageAPI struct { - mu sync.Mutex - images map[string][]string // image ID -> tags - shortSHA string // the commit's short hash the clone reports - nextID string // ID of the image the next build creates - removed []string // each tag or ID removed - forced bool // whether a removal was forced + mu sync.Mutex + images map[string][]string // image ID -> tags + shortSHA string // the commit's short hash the clone reports + nextID string // ID of the image the next build creates + removed []string // each tag or ID removed + forced bool // whether a removal was forced + containers []string // name of each named container created } func (api *fakeImageAPI) ServeHTTP(w http.ResponseWriter, r *http.Request) { @@ -67,6 +68,11 @@ func (api *fakeImageAPI) ServeHTTP(w http.ResponseWriter, r *http.Request) { case strings.HasSuffix(r.URL.Path, "/version"): _, _ = w.Write([]byte(`{"Version":"27.3.1","ApiVersion":"1.47"}`)) case strings.HasSuffix(r.URL.Path, "/containers/create"): + // The git clone's container has no name; the app's has. + if containerName := r.URL.Query().Get("name"); containerName != "" { + api.containers = append(api.containers, containerName) + } + _, _ = w.Write([]byte(`{"Id":"gitcontainer"}`)) case strings.HasSuffix(r.URL.Path, "/logs"): writeCloneOutput(w, api.shortSHA) diff --git a/internal/service/deploy/export_test.go b/internal/service/deploy/export_test.go index 8abfc34..0ea6119 100644 --- a/internal/service/deploy/export_test.go +++ b/internal/service/deploy/export_test.go @@ -113,6 +113,16 @@ func (svc *Service) BuildImage( return svc.buildImage(ctx, app, deployment) } +// DeployContainer exposes deployContainerWithTimeout for testing. +func (svc *Service) DeployContainer( + ctx context.Context, + app *models.App, + deployment *models.Deployment, + imageID docker.ImageID, +) error { + return svc.deployContainerWithTimeout(ctx, app, deployment, imageID) +} + // BuildContainerOptionsExported exposes buildContainerOptions for testing. func (svc *Service) BuildContainerOptionsExported( ctx context.Context, diff --git a/templates/app_edit.html b/templates/app_edit.html index 3bb3852..eece07f 100644 --- a/templates/app_edit.html +++ b/templates/app_edit.html @@ -30,10 +30,12 @@ name="name" value="{{.App.Name}}" required - pattern="[a-z0-9-]+" + minlength="2" + maxlength="63" + pattern="[a-z0-9]+((\.|-+)[a-z0-9]+)*" class="input" > -

Lowercase letters, numbers, and hyphens only

+

Lowercase letters, numbers, hyphens, and dots, such as my-app or example.com

diff --git a/templates/app_new.html b/templates/app_new.html index 101fc6a..8a32a92 100644 --- a/templates/app_new.html +++ b/templates/app_new.html @@ -30,11 +30,13 @@ name="name" value="{{.Name}}" required - pattern="[a-z0-9-]+" + minlength="2" + maxlength="63" + pattern="[a-z0-9]+((\.|-+)[a-z0-9]+)*" class="input" placeholder="my-app" > -

Lowercase letters, numbers, and hyphens only

+

Lowercase letters, numbers, hyphens, and dots, such as my-app or example.com