fix(ads): verify a message belongs to its campaign before edit/delete
CI / changes (pull_request) Successful in 2s
CI / unit (pull_request) Successful in 9s
CI / integration (pull_request) Successful in 13s
CI / ui (pull_request) Successful in 49s
CI / gate (pull_request) Successful in 0s
CI / deploy (pull_request) Successful in 1m5s
CI / changes (pull_request) Successful in 2s
CI / unit (pull_request) Successful in 9s
CI / integration (pull_request) Successful in 13s
CI / ui (pull_request) Successful in 49s
CI / gate (pull_request) Successful in 0s
CI / deploy (pull_request) Successful in 1m5s
DeleteMessage/EditMessage acted on the message id without checking it belonged to the campaign id in the URL. A mismatched pair could edit an unrelated campaign's message or — worse — delete the default campaign's last message via another campaign's URL, bypassing the "default keeps >=1 message" guard. Both now load the campaign and return ErrNotFound unless it owns the message (MoveMessage already did). Operator-only, but a real business-logic bypass. Regression test: TestBannerMessageOwnership.
This commit is contained in:
@@ -139,14 +139,23 @@ func (s *Service) AddMessage(ctx context.Context, campaignID uuid.UUID, bodyEn,
|
|||||||
return s.store.AddMessage(ctx, Message{CampaignID: campaignID, Position: len(c.Messages), BodyEn: en, BodyRu: ru})
|
return s.store.AddMessage(ctx, Message{CampaignID: campaignID, Position: len(c.Messages), BodyEn: en, BodyRu: ru})
|
||||||
}
|
}
|
||||||
|
|
||||||
// EditMessage validates and updates a message's bilingual bodies (its position is
|
// EditMessage validates and updates the bilingual bodies of a message that
|
||||||
// unchanged).
|
// belongs to campaignID (its position is unchanged). It returns ErrNotFound when
|
||||||
func (s *Service) EditMessage(ctx context.Context, id uuid.UUID, bodyEn, bodyRu string) error {
|
// the message is not part of that campaign, so a mismatched campaign/message pair
|
||||||
|
// never edits an unrelated campaign's message.
|
||||||
|
func (s *Service) EditMessage(ctx context.Context, campaignID, messageID uuid.UUID, bodyEn, bodyRu string) error {
|
||||||
en, ru, err := validBodies(bodyEn, bodyRu)
|
en, ru, err := validBodies(bodyEn, bodyRu)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return err
|
return err
|
||||||
}
|
}
|
||||||
return s.store.UpdateMessageBodies(ctx, id, en, ru)
|
c, err := s.store.Campaign(ctx, campaignID)
|
||||||
|
if err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
if !campaignOwnsMessage(c, messageID) {
|
||||||
|
return ErrNotFound
|
||||||
|
}
|
||||||
|
return s.store.UpdateMessageBodies(ctx, messageID, en, ru)
|
||||||
}
|
}
|
||||||
|
|
||||||
// MoveMessage reorders a message within its campaign by swapping its position
|
// MoveMessage reorders a message within its campaign by swapping its position
|
||||||
@@ -178,19 +187,35 @@ func (s *Service) MoveMessage(ctx context.Context, campaignID, messageID uuid.UU
|
|||||||
return s.store.SetMessagePosition(ctx, b.ID, idx)
|
return s.store.SetMessagePosition(ctx, b.ID, idx)
|
||||||
}
|
}
|
||||||
|
|
||||||
// DeleteMessage removes a message, refusing to remove the default campaign's last
|
// DeleteMessage removes a message that belongs to campaignID, refusing to remove
|
||||||
// remaining message (the default must always have a creative to show).
|
// the default campaign's last remaining message (the default must always have a
|
||||||
|
// creative to show). It returns ErrNotFound when the message is not part of that
|
||||||
|
// campaign — so a mismatched campaign/message pair can neither delete an
|
||||||
|
// unrelated campaign's message nor bypass the default's last-message guard.
|
||||||
func (s *Service) DeleteMessage(ctx context.Context, campaignID, messageID uuid.UUID) error {
|
func (s *Service) DeleteMessage(ctx context.Context, campaignID, messageID uuid.UUID) error {
|
||||||
c, err := s.store.Campaign(ctx, campaignID)
|
c, err := s.store.Campaign(ctx, campaignID)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return err
|
return err
|
||||||
}
|
}
|
||||||
|
if !campaignOwnsMessage(c, messageID) {
|
||||||
|
return ErrNotFound
|
||||||
|
}
|
||||||
if c.IsDefault && len(c.Messages) <= 1 {
|
if c.IsDefault && len(c.Messages) <= 1 {
|
||||||
return fmt.Errorf("%w: the default campaign must keep at least one message", ErrValidation)
|
return fmt.Errorf("%w: the default campaign must keep at least one message", ErrValidation)
|
||||||
}
|
}
|
||||||
return s.store.DeleteMessage(ctx, messageID)
|
return s.store.DeleteMessage(ctx, messageID)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// campaignOwnsMessage reports whether messageID is one of the campaign's messages.
|
||||||
|
func campaignOwnsMessage(c Campaign, messageID uuid.UUID) bool {
|
||||||
|
for _, m := range c.Messages {
|
||||||
|
if m.ID == messageID {
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
|
||||||
// Settings returns the global display timings.
|
// Settings returns the global display timings.
|
||||||
func (s *Service) Settings(ctx context.Context) (Timings, error) {
|
func (s *Service) Settings(ctx context.Context) (Timings, error) {
|
||||||
return s.store.Settings(ctx)
|
return s.store.Settings(ctx)
|
||||||
|
|||||||
@@ -144,6 +144,49 @@ func TestBannerConsoleCRUD(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestBannerMessageOwnership guards the campaign/message pairing: a message may
|
||||||
|
// only be edited or deleted through the URL of the campaign it belongs to. This
|
||||||
|
// closes a default-protection bypass — deleting the default's last message via an
|
||||||
|
// unrelated campaign's URL must be refused.
|
||||||
|
func TestBannerMessageOwnership(t *testing.T) {
|
||||||
|
ctx := context.Background()
|
||||||
|
srv, adsSvc := bannerServer(t)
|
||||||
|
h := srv.Handler()
|
||||||
|
base := "http://admin.test/_gm"
|
||||||
|
const origin = "http://admin.test"
|
||||||
|
|
||||||
|
def := defaultCampaign(t, adsSvc)
|
||||||
|
if len(def.Messages) == 0 {
|
||||||
|
t.Fatal("default campaign has no seeded message")
|
||||||
|
}
|
||||||
|
defMsg := def.Messages[0].ID
|
||||||
|
|
||||||
|
// A separate timed campaign whose URL we will (mis)use.
|
||||||
|
otherID, err := adsSvc.CreateCampaign(ctx, ads.Campaign{Name: "Own-" + uuid.NewString()[:8], Weight: 10, Enabled: true})
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("create campaign: %v", err)
|
||||||
|
}
|
||||||
|
t.Cleanup(func() { _ = adsSvc.DeleteCampaign(ctx, otherID) })
|
||||||
|
|
||||||
|
// Deleting the default's message through the other campaign's URL is refused,
|
||||||
|
// and the default keeps its message (the bypass is closed).
|
||||||
|
if code, body := consoleDo(h, http.MethodPost, base+"/banners/"+otherID.String()+"/messages/"+defMsg.String()+"/delete", "", origin); code != http.StatusOK || !strings.Contains(body, "Not found") {
|
||||||
|
t.Fatalf("cross-campaign delete = %d, has 'Not found'=%v", code, strings.Contains(body, "Not found"))
|
||||||
|
}
|
||||||
|
// Editing it through the wrong campaign is refused too.
|
||||||
|
if code, body := consoleDo(h, http.MethodPost, base+"/banners/"+otherID.String()+"/messages/"+defMsg.String(), "body_en=Hijacked&body_ru=Взлом", origin); code != http.StatusOK || !strings.Contains(body, "Not found") {
|
||||||
|
t.Fatalf("cross-campaign edit = %d, has 'Not found'=%v", code, strings.Contains(body, "Not found"))
|
||||||
|
}
|
||||||
|
// The default's message is intact.
|
||||||
|
again := defaultCampaign(t, adsSvc)
|
||||||
|
if len(again.Messages) != len(def.Messages) {
|
||||||
|
t.Fatalf("default messages = %d, want %d (unchanged)", len(again.Messages), len(def.Messages))
|
||||||
|
}
|
||||||
|
if again.Messages[0].BodyEn != def.Messages[0].BodyEn {
|
||||||
|
t.Fatalf("default message body changed: %q -> %q", def.Messages[0].BodyEn, again.Messages[0].BodyEn)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// profileBanner is the banner block of the profile.get JSON response.
|
// profileBanner is the banner block of the profile.get JSON response.
|
||||||
type profileBanner struct {
|
type profileBanner struct {
|
||||||
Banner *struct {
|
Banner *struct {
|
||||||
|
|||||||
@@ -159,7 +159,7 @@ func (s *Server) consoleEditBannerMessage(c *gin.Context) {
|
|||||||
return
|
return
|
||||||
}
|
}
|
||||||
back := "/_gm/banners/" + cid.String()
|
back := "/_gm/banners/" + cid.String()
|
||||||
if err := s.ads.EditMessage(c.Request.Context(), mid, trimForm(c, "body_en"), trimForm(c, "body_ru")); err != nil {
|
if err := s.ads.EditMessage(c.Request.Context(), cid, mid, trimForm(c, "body_en"), trimForm(c, "body_ru")); err != nil {
|
||||||
s.bannerError(c, err, back)
|
s.bannerError(c, err, back)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user