vbatts/ntfy
1
0
Fork 0
Code Pull requests Activity

Introduce text IDs for everything (esp user), to avoid security and accounting issues

Browse source
This commit is contained in:
binwiederhier 2023-01-21 23:15:22 -05:00
parent 88abd8872d
commit 9c082a8331
13 changed files with 160 additions and 108 deletions
Download patch file Download diff file Expand all files Collapse all files

8
server/message_cache.go
View file

@ -98,8 +98,8 @@ const (
updateAttachmentDeleted = `UPDATE messages SET attachment_deleted = 1 WHERE mid = ?`
selectAttachmentsExpiredQuery = `SELECT mid FROM messages WHERE attachment_expires > 0 AND attachment_expires <= ? AND attachment_deleted = 0`
selectAttachmentsSizeBySenderQuery = `SELECT IFNULL(SUM(attachment_size), 0) FROM messages WHERE sender = ? AND attachment_expires >= ?`
selectAttachmentsSizeByUserQuery = `SELECT IFNULL(SUM(attachment_size), 0) FROM messages WHERE user = ? AND attachment_expires >= ?`
selectAttachmentsSizeBySenderQuery = `SELECT IFNULL(SUM(attachment_size), 0) FROM messages WHERE user = '' AND sender = ? AND attachment_expires >= ?`
selectAttachmentsSizeByUserIDQuery = `SELECT IFNULL(SUM(attachment_size), 0) FROM messages WHERE user = ? AND attachment_expires >= ?`
)
// Schema management queries
@ -563,8 +563,8 @@ func (c *messageCache) AttachmentBytesUsedBySender(sender string) (int64, error)
return c.readAttachmentBytesUsed(rows)
}
func (c *messageCache) AttachmentBytesUsedByUser(user string) (int64, error) {
rows, err := c.db.Query(selectAttachmentsSizeByUserQuery, user, time.Now().Unix())
func (c *messageCache) AttachmentBytesUsedByUser(userID string) (int64, error) {
rows, err := c.db.Query(selectAttachmentsSizeByUserIDQuery, userID, time.Now().Unix())
if err != nil {
return 0, err
}

21
server/message_cache_test.go
View file

@ -12,10 +12,6 @@ import (
"github.com/stretchr/testify/require"
)
var (
exampleIP1234 = netip.MustParseAddr("1.2.3.4")
)
func TestSqliteCache_Messages(t *testing.T) {
testCacheMessages(t, newSqliteTestCache(t))
}
@ -294,10 +290,10 @@ func TestMemCache_Attachments(t *testing.T) {
}
func testCacheAttachments(t *testing.T, c *messageCache) {
expires1 := time.Now().Add(-4 * time.Hour).Unix()
expires1 := time.Now().Add(-4 * time.Hour).Unix() // Expired
m := newDefaultMessage("mytopic", "flower for you")
m.ID = "m1"
m.Sender = exampleIP1234
m.Sender = netip.MustParseAddr("1.2.3.4")
m.Attachment = &attachment{
Name: "flower.jpg",
Type: "image/jpeg",
@ -310,7 +306,7 @@ func testCacheAttachments(t *testing.T, c *messageCache) {
expires2 := time.Now().Add(2 * time.Hour).Unix() // Future
m = newDefaultMessage("mytopic", "sending you a car")
m.ID = "m2"
m.Sender = exampleIP1234
m.Sender = netip.MustParseAddr("1.2.3.4")
m.Attachment = &attachment{
Name: "car.jpg",
Type: "image/jpeg",
@ -323,7 +319,8 @@ func testCacheAttachments(t *testing.T, c *messageCache) {
expires3 := time.Now().Add(1 * time.Hour).Unix() // Future
m = newDefaultMessage("another-topic", "sending you another car")
m.ID = "m3"
m.Sender = exampleIP1234
m.User = "u_BAsbaAa"
m.Sender = netip.MustParseAddr("5.6.7.8")
m.Attachment = &attachment{
Name: "another-car.jpg",
Type: "image/jpeg",
@ -355,11 +352,15 @@ func testCacheAttachments(t *testing.T, c *messageCache) {
size, err := c.AttachmentBytesUsedBySender("1.2.3.4")
require.Nil(t, err)
require.Equal(t, int64(30000), size)
require.Equal(t, int64(10000), size)
size, err = c.AttachmentBytesUsedBySender("5.6.7.8")
require.Nil(t, err)
require.Equal(t, int64(0), size)
require.Equal(t, int64(0), size) // Accounted to the user, not the IP!
size, err = c.AttachmentBytesUsedByUser("u_BAsbaAa")
require.Nil(t, err)
require.Equal(t, int64(20000), size)
}
func TestSqliteCache_Attachments_Expired(t *testing.T) {

9
server/server.go
View file

@ -38,12 +38,13 @@ import (
TODO
--
- Security: Account re-creation leads to terrible behavior. Use user ID instead of user name for (a) visitor map, (b) messages.user column, (c) Stripe checkout session
- Reservation: Kill existing subscribers when topic is reserved (deadcade)
- Reservation (UI): Show "This topic is reserved" error message when trying to reserve a reserved topic (Thorben)
- Reservation (UI): Ask for confirmation when removing reservation (deadcade)
- Logging: Add detailed logging with username/customerID for all Stripe events (phil)
- Rate limiting: Sensitive endpoints (account/login/change-password/...)
- Stripe webhook: Do not respond wih error if user does not exist (after account deletion)
- Stripe: Add metadata to customer
races:
- v.user --> see publishSyncEventAsync() test
@ -581,7 +582,7 @@ func (s *Server) handlePublishWithoutResponse(r *http.Request, v *visitor) (*mes
m = newPollRequestMessage(t.ID, m.PollID)
}
if v.user != nil {
m.User = v.user.Name
m.User = v.user.ID
}
m.Expires = time.Now().Add(v.Limits().MessagesExpiryDuration).Unix()
if err := s.handlePublishBody(r, v, m, body, unifiedpush); err != nil {
@ -859,6 +860,7 @@ func (s *Server) handleBodyAsAttachment(r *http.Request, v *visitor, m *message,
if m.Time > attachmentExpiry {
return errHTTPBadRequestAttachmentsExpiryBeforeDelivery
}
fmt.Printf("v = %#v\nlimits = %#v\nstats = %#v\n", v, vinfo.Limits, vinfo.Stats)
contentLengthStr := r.Header.Get("Content-Length")
if contentLengthStr != "" { // Early "do-not-trust" check, hard limit see below
contentLength, err := strconv.ParseInt(contentLengthStr, 10, 64)
@ -885,6 +887,7 @@ func (s *Server) handleBodyAsAttachment(r *http.Request, v *visitor, m *message,
util.NewFixedLimiter(vinfo.Limits.AttachmentFileSizeLimit),
util.NewFixedLimiter(vinfo.Stats.AttachmentTotalSizeRemaining),
}
fmt.Printf("limiters = %#v\nv = %#v\n", limiters, v)
m.Attachment.Size, err = s.fileCache.Write(m.ID, body, limiters...)
if err == util.ErrLimitReached {
return errHTTPEntityTooLargeAttachment
@ -1657,7 +1660,7 @@ func (s *Server) visitorFromIP(ip netip.Addr) *visitor {
}
func (s *Server) visitorFromUser(user *user.User, ip netip.Addr) *visitor {
return s.visitorFromID(fmt.Sprintf("user:%s", user.Name), ip, user)
return s.visitorFromID(fmt.Sprintf("user:%s", user.ID), ip, user)
}
func (s *Server) writeJSON(w http.ResponseWriter, v any) error {

2
server/server_account.go
View file

@ -337,7 +337,7 @@ func (s *Server) handleAccountReservationAdd(w http.ResponseWriter, r *http.Requ
return errHTTPTooManyRequestsLimitReservations
}
}
if err := s.userManager.ReserveAccess(v.user.Name, req.Topic, everyone); err != nil {
if err := s.userManager.AddReservation(v.user.Name, req.Topic, everyone); err != nil {
return err
}
return s.writeJSON(w, newSuccessResponse())

2
server/server_account_test.go
View file

@ -212,7 +212,7 @@ func TestAccount_ChangePassword(t *testing.T) {
s := newTestServer(t, newTestConfigWithAuthFile(t))
require.Nil(t, s.userManager.AddUser("phil", "phil", user.RoleUser, "unit-test"))
rr := request(t, s, "POST", "/v1/account/password", `{"password": "new password"}`, map[string]string{
rr := request(t, s, "POST", "/v1/account/password", `{"password": "phil", "new_password": "new password"}`, map[string]string{
"Authorization": util.BasicAuth("phil", "phil"),
})
require.Equal(t, 200, rr.Code)

4
server/server_payments.go
View file

@ -128,7 +128,7 @@ func (s *Server) handleAccountBillingSubscriptionCreate(w http.ResponseWriter, r
successURL := s.config.BaseURL + apiAccountBillingSubscriptionCheckoutSuccessTemplate
params := &stripe.CheckoutSessionParams{
Customer: stripeCustomerID, // A user may have previously deleted their subscription
ClientReferenceID: &v.user.Name,
ClientReferenceID: &v.user.ID,
SuccessURL: &successURL,
Mode: stripe.String(string(stripe.CheckoutSessionModeSubscription)),
AllowPromotionCodes: stripe.Bool(true),
@ -178,7 +178,7 @@ func (s *Server) handleAccountBillingSubscriptionCreateSuccess(w http.ResponseWr
if err != nil {
return err
}
u, err := s.userManager.User(sess.ClientReferenceID)
u, err := s.userManager.UserByID(sess.ClientReferenceID)
if err != nil {
return err
}

4
server/server_payments_test.go
View file

@ -176,8 +176,8 @@ func TestPayments_Webhook_Subscription_Updated_Downgrade_From_PastDue_To_Active(
}))
require.Nil(t, s.userManager.AddUser("phil", "phil", user.RoleUser, "unit-test"))
require.Nil(t, s.userManager.ChangeTier("phil", "pro"))
require.Nil(t, s.userManager.ReserveAccess("phil", "atopic", user.PermissionDenyAll))
require.Nil(t, s.userManager.ReserveAccess("phil", "ztopic", user.PermissionDenyAll))
require.Nil(t, s.userManager.AddReservation("phil", "atopic", user.PermissionDenyAll))
require.Nil(t, s.userManager.AddReservation("phil", "ztopic", user.PermissionDenyAll))
// Add billing details
u, err := s.userManager.User("phil")

4
server/server_test.go
View file

@ -830,7 +830,7 @@ func TestServer_PublishTooRequests_Defaults_ExemptHosts(t *testing.T) {
func TestServer_PublishTooRequests_ShortReplenish(t *testing.T) {
c := newTestConfig(t)
c.VisitorRequestLimitBurst = 60
c.VisitorRequestLimitReplenish = 500 * time.Millisecond
c.VisitorRequestLimitReplenish = time.Second
s := newTestServer(t, c)
for i := 0; i < 60; i++ {
response := request(t, s, "PUT", "/mytopic", fmt.Sprintf("message %d", i), nil)
@ -839,7 +839,7 @@ func TestServer_PublishTooRequests_ShortReplenish(t *testing.T) {
response := request(t, s, "PUT", "/mytopic", "message", nil)
require.Equal(t, 429, response.Code)
time.Sleep(520 * time.Millisecond)
time.Sleep(1020 * time.Millisecond)
response = request(t, s, "PUT", "/mytopic", "message", nil)
require.Equal(t, 200, response.Code)
}

2
server/visitor.go
View file

@ -241,7 +241,7 @@ func (v *visitor) Info() (*visitorInfo, error) {
var attachmentsBytesUsed int64
var err error
if v.user != nil {
attachmentsBytesUsed, err = v.messageCache.AttachmentBytesUsedByUser(v.user.Name)
attachmentsBytesUsed, err = v.messageCache.AttachmentBytesUsedByUser(v.user.ID)
} else {
attachmentsBytesUsed, err = v.messageCache.AttachmentBytesUsedBySender(v.ip.String())
}