fix(rpc): answer not found to non-owners before parsing moderation requests
- commit
- ddab9a2132ce597552bf742634282cb85f6fc741
- author
- brice · · 2026-10-19 08:57 UTC
- parent
- f8b1e92
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
2 files · +30 -0
M internal/adapters/rpc/issue.go +19 -0
| @@ -198,7 +198,20 @@ func pendingRef(repo, id string) (domain.RepoName, domain.PendingIssueID, error) | |||
| 198 | 198 | return name, pid, nil | |
| 199 | 199 | } | |
| 200 | 200 | ||
| 201 | + | // requireOwner answers NotFound to anyone without owner rights, before the | |
| 202 | + | // request is parsed: a malformed repository must not tell a non-owner more | |
| 203 | + | // than a well-formed one does. | |
| 204 | + | func requireOwner(ctx context.Context) error { | |
| 205 | + | if !caller(ctx).HasOwnerRights() { | |
| 206 | + | return connectError(ctx, domain.ErrNotFound) | |
| 207 | + | } | |
| 208 | + | return nil | |
| 209 | + | } | |
| 210 | + | ||
| 201 | 211 | func (s *issueService) ListPendingIssues(ctx context.Context, req *connect.Request[pb.ListPendingIssuesRequest]) (*connect.Response[pb.ListPendingIssuesResponse], error) { | |
| 212 | + | if err := requireOwner(ctx); err != nil { | |
| 213 | + | return nil, err | |
| 214 | + | } | |
| 202 | 215 | var name domain.RepoName | |
| 203 | 216 | if req.Msg.GetRepo() != "" { | |
| 204 | 217 | var err error | |
| @@ -220,6 +233,9 @@ func (s *issueService) ListPendingIssues(ctx context.Context, req *connect.Reque | |||
| 220 | 233 | } | |
| 221 | 234 | ||
| 222 | 235 | func (s *issueService) ApproveIssue(ctx context.Context, req *connect.Request[pb.ApproveIssueRequest]) (*connect.Response[pb.ApproveIssueResponse], error) { | |
| 236 | + | if err := requireOwner(ctx); err != nil { | |
| 237 | + | return nil, err | |
| 238 | + | } | |
| 223 | 239 | name, id, err := pendingRef(req.Msg.GetRepo(), req.Msg.GetId()) | |
| 224 | 240 | if err != nil { | |
| 225 | 241 | return nil, connectError(ctx, err) | |
| @@ -232,6 +248,9 @@ func (s *issueService) ApproveIssue(ctx context.Context, req *connect.Request[pb | |||
| 232 | 248 | } | |
| 233 | 249 | ||
| 234 | 250 | func (s *issueService) RejectIssue(ctx context.Context, req *connect.Request[pb.RejectIssueRequest]) (*connect.Response[pb.RejectIssueResponse], error) { | |
| 251 | + | if err := requireOwner(ctx); err != nil { | |
| 252 | + | return nil, err | |
| 253 | + | } | |
| 235 | 254 | name, id, err := pendingRef(req.Msg.GetRepo(), req.Msg.GetId()) | |
| 236 | 255 | if err != nil { | |
| 237 | 256 | return nil, connectError(ctx, err) | |
| @@ -198,7 +198,20 @@ func pendingRef(repo, id string) (domain.RepoName, domain.PendingIssueID, error) | |||||
| 198 | return name, pid, nil | 198 | return name, pid, nil | ||
| 199 | } | 199 | } | ||
| 200 | 200 | ||||
| 201 | + | // requireOwner answers NotFound to anyone without owner rights, before the | |||
| 202 | + | // request is parsed: a malformed repository must not tell a non-owner more | |||
| 203 | + | // than a well-formed one does. | |||
| 204 | + | func requireOwner(ctx context.Context) error { | |||
| 205 | + | if !caller(ctx).HasOwnerRights() { | |||
| 206 | + | return connectError(ctx, domain.ErrNotFound) | |||
| 207 | + | } | |||
| 208 | + | return nil | |||
| 209 | + | } | |||
| 210 | + | ||||
| 201 | func (s *issueService) ListPendingIssues(ctx context.Context, req *connect.Request[pb.ListPendingIssuesRequest]) (*connect.Response[pb.ListPendingIssuesResponse], error) { | 211 | func (s *issueService) ListPendingIssues(ctx context.Context, req *connect.Request[pb.ListPendingIssuesRequest]) (*connect.Response[pb.ListPendingIssuesResponse], error) { | ||
| 212 | + | if err := requireOwner(ctx); err != nil { | |||
| 213 | + | return nil, err | |||
| 214 | + | } | |||
| 202 | var name domain.RepoName | 215 | var name domain.RepoName | ||
| 203 | if req.Msg.GetRepo() != "" { | 216 | if req.Msg.GetRepo() != "" { | ||
| 204 | var err error | 217 | var err error | ||
| @@ -220,6 +233,9 @@ func (s *issueService) ListPendingIssues(ctx context.Context, req *connect.Reque | |||||
| 220 | } | 233 | } | ||
| 221 | 234 | ||||
| 222 | func (s *issueService) ApproveIssue(ctx context.Context, req *connect.Request[pb.ApproveIssueRequest]) (*connect.Response[pb.ApproveIssueResponse], error) { | 235 | func (s *issueService) ApproveIssue(ctx context.Context, req *connect.Request[pb.ApproveIssueRequest]) (*connect.Response[pb.ApproveIssueResponse], error) { | ||
| 236 | + | if err := requireOwner(ctx); err != nil { | |||
| 237 | + | return nil, err | |||
| 238 | + | } | |||
| 223 | name, id, err := pendingRef(req.Msg.GetRepo(), req.Msg.GetId()) | 239 | name, id, err := pendingRef(req.Msg.GetRepo(), req.Msg.GetId()) | ||
| 224 | if err != nil { | 240 | if err != nil { | ||
| 225 | return nil, connectError(ctx, err) | 241 | return nil, connectError(ctx, err) | ||
| @@ -232,6 +248,9 @@ func (s *issueService) ApproveIssue(ctx context.Context, req *connect.Request[pb | |||||
| 232 | } | 248 | } | ||
| 233 | 249 | ||||
| 234 | func (s *issueService) RejectIssue(ctx context.Context, req *connect.Request[pb.RejectIssueRequest]) (*connect.Response[pb.RejectIssueResponse], error) { | 250 | func (s *issueService) RejectIssue(ctx context.Context, req *connect.Request[pb.RejectIssueRequest]) (*connect.Response[pb.RejectIssueResponse], error) { | ||
| 251 | + | if err := requireOwner(ctx); err != nil { | |||
| 252 | + | return nil, err | |||
| 253 | + | } | |||
| 235 | name, id, err := pendingRef(req.Msg.GetRepo(), req.Msg.GetId()) | 254 | name, id, err := pendingRef(req.Msg.GetRepo(), req.Msg.GetId()) | ||
| 236 | if err != nil { | 255 | if err != nil { | ||
| 237 | return nil, connectError(ctx, err) | 256 | return nil, connectError(ctx, err) | ||
M internal/app/issue_moderation_api_test.go +11 -0
| @@ -52,6 +52,13 @@ func TestIssueModerationOverTheAPI(t *testing.T) { | |||
| 52 | 52 | assert.Equal(t, "not_found", codeName(err), "%s approve", name) | |
| 53 | 53 | _, err = c.issue.RejectIssue(ctx, connect.NewRequest(&pb.RejectIssueRequest{Repo: "pub", Id: p1.ID.String(), Ban: true})) | |
| 54 | 54 | assert.Equal(t, "not_found", codeName(err), "%s reject", name) | |
| 55 | + | // A malformed repository tells a non-owner nothing more. | |
| 56 | + | _, err = c.issue.ListPendingIssues(ctx, connect.NewRequest(&pb.ListPendingIssuesRequest{Repo: "Bad Repo!"})) | |
| 57 | + | assert.Equal(t, "not_found", codeName(err), "%s list, malformed repo", name) | |
| 58 | + | _, err = c.issue.ApproveIssue(ctx, connect.NewRequest(&pb.ApproveIssueRequest{Repo: "Bad Repo!", Id: p1.ID.String()})) | |
| 59 | + | assert.Equal(t, "not_found", codeName(err), "%s approve, malformed repo", name) | |
| 60 | + | _, err = c.issue.RejectIssue(ctx, connect.NewRequest(&pb.RejectIssueRequest{Repo: "Bad Repo!", Id: "nope"})) | |
| 61 | + | assert.Equal(t, "not_found", codeName(err), "%s reject, malformed repo", name) | |
| 55 | 62 | } | |
| 56 | 63 | still, err := r.forge.PendingIssues.Count(ctx) | |
| 57 | 64 | require.NoError(t, err) | |
| @@ -68,6 +75,10 @@ func TestIssueModerationOverTheAPI(t *testing.T) { | |||
| 68 | 75 | require.NoError(t, err) | |
| 69 | 76 | assert.Empty(t, res.Msg.GetIssues()) | |
| 70 | 77 | ||
| 78 | + | // An owner does learn that a repository name is malformed. | |
| 79 | + | _, err = owner.issue.ApproveIssue(ctx, connect.NewRequest(&pb.ApproveIssueRequest{Repo: "Bad Repo!", Id: p1.ID.String()})) | |
| 80 | + | assert.Equal(t, "invalid_argument", codeName(err)) | |
| 81 | + | ||
| 71 | 82 | // A malformed or unknown id is NotFound. | |
| 72 | 83 | _, err = owner.issue.ApproveIssue(ctx, connect.NewRequest(&pb.ApproveIssueRequest{Repo: "pub", Id: "nope"})) | |
| 73 | 84 | assert.Equal(t, "not_found", codeName(err)) | |
| @@ -52,6 +52,13 @@ func TestIssueModerationOverTheAPI(t *testing.T) { | |||||
| 52 | assert.Equal(t, "not_found", codeName(err), "%s approve", name) | 52 | assert.Equal(t, "not_found", codeName(err), "%s approve", name) | ||
| 53 | _, err = c.issue.RejectIssue(ctx, connect.NewRequest(&pb.RejectIssueRequest{Repo: "pub", Id: p1.ID.String(), Ban: true})) | 53 | _, err = c.issue.RejectIssue(ctx, connect.NewRequest(&pb.RejectIssueRequest{Repo: "pub", Id: p1.ID.String(), Ban: true})) | ||
| 54 | assert.Equal(t, "not_found", codeName(err), "%s reject", name) | 54 | assert.Equal(t, "not_found", codeName(err), "%s reject", name) | ||
| 55 | + | // A malformed repository tells a non-owner nothing more. | |||
| 56 | + | _, err = c.issue.ListPendingIssues(ctx, connect.NewRequest(&pb.ListPendingIssuesRequest{Repo: "Bad Repo!"})) | |||
| 57 | + | assert.Equal(t, "not_found", codeName(err), "%s list, malformed repo", name) | |||
| 58 | + | _, err = c.issue.ApproveIssue(ctx, connect.NewRequest(&pb.ApproveIssueRequest{Repo: "Bad Repo!", Id: p1.ID.String()})) | |||
| 59 | + | assert.Equal(t, "not_found", codeName(err), "%s approve, malformed repo", name) | |||
| 60 | + | _, err = c.issue.RejectIssue(ctx, connect.NewRequest(&pb.RejectIssueRequest{Repo: "Bad Repo!", Id: "nope"})) | |||
| 61 | + | assert.Equal(t, "not_found", codeName(err), "%s reject, malformed repo", name) | |||
| 55 | } | 62 | } | ||
| 56 | still, err := r.forge.PendingIssues.Count(ctx) | 63 | still, err := r.forge.PendingIssues.Count(ctx) | ||
| 57 | require.NoError(t, err) | 64 | require.NoError(t, err) | ||
| @@ -68,6 +75,10 @@ func TestIssueModerationOverTheAPI(t *testing.T) { | |||||
| 68 | require.NoError(t, err) | 75 | require.NoError(t, err) | ||
| 69 | assert.Empty(t, res.Msg.GetIssues()) | 76 | assert.Empty(t, res.Msg.GetIssues()) | ||
| 70 | 77 | ||||
| 78 | + | // An owner does learn that a repository name is malformed. | |||
| 79 | + | _, err = owner.issue.ApproveIssue(ctx, connect.NewRequest(&pb.ApproveIssueRequest{Repo: "Bad Repo!", Id: p1.ID.String()})) | |||
| 80 | + | assert.Equal(t, "invalid_argument", codeName(err)) | |||
| 81 | + | ||||
| 71 | // A malformed or unknown id is NotFound. | 82 | // A malformed or unknown id is NotFound. | ||
| 72 | _, err = owner.issue.ApproveIssue(ctx, connect.NewRequest(&pb.ApproveIssueRequest{Repo: "pub", Id: "nope"})) | 83 | _, err = owner.issue.ApproveIssue(ctx, connect.NewRequest(&pb.ApproveIssueRequest{Repo: "pub", Id: "nope"})) | ||
| 73 | assert.Equal(t, "not_found", codeName(err)) | 84 | assert.Equal(t, "not_found", codeName(err)) | ||