diff --git a/appview/ingester.go b/appview/ingester.go --- a/appview/ingester.go +++ b/appview/ingester.go @@ -19,7 +19,6 @@ "tangled.org/core/appview/config" "tangled.org/core/appview/db" "tangled.org/core/appview/models" "tangled.org/core/appview/serververify" - "tangled.org/core/appview/validator" "tangled.org/core/idresolver" "tangled.org/core/orm" "tangled.org/core/rbac" @@ -31,7 +30,6 @@ Enforcer *rbac.Enforcer IdResolver *idresolver.Resolver Config *config.Config Logger *slog.Logger - Validator *validator.Validator } type processFunc func(ctx context.Context, e *jmodels.Event) error @@ -613,7 +611,7 @@ } string := models.StringFromRecord(did, rkey, record) - if err = i.Validator.ValidateString(&string); err != nil { + if err = string.Validate(); err != nil { l.Error("invalid record", "err", err) return err } @@ -822,7 +820,7 @@ } issue := models.IssueFromRecord(did, rkey, record) - if err := i.Validator.ValidateIssue(&issue); err != nil { + if err := issue.Validate(); err != nil { return fmt.Errorf("failed to validate issue: %w", err) } @@ -902,7 +900,7 @@ if err != nil { return fmt.Errorf("failed to parse comment from record: %w", err) } - if err := i.Validator.ValidateIssueComment(comment); err != nil { + if err := comment.Validate(); err != nil { return fmt.Errorf("failed to validate comment: %w", err) } @@ -962,7 +960,7 @@ if err != nil { return fmt.Errorf("failed to parse labeldef from record: %w", err) } - if err := i.Validator.ValidateLabelDefinition(def); err != nil { + if err := def.Validate(); err != nil { return fmt.Errorf("failed to validate labeldef: %w", err) } @@ -1038,7 +1036,19 @@ def, ok := actx.Defs[o.OperandKey] if !ok { return fmt.Errorf("failed to find label def for key: %s, expected: %q", o.OperandKey, slices.Collect(maps.Keys(actx.Defs))) } - if err := i.Validator.ValidateLabelOp(def, repo, &o); err != nil { + + // validate permissions: only collaborators can apply labels currently + // + // TODO: introduce a repo:triage permission + ok, err := i.Enforcer.IsPushAllowed(o.Did, repo.Knot, repo.DidSlashRepo()) + if err != nil { + return fmt.Errorf("enforcing permission: %w", err) + } + if !ok { + return fmt.Errorf("unauthorized label operation") + } + + if err := def.ValidateOperandValue(&o); err != nil { return fmt.Errorf("failed to validate labelop: %w", err) } } diff --git a/appview/issues/issues.go b/appview/issues/issues.go --- a/appview/issues/issues.go +++ b/appview/issues/issues.go @@ -28,7 +28,6 @@ "tangled.org/core/appview/pages/repoinfo" "tangled.org/core/appview/pagination" "tangled.org/core/appview/reporesolver" "tangled.org/core/appview/searchquery" - "tangled.org/core/appview/validator" "tangled.org/core/idresolver" "tangled.org/core/orm" "tangled.org/core/rbac" @@ -46,7 +45,6 @@ db *db.DB config *config.Config notifier notify.Notifier logger *slog.Logger - validator *validator.Validator indexer *issues_indexer.Indexer } @@ -60,7 +58,6 @@ mentionsResolver *mentions.Resolver, db *db.DB, config *config.Config, notifier notify.Notifier, - validator *validator.Validator, indexer *issues_indexer.Indexer, logger *slog.Logger, ) *Issues { @@ -75,7 +72,6 @@ db: db, config: config, notifier: notifier, logger: logger, - validator: validator, indexer: indexer, } } @@ -166,7 +162,7 @@ newIssue.Title = r.FormValue("title") newIssue.Body = r.FormValue("body") newIssue.Mentions, newIssue.References = rp.mentionsResolver.Resolve(r.Context(), newIssue.Body) - if err := rp.validator.ValidateIssue(newIssue); err != nil { + if err := newIssue.Validate(); err != nil { l.Error("validation error", "err", err) rp.pages.Notice(w, noticeId, fmt.Sprintf("Failed to edit issue: %s", err)) return @@ -425,7 +421,7 @@ Created: time.Now(), Mentions: mentions, References: references, } - if err = rp.validator.ValidateIssueComment(&comment); err != nil { + if err = comment.Validate(); err != nil { l.Error("failed to validate comment", "err", err) rp.pages.Notice(w, "issue-comment", "Failed to create comment.") return @@ -1022,7 +1018,7 @@ References: references, Repo: f, } - if err := rp.validator.ValidateIssue(issue); err != nil { + if err := issue.Validate(); err != nil { l.Error("validation error", "err", err) rp.pages.Notice(w, "issues", fmt.Sprintf("Failed to create issue: %s", err)) return diff --git a/appview/labels/labels.go b/appview/labels/labels.go --- a/appview/labels/labels.go +++ b/appview/labels/labels.go @@ -16,7 +16,6 @@ "tangled.org/core/appview/models" "tangled.org/core/appview/notify" "tangled.org/core/appview/oauth" "tangled.org/core/appview/pages" - "tangled.org/core/appview/validator" "tangled.org/core/orm" "tangled.org/core/rbac" "tangled.org/core/tid" @@ -29,32 +28,29 @@ "github.com/go-chi/chi/v5" ) type Labels struct { - oauth *oauth.OAuth - pages *pages.Pages - db *db.DB - logger *slog.Logger - validator *validator.Validator - enforcer *rbac.Enforcer - notifier notify.Notifier + oauth *oauth.OAuth + pages *pages.Pages + db *db.DB + logger *slog.Logger + enforcer *rbac.Enforcer + notifier notify.Notifier } func New( oauth *oauth.OAuth, pages *pages.Pages, db *db.DB, - validator *validator.Validator, enforcer *rbac.Enforcer, notifier notify.Notifier, logger *slog.Logger, ) *Labels { return &Labels{ - oauth: oauth, - pages: pages, - db: db, - logger: logger, - validator: validator, - enforcer: enforcer, - notifier: notifier, + oauth: oauth, + pages: pages, + db: db, + logger: logger, + enforcer: enforcer, + notifier: notifier, } } @@ -167,10 +163,26 @@ } for i := range labelOps { def := actx.Defs[labelOps[i].OperandKey] - if err := l.validator.ValidateLabelOp(def, repo, &labelOps[i]); err != nil { + op := labelOps[i] + + // validate permissions: only collaborators can apply labels currently + // + // TODO: introduce a repo:triage permission + ok, err := l.enforcer.IsPushAllowed(op.Did, repo.Knot, repo.DidSlashRepo()) + if err != nil { + fail("Failed to enforce permissions. Please try again later", fmt.Errorf("enforcing permission: %w", err)) + return + } + if !ok { + fail("Unauthorized label operation", fmt.Errorf("unauthorized label operation")) + return + } + + if err := def.ValidateOperandValue(&op); err != nil { fail(fmt.Sprintf("Invalid form data: %s", err), err) return } + labelOps[i] = op } // reduce the opset diff --git a/appview/models/issue.go b/appview/models/issue.go --- a/appview/models/issue.go +++ b/appview/models/issue.go @@ -3,10 +3,12 @@ import ( "fmt" "sort" + "strings" "time" "github.com/bluesky-social/indigo/atproto/syntax" "tangled.org/core/api/tangled" + "tangled.org/core/appview/pages/markup/sanitizer" ) type Issue struct { @@ -59,6 +61,26 @@ if i.Open { return "open" } return "closed" +} + +var _ Validator = new(Issue) + +func (i *Issue) Validate() error { + if i.Title == "" { + return fmt.Errorf("issue title is empty") + } + if i.Body == "" { + return fmt.Errorf("issue body is empty") + } + + if st := strings.TrimSpace(sanitizer.SanitizeDescription(i.Title)); st == "" { + return fmt.Errorf("title is empty after HTML sanitization") + } + + if st := strings.TrimSpace(sanitizer.SanitizeDefault(i.Body)); st == "" { + return fmt.Errorf("body is empty after HTML sanitization") + } + return nil } type CommentListItem struct { @@ -215,6 +237,16 @@ } func (i *IssueComment) IsReply() bool { return i.ReplyTo != nil +} + +var _ Validator = new(IssueComment) + +func (i *IssueComment) Validate() error { + if sb := strings.TrimSpace(sanitizer.SanitizeDefault(i.Body)); sb == "" { + return fmt.Errorf("body is empty after HTML sanitization") + } + + return nil } func IssueCommentFromRecord(did, rkey string, record tangled.RepoIssueComment) (*IssueComment, error) { diff --git a/appview/models/label.go b/appview/models/label.go --- a/appview/models/label.go +++ b/appview/models/label.go @@ -7,7 +7,9 @@ "encoding/hex" "encoding/json" "errors" "fmt" + "regexp" "slices" + "strings" "time" "github.com/bluesky-social/indigo/api/atproto" @@ -120,6 +122,167 @@ ValueType: &vt, } } +var ( + // Label name should be alphanumeric with hyphens/underscores, but not start/end with them + labelNameRegex = regexp.MustCompile(`^[a-zA-Z0-9]([a-zA-Z0-9_-]*[a-zA-Z0-9])?$`) + // Color should be a valid hex color + colorRegex = regexp.MustCompile(`^#[a-fA-F0-9]{6}$`) + // You can only label issues and pulls presently + validScopes = []string{tangled.RepoIssueNSID, tangled.RepoPullNSID} +) + +var _ Validator = new(LabelDefinition) + +func (l *LabelDefinition) Validate() error { + if l.Name == "" { + return fmt.Errorf("label name is empty") + } + if len(l.Name) > 40 { + return fmt.Errorf("label name too long (max 40 graphemes)") + } + if len(l.Name) < 1 { + return fmt.Errorf("label name too short (min 1 grapheme)") + } + if !labelNameRegex.MatchString(l.Name) { + return fmt.Errorf("label name contains invalid characters (use only letters, numbers, hyphens, and underscores)") + } + + if !l.ValueType.IsConcreteType() { + return fmt.Errorf("invalid value type: %q (must be one of: null, boolean, integer, string)", l.ValueType.Type) + } + + // null type checks: cannot be enums, multiple or explicit format + if l.ValueType.IsNull() && l.ValueType.IsEnum() { + return fmt.Errorf("null type cannot be used in conjunction with enum type") + } + if l.ValueType.IsNull() && l.Multiple { + return fmt.Errorf("null type labels cannot be multiple") + } + if l.ValueType.IsNull() && !l.ValueType.IsAnyFormat() { + return fmt.Errorf("format cannot be used in conjunction with null type") + } + + // format checks: cannot be used with enum, or integers + if !l.ValueType.IsAnyFormat() && l.ValueType.IsEnum() { + return fmt.Errorf("enum types cannot be used in conjunction with format specification") + } + + if !l.ValueType.IsAnyFormat() && !l.ValueType.IsString() { + return fmt.Errorf("format specifications are only permitted on string types") + } + + // validate scope (nsid format) + if l.Scope == nil { + return fmt.Errorf("scope is required") + } + for _, s := range l.Scope { + if _, err := syntax.ParseNSID(s); err != nil { + return fmt.Errorf("failed to parse scope: %w", err) + } + if !slices.Contains(validScopes, s) { + return fmt.Errorf("invalid scope: scope must be present in %q", validScopes) + } + } + + // validate color if provided + if l.Color != nil { + color := strings.TrimSpace(*l.Color) + if color == "" { + // empty color is fine, set to nil + l.Color = nil + } else { + if !colorRegex.MatchString(color) { + return fmt.Errorf("color must be a valid hex color (e.g. #79FFE1 or #000)") + } + // expand 3-digit hex to 6-digit hex + if len(color) == 4 { // #ABC + color = fmt.Sprintf("#%c%c%c%c%c%c", color[1], color[1], color[2], color[2], color[3], color[3]) + } + // convert to uppercase for consistency + color = strings.ToUpper(color) + l.Color = &color + } + } + + return nil +} + +// ValidateOperandValue validates the label operation operand value based on +// label definition. +// +// NOTE: This can modify the [LabelOp] +func (def *LabelDefinition) ValidateOperandValue(op *LabelOp) error { + expectedKey := def.AtUri().String() + if op.OperandKey != def.AtUri().String() { + return fmt.Errorf("operand key %q does not match label definition URI %q", op.OperandKey, expectedKey) + } + + valueType := def.ValueType + + // this is permitted, it "unsets" a label + if op.OperandValue == "" { + op.Operation = LabelOperationDel + return nil + } + + switch valueType.Type { + case ConcreteTypeNull: + // For null type, value should be empty + if op.OperandValue != "null" { + return fmt.Errorf("null type requires empty value, got %q", op.OperandValue) + } + + case ConcreteTypeString: + // For string type, validate enum constraints if present + if valueType.IsEnum() { + if !slices.Contains(valueType.Enum, op.OperandValue) { + return fmt.Errorf("value %q is not in allowed enum values %v", op.OperandValue, valueType.Enum) + } + } + + switch valueType.Format { + case ValueTypeFormatDid: + if _, err := syntax.ParseDID(op.OperandValue); err != nil { + return fmt.Errorf("failed to resolve did/handle: %w", err) + } + case ValueTypeFormatAny, "": + default: + return fmt.Errorf("unsupported format constraint: %q", valueType.Format) + } + + case ConcreteTypeInt: + if op.OperandValue == "" { + return fmt.Errorf("integer type requires non-empty value") + } + if _, err := fmt.Sscanf(op.OperandValue, "%d", new(int)); err != nil { + return fmt.Errorf("value %q is not a valid integer", op.OperandValue) + } + + if valueType.IsEnum() { + if !slices.Contains(valueType.Enum, op.OperandValue) { + return fmt.Errorf("value %q is not in allowed enum values %v", op.OperandValue, valueType.Enum) + } + } + + case ConcreteTypeBool: + if op.OperandValue != "true" && op.OperandValue != "false" { + return fmt.Errorf("boolean type requires value to be 'true' or 'false', got %q", op.OperandValue) + } + + // validate enum constraints if present (though uncommon for booleans) + if valueType.IsEnum() { + if !slices.Contains(valueType.Enum, op.OperandValue) { + return fmt.Errorf("value %q is not in allowed enum values %v", op.OperandValue, valueType.Enum) + } + } + + default: + return fmt.Errorf("unsupported value type: %q", valueType.Type) + } + + return nil +} + // random color for a given seed func randomColor(seed string) string { hash := sha1.Sum([]byte(seed)) @@ -131,14 +294,14 @@ return fmt.Sprintf("#%s%s%s", r, g, b) } -func (ld LabelDefinition) GetColor() string { - if ld.Color == nil { - seed := fmt.Sprintf("%d:%s:%s", ld.Id, ld.Did, ld.Rkey) +func (l LabelDefinition) GetColor() string { + if l.Color == nil { + seed := fmt.Sprintf("%d:%s:%s", l.Id, l.Did, l.Rkey) color := randomColor(seed) return color } - return *ld.Color + return *l.Color } func LabelDefinitionFromRecord(did, rkey string, record tangled.LabelDefinition) (*LabelDefinition, error) { @@ -203,6 +366,22 @@ } // otherwise, createdat is in the future relative to indexedat -> use indexedat return indexedAt +} + +var _ Validator = new(LabelOp) + +func (l *LabelOp) Validate() error { + if _, err := syntax.ParseATURI(string(l.Subject)); err != nil { + return fmt.Errorf("invalid subject URI: %w", err) + } + if l.Operation != LabelOperationAdd && l.Operation != LabelOperationDel { + return fmt.Errorf("invalid operation: %q (must be 'add' or 'del')", l.Operation) + } + // Validate performed time is not zero/invalid + if l.PerformedAt.IsZero() { + return fmt.Errorf("performed_at timestamp is required") + } + return nil } type LabelOperation string diff --git a/appview/models/string.go b/appview/models/string.go --- a/appview/models/string.go +++ b/appview/models/string.go @@ -2,10 +2,12 @@ package models import ( "bytes" + "errors" "fmt" "io" "strings" "time" + "unicode/utf8" "github.com/bluesky-social/indigo/atproto/syntax" "tangled.org/core/api/tangled" @@ -33,6 +35,25 @@ Description: s.Description, Contents: s.Contents, CreatedAt: s.Created.Format(time.RFC3339), } +} + +var _ Validator = new(String) + +func (s *String) Validate() error { + var err error + if utf8.RuneCountInString(s.Filename) > 140 { + err = errors.Join(err, fmt.Errorf("filename too long")) + } + + if utf8.RuneCountInString(s.Description) > 280 { + err = errors.Join(err, fmt.Errorf("description too long")) + } + + if len(s.Contents) == 0 { + err = errors.Join(err, fmt.Errorf("contents is empty")) + } + + return err } func StringFromRecord(did, rkey string, record tangled.String) String { diff --git a/appview/models/validator.go b/appview/models/validator.go new file mode 100644 --- /dev/null +++ b/appview/models/validator.go @@ -0,0 +1,6 @@ +package models + +type Validator interface { + // Validate checks the object and returns any error. + Validate() error +} diff --git a/appview/pages/funcmap.go b/appview/pages/funcmap.go --- a/appview/pages/funcmap.go +++ b/appview/pages/funcmap.go @@ -31,6 +31,7 @@ "tangled.org/core/appview/db" "tangled.org/core/appview/models" "tangled.org/core/appview/oauth" "tangled.org/core/appview/pages/markup" + "tangled.org/core/appview/pages/markup/sanitizer" "tangled.org/core/crypto" ) @@ -264,7 +265,7 @@ }, "markdown": func(text string) template.HTML { p.rctx.RendererType = markup.RendererTypeDefault htmlString := p.rctx.RenderMarkdown(text) - sanitized := p.rctx.SanitizeDefault(htmlString) + sanitized := sanitizer.SanitizeDefault(htmlString) return template.HTML(sanitized) }, "description": func(text string) template.HTML { @@ -274,13 +275,13 @@ goldmark.WithExtensions( emoji.Emoji, ), )) - sanitized := p.rctx.SanitizeDescription(htmlString) + sanitized := sanitizer.SanitizeDescription(htmlString) return template.HTML(sanitized) }, "readme": func(text string) template.HTML { p.rctx.RendererType = markup.RendererTypeRepoMarkdown htmlString := p.rctx.RenderMarkdown(text) - sanitized := p.rctx.SanitizeDefault(htmlString) + sanitized := sanitizer.SanitizeDefault(htmlString) return template.HTML(sanitized) }, "code": func(content, path string) string { diff --git a/appview/pages/markup/markdown.go b/appview/pages/markup/markdown.go --- a/appview/pages/markup/markdown.go +++ b/appview/pages/markup/markdown.go @@ -49,7 +49,6 @@ repoinfo.RepoInfo IsDev bool Hostname string RendererType RendererType - Sanitizer Sanitizer Files fs.FS } @@ -182,14 +181,6 @@ visitNode(ctx, n) } default: } -} - -func (rctx *RenderContext) SanitizeDefault(html string) string { - return rctx.Sanitizer.SanitizeDefault(html) -} - -func (rctx *RenderContext) SanitizeDescription(html string) string { - return rctx.Sanitizer.SanitizeDescription(html) } type MarkdownTransformer struct { diff --git a/appview/pages/markup/sanitizer.go b/appview/pages/markup/sanitizer/sanitizer.go rename from appview/pages/markup/sanitizer.go rename to appview/pages/markup/sanitizer/sanitizer.go --- a/appview/pages/markup/sanitizer.go +++ b/appview/pages/markup/sanitizer/sanitizer.go @@ -1,4 +1,4 @@ -package markup +package sanitizer import ( "maps" @@ -21,23 +21,11 @@ sharedDefaultPolicy = buildDefaultPolicy() sharedDescriptionPolicy = buildDescriptionPolicy() } -type Sanitizer struct { - defaultPolicy *bluemonday.Policy - descriptionPolicy *bluemonday.Policy -} - -func NewSanitizer() Sanitizer { - return Sanitizer{ - defaultPolicy: sharedDefaultPolicy, - descriptionPolicy: sharedDescriptionPolicy, - } -} - -func (s *Sanitizer) SanitizeDefault(html string) string { - return s.defaultPolicy.Sanitize(html) +func SanitizeDefault(html string) string { + return sharedDefaultPolicy.Sanitize(html) } -func (s *Sanitizer) SanitizeDescription(html string) string { - return s.descriptionPolicy.Sanitize(html) +func SanitizeDescription(html string) string { + return sharedDescriptionPolicy.Sanitize(html) } func buildDefaultPolicy() *bluemonday.Policy { diff --git a/appview/pages/pages.go b/appview/pages/pages.go --- a/appview/pages/pages.go +++ b/appview/pages/pages.go @@ -23,6 +23,7 @@ "tangled.org/core/appview/db" "tangled.org/core/appview/models" "tangled.org/core/appview/oauth" "tangled.org/core/appview/pages/markup" + "tangled.org/core/appview/pages/markup/sanitizer" "tangled.org/core/appview/pages/repoinfo" "tangled.org/core/appview/pagination" "tangled.org/core/idresolver" @@ -58,7 +59,6 @@ IsDev: config.Core.Dev, Hostname: config.Core.AppviewHost, CamoUrl: config.Camo.Host, CamoSecret: config.Camo.SharedSecret, - Sanitizer: markup.NewSanitizer(), Files: Files, } @@ -293,7 +293,7 @@ } p.rctx.RendererType = markup.RendererTypeDefault htmlString := p.rctx.RenderMarkdown(string(markdownBytes)) - sanitized := p.rctx.SanitizeDefault(htmlString) + sanitized := sanitizer.SanitizeDefault(htmlString) params.Content = template.HTML(sanitized) return p.execute("legal/terms", w, params) @@ -321,7 +321,7 @@ } p.rctx.RendererType = markup.RendererTypeDefault htmlString := p.rctx.RenderMarkdown(string(markdownBytes)) - sanitized := p.rctx.SanitizeDefault(htmlString) + sanitized := sanitizer.SanitizeDefault(htmlString) params.Content = template.HTML(sanitized) return p.execute("legal/privacy", w, params) @@ -744,7 +744,7 @@ switch ext { case ".md", ".markdown", ".mdown", ".mkdn", ".mkd": params.Raw = false htmlString := p.rctx.RenderMarkdown(params.Readme) - sanitized := p.rctx.SanitizeDefault(htmlString) + sanitized := sanitizer.SanitizeDefault(htmlString) params.HTMLReadme = template.HTML(sanitized) default: params.Raw = true @@ -837,7 +837,7 @@ switch ext { case ".md", ".markdown", ".mdown", ".mkdn", ".mkd": params.Raw = false htmlString := p.rctx.RenderMarkdown(params.Readme) - sanitized := p.rctx.SanitizeDefault(htmlString) + sanitized := sanitizer.SanitizeDefault(htmlString) params.HTMLReadme = template.HTML(sanitized) default: params.Raw = true diff --git a/appview/pulls/pulls.go b/appview/pulls/pulls.go --- a/appview/pulls/pulls.go +++ b/appview/pulls/pulls.go @@ -27,12 +27,11 @@ "tangled.org/core/appview/models" "tangled.org/core/appview/notify" "tangled.org/core/appview/oauth" "tangled.org/core/appview/pages" - "tangled.org/core/appview/pages/markup" + "tangled.org/core/appview/pages/markup/sanitizer" "tangled.org/core/appview/pages/repoinfo" "tangled.org/core/appview/pagination" "tangled.org/core/appview/reporesolver" "tangled.org/core/appview/searchquery" - "tangled.org/core/appview/validator" "tangled.org/core/appview/xrpcclient" "tangled.org/core/idresolver" "tangled.org/core/orm" @@ -63,7 +62,6 @@ config *config.Config notifier notify.Notifier enforcer *rbac.Enforcer logger *slog.Logger - validator *validator.Validator indexer *pulls_indexer.Indexer } @@ -77,7 +75,6 @@ db *db.DB, config *config.Config, notifier notify.Notifier, enforcer *rbac.Enforcer, - validator *validator.Validator, indexer *pulls_indexer.Indexer, logger *slog.Logger, ) *Pulls { @@ -92,7 +89,6 @@ config: config, notifier: notifier, enforcer: enforcer, logger: logger, - validator: validator, indexer: indexer, } } @@ -975,7 +971,6 @@ if title == "" { s.pages.Notice(w, "pull", "Title is required for git-diff patches.") return } - sanitizer := markup.NewSanitizer() if st := strings.TrimSpace(sanitizer.SanitizeDescription(title)); (st) == "" { s.pages.Notice(w, "pull", "Title is empty after HTML sanitization") return @@ -1103,7 +1098,7 @@ sourceRev := comparison.Rev2 patch := comparison.FormatPatchRaw combined := comparison.CombinedPatchRaw - if err := s.validator.ValidatePatch(&patch); err != nil { + if err := validatePatch(&patch); err != nil { s.logger.Error("failed to validate patch", "err", err) s.pages.Notice(w, "pull", "Invalid patch format. Please provide a valid diff.") return @@ -1121,7 +1116,7 @@ s.createPullRequest(w, r, repo, user, title, body, targetBranch, patch, combined, sourceRev, pullSource, recordPullSource, isStacked) } func (s *Pulls) handlePatchBasedPull(w http.ResponseWriter, r *http.Request, repo *models.Repo, user *oauth.MultiAccountUser, title, body, targetBranch, patch string, isStacked bool) { - if err := s.validator.ValidatePatch(&patch); err != nil { + if err := validatePatch(&patch); err != nil { s.logger.Error("patch validation failed", "err", err) s.pages.Notice(w, "pull", "Invalid patch format. Please provide a valid diff.") return @@ -1213,7 +1208,7 @@ sourceRev := comparison.Rev2 patch := comparison.FormatPatchRaw combined := comparison.CombinedPatchRaw - if err := s.validator.ValidatePatch(&patch); err != nil { + if err := validatePatch(&patch); err != nil { s.logger.Error("failed to validate patch", "err", err) s.pages.Notice(w, "pull", "Invalid patch format. Please provide a valid diff.") return @@ -1506,7 +1501,7 @@ s.pages.Notice(w, "patch-error", "Patch is required.") return } - if err := s.validator.ValidatePatch(&patch); err != nil { + if err := validatePatch(&patch); err != nil { s.logger.Error("faield to validate patch", "err", err) s.pages.Notice(w, "patch-error", "Invalid patch format. Please provide a valid git diff or format-patch.") return @@ -1927,7 +1922,7 @@ s.resubmitStackedPullHelper(w, r, repo, user, pull, patch, pull.StackId) return } - if err := s.validator.ValidatePatch(&patch); err != nil { + if err := validatePatch(&patch); err != nil { s.pages.Notice(w, "resubmit-error", err.Error()) return } @@ -2559,3 +2554,20 @@ return &b } func ptrPullState(s models.PullState) *models.PullState { return &s } + +func validatePatch(patch *string) error { + if patch == nil || *patch == "" { + return fmt.Errorf("patch is empty") + } + + // add newline if not present to diff style patches + if !patchutil.IsFormatPatch(*patch) && !strings.HasSuffix(*patch, "\n") { + *patch = *patch + "\n" + } + + if err := patchutil.IsPatchValid(*patch); err != nil { + return err + } + + return nil +} diff --git a/appview/repo/repo.go b/appview/repo/repo.go --- a/appview/repo/repo.go +++ b/appview/repo/repo.go @@ -22,7 +22,6 @@ "tangled.org/core/appview/notify" "tangled.org/core/appview/oauth" "tangled.org/core/appview/pages" "tangled.org/core/appview/reporesolver" - "tangled.org/core/appview/validator" xrpcclient "tangled.org/core/appview/xrpcclient" "tangled.org/core/eventconsumer" "tangled.org/core/idresolver" @@ -51,7 +50,6 @@ enforcer *rbac.Enforcer notifier notify.Notifier logger *slog.Logger serviceAuth *serviceauth.ServiceAuth - validator *validator.Validator cfClient *cloudflare.Client } @@ -66,7 +64,6 @@ config *config.Config, notifier notify.Notifier, enforcer *rbac.Enforcer, logger *slog.Logger, - validator *validator.Validator, cfClient *cloudflare.Client, ) *Repo { return &Repo{ @@ -80,7 +77,6 @@ db: db, notifier: notifier, enforcer: enforcer, logger: logger, - validator: validator, cfClient: cfClient, } } @@ -231,7 +227,7 @@ Color: &color, Multiple: multiple, Created: time.Now(), } - if err := rp.validator.ValidateLabelDefinition(&label); err != nil { + if err := label.Validate(); err != nil { fail(err.Error(), err) return } diff --git a/appview/repo/settings.go b/appview/repo/settings.go --- a/appview/repo/settings.go +++ b/appview/repo/settings.go @@ -5,7 +5,9 @@ "context" "encoding/json" "fmt" "net/http" + "net/url" "path" + "regexp" "slices" "strings" "time" @@ -19,6 +21,7 @@ "tangled.org/core/appview/pages" "tangled.org/core/appview/sites" xrpcclient "tangled.org/core/appview/xrpcclient" "tangled.org/core/orm" + "tangled.org/core/sets" "tangled.org/core/types" comatproto "github.com/bluesky-social/indigo/api/atproto" @@ -591,14 +594,15 @@ website = r.FormValue("website") topicStr = r.FormValue("topics") ) - err = rp.validator.ValidateURI(website) - if website != "" && err != nil { - l.Error("invalid uri", "err", err) - rp.pages.Notice(w, noticeId, err.Error()) - return + if website != "" { + if err := validateURI(website); err != nil { + l.Error("invalid uri", "err", err) + rp.pages.Notice(w, noticeId, err.Error()) + return + } } - topics, err := rp.validator.ValidateRepoTopicStr(topicStr) + topics, err := parseRepoTopicStr(topicStr) if err != nil { l.Error("invalid topics", "err", err) rp.pages.Notice(w, noticeId, err.Error()) @@ -658,3 +662,59 @@ } rp.pages.HxRefresh(w) } + +const ( + maxTopicLen = 50 + maxTopics = 20 +) + +var ( + topicRE = regexp.MustCompile(`\A[a-z0-9-]+\z`) +) + +// parseRepoTopicStr parses and validates whitespace-separated topic string. +// +// Rules: +// - topics are separated by whitespace +// - each topic may contain lowercase letters, digits, and hyphens only +// - each topic must be <= 50 characters long +// - no more than 20 topics allowed +// - duplicates are removed +func parseRepoTopicStr(topicStr string) ([]string, error) { + topicStr = strings.TrimSpace(topicStr) + if topicStr == "" { + return nil, nil + } + parts := strings.Fields(topicStr) + if len(parts) > maxTopics { + return nil, fmt.Errorf("too many topics: %d (maximum %d)", len(parts), maxTopics) + } + + topicSet := sets.New[string]() + + for _, t := range parts { + if topicSet.Contains(t) { + continue + } + if len(t) > maxTopicLen { + return nil, fmt.Errorf("topic '%s' is too long (maximum %d characters)", t, maxTopics) + } + if !topicRE.MatchString(t) { + return nil, fmt.Errorf("topic '%s' contains invalid characters (allowed: lowercase letters, digits, hyphens)", t) + } + topicSet.Insert(t) + } + return slices.Collect(topicSet.All()), nil +} + +// TODO(boltless): move this to models.Repo instead +func validateURI(uri string) error { + parsed, err := url.Parse(uri) + if err != nil { + return fmt.Errorf("invalid uri format") + } + if parsed.Scheme == "" { + return fmt.Errorf("uri scheme missing") + } + return nil +} diff --git a/appview/state/router.go b/appview/state/router.go --- a/appview/state/router.go +++ b/appview/state/router.go @@ -280,7 +280,6 @@ s.mentionsResolver, s.db, s.config, s.notifier, - s.validator, s.indexer.Issues, log.SubLogger(s.logger, "issues"), ) @@ -298,7 +297,6 @@ s.db, s.config, s.notifier, s.enforcer, - s.validator, s.indexer.Pulls, log.SubLogger(s.logger, "pulls"), ) @@ -317,7 +315,6 @@ s.config, s.notifier, s.enforcer, log.SubLogger(s.logger, "repo"), - s.validator, s.cfClient, ) return repo.Router(mw) @@ -343,7 +340,6 @@ ls := labels.New( s.oauth, s.pages, s.db, - s.validator, s.enforcer, s.notifier, log.SubLogger(s.logger, "labels"), diff --git a/appview/state/state.go b/appview/state/state.go --- a/appview/state/state.go +++ b/appview/state/state.go @@ -25,7 +25,6 @@ phnotify "tangled.org/core/appview/notify/posthog" "tangled.org/core/appview/oauth" "tangled.org/core/appview/pages" "tangled.org/core/appview/reporesolver" - "tangled.org/core/appview/validator" xrpcclient "tangled.org/core/appview/xrpcclient" "tangled.org/core/consts" "tangled.org/core/eventconsumer" @@ -63,7 +62,6 @@ repoResolver *reporesolver.RepoResolver knotstream *eventconsumer.Consumer spindlestream *eventconsumer.Consumer logger *slog.Logger - validator *validator.Validator cfClient *cloudflare.Client } @@ -102,7 +100,6 @@ oauth, err := oauth.New(config, posthog, d, enforcer, res, log.SubLogger(logger, "oauth")) if err != nil { return nil, fmt.Errorf("failed to start oauth handler: %w", err) } - validator := validator.New(d, res, enforcer) repoResolver := reporesolver.New(config, enforcer, d) @@ -150,7 +147,6 @@ Enforcer: enforcer, IdResolver: res, Config: config, Logger: log.SubLogger(logger, "ingester"), - Validator: validator, } err = jc.StartJetstream(ctx, ingester.Ingest()) if err != nil { @@ -211,7 +207,6 @@ repoResolver: repoResolver, knotstream: knotstream, spindlestream: spindlestream, logger: logger, - validator: validator, cfClient: cfClient, } diff --git a/appview/validator/issue.go b/appview/validator/issue.go deleted file mode 100644 --- a/appview/validator/issue.go +++ /dev/null @@ -1,55 +0,0 @@ -package validator - -import ( - "fmt" - "strings" - - "tangled.org/core/appview/db" - "tangled.org/core/appview/models" - "tangled.org/core/orm" -) - -func (v *Validator) ValidateIssueComment(comment *models.IssueComment) error { - // if comments have parents, only ingest ones that are 1 level deep - if comment.ReplyTo != nil { - parents, err := db.GetIssueComments(v.db, orm.FilterEq("at_uri", *comment.ReplyTo)) - if err != nil { - return fmt.Errorf("failed to fetch parent comment: %w", err) - } - if len(parents) != 1 { - return fmt.Errorf("incorrect number of parent comments returned: %d", len(parents)) - } - - // depth check - parent := parents[0] - if parent.ReplyTo != nil { - return fmt.Errorf("incorrect depth, this comment is replying at depth >1") - } - } - - if sb := strings.TrimSpace(v.sanitizer.SanitizeDefault(comment.Body)); sb == "" { - return fmt.Errorf("body is empty after HTML sanitization") - } - - return nil -} - -func (v *Validator) ValidateIssue(issue *models.Issue) error { - if issue.Title == "" { - return fmt.Errorf("issue title is empty") - } - - if issue.Body == "" { - return fmt.Errorf("issue body is empty") - } - - if st := strings.TrimSpace(v.sanitizer.SanitizeDescription(issue.Title)); st == "" { - return fmt.Errorf("title is empty after HTML sanitization") - } - - if sb := strings.TrimSpace(v.sanitizer.SanitizeDefault(issue.Body)); sb == "" { - return fmt.Errorf("body is empty after HTML sanitization") - } - - return nil -} diff --git a/appview/validator/label.go b/appview/validator/label.go deleted file mode 100644 --- a/appview/validator/label.go +++ /dev/null @@ -1,217 +0,0 @@ -package validator - -import ( - "context" - "fmt" - "regexp" - "slices" - "strings" - - "github.com/bluesky-social/indigo/atproto/syntax" - "tangled.org/core/api/tangled" - "tangled.org/core/appview/models" -) - -var ( - // Label name should be alphanumeric with hyphens/underscores, but not start/end with them - labelNameRegex = regexp.MustCompile(`^[a-zA-Z0-9]([a-zA-Z0-9_-]*[a-zA-Z0-9])?$`) - // Color should be a valid hex color - colorRegex = regexp.MustCompile(`^#[a-fA-F0-9]{6}$`) - // You can only label issues and pulls presently - validScopes = []string{tangled.RepoIssueNSID, tangled.RepoPullNSID} -) - -func (v *Validator) ValidateLabelDefinition(label *models.LabelDefinition) error { - if label.Name == "" { - return fmt.Errorf("label name is empty") - } - if len(label.Name) > 40 { - return fmt.Errorf("label name too long (max 40 graphemes)") - } - if len(label.Name) < 1 { - return fmt.Errorf("label name too short (min 1 grapheme)") - } - if !labelNameRegex.MatchString(label.Name) { - return fmt.Errorf("label name contains invalid characters (use only letters, numbers, hyphens, and underscores)") - } - - if !label.ValueType.IsConcreteType() { - return fmt.Errorf("invalid value type: %q (must be one of: null, boolean, integer, string)", label.ValueType.Type) - } - - // null type checks: cannot be enums, multiple or explicit format - if label.ValueType.IsNull() && label.ValueType.IsEnum() { - return fmt.Errorf("null type cannot be used in conjunction with enum type") - } - if label.ValueType.IsNull() && label.Multiple { - return fmt.Errorf("null type labels cannot be multiple") - } - if label.ValueType.IsNull() && !label.ValueType.IsAnyFormat() { - return fmt.Errorf("format cannot be used in conjunction with null type") - } - - // format checks: cannot be used with enum, or integers - if !label.ValueType.IsAnyFormat() && label.ValueType.IsEnum() { - return fmt.Errorf("enum types cannot be used in conjunction with format specification") - } - - if !label.ValueType.IsAnyFormat() && !label.ValueType.IsString() { - return fmt.Errorf("format specifications are only permitted on string types") - } - - // validate scope (nsid format) - if label.Scope == nil { - return fmt.Errorf("scope is required") - } - for _, s := range label.Scope { - if _, err := syntax.ParseNSID(s); err != nil { - return fmt.Errorf("failed to parse scope: %w", err) - } - if !slices.Contains(validScopes, s) { - return fmt.Errorf("invalid scope: scope must be present in %q", validScopes) - } - } - - // validate color if provided - if label.Color != nil { - color := strings.TrimSpace(*label.Color) - if color == "" { - // empty color is fine, set to nil - label.Color = nil - } else { - if !colorRegex.MatchString(color) { - return fmt.Errorf("color must be a valid hex color (e.g. #79FFE1 or #000)") - } - // expand 3-digit hex to 6-digit hex - if len(color) == 4 { // #ABC - color = fmt.Sprintf("#%c%c%c%c%c%c", color[1], color[1], color[2], color[2], color[3], color[3]) - } - // convert to uppercase for consistency - color = strings.ToUpper(color) - label.Color = &color - } - } - - return nil -} - -func (v *Validator) ValidateLabelOp(labelDef *models.LabelDefinition, repo *models.Repo, labelOp *models.LabelOp) error { - if labelDef == nil { - return fmt.Errorf("label definition is required") - } - if repo == nil { - return fmt.Errorf("repo is required") - } - if labelOp == nil { - return fmt.Errorf("label operation is required") - } - - // validate permissions: only collaborators can apply labels currently - // - // TODO: introduce a repo:triage permission - ok, err := v.enforcer.IsPushAllowed(labelOp.Did, repo.Knot, repo.DidSlashRepo()) - if err != nil { - return fmt.Errorf("failed to enforce permissions: %w", err) - } - if !ok { - return fmt.Errorf("unauhtorized label operation") - } - - expectedKey := labelDef.AtUri().String() - if labelOp.OperandKey != expectedKey { - return fmt.Errorf("operand key %q does not match label definition URI %q", labelOp.OperandKey, expectedKey) - } - - if labelOp.Operation != models.LabelOperationAdd && labelOp.Operation != models.LabelOperationDel { - return fmt.Errorf("invalid operation: %q (must be 'add' or 'del')", labelOp.Operation) - } - - if labelOp.Subject == "" { - return fmt.Errorf("subject URI is required") - } - if _, err := syntax.ParseATURI(string(labelOp.Subject)); err != nil { - return fmt.Errorf("invalid subject URI: %w", err) - } - - if err := v.validateOperandValue(labelDef, labelOp); err != nil { - return fmt.Errorf("invalid operand value: %w", err) - } - - // Validate performed time is not zero/invalid - if labelOp.PerformedAt.IsZero() { - return fmt.Errorf("performed_at timestamp is required") - } - - return nil -} - -func (v *Validator) validateOperandValue(labelDef *models.LabelDefinition, labelOp *models.LabelOp) error { - valueType := labelDef.ValueType - - // this is permitted, it "unsets" a label - if labelOp.OperandValue == "" { - labelOp.Operation = models.LabelOperationDel - return nil - } - - switch valueType.Type { - case models.ConcreteTypeNull: - // For null type, value should be empty - if labelOp.OperandValue != "null" { - return fmt.Errorf("null type requires empty value, got %q", labelOp.OperandValue) - } - - case models.ConcreteTypeString: - // For string type, validate enum constraints if present - if valueType.IsEnum() { - if !slices.Contains(valueType.Enum, labelOp.OperandValue) { - return fmt.Errorf("value %q is not in allowed enum values %v", labelOp.OperandValue, valueType.Enum) - } - } - - switch valueType.Format { - case models.ValueTypeFormatDid: - id, err := v.resolver.ResolveIdent(context.Background(), labelOp.OperandValue) - if err != nil { - return fmt.Errorf("failed to resolve did/handle: %w", err) - } - - labelOp.OperandValue = id.DID.String() - - case models.ValueTypeFormatAny, "": - default: - return fmt.Errorf("unsupported format constraint: %q", valueType.Format) - } - - case models.ConcreteTypeInt: - if labelOp.OperandValue == "" { - return fmt.Errorf("integer type requires non-empty value") - } - if _, err := fmt.Sscanf(labelOp.OperandValue, "%d", new(int)); err != nil { - return fmt.Errorf("value %q is not a valid integer", labelOp.OperandValue) - } - - if valueType.IsEnum() { - if !slices.Contains(valueType.Enum, labelOp.OperandValue) { - return fmt.Errorf("value %q is not in allowed enum values %v", labelOp.OperandValue, valueType.Enum) - } - } - - case models.ConcreteTypeBool: - if labelOp.OperandValue != "true" && labelOp.OperandValue != "false" { - return fmt.Errorf("boolean type requires value to be 'true' or 'false', got %q", labelOp.OperandValue) - } - - // validate enum constraints if present (though uncommon for booleans) - if valueType.IsEnum() { - if !slices.Contains(valueType.Enum, labelOp.OperandValue) { - return fmt.Errorf("value %q is not in allowed enum values %v", labelOp.OperandValue, valueType.Enum) - } - } - - default: - return fmt.Errorf("unsupported value type: %q", valueType.Type) - } - - return nil -} diff --git a/appview/validator/patch.go b/appview/validator/patch.go deleted file mode 100644 --- a/appview/validator/patch.go +++ /dev/null @@ -1,25 +0,0 @@ -package validator - -import ( - "fmt" - "strings" - - "tangled.org/core/patchutil" -) - -func (v *Validator) ValidatePatch(patch *string) error { - if patch == nil || *patch == "" { - return fmt.Errorf("patch is empty") - } - - // add newline if not present to diff style patches - if !patchutil.IsFormatPatch(*patch) && !strings.HasSuffix(*patch, "\n") { - *patch = *patch + "\n" - } - - if err := patchutil.IsPatchValid(*patch); err != nil { - return err - } - - return nil -} diff --git a/appview/validator/repo_topics.go b/appview/validator/repo_topics.go deleted file mode 100644 --- a/appview/validator/repo_topics.go +++ /dev/null @@ -1,53 +0,0 @@ -package validator - -import ( - "fmt" - "maps" - "regexp" - "slices" - "strings" -) - -const ( - maxTopicLen = 50 - maxTopics = 20 -) - -var ( - topicRE = regexp.MustCompile(`\A[a-z0-9-]+\z`) -) - -// ValidateRepoTopicStr parses and validates whitespace-separated topic string. -// -// Rules: -// - topics are separated by whitespace -// - each topic may contain lowercase letters, digits, and hyphens only -// - each topic must be <= 50 characters long -// - no more than 20 topics allowed -// - duplicates are removed -func (v *Validator) ValidateRepoTopicStr(topicsStr string) ([]string, error) { - topicsStr = strings.TrimSpace(topicsStr) - if topicsStr == "" { - return nil, nil - } - parts := strings.Fields(topicsStr) - if len(parts) > maxTopics { - return nil, fmt.Errorf("too many topics: %d (maximum %d)", len(parts), maxTopics) - } - - topicSet := make(map[string]struct{}) - - for _, t := range parts { - if _, exists := topicSet[t]; exists { - continue - } - if len(t) > maxTopicLen { - return nil, fmt.Errorf("topic '%s' is too long (maximum %d characters)", t, maxTopics) - } - if !topicRE.MatchString(t) { - return nil, fmt.Errorf("topic '%s' contains invalid characters (allowed: lowercase letters, digits, hyphens)", t) - } - topicSet[t] = struct{}{} - } - return slices.Collect(maps.Keys(topicSet)), nil -} diff --git a/appview/validator/string.go b/appview/validator/string.go deleted file mode 100644 --- a/appview/validator/string.go +++ /dev/null @@ -1,27 +0,0 @@ -package validator - -import ( - "errors" - "fmt" - "unicode/utf8" - - "tangled.org/core/appview/models" -) - -func (v *Validator) ValidateString(s *models.String) error { - var err error - - if utf8.RuneCountInString(s.Filename) > 140 { - err = errors.Join(err, fmt.Errorf("filename too long")) - } - - if utf8.RuneCountInString(s.Description) > 280 { - err = errors.Join(err, fmt.Errorf("description too long")) - } - - if len(s.Contents) == 0 { - err = errors.Join(err, fmt.Errorf("contents is empty")) - } - - return err -} diff --git a/appview/validator/uri.go b/appview/validator/uri.go deleted file mode 100644 --- a/appview/validator/uri.go +++ /dev/null @@ -1,17 +0,0 @@ -package validator - -import ( - "fmt" - "net/url" -) - -func (v *Validator) ValidateURI(uri string) error { - parsed, err := url.Parse(uri) - if err != nil { - return fmt.Errorf("invalid uri format") - } - if parsed.Scheme == "" { - return fmt.Errorf("uri scheme missing") - } - return nil -} diff --git a/appview/validator/validator.go b/appview/validator/validator.go deleted file mode 100644 --- a/appview/validator/validator.go +++ /dev/null @@ -1,24 +0,0 @@ -package validator - -import ( - "tangled.org/core/appview/db" - "tangled.org/core/appview/pages/markup" - "tangled.org/core/idresolver" - "tangled.org/core/rbac" -) - -type Validator struct { - db *db.DB - sanitizer markup.Sanitizer - resolver *idresolver.Resolver - enforcer *rbac.Enforcer -} - -func New(db *db.DB, res *idresolver.Resolver, enforcer *rbac.Enforcer) *Validator { - return &Validator{ - db: db, - sanitizer: markup.NewSanitizer(), - resolver: res, - enforcer: enforcer, - } -}