Skip to content

Commit 03b9a32

Browse files
JamesKaneJamesKane
committed
**feat(api): Enforce bulk operation limits, secure endpoints, and improve error handling**
- Added limits (`MaxBulkSize`) to bulk operations (`bulkCreateRegions`, `bulkAddBuilds`, `bulkUpdateRsIds`, `bulkAssignDuNames`) to prevent excessive requests. - Introduced `ApiSecurityAction` to secure endpoints, including `registerPDS` and `triggerDiscovery`. - Improved error handling with more descriptive messages and consistent formatting across APIs (e.g., database error messaging). - Enhanced input validation (e.g., `safePage` in `listFragment`, referer sanitization in `switchLanguage`). - Updated session cookie security settings in `application.conf` to ensure compliance with secure practices.
1 parent d2f262f commit 03b9a32

15 files changed

+118
-77
lines changed

‎app/actions/AuthenticatedAction.scala‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -52,9 +52,14 @@ class RoleAction @Inject()(authService: AuthService)(implicit ec: ExecutionConte
5252
override protected def executionContext: ExecutionContext = ec
5353

5454
override protected def filter[A](request: AuthenticatedRequest[A]): Future[Option[Result]] = {
55-
authService.hasAnyRole(request.user.id.get, requiredRoles).map { hasRole =>
56-
if (hasRole) None
57-
else Some(Results.Forbidden("You do not have the required permissions to access this resource."))
55+
request.user.id match {
56+
case Some(userId) =>
57+
authService.hasAnyRole(userId, requiredRoles).map { hasRole =>
58+
if (hasRole) None
59+
else Some(Results.Forbidden("You do not have the required permissions to access this resource."))
60+
}
61+
case None =>
62+
Future.successful(Some(Results.Forbidden("You do not have the required permissions to access this resource.")))
5863
}
5964
}
6065
}
@@ -70,9 +75,14 @@ class PermissionAction @Inject()(authService: AuthService)(implicit ec: Executio
7075
override protected def executionContext: ExecutionContext = ec
7176

7277
override protected def filter[A](request: AuthenticatedRequest[A]): Future[Option[Result]] = {
73-
authService.hasPermission(request.user.id.get, permission).map { hasPermission =>
74-
if (hasPermission) None
75-
else Some(Results.Forbidden(s"Missing required permission: $permission"))
78+
request.user.id match {
79+
case Some(userId) =>
80+
authService.hasPermission(userId, permission).map { hasPermission =>
81+
if (hasPermission) None
82+
else Some(Results.Forbidden("You do not have the required permissions to access this resource."))
83+
}
84+
case None =>
85+
Future.successful(Some(Results.Forbidden("You do not have the required permissions to access this resource.")))
7686
}
7787
}
7888
}

‎app/controllers/ExternalBiosampleController.scala‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ class ExternalBiosampleController @Inject()(
112112
case e: Exception =>
113113
InternalServerError(Json.obj(
114114
"error" -> "Internal server error",
115-
"message" -> s"An unexpected error occurred while attempting to delete biosample with accession '$accession': ${e.getMessage}"
115+
"message" -> s"An unexpected error occurred while attempting to delete biosample with accession '$accession'"
116116
))
117117
}
118118
}

‎app/controllers/GenomeRegionsApiManagementController.scala‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,11 +74,17 @@ class GenomeRegionsApiManagementController @Inject()(
7474
}
7575
}
7676

77+
private val MaxBulkRegions = 1000
78+
7779
def bulkCreateRegions(): Action[BulkCreateGenomeRegionsRequest] =
7880
secureApi.jsonAction[BulkCreateGenomeRegionsRequest].async { request =>
79-
logger.info(s"API: Bulk creating ${request.body.regions.size} genome regions")
80-
managementService.bulkCreateRegions(request.body, None).map { response =>
81-
Ok(Json.toJson(response))
81+
if (request.body.regions.size > MaxBulkRegions) {
82+
Future.successful(BadRequest(Json.obj("error" -> s"Bulk create limited to $MaxBulkRegions regions per request")))
83+
} else {
84+
logger.info(s"API: Bulk creating ${request.body.regions.size} genome regions")
85+
managementService.bulkCreateRegions(request.body, None).map { response =>
86+
Ok(Json.toJson(response))
87+
}
8288
}
8389
}
8490
}

