From 8daa4dae3435e45b4367c9d59bfa27a063fba2d4 Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Wed, 8 Jan 2025 22:38:27 +0100 Subject: [bugfix] More permissive CSV parsing for perm subs, text parse fix (#3638) * [bugfix] More permissive CSV parsing for perm subs, text parse fix * wee * change the way dry works, slightly * me oh my, i'm just a little guy * we're just normal men --- .../admin/domainpermissionsubscriptiontest_test.go | 204 +++++++++++++++++++++ .../admin/domainpermissionsubscruptiontest_test.go | 125 ------------- .../admin/domainpermissionsubscription.go | 8 +- internal/subscriptions/domainperms.go | 144 ++++++++++----- internal/subscriptions/subscriptions_test.go | 2 +- internal/typeutils/internaltofrontend.go | 2 +- 6 files changed, 308 insertions(+), 177 deletions(-) create mode 100644 internal/api/client/admin/domainpermissionsubscriptiontest_test.go delete mode 100644 internal/api/client/admin/domainpermissionsubscruptiontest_test.go (limited to 'internal') diff --git a/internal/api/client/admin/domainpermissionsubscriptiontest_test.go b/internal/api/client/admin/domainpermissionsubscriptiontest_test.go new file mode 100644 index 000000000..c03b950a9 --- /dev/null +++ b/internal/api/client/admin/domainpermissionsubscriptiontest_test.go @@ -0,0 +1,204 @@ +// GoToSocial +// Copyright (C) GoToSocial Authors admin@gotosocial.org +// SPDX-License-Identifier: AGPL-3.0-or-later +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package admin_test + +import ( + "bytes" + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/suite" + "github.com/superseriousbusiness/gotosocial/internal/api/client/admin" + apiutil "github.com/superseriousbusiness/gotosocial/internal/api/util" + "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" + "github.com/superseriousbusiness/gotosocial/internal/util" +) + +type DomainPermissionSubscriptionTestTestSuite struct { + AdminStandardTestSuite +} + +func (suite *DomainPermissionSubscriptionTestTestSuite) TestDomainPermissionSubscriptionTestCSV() { + var ( + ctx = context.Background() + testAccount = suite.testAccounts["admin_account"] + permSub = >smodel.DomainPermissionSubscription{ + ID: "01JGE681TQSBPAV59GZXPKE62H", + Priority: 255, + Title: "whatever!", + PermissionType: gtsmodel.DomainPermissionBlock, + AsDraft: util.Ptr(false), + AdoptOrphans: util.Ptr(true), + CreatedByAccountID: testAccount.ID, + CreatedByAccount: testAccount, + URI: "https://lists.example.org/baddies.csv", + ContentType: gtsmodel.DomainPermSubContentTypeCSV, + } + ) + + // Create a subscription for a CSV list of baddies. + err := suite.state.DB.PutDomainPermissionSubscription(ctx, permSub) + if err != nil { + suite.FailNow(err.Error()) + } + + // Prepare the request to the /test endpoint. + subPath := strings.ReplaceAll( + admin.DomainPermissionSubscriptionTestPath, + ":id", permSub.ID, + ) + path := "/api" + subPath + recorder := httptest.NewRecorder() + ginCtx := suite.newContext(recorder, http.MethodPost, nil, path, "application/json") + ginCtx.Params = gin.Params{ + gin.Param{ + Key: apiutil.IDKey, + Value: permSub.ID, + }, + } + + // Trigger the handler. + suite.adminModule.DomainPermissionSubscriptionTestPOSTHandler(ginCtx) + suite.Equal(http.StatusOK, recorder.Code) + + // Read the body back. + b, err := io.ReadAll(recorder.Body) + if err != nil { + suite.FailNow(err.Error()) + } + + dst := new(bytes.Buffer) + if err := json.Indent(dst, b, "", " "); err != nil { + suite.FailNow(err.Error()) + } + + // Ensure expected. + suite.Equal(`[ + { + "domain": "bumfaces.net", + "public_comment": "big jerks" + }, + { + "domain": "peepee.poopoo", + "public_comment": "harassment" + }, + { + "domain": "nothanks.com" + } +]`, dst.String()) + + // No permissions should be created + // since this is a dry run / test. + blocked, err := suite.state.DB.AreDomainsBlocked( + ctx, + []string{"bumfaces.net", "peepee.poopoo", "nothanks.com"}, + ) + if err != nil { + suite.FailNow(err.Error()) + } + suite.False(blocked) +} + +func (suite *DomainPermissionSubscriptionTestTestSuite) TestDomainPermissionSubscriptionTestText() { + var ( + ctx = context.Background() + testAccount = suite.testAccounts["admin_account"] + permSub = >smodel.DomainPermissionSubscription{ + ID: "01JGE681TQSBPAV59GZXPKE62H", + Priority: 255, + Title: "whatever!", + PermissionType: gtsmodel.DomainPermissionBlock, + AsDraft: util.Ptr(false), + AdoptOrphans: util.Ptr(true), + CreatedByAccountID: testAccount.ID, + CreatedByAccount: testAccount, + URI: "https://lists.example.org/baddies.txt", + ContentType: gtsmodel.DomainPermSubContentTypePlain, + } + ) + + // Create a subscription for a plaintext list of baddies. + err := suite.state.DB.PutDomainPermissionSubscription(ctx, permSub) + if err != nil { + suite.FailNow(err.Error()) + } + + // Prepare the request to the /test endpoint. + subPath := strings.ReplaceAll( + admin.DomainPermissionSubscriptionTestPath, + ":id", permSub.ID, + ) + path := "/api" + subPath + recorder := httptest.NewRecorder() + ginCtx := suite.newContext(recorder, http.MethodPost, nil, path, "application/json") + ginCtx.Params = gin.Params{ + gin.Param{ + Key: apiutil.IDKey, + Value: permSub.ID, + }, + } + + // Trigger the handler. + suite.adminModule.DomainPermissionSubscriptionTestPOSTHandler(ginCtx) + suite.Equal(http.StatusOK, recorder.Code) + + // Read the body back. + b, err := io.ReadAll(recorder.Body) + if err != nil { + suite.FailNow(err.Error()) + } + + dst := new(bytes.Buffer) + if err := json.Indent(dst, b, "", " "); err != nil { + suite.FailNow(err.Error()) + } + + // Ensure expected. + suite.Equal(`[ + { + "domain": "bumfaces.net" + }, + { + "domain": "peepee.poopoo" + }, + { + "domain": "nothanks.com" + } +]`, dst.String()) + + // No permissions should be created + // since this is a dry run / test. + blocked, err := suite.state.DB.AreDomainsBlocked( + ctx, + []string{"bumfaces.net", "peepee.poopoo", "nothanks.com"}, + ) + if err != nil { + suite.FailNow(err.Error()) + } + suite.False(blocked) +} + +func TestDomainPermissionSubscriptionTestTestSuite(t *testing.T) { + suite.Run(t, &DomainPermissionSubscriptionTestTestSuite{}) +} diff --git a/internal/api/client/admin/domainpermissionsubscruptiontest_test.go b/internal/api/client/admin/domainpermissionsubscruptiontest_test.go deleted file mode 100644 index 46861aba1..000000000 --- a/internal/api/client/admin/domainpermissionsubscruptiontest_test.go +++ /dev/null @@ -1,125 +0,0 @@ -// GoToSocial -// Copyright (C) GoToSocial Authors admin@gotosocial.org -// SPDX-License-Identifier: AGPL-3.0-or-later -// -// This program is free software: you can redistribute it and/or modify -// it under the terms of the GNU Affero General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. -// -// This program is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU Affero General Public License for more details. -// -// You should have received a copy of the GNU Affero General Public License -// along with this program. If not, see . - -package admin_test - -import ( - "bytes" - "context" - "encoding/json" - "io" - "net/http" - "net/http/httptest" - "strings" - "testing" - - "github.com/gin-gonic/gin" - "github.com/stretchr/testify/suite" - "github.com/superseriousbusiness/gotosocial/internal/api/client/admin" - apiutil "github.com/superseriousbusiness/gotosocial/internal/api/util" - "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" - "github.com/superseriousbusiness/gotosocial/internal/util" -) - -type DomainPermissionSubscriptionTestTestSuite struct { - AdminStandardTestSuite -} - -func (suite *DomainPermissionSubscriptionTestTestSuite) TestDomainPermissionSubscriptionTest() { - var ( - ctx = context.Background() - testAccount = suite.testAccounts["admin_account"] - permSub = >smodel.DomainPermissionSubscription{ - ID: "01JGE681TQSBPAV59GZXPKE62H", - Priority: 255, - Title: "whatever!", - PermissionType: gtsmodel.DomainPermissionBlock, - AsDraft: util.Ptr(false), - AdoptOrphans: util.Ptr(true), - CreatedByAccountID: testAccount.ID, - CreatedByAccount: testAccount, - URI: "https://lists.example.org/baddies.csv", - ContentType: gtsmodel.DomainPermSubContentTypeCSV, - } - ) - - // Create a subscription for a CSV list of baddies. - err := suite.state.DB.PutDomainPermissionSubscription(ctx, permSub) - if err != nil { - suite.FailNow(err.Error()) - } - - // Prepare the request to the /test endpoint. - subPath := strings.ReplaceAll( - admin.DomainPermissionSubscriptionTestPath, - ":id", permSub.ID, - ) - path := "/api" + subPath - recorder := httptest.NewRecorder() - ginCtx := suite.newContext(recorder, http.MethodPost, nil, path, "application/json") - ginCtx.Params = gin.Params{ - gin.Param{ - Key: apiutil.IDKey, - Value: permSub.ID, - }, - } - - // Trigger the handler. - suite.adminModule.DomainPermissionSubscriptionTestPOSTHandler(ginCtx) - suite.Equal(http.StatusOK, recorder.Code) - - // Read the body back. - b, err := io.ReadAll(recorder.Body) - if err != nil { - suite.FailNow(err.Error()) - } - - dst := new(bytes.Buffer) - if err := json.Indent(dst, b, "", " "); err != nil { - suite.FailNow(err.Error()) - } - - // Ensure expected. - suite.Equal(`[ - { - "domain": "bumfaces.net", - "public_comment": "big jerks" - }, - { - "domain": "peepee.poopoo", - "public_comment": "harassment" - }, - { - "domain": "nothanks.com" - } -]`, dst.String()) - - // No permissions should be created - // since this is a dry run / test. - blocked, err := suite.state.DB.AreDomainsBlocked( - ctx, - []string{"bumfaces.net", "peepee.poopoo", "nothanks.com"}, - ) - if err != nil { - suite.FailNow(err.Error()) - } - suite.False(blocked) -} - -func TestDomainPermissionSubscriptionTestTestSuite(t *testing.T) { - suite.Run(t, &DomainPermissionSubscriptionTestTestSuite{}) -} diff --git a/internal/processing/admin/domainpermissionsubscription.go b/internal/processing/admin/domainpermissionsubscription.go index 6c051222c..bdc38df63 100644 --- a/internal/processing/admin/domainpermissionsubscription.go +++ b/internal/processing/admin/domainpermissionsubscription.go @@ -272,6 +272,12 @@ func (p *Processor) DomainPermissionSubscriptionRemove( return nil, gtserror.NewErrorNotFound(err, err.Error()) } + // Convert to API perm sub *before* doing the deletion. + apiPermSub, errWithCode := p.apiDomainPermSub(ctx, permSub) + if errWithCode != nil { + return nil, errWithCode + } + // TODO in next PR: if removeChildren, then remove all // domain permissions that are children of this domain // permission subscription. If not removeChildren, then @@ -282,7 +288,7 @@ func (p *Processor) DomainPermissionSubscriptionRemove( return nil, gtserror.NewErrorInternalError(err) } - return p.apiDomainPermSub(ctx, permSub) + return apiPermSub, nil } func (p *Processor) DomainPermissionSubscriptionTest( diff --git a/internal/subscriptions/domainperms.go b/internal/subscriptions/domainperms.go index b1e22a0be..2ecf259a4 100644 --- a/internal/subscriptions/domainperms.go +++ b/internal/subscriptions/domainperms.go @@ -345,6 +345,10 @@ func (s *Subscriptions) ProcessDomainPermissionSubscription( // processDomainPermission processes one wanted domain // permission discovered via a domain permission sub's URI. // +// If dry == true, then the returned boolean indicates whether +// the permission would actually be created. If dry == false, +// the bool indicates whether the permission was created or adopted. +// // Error will only be returned in case of an actual database // error, else the error will be logged and nil returned. func (s *Subscriptions) processDomainPermission( @@ -355,22 +359,18 @@ func (s *Subscriptions) processDomainPermission( higherPrios []*gtsmodel.DomainPermissionSubscription, dry bool, ) (bool, error) { - // Set to true if domain permission - // actually (would be) created. - var created bool - // If domain is excluded from automatic // permission creation, don't process it. domain := wantedPerm.GetDomain() excluded, err := s.state.DB.IsDomainPermissionExcluded(ctx, domain) if err != nil { // Proper db error. - return created, err + return false, err } if excluded { l.Debug("domain is excluded, skipping") - return created, nil + return false, err } // Check if a permission already exists for @@ -381,27 +381,27 @@ func (s *Subscriptions) processDomainPermission( ) if err != nil { // Proper db error. - return created, err + return false, err } if covered { l.Debug("domain is covered by a higher-priority subscription, skipping") - return created, nil + return false, err } - // At this point we know we - // should create the perm. - created = true + // True if a perm already exists. + // Note: != nil doesn't work because + // of Go interface idiosyncracies. + existing := !util.IsNil(existingPerm) if dry { - // Don't do creation or side - // effects if we're dry running. - return created, nil + // If this is a dry run, return + // now without doing any DB changes. + return !existing, nil } // Handle perm creation differently depending // on whether or not a perm already existed. - existing := !util.IsNil(existingPerm) switch { case !existing && *permSub.AsDraft: @@ -512,11 +512,10 @@ func (s *Subscriptions) processDomainPermission( if err != nil && !errors.Is(err, db.ErrAlreadyExists) { // Proper db error. - return created, err + return false, err } - created = true - return created, nil + return true, nil } func permsFromCSV( @@ -533,19 +532,60 @@ func permsFromCSV( return nil, gtserror.NewfAt(3, "error decoding csv column headers: %w", err) } - if !slices.Equal( - columnHeaders, - []string{ - "#domain", - "#severity", - "#reject_media", - "#reject_reports", - "#public_comment", - "#obfuscate", - }, - ) { + var ( + domainI *int + severityI *int + publicCommentI *int + obfuscateI *int + ) + + for i, columnHeader := range columnHeaders { + // Remove leading # if present. + normal := strings.TrimLeft(columnHeader, "#") + + // Find index of each column header we + // care about, ensuring no duplicates. + switch normal { + + case "domain": + if domainI != nil { + body.Close() + err := gtserror.NewfAt(3, "duplicate domain column header in csv: %+v", columnHeaders) + return nil, err + } + domainI = &i + + case "severity": + if severityI != nil { + body.Close() + err := gtserror.NewfAt(3, "duplicate severity column header in csv: %+v", columnHeaders) + return nil, err + } + severityI = &i + + case "public_comment": + if publicCommentI != nil { + body.Close() + err := gtserror.NewfAt(3, "duplicate public_comment column header in csv: %+v", columnHeaders) + return nil, err + } + publicCommentI = &i + + case "obfuscate": + if obfuscateI != nil { + body.Close() + err := gtserror.NewfAt(3, "duplicate obfuscate column header in csv: %+v", columnHeaders) + return nil, err + } + obfuscateI = &i + } + } + + // Ensure we have at least a domain + // index, as that's the bare minimum. + if domainI == nil { body.Close() - err := gtserror.NewfAt(3, "unexpected column headers in csv: %+v", columnHeaders) + err := gtserror.NewfAt(3, "no domain column header in csv: %+v", columnHeaders) return nil, err } @@ -576,25 +616,19 @@ func permsFromCSV( continue } - var ( - domainRaw = record[0] - severity = record[1] - publicComment = record[4] - obfuscateStr = record[5] - ) - - if severity != "suspend" { - l.Warnf("skipping non-suspend record: %+v", record) - continue - } - - obfuscate, err := strconv.ParseBool(obfuscateStr) - if err != nil { - l.Warnf("couldn't parse obfuscate field of record: %+v", record) - continue + // Skip records that specify severity + // that's not "suspend" (we don't support + // "silence" or "limit" or whatever yet). + if severityI != nil { + severity := record[*severityI] + if severity != "suspend" { + l.Warnf("skipping non-suspend record: %+v", record) + continue + } } // Normalize + validate domain. + domainRaw := record[*domainI] domain, err := validateDomain(domainRaw) if err != nil { l.Warnf("skipping invalid domain %s: %+v", domainRaw, err) @@ -611,9 +645,21 @@ func permsFromCSV( perm = >smodel.DomainAllow{Domain: domain} } - // Set remaining fields. - perm.SetPublicComment(publicComment) - perm.SetObfuscate(&obfuscate) + // Set remaining optional fields + // if they're present in the CSV. + if publicCommentI != nil { + perm.SetPublicComment(record[*publicCommentI]) + } + + if obfuscateI != nil { + obfuscate, err := strconv.ParseBool(record[*obfuscateI]) + if err != nil { + l.Warnf("couldn't parse obfuscate field of record: %+v", record) + continue + } + + perm.SetObfuscate(&obfuscate) + } // We're done. perms = append(perms, perm) diff --git a/internal/subscriptions/subscriptions_test.go b/internal/subscriptions/subscriptions_test.go index 0d3003a79..1f6e437fb 100644 --- a/internal/subscriptions/subscriptions_test.go +++ b/internal/subscriptions/subscriptions_test.go @@ -472,7 +472,7 @@ func (suite *SubscriptionsTestSuite) TestDomainBlocksWrongContentTypeCSV() { suite.Zero(count) suite.WithinDuration(time.Now(), permSub.FetchedAt, 1*time.Minute) suite.Zero(permSub.SuccessfullyFetchedAt) - suite.Equal(`ProcessDomainPermissionSubscription: unexpected column headers in csv: [bumfaces.net]`, permSub.Error) + suite.Equal(`ProcessDomainPermissionSubscription: no domain column header in csv: [bumfaces.net]`, permSub.Error) } func (suite *SubscriptionsTestSuite) TestDomainBlocksWrongContentTypePlain() { diff --git a/internal/typeutils/internaltofrontend.go b/internal/typeutils/internaltofrontend.go index 2af479125..cdc250f98 100644 --- a/internal/typeutils/internaltofrontend.go +++ b/internal/typeutils/internaltofrontend.go @@ -2115,7 +2115,7 @@ func (c *Converter) DomainPermToAPIDomainPerm( } domainPerm.ID = d.GetID() - domainPerm.Obfuscate = *d.GetObfuscate() + domainPerm.Obfuscate = util.PtrOrZero(d.GetObfuscate()) domainPerm.PrivateComment = d.GetPrivateComment() domainPerm.SubscriptionID = d.GetSubscriptionID() domainPerm.CreatedBy = d.GetCreatedByAccountID() -- cgit v1.2.3