Skip to content

Commit 89765fe

Browse files
committed
ECOPROJECT-4722 | fix: Add missing organization check to GetSourceDownloadURL endpoint
Signed-off-by: Ronen Avraham <ravraham@redhat.com>
1 parent d7ba2e1 commit 89765fe

2 files changed

Lines changed: 87 additions & 2 deletions

File tree

internal/handlers/v1alpha1/source.go

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -234,15 +234,26 @@ func (s *ServiceHandler) HeadImage(ctx context.Context, request server.HeadImage
234234

235235
// (GET /api/v1/sources/{id}/image-url)
236236
func (s *ServiceHandler) GetSourceDownloadURL(ctx context.Context, request server.GetSourceDownloadURLRequestObject) (server.GetSourceDownloadURLResponseObject, error) {
237-
url, expireAt, err := s.sourceSrv.GetSourceDownloadURL(ctx, request.Id)
237+
source, err := s.sourceSrv.GetSource(ctx, request.Id)
238238
if err != nil {
239239
switch err.(type) {
240240
case *service.ErrResourceNotFound:
241241
return server.GetSourceDownloadURL404JSONResponse{Message: err.Error()}, nil
242242
default:
243-
return server.GetSourceDownloadURL400JSONResponse{}, nil // FIX: should be 500
243+
return server.GetSourceDownloadURL400JSONResponse{}, nil
244244
}
245245
}
246+
247+
user := auth.MustHaveUser(ctx)
248+
if user.Username != source.Username || user.Organization != source.OrgID {
249+
message := fmt.Sprintf("source %s not found", request.Id)
250+
return server.GetSourceDownloadURL404JSONResponse{Message: message}, nil
251+
}
252+
253+
url, expireAt, err := s.sourceSrv.GetSourceDownloadURL(ctx, request.Id)
254+
if err != nil {
255+
return server.GetSourceDownloadURL400JSONResponse{}, nil
256+
}
246257
return server.GetSourceDownloadURL200JSONResponse{Url: url, ExpiresAt: &expireAt}, nil
247258
}
248259

internal/handlers/v1alpha1/source_test.go

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1091,4 +1091,78 @@ var _ = Describe("source handler", Ordered, func() {
10911091
gormdb.Exec("DELETE FROM sources;")
10921092
})
10931093
})
1094+
1095+
Context("GetSourceDownloadURL", func() {
1096+
It("successfully returns image URL for owned source", func() {
1097+
sourceID := uuid.New()
1098+
tx := gormdb.Exec(fmt.Sprintf(insertSourceWithUsernameStm, sourceID, "admin", "admin"))
1099+
Expect(tx.Error).To(BeNil())
1100+
1101+
// Insert image_infra record (required for URL generation)
1102+
insertImageInfraStm := `INSERT INTO image_infras (source_id) VALUES ('%s');`
1103+
tx = gormdb.Exec(fmt.Sprintf(insertImageInfraStm, sourceID))
1104+
Expect(tx.Error).To(BeNil())
1105+
1106+
user := auth.User{
1107+
Username: "admin",
1108+
Organization: "admin",
1109+
EmailDomain: "admin.example.com",
1110+
}
1111+
ctx := auth.NewTokenContext(context.TODO(), user)
1112+
1113+
srv := handlers.NewServiceHandler(service.NewSourceService(s, nil), service.NewAssessmentService(s, nil, nil), nil, service.NewSizerService(nil, s), nil, nil, nil)
1114+
resp, err := srv.GetSourceDownloadURL(ctx, server.GetSourceDownloadURLRequestObject{Id: sourceID})
1115+
Expect(err).To(BeNil())
1116+
Expect(reflect.TypeOf(resp).String()).To(Equal(reflect.TypeOf(server.GetSourceDownloadURL200JSONResponse{}).String()))
1117+
1118+
result := resp.(server.GetSourceDownloadURL200JSONResponse)
1119+
Expect(result.Url).NotTo(BeEmpty())
1120+
})
1121+
1122+
It("returns 404 when trying to access another org's source - SECURITY TEST", func() {
1123+
// Create source owned by "batman" org
1124+
victimSourceID := uuid.New()
1125+
tx := gormdb.Exec(fmt.Sprintf(insertSourceWithUsernameStm, victimSourceID, "batman", "batman"))
1126+
Expect(tx.Error).To(BeNil())
1127+
1128+
insertImageInfraStm := `INSERT INTO image_infras (source_id) VALUES ('%s');`
1129+
tx = gormdb.Exec(fmt.Sprintf(insertImageInfraStm, victimSourceID))
1130+
Expect(tx.Error).To(BeNil())
1131+
1132+
// Attempt to access with "joker" credentials
1133+
attackerUser := auth.User{
1134+
Username: "joker",
1135+
Organization: "joker",
1136+
EmailDomain: "joker.example.com",
1137+
}
1138+
ctx := auth.NewTokenContext(context.TODO(), attackerUser)
1139+
1140+
srv := handlers.NewServiceHandler(service.NewSourceService(s, nil), service.NewAssessmentService(s, nil, nil), nil, service.NewSizerService(nil, s), nil, nil, nil)
1141+
resp, err := srv.GetSourceDownloadURL(ctx, server.GetSourceDownloadURLRequestObject{Id: victimSourceID})
1142+
1143+
Expect(err).To(BeNil())
1144+
// CRITICAL: Must return 404 (not 403) to prevent existence oracle
1145+
Expect(reflect.TypeOf(resp).String()).To(Equal(reflect.TypeOf(server.GetSourceDownloadURL404JSONResponse{}).String()))
1146+
})
1147+
1148+
It("returns 404 for non-existent source", func() {
1149+
user := auth.User{
1150+
Username: "admin",
1151+
Organization: "admin",
1152+
EmailDomain: "admin.example.com",
1153+
}
1154+
ctx := auth.NewTokenContext(context.TODO(), user)
1155+
1156+
srv := handlers.NewServiceHandler(service.NewSourceService(s, nil), service.NewAssessmentService(s, nil, nil), nil, service.NewSizerService(nil, s), nil, nil, nil)
1157+
resp, err := srv.GetSourceDownloadURL(ctx, server.GetSourceDownloadURLRequestObject{Id: uuid.New()})
1158+
1159+
Expect(err).To(BeNil())
1160+
Expect(reflect.TypeOf(resp).String()).To(Equal(reflect.TypeOf(server.GetSourceDownloadURL404JSONResponse{}).String()))
1161+
})
1162+
1163+
AfterEach(func() {
1164+
gormdb.Exec("DELETE FROM image_infras;")
1165+
gormdb.Exec("DELETE FROM sources;")
1166+
})
1167+
})
10941168
})

0 commit comments

Comments
 (0)