‎app/controllers/HaplogroupTreeMergeController.scala‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -116,8 +116,7 @@ class HaplogroupTreeMergeController @Inject()(
116116
}.recover { case e: Exception =>
117117
logger.error(s"Merge preview failed: ${e.getMessage}", e)
118118
InternalServerError(Json.obj(
119-
"error" -> "Preview operation failed",
120-
"details" -> e.getMessage
119+
"error" -> "Preview operation failed"
121120
))
122121
}
123122
}

‎app/controllers/LanguageController.scala‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,14 @@ class LanguageController @Inject()(val controllerComponents: ControllerComponent
1111

1212
def switchLanguage(lang: String): Action[AnyContent] = Action { implicit request =>
1313
val referer = request.headers.get(REFERER).getOrElse("/")
14+
// Prevent open redirect: only allow relative paths
15+
val safeTarget = if (referer.startsWith("/") && !referer.startsWith("//")) referer else "/"
1416
val supportedLangs = messagesApi.messages.keys.filter(_ != "default").toSet
1517

1618
if (supportedLangs.contains(lang)) {
17-
Redirect(referer).withLang(Lang(lang))
19+
Redirect(safeTarget).withLang(Lang(lang))
1820
} else {
19-
Redirect(referer)
21+
Redirect(safeTarget)
2022
}
2123
}
2224
}

‎app/controllers/PDSRegistrationController.scala‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package controllers
22

