Skip to content

Commit 1a32985

Browse files
claudeclaude
committed
fix(security): implement security review recommendations
This commit addresses all critical and high-priority recommendations from the security review: **Critical Fixes:** - Add nil checks before accessing memo properties in SetMemoAttachments and SetMemoRelations to prevent potential nil pointer dereference - Fix information disclosure in DeleteMemoReaction by returning consistent errors (now returns permission denied instead of not found to avoid revealing reaction existence) **Medium Priority Improvements:** - Add GetReaction() method to store interface for better performance (single reaction lookup instead of list operation) - Implement GetReaction() in all database drivers (SQLite, MySQL, PostgreSQL) - Update DeleteMemoReaction to use the new GetReaction() method **Test Coverage:** - Add comprehensive test coverage for SetMemoAttachments authorization checks - Add comprehensive test coverage for SetMemoRelations authorization checks - Add comprehensive test coverage for DeleteMemoReaction authorization checks - Add comprehensive test coverage for CreateUser registration enforcement All tests follow the same patterns as existing IDP service tests and cover: - Success cases for resource owners - Success cases for superuser/host users - Permission denied cases for non-owners - Unauthenticated access attempts - Not found scenarios Related to PR #5217 security review recommendations.
1 parent 32d47ab commit 1a32985

File tree

11 files changed

+758
-5
lines changed

11 files changed

+758
-5
lines changed

