From e4e3a438255a62bf85f1c8c370e5b63ffa65536b Mon Sep 17 00:00:00 2001 From: Catherine Date: Wed, 21 Oct 2026 15:24:47 +0000 Subject: [PATCH] [breaking-change] Refactor handling of project name. Define a proper abstraction for project names and use it elsewhere in the codebase. The character `@` is formally reserved for PR previews from this point, and any project names using it must be well-formed: contain no more than a single `@` followed by a positive number. --- src/auth.go | 53 ++++------------- src/main.go | 5 +- src/names/project.go | 116 ++++++++++++++++++++++++++++++++++++++ src/names/project_test.go | 104 ++++++++++++++++++++++++++++++++++ src/pages.go | 10 +++- 5 files changed, 241 insertions(+), 47 deletions(-) create mode 100644 src/names/project.go create mode 100644 src/names/project_test.go diff --git a/src/auth.go b/src/auth.go index 264a33e..a906cf2 100644 --- a/src/auth.go +++ b/src/auth.go @@ -13,6 +13,7 @@ import ( "strings" "time" + "codeberg.org/git-pages/git-pages/src/names" "golang.org/x/net/idna" ) @@ -78,38 +79,6 @@ func GetHost(r *http.Request) (string, error) { return host, nil } -func ValidateProjectName(name string) error { - if strings.HasPrefix(name, ".") { - return fmt.Errorf("must not start with %q", ".") - } - - forbiddenChars := "%*\\" - if strings.ContainsAny(name, forbiddenChars) { - return fmt.Errorf("must not contain any of %q", forbiddenChars) - } - - return nil -} - -func GetProjectName(r *http.Request) (string, error) { - // path must be either `/` or `/foo/` (`/foo` is accepted as an alias) - path := strings.TrimPrefix(strings.TrimSuffix(r.URL.Path, "/"), "/") - if err := ValidateProjectName(path); err != nil { - return "", AuthError{http.StatusBadRequest, - fmt.Sprintf("directory name: %v", err)} - } else if strings.Contains(path, "/") { - return "", AuthError{http.StatusBadRequest, - "directories nested too deep"} - } - - if path == "" { - // path `/` corresponds to pseudo-project `.index` - return ".index", nil - } else { - return path, nil - } -} - type Authorization struct { // If `nil`, any URL is allowed. If not, only those in the set are allowed. repoURLs []string @@ -203,7 +172,7 @@ func authorizeDNSAllowlist(r *http.Request, scope string) (*Authorization, error return nil, err } - projectName, err := GetProjectName(r) + projectName, err := names.ParseProjectURL(r.URL) if err != nil { return nil, err } @@ -215,7 +184,7 @@ func authorizeDNSAllowlist(r *http.Request, scope string) (*Authorization, error fmt.Sprintf("failed to look up DNS repository allowlist: %s TXT", allowlistHostname)} } - if projectName != ".index" { + if !projectName.IsIndex() { return nil, AuthError{http.StatusUnauthorized, "DNS repository allowlist only authorizes index site"} } @@ -279,13 +248,13 @@ func authorizeWildcardMatchSite(r *http.Request, pattern *WildcardPattern) (*Aut return nil, err } - projectName, err := GetProjectName(r) + projectName, err := names.ParseProjectURL(r.URL) if err != nil { return nil, err } if userName, found := pattern.Matches(host, WildcardDomainPrimary); found { - repoURL, branch := pattern.ApplyTemplate(userName, projectName) + repoURL, branch := pattern.ApplyTemplate(userName, projectName.String()) return &Authorization{repoURLs: []string{repoURL}, branch: branch}, nil } else { return nil, AuthError{ @@ -604,7 +573,7 @@ func authorizeGogsUser(repoURL string, forgeToken string) (*Authorization, error }, nil } -func authorizeForgejoActionRun(repoURL string, forgeToken string, prNumber string) ( +func authorizeForgejoActionRun(repoURL string, forgeToken string, prNumber int64) ( *Authorization, error, ) { parsedRepoURL, err := url.Parse(repoURL) @@ -623,7 +592,7 @@ func authorizeForgejoActionRun(repoURL string, forgeToken string, prNumber strin if fmt.Sprintf("/%s/%s.git", strings.ToLower(prOwner), prRepository) != parsedRepoURL.Path { return nil, AuthError{http.StatusUnauthorized, "pull request repository does not match"} } - if fmt.Sprintf("%d", actionRun.PullRequest.Number) != prNumber { + if actionRun.PullRequest.Number != prNumber { return nil, AuthError{http.StatusUnauthorized, "pull request number does not match"} } @@ -646,7 +615,7 @@ func authorizeForgeWildcard(r *http.Request) (*Authorization, error) { return nil, err } - projectName, err := GetProjectName(r) + projectName, err := names.ParseProjectURL(r.URL) if err != nil { return nil, err } @@ -655,7 +624,7 @@ func authorizeForgeWildcard(r *http.Request) (*Authorization, error) { for _, pattern := range wildcards { if pattern.Authorization { if userName, found := pattern.Matches(host, WildcardDomainPrimary); found { - repoName := projectName + repoName := projectName.String() repoURL, branch := pattern.ApplyTemplate(userName, repoName) auth, err := authorizeGogsUser(repoURL, forgeToken) if err != nil { @@ -667,13 +636,13 @@ func authorizeForgeWildcard(r *http.Request) (*Authorization, error) { } if userName, found := pattern.Matches(host, WildcardDomainPreview); found { - if repoName, atSuffix, foundAt := strings.Cut(projectName, "@"); !foundAt { + if repoName, prNumber, isPreview := projectName.GetPullRequest(); !isPreview { errs = append([]error{ AuthError{http.StatusUnauthorized, "not a preview project name (no `@`)"}, }, errs...) } else { repoURL, branch := pattern.ApplyTemplate(userName, repoName) - auth, err := authorizeForgejoActionRun(repoURL, forgeToken, atSuffix) + auth, err := authorizeForgejoActionRun(repoURL, forgeToken, prNumber) if err != nil { errs = append(errs, err) } else { diff --git a/src/main.go b/src/main.go index 2a52ce8..3dd3b66 100644 --- a/src/main.go +++ b/src/main.go @@ -23,6 +23,7 @@ import ( "strings" "time" + "codeberg.org/git-pages/git-pages/src/names" sys "codeberg.org/git-pages/git-pages/src/sys" automemlimit "github.com/KimMachineGun/automemlimit/memlimit" "github.com/c2h5oh/datasize" @@ -679,8 +680,8 @@ func Main(versionInfo string) { "'domain.tld/project' or 'domain.tld/*'") } - if project != "*" && project != ".index" { - if err := ValidateProjectName(project); err != nil { + if project != "*" { + if _, err := names.ParseProjectName(project); err != nil { logc.Fatalf(ctx, "audit detach: project name: %v\n", err) } } diff --git a/src/names/project.go b/src/names/project.go new file mode 100644 index 0000000..39ab9b5 --- /dev/null +++ b/src/names/project.go @@ -0,0 +1,116 @@ +package names + +import ( + "errors" + "fmt" + "net/url" + "strconv" + "strings" +) + +var ErrInvalidProjectName = errors.New("invalid project name") +var ErrInvalidProjectURL = errors.New("invalid project URL") + +const indexProject = ".index" +const previewSeparator = "@" + +// Name of a git-pages project. The index project corresponds to the root URL of a domain, and +// non-index projects are distinguished by the first path segment of the URL. +// +// Most operations (other than IsValid) on an invalid project name will panic. +type Project struct { + name string +} + +// IsValid checks whether the project name is valid. The zero value is the only invalid value. +func (p Project) IsValid() bool { + return p.name != "" +} + +// MustValid ensures the project name is valid by panicking if it is not. +func (p Project) MustValid() Project { + if !p.IsValid() { + panic("nil project name") + } + return p +} + +// String returns the project name (".index" for the index project). +func (p Project) String() string { + return p.MustValid().name +} + +// IsIndex checks whether this name corresponds to the index project. +func (p Project) IsIndex() bool { + return p.String() == indexProject +} + +// GetPullRequest returns (baseName, prNumber, isPreview), where isPreview is true if this is +// a preview project name for a pull request. In the latter case, prNumber is set correspondingly, +// and baseName contains the repository name. Otherwise, baseName equals p.String(). +func (p Project) GetPullRequest() (baseName string, prNumber int64, isPreview bool) { + var atSuffix string + if baseName, atSuffix, isPreview = strings.Cut(p.String(), "@"); isPreview { + prNumber, _ = strconv.ParseInt(atSuffix, 10, 64) + } else { + baseName = p.String() + } + return +} + +func validateProjectName(name string) (proj Project, err error) { + if strings.HasPrefix(name, ".") { + err = fmt.Errorf("%w: must not start with %q", ErrInvalidProjectName, ".") + return + } + + forbiddenChars := "%*\\/" + if strings.ContainsAny(name, forbiddenChars) { + err = fmt.Errorf("%w: must not contain any of %q", ErrInvalidProjectName, forbiddenChars) + return + } + + if strings.Count(name, previewSeparator) > 1 { + err = fmt.Errorf("%w: must contain at most one %q", ErrInvalidProjectName, previewSeparator) + return + } + + if _, atSuffix, isPreview := strings.Cut(name, "@"); isPreview { + if prNumber, parseErr := strconv.ParseInt(atSuffix, 10, 64); parseErr != nil { + err = fmt.Errorf("%w: malformed pull request number", ErrInvalidProjectName) + return + } else if prNumber <= 0 { + err = fmt.Errorf("%w: malformed pull request number", ErrInvalidProjectName) + return + } + } + + proj.name = name + return +} + +// ParseProjectName parses the given string as a project name. The string must not be empty, and +// ".index" represents the index project. +func ParseProjectName(name string) (Project, error) { + if name == indexProject { + return Project{indexProject}, nil + } + + return validateProjectName(name) +} + +// ParseProjectURL parses the given project URL and extracts the project name. The URL must have +// zero or one path segments, +func ParseProjectURL(url *url.URL) (Project, error) { + // path must be either `/`, `/foo`, or `/foo/` + path := strings.TrimPrefix(strings.TrimSuffix(url.Path, "/"), "/") + if strings.Contains(path, "/") { + return Project{}, fmt.Errorf("%w: must contain zero or one path segments", ErrInvalidProjectURL) + } else if path == "" { + return Project{indexProject}, nil + } else if name, err := validateProjectName(path); err != nil { + return Project{}, fmt.Errorf("%w: %w", ErrInvalidProjectURL, err) + } else { + return name, nil + } +} diff --git a/src/names/project_test.go b/src/names/project_test.go new file mode 100644 index 0000000..b318dba --- /dev/null +++ b/src/names/project_test.go @@ -0,0 +1,104 @@ +package names + +import ( + "errors" + "net/url" + "testing" +) + +func must[T any](value T, err error) T { + if err != nil { + panic(err) + } + return value +} + +func checkEq[T comparable](t *testing.T, value T, expect T) { + t.Helper() + if value != expect { + t.Errorf("unexpected: %v != %v", value, expect) + } +} + +func checkPanic[T any](t *testing.T, f func() T) { + defer func() { + if r := recover(); r == nil { + t.Errorf("function did not panic") + } + }() + + f() +} + +func checkOk[T any](t *testing.T, parse func(T) (Project, error), value T, expect Project) { + t.Helper() + actual, err := parse(value) + if err != nil { + t.Errorf("unexpected error: %v", err) + } else if actual != expect { + t.Errorf("unexpected result: %v, expected %v", actual, expect) + } +} + +func checkErr[T any](t *testing.T, parse func(T) (Project, error), value T, expect error) { + t.Helper() + _, err := parse(value) + if !errors.Is(err, expect) { + t.Errorf("unexpected error: %v, expected %v", err, expect) + } +} + +func TestParseProjectName(t *testing.T) { + checkOk(t, ParseProjectName, ".index", Project{".index"}) + checkOk(t, ParseProjectName, "foo", Project{"foo"}) + checkOk(t, ParseProjectName, "foo@10", Project{"foo@10"}) + + checkErr(t, ParseProjectName, ".foo", ErrInvalidProjectName) + checkErr(t, ParseProjectName, "meow/miau", ErrInvalidProjectName) + checkErr(t, ParseProjectName, "meow\\miau", ErrInvalidProjectName) + checkErr(t, ParseProjectName, "meow%miau", ErrInvalidProjectName) + checkErr(t, ParseProjectName, "meow*", ErrInvalidProjectName) + checkErr(t, ParseProjectName, "foo@10@11", ErrInvalidProjectName) + checkErr(t, ParseProjectName, "foo@awa", ErrInvalidProjectName) + checkErr(t, ParseProjectName, "foo@0", ErrInvalidProjectName) + checkErr(t, ParseProjectName, "foo@-10", ErrInvalidProjectName) +} + +func TestParseProjectURL(t *testing.T) { + checkOk(t, ParseProjectURL, must(url.Parse("")), Project{".index"}) + checkOk(t, ParseProjectURL, must(url.Parse("/")), Project{".index"}) + checkOk(t, ParseProjectURL, must(url.Parse("/foo")), Project{"foo"}) + checkOk(t, ParseProjectURL, must(url.Parse("/foo/")), Project{"foo"}) + + checkErr(t, ParseProjectURL, must(url.Parse("/foo/bar")), ErrInvalidProjectURL) + checkErr(t, ParseProjectURL, must(url.Parse("/foo*")), ErrInvalidProjectURL) + checkErr(t, ParseProjectURL, must(url.Parse("/.index")), ErrInvalidProjectURL) +} + +func TestProjectProperties(t *testing.T) { + checkEq(t, Project{}.IsValid(), false) + checkEq(t, Project{"foo"}.IsValid(), true) + + checkPanic(t, func() Project { return Project{}.MustValid() }) + checkEq(t, Project{"foo"}.MustValid(), Project{"foo"}) + + checkPanic(t, func() string { return Project{}.String() }) + checkEq(t, Project{"foo"}.String(), "foo") + checkEq(t, Project{".index"}.String(), ".index") + + checkPanic(t, func() bool { return Project{}.IsIndex() }) + checkEq(t, Project{"foo"}.IsIndex(), false) + checkEq(t, Project{".index"}.IsIndex(), true) + + checkPanic(t, func() string { x, _, _ := Project{}.GetPullRequest(); return x }) + + baseName, prNumber, isPreview := Project{"foo"}.GetPullRequest() + checkEq(t, baseName, "foo") + checkEq(t, prNumber, 0) + checkEq(t, isPreview, false) + + baseName, prNumber, isPreview = Project{"foo@10"}.GetPullRequest() + checkEq(t, baseName, "foo") + checkEq(t, prNumber, 10) + checkEq(t, isPreview, true) +} diff --git a/src/pages.go b/src/pages.go index 23486f6..000021e 100644 --- a/src/pages.go +++ b/src/pages.go @@ -18,6 +18,7 @@ import ( "strings" "time" + "codeberg.org/git-pages/git-pages/src/names" "github.com/klauspost/compress/zstd" "github.com/pquerna/cachecontrol/cacheobject" "github.com/prometheus/client_golang/prometheus" @@ -90,12 +91,12 @@ func getWebRoot(r *http.Request) (string, error) { return "", err } - projectName, err := GetProjectName(r) + projectName, err := names.ParseProjectURL(r.URL) if err != nil { return "", err } - return makeWebRoot(host, projectName), nil + return makeWebRoot(host, projectName.String()), nil } func writeRedirect(w http.ResponseWriter, code int, path string) { @@ -155,7 +156,7 @@ func getPage(w http.ResponseWriter, r *http.Request) error { projectPathPrefix := "/" // 2xx and 4xx redirect target URLs must start with this prefix if projectName, projectPath, hasProjectSlash := strings.Cut(sitePath, "/"); projectName != "" { webRoot := makeWebRoot(host, projectName) - if ValidateProjectName(projectName) == nil && + if _, err = names.ParseProjectName(projectName); err == nil && existenceCache.CheckSite(r.Context(), webRoot).IsPossible() { var projectManifest *Manifest var projectMetadata ManifestMetadata @@ -992,6 +993,9 @@ func ServePages(w http.ResponseWriter, r *http.Request) { err = fmt.Errorf("method %s not allowed", r.Method) } if err != nil { + if errors.Is(err, names.ErrInvalidProjectURL) { + http.Error(w, prettyErrMsg(err), http.StatusBadRequest) + } var authErr AuthError if errors.As(err, &authErr) { http.Error(w, prettyErrMsg(err), authErr.code)