3+
import actions.ApiSecurityAction
34
import api.PdsRegistrationRequest
45
import play.api.libs.json.{JsError, JsSuccess, Json}
56
import play.api.mvc.{Action, AnyContent, BaseController, ControllerComponents}
@@ -14,6 +15,7 @@ import scala.concurrent.{ExecutionContext, Future}
1415
@Singleton
1516
class PDSRegistrationController @Inject()(
1617
val controllerComponents: ControllerComponents,
18+
secureApi: ApiSecurityAction,
1719
pdsRegistrationService: PDSRegistrationService,
1820
reputationService: ReputationService,
1921
userRepository: UserRepository
@@ -22,10 +24,11 @@ class PDSRegistrationController @Inject()(
2224
/**
2325
* Handles the registration of a new Personal Data Server (PDS).
2426
* Expects a JSON body containing PdsRegistrationRequest.
27+
* Secured with X-API-Key authentication.
2528
*
2629
* @return An `Action` that processes the registration request.
2730
*/
28-
def registerPDS(): Action[play.api.libs.json.JsValue] = Action.async(parse.json) { implicit request =>
31+
def registerPDS(): Action[play.api.libs.json.JsValue] = secureApi.async(parse.json) { implicit request =>
2932
request.body.validate[PdsRegistrationRequest] match {
3033
case JsSuccess(pdsRegistrationRequest, _) =>
3134
pdsRegistrationService.registerPDS(

‎app/controllers/PublicationCandidateController.scala‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,8 @@ class PublicationCandidateController @Inject()(
8484
def bulkAction(): Action[AnyContent] = CuratorAction.async { implicit request =>
8585
val reviewerId = request.user.id.get
8686
val formData = request.body.asFormUrlEncoded.getOrElse(Map.empty)
87-
val ids = formData.getOrElse("candidateIds", Seq.empty).flatMap(_.split(",")).flatMap(_.toIntOption).toSeq
87+
val MaxBulkSize = 500
88+
val ids = formData.getOrElse("candidateIds", Seq.empty).flatMap(_.split(",")).flatMap(_.toIntOption).take(MaxBulkSize).toSeq
8889
val action = formData.get("bulkAction").flatMap(_.headOption).getOrElse("")
8990
val reason = formData.get("reason").flatMap(_.headOption)
9091

‎app/controllers/PublicationDiscoveryController.scala‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package controllers
22

3+
import actions.ApiSecurityAction
34
import actors.PublicationDiscoveryActor
45
import jakarta.inject.{Inject, Named, Singleton}
56
import org.apache.pekko.actor.ActorRef
@@ -9,10 +10,11 @@ import play.api.mvc.{Action, AnyContent, BaseController, ControllerComponents}
910
@Singleton
1011
class PublicationDiscoveryController @Inject()(
1112
val controllerComponents: ControllerComponents,
13+
secureApi: ApiSecurityAction,
1214
@Named("publication-discovery-actor") publicationDiscoveryActor: ActorRef
1315
) extends BaseController with Logging {
1416

15-
def triggerDiscovery(): Action[AnyContent] = Action {
17+
def triggerDiscovery(): Action[AnyContent] = secureApi {
1618
logger.info("Manually triggering publication discovery via API.")
1719
publicationDiscoveryActor ! PublicationDiscoveryActor.RunDiscovery
1820
Ok("Publication discovery run triggered.")

‎app/controllers/TreeController.scala‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -231,7 +231,8 @@ class TreeController @Inject()(val controllerComponents: MessagesControllerCompo
231231
case _: IllegalArgumentException =>
232232
Ok(views.html.fragments.error(s"Haplogroup $haplogroupName not found"))
233233
case e =>
234-
Ok(views.html.fragments.error(e.getMessage))
234+
logger.error(s"Error loading tree fragment: ${e.getMessage}", e)
235+
Ok(views.html.fragments.error("An unexpected error occurred while loading this view."))
235236
}
236237
}
237238

‎app/controllers/VariantApiController.scala‎

Lines changed: 62 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -28,25 +28,31 @@ class VariantApiController @Inject()(
2828
* Bulk add reference builds (coordinates) to existing variants.
2929
* Matches variants by name or rsId, then adds coordinates for the specified reference genome.
3030
*/
31+
private val MaxBulkSize = 1000
32+
3133
def bulkAddBuilds(): Action[BulkAddVariantBuildsRequest] =
3234
secureApi.jsonAction[BulkAddVariantBuildsRequest].async { request =>
33-
val requests = request.body.variants
34-
logger.info(s"Bulk add builds request for ${requests.size} variants")
35+
if (request.body.variants.size > MaxBulkSize) {
36+
Future.successful(BadRequest(Json.obj("error" -> s"Bulk operations limited to $MaxBulkSize items per request")))
37+
} else {
38+
val requests = request.body.variants
39+
logger.info(s"Bulk add builds request for ${requests.size} variants")
3540

36-
val resultFutures = requests.map(processAddBuildRequest)
41+
val resultFutures = requests.map(processAddBuildRequest)
3742

38-
Future.sequence(resultFutures).map { results =>
39-
val succeeded = results.count(_.status == "success")
40-
val failed = results.count(_.status != "success")
43+
Future.sequence(resultFutures).map { results =>
44+
val succeeded = results.count(_.status == "success")
45+
val failed = results.count(_.status != "success")
4146

42-
logger.info(s"Bulk add builds completed: $succeeded succeeded, $failed failed")
47+
logger.info(s"Bulk add builds completed: $succeeded succeeded, $failed failed")
4348

44-
Ok(Json.toJson(BulkVariantOperationResponse(
45-
total = results.size,
46-
succeeded = succeeded,
47-
failed = failed,
48-
results = results
49-
)))
49+
Ok(Json.toJson(BulkVariantOperationResponse(
50+
total = results.size,
51+
succeeded = succeeded,
52+
failed = failed,
53+
results = results
54+
)))
55+
}
5056
}
5157
}
5258

@@ -56,23 +62,27 @@ class VariantApiController @Inject()(
5662
*/
5763
def bulkUpdateRsIds(): Action[BulkUpdateRsIdsRequest] =
5864
secureApi.jsonAction[BulkUpdateRsIdsRequest].async { request =>
59-
val requests = request.body.variants
60-
logger.info(s"Bulk update rsIds request for ${requests.size} variants")
65+
if (request.body.variants.size > MaxBulkSize) {
66+
Future.successful(BadRequest(Json.obj("error" -> s"Bulk operations limited to $MaxBulkSize items per request")))
67+
} else {
68+
val requests = request.body.variants
69+
logger.info(s"Bulk update rsIds request for ${requests.size} variants")
6170

62-
val resultFutures = requests.map(processUpdateRsIdRequest)
71+
val resultFutures = requests.map(processUpdateRsIdRequest)
6372

64-
Future.sequence(resultFutures).map { results =>
65-
val succeeded = results.count(_.status == "success")
66-
val failed = results.count(_.status != "success")
73+
Future.sequence(resultFutures).map { results =>
74+
val succeeded = results.count(_.status == "success")
75+
val failed = results.count(_.status != "success")
6776

68-
logger.info(s"Bulk update rsIds completed: $succeeded succeeded, $failed failed")
77+
logger.info(s"Bulk update rsIds completed: $succeeded succeeded, $failed failed")
6978

70-
Ok(Json.toJson(BulkVariantOperationResponse(
71-
total = results.size,
72-
succeeded = succeeded,
73-
failed = failed,
74-
results = results
75-
)))
79+
Ok(Json.toJson(BulkVariantOperationResponse(
80+
total = results.size,
81+
succeeded = succeeded,
82+
failed = failed,
83+
results = results
84+
)))
85+
}
7686
}
7787
}
7888

@@ -138,7 +148,7 @@ class VariantApiController @Inject()(
138148
name = req.name,
139149
rsId = req.rsId,
140150
status = "error",
141-
message = Some(s"Database error: ${e.getMessage}")
151+
message = Some("A database error occurred while processing this request")
142152
)
143153
}
144154
}
@@ -184,7 +194,7 @@ class VariantApiController @Inject()(
184194
name = Some(req.name),
185195
rsId = Some(req.rsId),
186196
status = "error",
187-
message = Some(s"Database error: ${e.getMessage}")
197+
message = Some("A database error occurred while processing this request")
188198
)
189199
}
190200
}
@@ -218,7 +228,7 @@ class VariantApiController @Inject()(
218228
newSource = req.newSource,
219229
aliasesUpdated = 0,
220230
status = "error",
221-
message = Some(s"Database error: ${e.getMessage}")
231+
message = Some("A database error occurred while processing this request")
222232
)
223233
}
224234
}
@@ -305,28 +315,32 @@ class VariantApiController @Inject()(
305315
*/
306316
def bulkAssignDuNames(): Action[BulkAssignDuNamesRequest] =
307317
secureApi.jsonAction[BulkAssignDuNamesRequest].async { request =>
308-
val variantIds = request.body.variantIds
309-
logger.info(s"Bulk assign DU names request for ${variantIds.size} variants")
310-
311-
// Process sequentially to maintain name ordering
312-
variantIds.foldLeft(Future.successful(Seq.empty[DuNameAssignmentResult])) { (accFuture, variantId) =>
313-
accFuture.flatMap { acc =>
314-
processAssignDuName(variantId).map(result => acc :+ result)
315-
}
316-
}.map { results =>
317-
val succeeded = results.count(_.status == "success")
318-
val failed = results.count(_.status == "error")
319-
val skipped = results.count(_.status == "skipped")
320-
321-
logger.info(s"Bulk assign DU names completed: $succeeded succeeded, $failed failed, $skipped skipped")
318+
if (request.body.variantIds.size > MaxBulkSize) {
319+
Future.successful(BadRequest(Json.obj("error" -> s"Bulk operations limited to $MaxBulkSize items per request")))
320+
} else {
321+
val variantIds = request.body.variantIds
322+
logger.info(s"Bulk assign DU names request for ${variantIds.size} variants")
322323

323-
Ok(Json.toJson(BulkDuNameAssignmentResponse(
324-
total = results.size,
325-
succeeded = succeeded,
326-
failed = failed,
327-
skipped = skipped,
324+
// Process sequentially to maintain name ordering
325+
variantIds.foldLeft(Future.successful(Seq.empty[DuNameAssignmentResult])) { (accFuture, variantId) =>
326+
accFuture.flatMap { acc =>
327+
processAssignDuName(variantId).map(result => acc :+ result)
328+
}
329+
}.map { results =>
330+
val succeeded = results.count(_.status == "success")
331+
val failed = results.count(_.status == "error")
332+
val skipped = results.count(_.status == "skipped")
333+
334+
logger.info(s"Bulk assign DU names completed: $succeeded succeeded, $failed failed, $skipped skipped")
335+
336+
Ok(Json.toJson(BulkDuNameAssignmentResponse(
337+
total = results.size,
338+
succeeded = succeeded,
339+
failed = failed,
340+
skipped = skipped,
328341
results = results
329342
)))
343+
}
330344
}
331345
}
332346

0 commit comments

Comments
 (0)