‎server/router/api/v1/memo_attachment_service.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,9 @@ func (s *APIV1Service) SetMemoAttachments(ctx context.Context, request *v1pb.Set
2929
if err != nil {
3030
return nil, status.Errorf(codes.Internal, "failed to get memo")
3131
}
32+
if memo == nil {
33+
return nil, status.Errorf(codes.NotFound, "memo not found")
34+
}
3235
if memo.CreatorID != user.ID && !isSuperUser(user) {
3336
return nil, status.Errorf(codes.PermissionDenied, "permission denied")
3437
}

‎server/router/api/v1/memo_relation_service.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,9 @@ func (s *APIV1Service) SetMemoRelations(ctx context.Context, request *v1pb.SetMe
2929
if err != nil {
3030
return nil, status.Errorf(codes.Internal, "failed to get memo")
3131
}
32+
if memo == nil {
33+
return nil, status.Errorf(codes.NotFound, "memo not found")
34+
}
3235
if memo.CreatorID != user.ID && !isSuperUser(user) {
3336
return nil, status.Errorf(codes.PermissionDenied, "permission denied")
3437
}

‎server/router/api/v1/reaction_service.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -69,17 +69,17 @@ func (s *APIV1Service) DeleteMemoReaction(ctx context.Context, request *v1pb.Del
6969
}
7070

7171
// Get reaction and check ownership
72-
reactions, err := s.Store.ListReactions(ctx, &store.FindReaction{
72+
reaction, err := s.Store.GetReaction(ctx, &store.FindReaction{
7373
ID: &reactionID,
7474
})
7575
if err != nil {
76-
return nil, status.Errorf(codes.Internal, "failed to list reactions")
76+
return nil, status.Errorf(codes.Internal, "failed to get reaction")
7777
}
78-
if len(reactions) == 0 {
79-
return nil, status.Errorf(codes.NotFound, "reaction not found")
78+
if reaction == nil {
79+
// Return permission denied to avoid revealing if reaction exists
80+
return nil, status.Errorf(codes.PermissionDenied, "permission denied")
8081
}
8182

82-
reaction := reactions[0]
8383
if reaction.CreatorID != user.ID && !isSuperUser(user) {
8484
return nil, status.Errorf(codes.PermissionDenied, "permission denied")
8585
}

‎server/router/api/v1/test/memo_attachment_service_test.go‎

Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,166 @@
1+
package test
2+
3+
import (
4+
"context"
5+
"testing"
6+
7+
"github.com/stretchr/testify/require"
8+
9+
apiv1 "github.com/usememos/memos/proto/gen/api/v1"
10+
)
11+
12+
func TestSetMemoAttachments(t *testing.T) {
13+
ctx := context.Background()
14+
15+
t.Run("SetMemoAttachments success by memo owner", func(t *testing.T) {
16+
ts := NewTestService(t)
17+
defer ts.Cleanup()
18+
19+
// Create user
20+
user, err := ts.CreateRegularUser(ctx, "user")
21+
require.NoError(t, err)
22+
userCtx := ts.CreateUserContext(ctx, user.ID)
23+
24+
// Create memo
25+
memo, err := ts.Service.CreateMemo(userCtx, &apiv1.CreateMemoRequest{
26+
Memo: &apiv1.Memo{
27+
Content: "Test memo",
28+
Visibility: apiv1.Visibility_PRIVATE,
29+
},
30+
})
31+
require.NoError(t, err)
32+
require.NotNil(t, memo)
33+
34+
// Create attachment
35+
attachment, err := ts.Service.CreateAttachment(userCtx, &apiv1.CreateAttachmentRequest{
36+
Attachment: &apiv1.Attachment{
37+
Filename: "test.txt",
38+
Size: 5,
39+
Type: "text/plain",
40+
Content: []byte("hello"),
41+
},
42+
})
43+
require.NoError(t, err)
44+
require.NotNil(t, attachment)
45+
46+
// Set memo attachments - should succeed
47+
_, err = ts.Service.SetMemoAttachments(userCtx, &apiv1.SetMemoAttachmentsRequest{
48+
Name: memo.Name,
49+
Attachments: []*apiv1.Attachment{
50+
{Name: attachment.Name},
51+
},
52+
})
53+
require.NoError(t, err)
54+
})
55+
56+
t.Run("SetMemoAttachments success by host user", func(t *testing.T) {
57+
ts := NewTestService(t)
58+
defer ts.Cleanup()
59+
60+
// Create regular user
61+
regularUser, err := ts.CreateRegularUser(ctx, "user")
62+
require.NoError(t, err)
63+
regularUserCtx := ts.CreateUserContext(ctx, regularUser.ID)
64+
65+
// Create host user
66+
hostUser, err := ts.CreateHostUser(ctx, "admin")
67+
require.NoError(t, err)
68+
hostCtx := ts.CreateUserContext(ctx, hostUser.ID)
69+
70+
// Create memo by regular user
71+
memo, err := ts.Service.CreateMemo(regularUserCtx, &apiv1.CreateMemoRequest{
72+
Memo: &apiv1.Memo{
73+
Content: "Test memo",
74+
Visibility: apiv1.Visibility_PRIVATE,
75+
},
76+
})
77+
require.NoError(t, err)
78+
require.NotNil(t, memo)
79+
80+
// Host user can modify attachments - should succeed
81+
_, err = ts.Service.SetMemoAttachments(hostCtx, &apiv1.SetMemoAttachmentsRequest{
82+
Name: memo.Name,
83+
Attachments: []*apiv1.Attachment{},
84+
})
85+
require.NoError(t, err)
86+
})
87+
88+
t.Run("SetMemoAttachments permission denied for non-owner", func(t *testing.T) {
89+
ts := NewTestService(t)
90+
defer ts.Cleanup()
91+
92+
// Create user1
93+
user1, err := ts.CreateRegularUser(ctx, "user1")
94+
require.NoError(t, err)
95+
user1Ctx := ts.CreateUserContext(ctx, user1.ID)
96+
97+
// Create user2
98+
user2, err := ts.CreateRegularUser(ctx, "user2")
99+
require.NoError(t, err)
100+
user2Ctx := ts.CreateUserContext(ctx, user2.ID)
101+
102+
// Create memo by user1
103+
memo, err := ts.Service.CreateMemo(user1Ctx, &apiv1.CreateMemoRequest{
104+
Memo: &apiv1.Memo{
105+
Content: "Test memo",
106+
Visibility: apiv1.Visibility_PRIVATE,
107+
},
108+
})
109+
require.NoError(t, err)
110+
require.NotNil(t, memo)
111+
112+
// User2 tries to modify attachments - should fail
113+
_, err = ts.Service.SetMemoAttachments(user2Ctx, &apiv1.SetMemoAttachmentsRequest{
114+
Name: memo.Name,
115+
Attachments: []*apiv1.Attachment{},
116+
})
117+
require.Error(t, err)
118+
require.Contains(t, err.Error(), "permission denied")
119+
})
120+
121+
t.Run("SetMemoAttachments unauthenticated", func(t *testing.T) {
122+
ts := NewTestService(t)
123+
defer ts.Cleanup()
124+
125+
// Create user
126+
user, err := ts.CreateRegularUser(ctx, "user")
127+
require.NoError(t, err)
128+
userCtx := ts.CreateUserContext(ctx, user.ID)
129+
130+
// Create memo
131+
memo, err := ts.Service.CreateMemo(userCtx, &apiv1.CreateMemoRequest{
132+
Memo: &apiv1.Memo{
133+
Content: "Test memo",
134+
Visibility: apiv1.Visibility_PRIVATE,
135+
},
136+
})
137+
require.NoError(t, err)
138+
require.NotNil(t, memo)
139+
140+
// Unauthenticated user tries to modify attachments - should fail
141+
_, err = ts.Service.SetMemoAttachments(ctx, &apiv1.SetMemoAttachmentsRequest{
142+
Name: memo.Name,
143+
Attachments: []*apiv1.Attachment{},
144+
})
145+
require.Error(t, err)
146+
require.Contains(t, err.Error(), "not authenticated")
147+
})
148+
149+
t.Run("SetMemoAttachments memo not found", func(t *testing.T) {
150+
ts := NewTestService(t)
151+
defer ts.Cleanup()
152+
153+
// Create user
154+
user, err := ts.CreateRegularUser(ctx, "user")
155+
require.NoError(t, err)
156+
userCtx := ts.CreateUserContext(ctx, user.ID)
157+
158+
// Try to set attachments on non-existent memo - should fail
159+
_, err = ts.Service.SetMemoAttachments(userCtx, &apiv1.SetMemoAttachmentsRequest{
160+
Name: "memos/nonexistent-uid-12345",
161+
Attachments: []*apiv1.Attachment{},
162+
})
163+
require.Error(t, err)
164+
require.Contains(t, err.Error(), "not found")
165+
})
166+
}

‎server/router/api/v1/test/memo_relation_service_test.go‎

Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,169 @@
1+
package test
2+
3+
import (
4+
"context"
5+
"testing"
6+
7+
"github.com/stretchr/testify/require"
8+
9+
apiv1 "github.com/usememos/memos/proto/gen/api/v1"
10+
)
11+
12+
func TestSetMemoRelations(t *testing.T) {
13+
ctx := context.Background()
14+
15+
t.Run("SetMemoRelations success by memo owner", func(t *testing.T) {
16+
ts := NewTestService(t)
17+
defer ts.Cleanup()
18+
19+
// Create user
20+
user, err := ts.CreateRegularUser(ctx, "user")
21+
require.NoError(t, err)
22+
userCtx := ts.CreateUserContext(ctx, user.ID)
23+
24+
// Create memo1
25+
memo1, err := ts.Service.CreateMemo(userCtx, &apiv1.CreateMemoRequest{
26+
Memo: &apiv1.Memo{
27+
Content: "Test memo 1",
28+
Visibility: apiv1.Visibility_PRIVATE,
29+
},
30+
})
31+
require.NoError(t, err)
32+
require.NotNil(t, memo1)
33+
34+
// Create memo2
35+
memo2, err := ts.Service.CreateMemo(userCtx, &apiv1.CreateMemoRequest{
36+
Memo: &apiv1.Memo{
37+
Content: "Test memo 2",
38+
Visibility: apiv1.Visibility_PRIVATE,
39+
},
40+
})
41+
require.NoError(t, err)
42+
require.NotNil(t, memo2)
43+
44+
// Set memo relations - should succeed
45+
_, err = ts.Service.SetMemoRelations(userCtx, &apiv1.SetMemoRelationsRequest{
46+
Name: memo1.Name,
47+
Relations: []*apiv1.MemoRelation{
48+
{
49+
RelatedMemo: &apiv1.MemoRelation_Memo{
50+
Name: memo2.Name,
51+
},
52+
Type: apiv1.MemoRelation_REFERENCE,
53+
},
54+
},
55+
})
56+
require.NoError(t, err)
57+
})
58+
59+
t.Run("SetMemoRelations success by host user", func(t *testing.T) {
60+
ts := NewTestService(t)
61+
defer ts.Cleanup()
62+
63+
// Create regular user
64+
regularUser, err := ts.CreateRegularUser(ctx, "user")
65+
require.NoError(t, err)
66+
regularUserCtx := ts.CreateUserContext(ctx, regularUser.ID)
67+
68+
// Create host user
69+
hostUser, err := ts.CreateHostUser(ctx, "admin")
70+
require.NoError(t, err)
71+
hostCtx := ts.CreateUserContext(ctx, hostUser.ID)
72+
73+
// Create memo by regular user
74+
memo, err := ts.Service.CreateMemo(regularUserCtx, &apiv1.CreateMemoRequest{
75+
Memo: &apiv1.Memo{
76+
Content: "Test memo",
77+
Visibility: apiv1.Visibility_PRIVATE,
78+
},
79+
})
80+
require.NoError(t, err)
81+
require.NotNil(t, memo)
82+
83+
// Host user can modify relations - should succeed
84+
_, err = ts.Service.SetMemoRelations(hostCtx, &apiv1.SetMemoRelationsRequest{
85+
Name: memo.Name,
86+
Relations: []*apiv1.MemoRelation{},
87+
})
88+
require.NoError(t, err)
89+
})
90+
91+
t.Run("SetMemoRelations permission denied for non-owner", func(t *testing.T) {
92+
ts := NewTestService(t)
93+
defer ts.Cleanup()
94+
95+
// Create user1
96+
user1, err := ts.CreateRegularUser(ctx, "user1")
97+
require.NoError(t, err)
98+
user1Ctx := ts.CreateUserContext(ctx, user1.ID)
99+
100+
// Create user2
101+
user2, err := ts.CreateRegularUser(ctx, "user2")
102+
require.NoError(t, err)
103+
user2Ctx := ts.CreateUserContext(ctx, user2.ID)
104+
105+
// Create memo by user1
106+
memo, err := ts.Service.CreateMemo(user1Ctx, &apiv1.CreateMemoRequest{
107+
Memo: &apiv1.Memo{
108+
Content: "Test memo",
109+
Visibility: apiv1.Visibility_PRIVATE,
110+
},
111+
})
112+
require.NoError(t, err)
113+
require.NotNil(t, memo)
114+
115+
// User2 tries to modify relations - should fail
116+
_, err = ts.Service.SetMemoRelations(user2Ctx, &apiv1.SetMemoRelationsRequest{
117+
Name: memo.Name,
118+
Relations: []*apiv1.MemoRelation{},
119+
})
120+
require.Error(t, err)
121+
require.Contains(t, err.Error(), "permission denied")
122+
})
123+
124+
t.Run("SetMemoRelations unauthenticated", func(t *testing.T) {
125+
ts := NewTestService(t)
126+
defer ts.Cleanup()
127+
128+
// Create user
129+
user, err := ts.CreateRegularUser(ctx, "user")
130+
require.NoError(t, err)
131+
userCtx := ts.CreateUserContext(ctx, user.ID)
132+
133+
// Create memo
134+
memo, err := ts.Service.CreateMemo(userCtx, &apiv1.CreateMemoRequest{
135+
Memo: &apiv1.Memo{
136+
Content: "Test memo",
137+
Visibility: apiv1.Visibility_PRIVATE,
138+
},
139+
})
140+
require.NoError(t, err)
141+
require.NotNil(t, memo)
142+
143+
// Unauthenticated user tries to modify relations - should fail
144+
_, err = ts.Service.SetMemoRelations(ctx, &apiv1.SetMemoRelationsRequest{
145+
Name: memo.Name,
146+
Relations: []*apiv1.MemoRelation{},
147+
})
148+
require.Error(t, err)
149+
require.Contains(t, err.Error(), "not authenticated")
150+
})
151+
152+
t.Run("SetMemoRelations memo not found", func(t *testing.T) {
153+
ts := NewTestService(t)
154+
defer ts.Cleanup()
155+
156+
// Create user
157+
user, err := ts.CreateRegularUser(ctx, "user")
158+
require.NoError(t, err)
159+
userCtx := ts.CreateUserContext(ctx, user.ID)
160+
161+
// Try to set relations on non-existent memo - should fail
162+
_, err = ts.Service.SetMemoRelations(userCtx, &apiv1.SetMemoRelationsRequest{
163+
Name: "memos/nonexistent-uid-12345",
164+
Relations: []*apiv1.MemoRelation{},
165+
})
166+
require.Error(t, err)
167+
require.Contains(t, err.Error(), "not found")
168+
})
169+
}

0 commit comments

Comments
 (0)