From 0e75a74bc7151781dfcf5cc9d0131b6e133804c0 Mon Sep 17 00:00:00 2001 From: Seongmin Lee Date: Fri, 16 Jan 2026 10:41:29 +0000 Subject: [PATCH] appview: remove validator - RBAC should be enforced on service logic. - We should not check for referenced records existence from db due to the nature of atproto. - Comment depth validation is not necessary. We can accept them and just don't render replies with deeper depth. Move markdown sanitizer to dedicated package to avoid import cycle Signed-off-by: Seongmin Lee --- appview/ingester.go | 48 ++++++++++++++++++++++++++++++++++++++---------- appview/ingester_string_test.go | 6 ++---- appview/issues/issues.go | 8 ++------ appview/labels/labels.go | 66 +++++++++++++++++++++++++++++++++++++++++++++++------------------- appview/models/issue.go | 22 ++++++++++++++++++++++ appview/models/label.go | 187 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---- appview/models/pull.go | 34 ++++++++++++++++++++++++++++++++++ appview/models/string.go | 21 +++++++++++++++++++++ appview/models/validator.go | 6 ++++++ appview/pages/funcmap.go | 7 ++++--- appview/pages/pages.go | 10 +++++----- appview/pipelines/logs.go | 10 ++++------ appview/pulls/compose.go | 5 ++--- appview/pulls/compose_helpers_test.go | 10 +++------- appview/pulls/create.go | 6 +++--- appview/pulls/labels.go | 28 +++++++++++++++++++++++++++- appview/pulls/pulls.go | 23 +++++++++++++++++++---- appview/pulls/resubmit.go | 2 +- appview/repo/repo.go | 6 +----- appview/repo/settings.go | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------ appview/state/router.go | 7 ++----- appview/state/state.go | 7 ------- appview/validator/issue.go | 28 ---------------------------- appview/validator/label.go | 217 ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- appview/validator/label_test.go | 87 --------------------------------------------------------------------------------------- appview/validator/patch.go | 25 ------------------------- appview/validator/pull.go | 68 -------------------------------------------------------------------- appview/validator/repo_topics.go | 53 ----------------------------------------------------- appview/validator/string.go | 27 --------------------------- appview/validator/uri.go | 17 ----------------- appview/validator/validator.go | 24 ------------------------ appview/pages/markup/markdown.go | 9 --------- appview/pages/markup/sanitizer.go | 176 -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- appview/pages/markup/sanitizer/sanitizer.go | 161 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 34 file(s) changed, 653 insertion(s)(+), 830 deletion(s)(-) diff --git a/appview/ingester.go b/appview/ingester.go --- a/appview/ingester.go +++ b/appview/ingester.go @@ -33,7 +33,6 @@ "tangled.org/core/appview/notify" "tangled.org/core/appview/repoverify" "tangled.org/core/appview/serververify" - "tangled.org/core/appview/validator" "tangled.org/core/idresolver" "tangled.org/core/orm" "tangled.org/core/rbac" @@ -48,7 +47,6 @@ Cache *cache.Cache Config *config.Config Logger *slog.Logger - Validator *validator.Validator MentionsResolver *mentions.Resolver Notifier notify.Notifier Verifier repoverify.Verifier @@ -932,7 +930,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 } @@ -1363,7 +1361,7 @@ return fmt.Errorf("issue record repo field is not a valid DID: %w", err) } - if err := i.Validator.ValidateIssue(&issue); err != nil { + if err := issue.Validate(); err != nil { return fmt.Errorf("failed to validate issue: %w", err) } @@ -1512,8 +1510,28 @@ if err != nil { return fmt.Errorf("failed to parse pull from record: %w", err) } - if err := i.Validator.ValidatePull(pull); err != nil { + if err := pull.Validate(); err != nil { return fmt.Errorf("failed to validate pull: %w", err) + } + if pull.DependentOn != nil { + if err := func() error { + dependentPull, err := db.GetPull( + i.Db, + orm.FilterEq("dependent_on", pull.DependentOn.String()), + ) + if errors.Is(err, sql.ErrNoRows) { + return nil + } + if err != nil { + return fmt.Errorf("failed to fetch pulls with same dependency: %w", err) + } + if dependentPull.AtUri() == pull.AtUri() { + return nil + } + return fmt.Errorf("another pull already depends on %s, which would form a DAG, this is presently disallowed", pull.DependentOn.String()) + }(); err != nil { + return fmt.Errorf("failed to validate pull stack: %w", err) + } } tx, err := i.Db.BeginTx(ctx, nil) @@ -1755,7 +1773,7 @@ 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) } @@ -1833,11 +1851,21 @@ 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(ctx, def, repo, &o); err != nil { - if !errors.Is(err, knotacl.ErrKnotUnreachable) { - return fmt.Errorf("failed to validate labelop: %w", err) + // validate permissions: only collaborators can apply labels currently + // + // TODO: introduce a repo:triage permission + allowed, permErr := i.Acl.HasRepoPermissionErr(ctx, repo, o.Did, "repo:push") + if permErr != nil { + if !errors.Is(permErr, knotacl.ErrKnotUnreachable) { + return fmt.Errorf("enforcing permission: %w", permErr) } - l.Warn("ingesting labelop without permission check", "did", o.Did, "err", err) + l.Warn("ingesting labelop without permission check", "did", o.Did, "err", permErr) + } else if !allowed { + 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/ingester_string_test.go b/appview/ingester_string_test.go --- a/appview/ingester_string_test.go +++ b/appview/ingester_string_test.go @@ -12,7 +12,6 @@ "tangled.org/core/api/tangled" "tangled.org/core/appview/db" "tangled.org/core/appview/models" - "tangled.org/core/appview/validator" "tangled.org/core/orm" ) @@ -25,9 +24,8 @@ } t.Cleanup(func() { d.Close() }) return &Ingester{ - Db: d, - Logger: slog.New(slog.DiscardHandler), - Validator: &validator.Validator{}, + Db: d, + Logger: slog.New(slog.DiscardHandler), } } 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/pagination" "tangled.org/core/appview/reporesolver" "tangled.org/core/appview/searchquery" - "tangled.org/core/appview/validator" "tangled.org/core/idresolver" "tangled.org/core/ogre" "tangled.org/core/orm" @@ -46,7 +45,6 @@ config *config.Config notifier notify.Notifier logger *slog.Logger - validator *validator.Validator indexer *issues_indexer.Indexer ogreClient *ogre.Client } @@ -61,7 +59,6 @@ db *db.DB, config *config.Config, notifier notify.Notifier, - validator *validator.Validator, indexer *issues_indexer.Indexer, logger *slog.Logger, ) *Issues { @@ -76,7 +73,6 @@ config: config, notifier: notifier, logger: logger, - validator: validator, indexer: indexer, ogreClient: ogre.NewClient(config.Ogre.Host), } @@ -206,7 +202,7 @@ 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 @@ -680,7 +676,7 @@ 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 @@ -11,50 +11,50 @@ "tangled.org/core/api/tangled" "tangled.org/core/appview/db" + "tangled.org/core/appview/knotacl" "tangled.org/core/appview/middleware" "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" comatproto "github.com/bluesky-social/indigo/api/atproto" "github.com/bluesky-social/indigo/atproto/atclient" + "github.com/bluesky-social/indigo/atproto/identity" "github.com/bluesky-social/indigo/atproto/syntax" lexutil "github.com/bluesky-social/indigo/lex/util" "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 + dir identity.Directory + logger *slog.Logger + acl *knotacl.Service + notifier notify.Notifier } func New( oauth *oauth.OAuth, pages *pages.Pages, db *db.DB, - validator *validator.Validator, - enforcer *rbac.Enforcer, + dir identity.Directory, + acl *knotacl.Service, 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, + dir: dir, + logger: logger, + acl: acl, + notifier: notifier, } } @@ -167,10 +167,38 @@ for i := range labelOps { def := actx.Defs[labelOps[i].OperandKey] - if err := l.validator.ValidateLabelOp(r.Context(), 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.acl.HasRepoPermissionErr(r.Context(), repo, op.Did, "repo:push") + 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 + } + + // resolve Handle to DID + if def.ValueType.IsString() && def.ValueType.IsDidFormat() { + val := syntax.AtIdentifier(op.OperandValue) + if val.IsHandle() { + ident, err := l.dir.Lookup(r.Context(), val) + if err != nil { + fail(fmt.Sprintf("Failed to resolve handle %q: %s", val, err), err) + } + op.OperandValue = ident.DID.String() + } + } + + 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 @@ -2,10 +2,12 @@ import ( "fmt" + "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 @@ 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 } func (i *Issue) Participants() []syntax.DID { 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/json" "errors" "fmt" + "regexp" "slices" + "strings" "time" "github.com/bluesky-social/indigo/api/atproto" @@ -120,6 +122,167 @@ } } +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/pull.go b/appview/models/pull.go --- a/appview/models/pull.go +++ b/appview/models/pull.go @@ -11,6 +11,7 @@ "time" "tangled.org/core/api/tangled" + "tangled.org/core/appview/pages/markup/sanitizer" "tangled.org/core/patchutil" "tangled.org/core/types" @@ -122,6 +123,39 @@ Source: p.PullSource.AsRecord(), DependentOn: dependentOn, } +} + +func (pull *Pull) Validate() error { + if len(pull.Submissions) == 0 { + return fmt.Errorf("pull must have at least one submission") + } + + latestSubmission := pull.LatestSubmission() + if latestSubmission == nil { + return fmt.Errorf("pull must have a valid latest submission") + } + + isFormatPatch := patchutil.IsFormatPatch(latestSubmission.Patch) + + // title and body can only be empty if the patch is a format-patch + if !isFormatPatch { + if pull.Title == "" { + return fmt.Errorf("pull title is empty (required for non-format-patch pulls)") + } + + if pull.Body == "" { + return fmt.Errorf("pull body is empty (required for non-format-patch pulls)") + } + + if st := strings.TrimSpace(sanitizer.SanitizeDescription(pull.Title)); st == "" { + return fmt.Errorf("title is empty after HTML sanitization") + } + + if sb := strings.TrimSpace(sanitizer.SanitizeDefault(pull.Body)); sb == "" { + return fmt.Errorf("body is empty after HTML sanitization") + } + } + return nil } func PullFromRecord(did, rkey string, record tangled.RepoPull, blobs []*io.ReadCloser) (*Pull, error) { 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 @@ 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 @@ 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 @@ -34,6 +34,7 @@ "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" "tangled.org/core/idresolver" ) @@ -313,7 +314,7 @@ rctx := p.rctx.Clone() rctx.RendererType = markup.RendererTypeDefault htmlString := rctx.RenderMarkdown(text) - sanitized := rctx.SanitizeDefault(htmlString) + sanitized := sanitizer.SanitizeDefault(htmlString) return template.HTML(sanitized) }, "description": func(text string) template.HTML { @@ -324,14 +325,14 @@ emoji.Emoji, ), )) - sanitized := rctx.SanitizeDescription(htmlString) + sanitized := sanitizer.SanitizeDescription(htmlString) return template.HTML(sanitized) }, "readme": func(text string) template.HTML { rctx := p.rctx.Clone() rctx.RendererType = markup.RendererTypeRepoMarkdown htmlString := rctx.RenderMarkdown(text) - sanitized := rctx.SanitizeDefault(htmlString) + sanitized := sanitizer.SanitizeDefault(htmlString) return template.HTML(sanitized) }, "code": func(content, path string) string { diff --git a/appview/pages/pages.go b/appview/pages/pages.go --- a/appview/pages/pages.go +++ b/appview/pages/pages.go @@ -25,6 +25,7 @@ "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" @@ -90,7 +91,6 @@ Hostname: config.Core.AppviewHost, CamoUrl: config.Camo.Host, CamoSecret: config.Camo.SharedSecret, - Sanitizer: markup.NewSanitizer(), Files: Files, } @@ -346,7 +346,7 @@ rctx := p.rctx.Clone() rctx.RendererType = markup.RendererTypeDefault htmlString := rctx.RenderMarkdown(string(markdownBytes)) - sanitized := rctx.SanitizeDefault(htmlString) + sanitized := sanitizer.SanitizeDefault(htmlString) params.Content = template.HTML(sanitized) return p.execute("legal/terms", w, params) @@ -375,7 +375,7 @@ rctx := p.rctx.Clone() rctx.RendererType = markup.RendererTypeDefault htmlString := rctx.RenderMarkdown(string(markdownBytes)) - sanitized := rctx.SanitizeDefault(htmlString) + sanitized := sanitizer.SanitizeDefault(htmlString) params.Content = template.HTML(sanitized) return p.execute("legal/privacy", w, params) @@ -920,7 +920,7 @@ case markup.FormatMarkdown: params.Raw = false htmlString := rctx.RenderMarkdown(params.Readme) - sanitized := rctx.SanitizeDefault(htmlString) + sanitized := sanitizer.SanitizeDefault(htmlString) params.HTMLReadme = template.HTML(sanitized) default: params.Raw = true @@ -1039,7 +1039,7 @@ case markup.FormatMarkdown: params.Raw = false htmlString := rctx.RenderMarkdown(params.Readme) - sanitized := rctx.SanitizeDefault(htmlString) + sanitized := sanitizer.SanitizeDefault(htmlString) params.HTMLReadme = template.HTML(sanitized) default: params.Raw = true diff --git a/appview/pipelines/logs.go b/appview/pipelines/logs.go --- a/appview/pipelines/logs.go +++ b/appview/pipelines/logs.go @@ -8,7 +8,7 @@ terminal "github.com/buildkite/terminal-to-html/v3" "github.com/gorilla/websocket" - "tangled.org/core/appview/pages/markup" + "tangled.org/core/appview/pages/markup/sanitizer" "tangled.org/core/hostutil" ) @@ -20,14 +20,12 @@ // // the stack contents are prepended to each new line so colours carry over. type ansiState struct { - stack []string - sanitizer markup.Sanitizer + stack []string } func NewAnsiState() *ansiState { return &ansiState{ - stack: []string{}, - sanitizer: markup.NewSanitizer(), + stack: []string{}, } } @@ -37,7 +35,7 @@ // render current line with the existing prefix rendered := terminal.Render([]byte(prefix + line)) // sanitize - sanitized := a.sanitizer.SanitizeLogs(rendered) + sanitized := sanitizer.SanitizeLogs(rendered) // update the stack with sequences from current line for _, m := range sequenceRe.FindAllStringSubmatch(line, -1) { diff --git a/appview/pulls/compose.go b/appview/pulls/compose.go --- a/appview/pulls/compose.go +++ b/appview/pulls/compose.go @@ -17,7 +17,7 @@ "tangled.org/core/appview/models" "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/xrpcclient" "tangled.org/core/patchutil" "tangled.org/core/types" @@ -78,7 +78,6 @@ 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 @@ -426,7 +425,7 @@ if strings.TrimSpace(patch) == "" { return nil, nil, nil } - if verr := s.validator.ValidatePatch(&patch); verr != nil { + if verr := validatePatch(&patch); verr != nil { return nil, nil, fmt.Errorf("invalid patch: paste a valid git diff or format-patch") } comparison = parsePastedPatch(patch) diff --git a/appview/pulls/compose_helpers_test.go b/appview/pulls/compose_helpers_test.go --- a/appview/pulls/compose_helpers_test.go +++ b/appview/pulls/compose_helpers_test.go @@ -12,7 +12,6 @@ "tangled.org/core/appview/models" "tangled.org/core/appview/pages" "tangled.org/core/appview/pages/repoinfo" - "tangled.org/core/appview/validator" "tangled.org/core/patchutil" "tangled.org/core/types" ) @@ -371,8 +370,7 @@ func TestPrefetchComparisonPatch(t *testing.T) { s := &Pulls{ - validator: &validator.Validator{}, - logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + logger: slog.New(slog.NewTextHandler(io.Discard, nil)), } cases := []struct { @@ -408,8 +406,7 @@ func TestPrefetchComparisonValidPatch(t *testing.T) { s := &Pulls{ - validator: &validator.Validator{}, - logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + logger: slog.New(slog.NewTextHandler(io.Discard, nil)), } patch := `diff --git a/a.txt b/a.txt index 0000000..1111111 100644 @@ -435,8 +432,7 @@ func TestPrefetchComparisonMissingInputs(t *testing.T) { s := &Pulls{ - validator: &validator.Validator{}, - logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + logger: slog.New(slog.NewTextHandler(io.Discard, nil)), } cases := []struct { diff --git a/appview/pulls/create.go b/appview/pulls/create.go --- a/appview/pulls/create.go +++ b/appview/pulls/create.go @@ -71,7 +71,7 @@ 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 @@ -85,7 +85,7 @@ } func (s *Pulls) handlePatchBasedPull(w http.ResponseWriter, r *http.Request, repo *models.Repo, userDid syntax.DID, title, body, targetBranch, patch string, isStacked bool, stackTitles, stackBodies map[string]string) { - 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 @@ -178,7 +178,7 @@ 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 diff --git a/appview/pulls/labels.go b/appview/pulls/labels.go --- a/appview/pulls/labels.go +++ b/appview/pulls/labels.go @@ -140,7 +140,33 @@ valid := make([]models.LabelOp, 0, len(raw)) for _, op := range raw { def := defs[op.OperandKey] - if err := s.validator.ValidateLabelOp(ctx, def, repo, &op); err != nil { + + // validate permissions: only collaborators can apply labels currently + // + // TODO: introduce a repo:triage permission + ok, err := s.acl.HasRepoPermissionErr(ctx, repo, op.Did, "repo:push") + if err != nil { + l.Warn("invalid label op", "err", err, "subject", op.Subject, "key", op.OperandKey) + continue + } + if !ok { + l.Warn("forbidden label op", "subject", op.Subject, "key", op.OperandKey) + continue + } + + // resolve Handle to DID + if def.ValueType.IsString() && def.ValueType.IsDidFormat() { + val := syntax.AtIdentifier(op.OperandValue) + if val.IsHandle() { + ident, err := s.idResolver.Directory().Lookup(ctx, val) + if err != nil { + l.Warn("failed to resolve handle", "err", err, "subject", op.Subject, "key", op.OperandKey) + } + op.OperandValue = ident.DID.String() + } + } + + if err := def.ValidateOperandValue(&op); err != nil { l.Warn("invalid label op", "err", err, "subject", op.Subject, "key", op.OperandKey) continue } diff --git a/appview/pulls/pulls.go b/appview/pulls/pulls.go --- a/appview/pulls/pulls.go +++ b/appview/pulls/pulls.go @@ -6,6 +6,7 @@ "fmt" "io" "log/slog" + "strings" "tangled.org/core/appview/config" "tangled.org/core/appview/db" @@ -17,9 +18,9 @@ "tangled.org/core/appview/oauth" "tangled.org/core/appview/pages" "tangled.org/core/appview/reporesolver" - "tangled.org/core/appview/validator" "tangled.org/core/idresolver" "tangled.org/core/ogre" + "tangled.org/core/patchutil" indigoxrpc "github.com/bluesky-social/indigo/xrpc" ) @@ -37,7 +38,6 @@ notifier notify.Notifier acl *knotacl.Service logger *slog.Logger - validator *validator.Validator indexer *pulls_indexer.Indexer ogreClient *ogre.Client } @@ -52,7 +52,6 @@ config *config.Config, notifier notify.Notifier, acl *knotacl.Service, - validator *validator.Validator, indexer *pulls_indexer.Indexer, logger *slog.Logger, ) *Pulls { @@ -67,7 +66,6 @@ notifier: notifier, acl: acl, logger: logger, - validator: validator, indexer: indexer, ogreClient: ogre.NewClient(config.Ogre.Host), } @@ -90,3 +88,20 @@ } 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/pulls/resubmit.go b/appview/pulls/resubmit.go --- a/appview/pulls/resubmit.go +++ b/appview/pulls/resubmit.go @@ -274,7 +274,7 @@ return } - if err := s.validator.ValidatePatch(&patch); err != nil { + if err := validatePatch(&patch); err != nil { s.pages.Notice(w, "resubmit-error", err.Error()) return } diff --git a/appview/repo/repo.go b/appview/repo/repo.go --- a/appview/repo/repo.go +++ b/appview/repo/repo.go @@ -27,7 +27,6 @@ "tangled.org/core/appview/pagination" "tangled.org/core/appview/reporesolver" "tangled.org/core/appview/sites" - "tangled.org/core/appview/validator" xrpcclient "tangled.org/core/appview/xrpcclient" "tangled.org/core/consts" "tangled.org/core/eventconsumer" @@ -59,7 +58,6 @@ notifier notify.Notifier logger *slog.Logger serviceAuth *serviceauth.ServiceAuth - validator *validator.Validator cfClient *cloudflare.Client ogreClient *ogre.Client codesearch *codesearch.CodeSearch @@ -77,7 +75,6 @@ enforcer *rbac.Enforcer, acl *knotacl.Service, logger *slog.Logger, - validator *validator.Validator, cfClient *cloudflare.Client, codesearch *codesearch.CodeSearch, ) *Repo { @@ -93,7 +90,6 @@ enforcer: enforcer, acl: acl, logger: logger, - validator: validator, cfClient: cfClient, ogreClient: ogre.NewClient(config.Ogre.Host), codesearch: codesearch, @@ -255,7 +251,7 @@ 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 @@ "encoding/json" "fmt" "net/http" + "net/url" "path" + "regexp" "slices" "strings" "time" @@ -21,6 +23,7 @@ xrpcclient "tangled.org/core/appview/xrpcclient" "tangled.org/core/consts" "tangled.org/core/orm" + "tangled.org/core/sets" "tangled.org/core/types" comatproto "github.com/bluesky-social/indigo/api/atproto" @@ -552,14 +555,15 @@ 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()) @@ -618,4 +622,60 @@ } 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 @@ -372,7 +372,6 @@ s.db, s.config, s.notifier, - s.validator, s.indexer.Issues, log.SubLogger(s.logger, "issues"), ) @@ -390,7 +389,6 @@ s.config, s.notifier, s.aclService, - s.validator, s.indexer.Pulls, log.SubLogger(s.logger, "pulls"), ) @@ -410,7 +408,6 @@ s.enforcer, s.aclService, log.SubLogger(s.logger, "repo"), - s.validator, s.cfClient, s.codesearch, ) @@ -438,8 +435,8 @@ s.oauth, s.pages, s.db, - s.validator, - s.enforcer, + s.idResolver.Directory(), + s.aclService, 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 @@ -35,7 +35,6 @@ pipelinessh "tangled.org/core/appview/pipelines/ssh" "tangled.org/core/appview/reporesolver" "tangled.org/core/appview/repoverify" - "tangled.org/core/appview/validator" xrpcclient "tangled.org/core/appview/xrpcclient" "tangled.org/core/consts" "tangled.org/core/eventconsumer" @@ -75,7 +74,6 @@ spindlestream *eventconsumer.Consumer pipelineNotifier *pipelines.StatusNotifier logger *slog.Logger - validator *validator.Validator cfClient *cloudflare.Client codesearch *codesearch.CodeSearch } @@ -122,9 +120,6 @@ if err != nil { return nil, fmt.Errorf("failed to start oauth handler: %w", err) } - - validator := validator.New(d, res, aclService) - repoResolver := reporesolver.New(config, aclService, d, rdb) mentionsResolver := mentions.New(config, res, d, log.SubLogger(logger, "mentionsResolver")) @@ -196,7 +191,6 @@ Cache: rdb, Config: config, Logger: log.SubLogger(logger, "ingester"), - Validator: validator, MentionsResolver: mentionsResolver, Notifier: notifier, Verifier: repoverify.New(res, config.Core.Dev), @@ -250,7 +244,6 @@ spindlestream: spindlestream, pipelineNotifier: pipelineNotifier, logger: logger, - validator: validator, cfClient: cfClient, codesearch: &codesearch.CodeSearch{Host: config.CodeSearch.ZoektUrl}, } diff --git a/appview/validator/issue.go b/appview/validator/issue.go deleted file mode 100644 --- a/appview/validator/issue.go +++ /dev/null @@ -1,28 +0,0 @@ -package validator - -import ( - "fmt" - "strings" - - "tangled.org/core/appview/models" -) - -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(ctx context.Context, 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") - } - - 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") - } - - // validate permissions: only collaborators can apply labels currently - // - // TODO: introduce a repo:triage permission - ok, err := v.acl.HasRepoPermissionErr(ctx, repo, labelOp.Did, "repo:push") - if err != nil { - return err - } - if !ok { - return fmt.Errorf("unauthorized label operation") - } - - 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/label_test.go b/appview/validator/label_test.go deleted file mode 100644 --- a/appview/validator/label_test.go +++ /dev/null @@ -1,87 +0,0 @@ -package validator - -import ( - "context" - "encoding/json" - "errors" - "io" - "log/slog" - "net/http" - "net/http/httptest" - "path/filepath" - "strings" - "testing" - "time" - - "tangled.org/core/api/tangled" - "tangled.org/core/appview/db" - "tangled.org/core/appview/knotacl" - "tangled.org/core/appview/models" - "tangled.org/core/consts" - "tangled.org/core/rbac" -) - -func unreachableListValidator(t *testing.T) (*Validator, string) { - t.Helper() - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - switch { - case strings.HasSuffix(r.URL.Path, tangled.KnotVersionNSID): - json.NewEncoder(w).Encode(tangled.KnotVersion_Output{Version: "v1.15.0", Capabilities: []string{string(consts.CapKnotACL)}}) - case strings.HasSuffix(r.URL.Path, tangled.RepoListCollaboratorsNSID): - http.Error(w, "list down", http.StatusInternalServerError) - default: - http.NotFound(w, r) - } - })) - t.Cleanup(srv.Close) - host := strings.TrimPrefix(srv.URL, "http://") - - dir := t.TempDir() - enforcer, err := rbac.NewEnforcer(filepath.Join(dir, "rbac.db")) - if err != nil { - t.Fatalf("NewEnforcer: %v", err) - } - d, err := db.Make(context.Background(), filepath.Join(dir, "appview.db")) - if err != nil { - t.Fatalf("db.Make: %v", err) - } - svc := knotacl.NewService(enforcer, d, true, slog.New(slog.NewTextHandler(io.Discard, nil))) - return &Validator{acl: svc}, host -} - -func TestValidateLabelOp_MalformedRejectedBeforePermCheck(t *testing.T) { - v, host := unreachableListValidator(t) - def := &models.LabelDefinition{Did: "did:plc:akshay", Rkey: "deadbeef"} - repo := &models.Repo{Did: "did:plc:akshay", Knot: host, RepoDid: "did:plc:limpet"} - - op := &models.LabelOp{ - Did: "did:plc:scallop", - OperandKey: "does-not-match-the-def-aturi", - Operation: "garbage", - } - err := v.ValidateLabelOp(context.Background(), def, repo, op) - if errors.Is(err, knotacl.ErrKnotUnreachable) { - t.Fatalf("malformed op returned ErrKnotUnreachable; structural validation did not run before the perm check") - } - if err == nil || !strings.Contains(err.Error(), "operand key") { - t.Fatalf("want a structural operand-key error, got %v", err) - } -} - -func TestValidateLabelOp_WellFormedFailsOpenWhenKnotUnreachable(t *testing.T) { - v, host := unreachableListValidator(t) - def := &models.LabelDefinition{Did: "did:plc:akshay", Rkey: "deadbeef"} - repo := &models.Repo{Did: "did:plc:akshay", Knot: host, RepoDid: "did:plc:limpet"} - - op := &models.LabelOp{ - Did: "did:plc:scallop", - OperandKey: def.AtUri().String(), - Operation: models.LabelOperationAdd, - Subject: "at://did:plc:limpet/sh.tangled.repo.issue/abc123", - PerformedAt: time.Now(), - } - err := v.ValidateLabelOp(context.Background(), def, repo, op) - if !errors.Is(err, knotacl.ErrKnotUnreachable) { - t.Fatalf("well-formed op against an unreachable knot = %v, want ErrKnotUnreachable so the ingester fails open", err) - } -} 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/pull.go b/appview/validator/pull.go deleted file mode 100644 --- a/appview/validator/pull.go +++ /dev/null @@ -1,68 +0,0 @@ -package validator - -import ( - "database/sql" - "fmt" - "strings" - - "tangled.org/core/appview/db" - "tangled.org/core/appview/models" - "tangled.org/core/orm" - "tangled.org/core/patchutil" -) - -func (v *Validator) ValidatePull(pull *models.Pull) error { - if len(pull.Submissions) == 0 { - return fmt.Errorf("pull must have at least one submission") - } - - latestSubmission := pull.LatestSubmission() - if latestSubmission == nil { - return fmt.Errorf("pull must have a valid latest submission") - } - - isFormatPatch := patchutil.IsFormatPatch(latestSubmission.Patch) - - // title and body can only be empty if the patch is a format-patch - if !isFormatPatch { - if pull.Title == "" { - return fmt.Errorf("pull title is empty (required for non-format-patch pulls)") - } - - if pull.Body == "" { - return fmt.Errorf("pull body is empty (required for non-format-patch pulls)") - } - - if st := strings.TrimSpace(v.sanitizer.SanitizeDescription(pull.Title)); st == "" { - return fmt.Errorf("title is empty after HTML sanitization") - } - - if sb := strings.TrimSpace(v.sanitizer.SanitizeDefault(pull.Body)); sb == "" { - return fmt.Errorf("body is empty after HTML sanitization") - } - } - - // the dependent_on should not form a DAG, aka, two PRs should not have the same dependent - if pull.DependentOn != nil { - dependentPull, err := db.GetPull( - v.db, - orm.FilterEq("dependent_on", pull.DependentOn.String()), - ) - - if err == sql.ErrNoRows { - return nil - } - - if err != nil { - return fmt.Errorf("failed to fetch pulls with same dependency: %w", err) - } - - if dependentPull.AtUri() == pull.AtUri() { - return nil - } - - return fmt.Errorf("another pull already depends on %s, which would form a DAG, this is presently disallowed", pull.DependentOn.String()) - } - - 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/knotacl" - "tangled.org/core/appview/pages/markup" - "tangled.org/core/idresolver" -) - -type Validator struct { - db *db.DB - sanitizer markup.Sanitizer - resolver *idresolver.Resolver - acl *knotacl.Service -} - -func New(db *db.DB, res *idresolver.Resolver, acl *knotacl.Service) *Validator { - return &Validator{ - db: db, - sanitizer: markup.NewSanitizer(), - resolver: res, - acl: acl, - } -} 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 @@ -48,7 +48,6 @@ IsDev bool Hostname string RendererType RendererType - Sanitizer Sanitizer Files fs.FS } @@ -203,14 +202,6 @@ } 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.go deleted file mode 100644 --- a/appview/pages/markup/sanitizer.go +++ /dev/null @@ -1,176 +0,0 @@ -package markup - -import ( - "maps" - "regexp" - "slices" - "strings" - - "github.com/alecthomas/chroma/v2" - "github.com/microcosm-cc/bluemonday" -) - -// shared policies built once at init; safe for concurrent use per bluemonday docs -var ( - sharedDefaultPolicy *bluemonday.Policy - sharedDescriptionPolicy *bluemonday.Policy - sharedLogsPolicy *bluemonday.Policy -) - -func init() { - sharedDefaultPolicy = buildDefaultPolicy() - sharedDescriptionPolicy = buildDescriptionPolicy() - sharedLogsPolicy = buildLogsPolicy() -} - -type Sanitizer struct { - defaultPolicy *bluemonday.Policy - descriptionPolicy *bluemonday.Policy - logsPolicy *bluemonday.Policy -} - -func NewSanitizer() Sanitizer { - return Sanitizer{ - defaultPolicy: sharedDefaultPolicy, - descriptionPolicy: sharedDescriptionPolicy, - logsPolicy: sharedLogsPolicy, - } -} - -func (s *Sanitizer) SanitizeDefault(html string) string { - return s.defaultPolicy.Sanitize(html) -} -func (s *Sanitizer) SanitizeDescription(html string) string { - return s.descriptionPolicy.Sanitize(html) -} -func (s *Sanitizer) SanitizeLogs(html string) string { - return s.logsPolicy.Sanitize(html) -} - -func buildDefaultPolicy() *bluemonday.Policy { - policy := bluemonday.UGCPolicy() - - // Allow generally safe attributes - generalSafeAttrs := []string{ - "abbr", "accept", "accept-charset", - "accesskey", "action", "align", "alt", - "aria-describedby", "aria-hidden", "aria-label", "aria-labelledby", - "axis", "border", "cellpadding", "cellspacing", "char", - "charoff", "charset", "checked", - "clear", "cols", "colspan", "color", - "compact", "coords", "datetime", "dir", - "disabled", "enctype", "for", "frame", - "headers", "height", "hreflang", - "hspace", "ismap", "label", "lang", - "maxlength", "media", "method", - "multiple", "name", "nohref", "noshade", - "nowrap", "open", "prompt", "readonly", "rel", "rev", - "rows", "rowspan", "rules", "scope", - "selected", "shape", "size", "span", - "start", "summary", "tabindex", "target", - "title", "type", "usemap", "valign", "value", - "vspace", "width", "itemprop", - } - - generalSafeElements := []string{ - "h1", "h2", "h3", "h4", "h5", "h6", "h7", "h8", "br", "b", "i", "strong", "em", "a", "pre", "code", "img", "tt", - "div", "ins", "del", "sup", "sub", "p", "ol", "ul", "table", "thead", "tbody", "tfoot", "blockquote", "label", - "dl", "dt", "dd", "kbd", "q", "samp", "var", "hr", "ruby", "rt", "rp", "li", "tr", "td", "th", "s", "strike", "summary", - "details", "caption", "figure", "figcaption", - "abbr", "bdo", "cite", "dfn", "mark", "small", "span", "time", "video", "wbr", - "picture", "source", - } - - policy.AllowAttrs(generalSafeAttrs...).OnElements(generalSafeElements...) - - // video - policy.AllowAttrs("src", "autoplay", "controls").OnElements("video") - - // picture/source for modern image formats (avif, webp, etc.) - policy.AllowAttrs("srcset", "type", "media").OnElements("source") - - // checkboxes - policy.AllowAttrs("type").Matching(regexp.MustCompile(`^checkbox$`)).OnElements("input") - policy.AllowAttrs("checked", "disabled", "data-source-position").OnElements("input") - - // for code blocks - policy.AllowAttrs("class").Matching(regexp.MustCompile(`chroma|mermaid`)).OnElements("pre") - policy.AllowAttrs("class").Matching(regexp.MustCompile(`anchor|footnote-ref|footnote-backref`)).OnElements("a") - policy.AllowAttrs("class").Matching(regexp.MustCompile(`heading`)).OnElements("h1", "h2", "h3", "h4", "h5", "h6", "h7", "h8") - policy.AllowAttrs("class").Matching(regexp.MustCompile(strings.Join(slices.Collect(maps.Values(chroma.StandardTypes)), "|"))).OnElements("span") - - // at-mentions - policy.AllowAttrs("class").Matching(regexp.MustCompile(`mention`)).OnElements("a") - - // centering content - policy.AllowElements("center") - - policy.AllowAttrs("align", "style", "width", "height").Globally() - policy.AllowStyles( - "margin", - "padding", - "text-align", - "font-weight", - "text-decoration", - "padding-left", - "padding-right", - "padding-top", - "padding-bottom", - "margin-left", - "margin-right", - "margin-top", - "margin-bottom", - ) - - // math - mathAttrs := []string{ - "accent", "columnalign", "columnlines", "columnspan", "dir", "display", - "displaystyle", "encoding", "fence", "form", "largeop", "linebreak", - "linethickness", "lspace", "mathcolor", "mathsize", "mathvariant", "minsize", - "movablelimits", "notation", "rowalign", "rspace", "rowspacing", "rowspan", - "scriptlevel", "stretchy", "symmetric", "title", "voffset", "width", - } - mathElements := []string{ - "annotation", "math", "menclose", "merror", "mfrac", "mi", "mmultiscripts", - "mn", "mo", "mover", "mpadded", "mprescripts", "mroot", "mrow", "mspace", - "msqrt", "mstyle", "msub", "msubsup", "msup", "mtable", "mtd", "mtext", - "mtr", "munder", "munderover", "semantics", - } - policy.AllowNoAttrs().OnElements(mathElements...) - policy.AllowAttrs(mathAttrs...).OnElements(mathElements...) - - // goldmark-callout - policy.AllowAttrs("data-callout").OnElements("details") - - return policy -} - -func buildDescriptionPolicy() *bluemonday.Policy { - policy := bluemonday.NewPolicy() - policy.AllowStandardURLs() - - // allow italics and bold. - policy.AllowElements("i", "b", "em", "strong") - - // allow code. - policy.AllowElements("code") - - // allow links - policy.AllowAttrs("href", "target", "rel").OnElements("a") - - return policy -} - -func buildLogsPolicy() *bluemonday.Policy { - policy := bluemonday.NewPolicy() - - policy.AllowElements("p", "span") - - // allow italics and bold - policy.AllowElements("i", "b", "em", "strong") - - // allow fg/bg classes from terminal-to-html - policy.AllowAttrs("class").Matching(regexp.MustCompile(`term-*`)).OnElements("span") - - return policy -} diff --git a/appview/pages/markup/sanitizer/sanitizer.go b/appview/pages/markup/sanitizer/sanitizer.go new file mode 100644 --- /dev/null +++ b/appview/pages/markup/sanitizer/sanitizer.go @@ -0,0 +1,161 @@ +package sanitizer + +import ( + "maps" + "regexp" + "slices" + "strings" + + "github.com/alecthomas/chroma/v2" + "github.com/microcosm-cc/bluemonday" +) + +// shared policies built once at init; safe for concurrent use per bluemonday docs +var ( + sharedDefaultPolicy *bluemonday.Policy + sharedDescriptionPolicy *bluemonday.Policy + sharedLogsPolicy *bluemonday.Policy +) + +func init() { + sharedDefaultPolicy = buildDefaultPolicy() + sharedDescriptionPolicy = buildDescriptionPolicy() + sharedLogsPolicy = buildLogsPolicy() +} + +func SanitizeDefault(html string) string { + return sharedDefaultPolicy.Sanitize(html) +} +func SanitizeDescription(html string) string { + return sharedDescriptionPolicy.Sanitize(html) +} +func SanitizeLogs(html string) string { + return sharedLogsPolicy.Sanitize(html) +} + +func buildDefaultPolicy() *bluemonday.Policy { + policy := bluemonday.UGCPolicy() + + // Allow generally safe attributes + generalSafeAttrs := []string{ + "abbr", "accept", "accept-charset", + "accesskey", "action", "align", "alt", + "aria-describedby", "aria-hidden", "aria-label", "aria-labelledby", + "axis", "border", "cellpadding", "cellspacing", "char", + "charoff", "charset", "checked", + "clear", "cols", "colspan", "color", + "compact", "coords", "datetime", "dir", + "disabled", "enctype", "for", "frame", + "headers", "height", "hreflang", + "hspace", "ismap", "label", "lang", + "maxlength", "media", "method", + "multiple", "name", "nohref", "noshade", + "nowrap", "open", "prompt", "readonly", "rel", "rev", + "rows", "rowspan", "rules", "scope", + "selected", "shape", "size", "span", + "start", "summary", "tabindex", "target", + "title", "type", "usemap", "valign", "value", + "vspace", "width", "itemprop", + } + + generalSafeElements := []string{ + "h1", "h2", "h3", "h4", "h5", "h6", "h7", "h8", "br", "b", "i", "strong", "em", "a", "pre", "code", "img", "tt", + "div", "ins", "del", "sup", "sub", "p", "ol", "ul", "table", "thead", "tbody", "tfoot", "blockquote", "label", + "dl", "dt", "dd", "kbd", "q", "samp", "var", "hr", "ruby", "rt", "rp", "li", "tr", "td", "th", "s", "strike", "summary", + "details", "caption", "figure", "figcaption", + "abbr", "bdo", "cite", "dfn", "mark", "small", "span", "time", "video", "wbr", + } + + policy.AllowAttrs(generalSafeAttrs...).OnElements(generalSafeElements...) + + // video + policy.AllowAttrs("src", "autoplay", "controls").OnElements("video") + + // picture/source for modern image formats (avif, webp, etc.) + policy.AllowAttrs("srcset", "type", "media").OnElements("source") + + // checkboxes + policy.AllowAttrs("type").Matching(regexp.MustCompile(`^checkbox$`)).OnElements("input") + policy.AllowAttrs("checked", "disabled", "data-source-position").OnElements("input") + + // for code blocks + policy.AllowAttrs("class").Matching(regexp.MustCompile(`chroma|mermaid`)).OnElements("pre") + policy.AllowAttrs("class").Matching(regexp.MustCompile(`anchor|footnote-ref|footnote-backref`)).OnElements("a") + policy.AllowAttrs("class").Matching(regexp.MustCompile(`heading`)).OnElements("h1", "h2", "h3", "h4", "h5", "h6", "h7", "h8") + policy.AllowAttrs("class").Matching(regexp.MustCompile(strings.Join(slices.Collect(maps.Values(chroma.StandardTypes)), "|"))).OnElements("span") + + // at-mentions + policy.AllowAttrs("class").Matching(regexp.MustCompile(`mention`)).OnElements("a") + + // centering content + policy.AllowElements("center") + + policy.AllowAttrs("align", "style", "width", "height").Globally() + policy.AllowStyles( + "margin", + "padding", + "text-align", + "font-weight", + "text-decoration", + "padding-left", + "padding-right", + "padding-top", + "padding-bottom", + "margin-left", + "margin-right", + "margin-top", + "margin-bottom", + ) + + // math + mathAttrs := []string{ + "accent", "columnalign", "columnlines", "columnspan", "dir", "display", + "displaystyle", "encoding", "fence", "form", "largeop", "linebreak", + "linethickness", "lspace", "mathcolor", "mathsize", "mathvariant", "minsize", + "movablelimits", "notation", "rowalign", "rspace", "rowspacing", "rowspan", + "scriptlevel", "stretchy", "symmetric", "title", "voffset", "width", + } + mathElements := []string{ + "annotation", "math", "menclose", "merror", "mfrac", "mi", "mmultiscripts", + "mn", "mo", "mover", "mpadded", "mprescripts", "mroot", "mrow", "mspace", + "msqrt", "mstyle", "msub", "msubsup", "msup", "mtable", "mtd", "mtext", + "mtr", "munder", "munderover", "semantics", + } + policy.AllowNoAttrs().OnElements(mathElements...) + policy.AllowAttrs(mathAttrs...).OnElements(mathElements...) + + // goldmark-callout + policy.AllowAttrs("data-callout").OnElements("details") + + return policy +} + +func buildDescriptionPolicy() *bluemonday.Policy { + policy := bluemonday.NewPolicy() + policy.AllowStandardURLs() + + // allow italics and bold. + policy.AllowElements("i", "b", "em", "strong") + + // allow code. + policy.AllowElements("code") + + // allow links + policy.AllowAttrs("href", "target", "rel").OnElements("a") + + return policy +} + +func buildLogsPolicy() *bluemonday.Policy { + policy := bluemonday.NewPolicy() + + policy.AllowElements("p", "span") + + // allow italics and bold + policy.AllowElements("i", "b", "em", "strong") + + // allow fg/bg classes from terminal-to-html + policy.AllowAttrs("class").Matching(regexp.MustCompile(`term-*`)).OnElements("span") + + return policy +} -- tangled.sh