Skip to content

Commit

Permalink
feat(/sources): limit DELETE requests to MAX_BATCH_SIZE
Browse files Browse the repository at this point in the history
  • Loading branch information
cfm committed Sep 12, 2024
1 parent 924c96c commit 23d3e22
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 9 deletions.
2 changes: 1 addition & 1 deletion securedrop/journalist_app/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import template_filters
import version
from db import db
from flask import Flask, abort, g, json, redirect, render_template, request, url_for
from flask import Flask, g, json, redirect, render_template, request, url_for
from flask_babel import gettext
from flask_wtf.csrf import CSRFError, CSRFProtect
from journalist_app import account, admin, api, col, main
Expand Down
24 changes: 16 additions & 8 deletions securedrop/journalist_app/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
from two_factor import OtpSecretInvalid, OtpTokenInvalid
from werkzeug.exceptions import default_exceptions

MAX_BATCH_SIZE = 50 # requests


def get_or_404(model: db.Model, object_id: str, column: Column) -> db.Model:
result = model.query.filter(column == object_id).one_or_none()
Expand Down Expand Up @@ -128,24 +130,30 @@ def get_all_sources() -> Tuple[flask.Response, int]:
@api.route("/sources", methods=["DELETE"])
def delete_sources() -> Tuple[flask.Response, int]:
"""
Given a list of `Source` UUIDs, iterate over the list and try to delete
each `Source`. Return HTTP 200 "Success" if all `Source`s were deleted
or HTTP 207 "Multi-Status" if some failed. (There's an argument for
HTTP 202 "Accepted", since filesystem-level deletion is still deferred
to the shredder; but that's an implementation detail that we can hide
from the client.)
Given a list of at most `MAX_BATCH_SIZE `Source` UUIDs, iterate over the
list and try to delete each `Source`. Return HTTP 200 "Success" if all
`Source`s were deleted or HTTP 207 "Multi-Status" if some failed.
(There's an argument for HTTP 202 "Accepted", since filesystem-level
deletion is still deferred to the shredder; but that's an implementation
detail that we can hide from the client.)
Batching (under `MAX_BATCH_SIZE`) and retrying (of `Source`s returned as
`failed`) are responsibilities of the client.
NB. According to RFC 9110 §9.3.5, a client may not assume that a DELETE
endpoint will accept a request body, but a DELETE endpoint may do so,
and in our case we can rule out middleboxes that might mangle it in
transit.
"""
if not isinstance(request.json, list):
data = request.json
if not isinstance(data, list):
abort(400, "no sources specified")
elif len(data) > MAX_BATCH_SIZE:
abort(413, f"bulk requests may have at most {MAX_BATCH_SIZE} items")

succeeded = []
failed = []
for source_uuid in request.json:
for source_uuid in data:
try:
# Don't use `get_or_404()`: we'll handle the `NoResultFound`
# case ourselves, rather than abort with HTTP 404.
Expand Down

0 comments on commit 23d3e22

Please sign in to comment.