Skip to content

Differentiate authentication/request error messages (#274) #3633

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wakqasahmed wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Differentiate authentication/request error messages (#274) #3633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
26ab099
fix: differentiate authentication/request error messages (#274)
wakqasahmed Aug 15, 2026
914af9c
fix: reject non-finite chapterId and out-of-range limit/page (#274)
wakqasahmed Aug 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 44 additions & 5 deletions main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import functools
import math
from flask import Flask, jsonify, request, abort
from sqlalchemy import and_, func, or_
from werkzeug.exceptions import HTTPException
Expand All @@ -11,8 +12,14 @@

@app.before_request
def verify_secret():
if not app.debug and request.headers.get("x-aws-secret") != app.config["AWS_SECRET"]:
abort(401)
if app.debug:
return

secret = request.headers.get("x-aws-secret")
if secret is None:
abort(401, "Missing 'x-aws-secret' header.")
if secret != app.config["AWS_SECRET"]:
abort(401, "Invalid 'x-aws-secret' header value.")

@wakqasahmed wakqasahmed Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low / informational. Two small things about differentiating these two 401s:

  1. The response body now names the internal auth header (x-aws-secret) to unauthenticated callers, which the previous generic Werkzeug message did not. That is a small disclosure of the internal gateway mechanism. If the intent is to help legitimate integrators, naming the header in the "missing" case is defensible; echoing it again in the "invalid value" case adds nothing they do not already know and gives a scanner a positive signal that the header is the right one. Consider collapsing the second message to something that does not confirm header correctness, or keep both and accept the (small) tradeoff deliberately.

  2. Unrelated to this PR but adjacent to the line you are editing: secret != app.config["AWS_SECRET"] is a non-constant-time comparison. Pre-existing, not introduced here — flagging only because you are already in this block; hmac.compare_digest would be a one-line hardening if you want it, otherwise leave it for a separate change.



@app.errorhandler(HTTPException)
Expand All @@ -22,11 +29,36 @@ def jsonify_http_error(error):
return jsonify(response), error.code


MAX_PARAM_ECHO_LEN = 50


def _truncate_param(value):
value = str(value)
if len(value) > MAX_PARAM_ECHO_LEN:
return value[:MAX_PARAM_ECHO_LEN] + "..."
return value


def paginate_results(f):
@functools.wraps(f)
def decorated_function(*args, **kwargs):
limit = int(request.args.get("limit", 50))
page = int(request.args.get("page", 1))
limit_param = request.args.get("limit", 50)
page_param = request.args.get("page", 1)

try:
limit = int(limit_param)
except (TypeError, ValueError):
abort(400, f"Invalid 'limit' query parameter: '{_truncate_param(limit_param)}' is not an integer.")

try:
page = int(page_param)
except (TypeError, ValueError):
abort(400, f"Invalid 'page' query parameter: '{_truncate_param(page_param)}' is not an integer.")

if limit < 1:
abort(400, "Invalid 'limit' query parameter: must be >= 1.")
if page < 1:
abort(400, "Invalid 'page' query parameter: must be >= 1.")

queryset = f(*args, **kwargs).paginate(page=page, per_page=limit, max_per_page=100)

@wakqasahmed wakqasahmed Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main finding: range is still unvalidated, so the misleading-error case from #274 survives on this exact code path.

The try/except int(...) above only rules out non-integers. Flask-SQLAlchemy 2.5.1 BaseQuery.paginate() (pinned in requirements.txt) then does its own validation with error_out=True defaulted on:

  • page < 1 -> bare abort(404)
  • per_page < 0 -> bare abort(404)
  • not items and page != 1 -> bare abort(404)

So after this PR:

  • ?page=0 -> {"error": {"details": "Not Found", "code": 404}}
  • ?page=-1 -> same
  • ?limit=-1 -> same
  • ?page=999999 (past the last page) -> same

Those are exactly the "error message does not tell you what is actually wrong" reports #274 is about, and arguably more common in practice than passing a non-integer. A caller who typos ?page=0 gets a 404 that reads as "this collection does not exist".

Suggested minimum: after the two int() conversions, add

if limit < 1:
    abort(400, f"Invalid 'limit' query parameter: must be >= 1 (max 100).")
if page < 1:
    abort(400, f"Invalid 'page' query parameter: must be >= 1.")

and consider whether an out-of-range page should stay a 404 or become a 400 with a message naming the last valid page (error_out=False + an explicit check gives you that).

Two related edge cases while you are here:

  • ?limit=0 slips past per_page < 0 and yields LIMIT 0 -> a 200 with "data": [] and "limit": 0, which is silently wrong rather than an error.
  • A very large ?page= (e.g. 10**19) is a valid Python int and reaches the DB as an enormous OFFSET, which MySQL can reject outright -> another unformatted 500. An upper bound, or catching the driver error, closes that.

Note also that ?limit=1000 is silently clamped by max_per_page=100 rather than reported — consistent with the current spec (maximum: 100) so probably fine to leave, but worth a conscious decision given the PR's theme.

result = {
Expand Down Expand Up @@ -123,7 +155,14 @@ def api_hadiths():

chapter_id = request.args.get("chapterId")
if chapter_id:
query = query.filter_by(babID=float(chapter_id))
try:
parsed_chapter_id = float(chapter_id)
except (TypeError, ValueError):
abort(400, f"Invalid 'chapterId' query parameter: '{_truncate_param(chapter_id)}' is not a number.")
if not math.isfinite(parsed_chapter_id):
abort(400, f"Invalid 'chapterId' query parameter: '{_truncate_param(chapter_id)}' is not a finite number.")
chapter_id = parsed_chapter_id
query = query.filter_by(babID=chapter_id)

hadith_number = request.args.get("hadithNumber")
if hadith_number:
Expand Down
10 changes: 10 additions & 0 deletions spec.v1.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ paths:
items:
$ref: "#/components/schemas/Collection"
- $ref: "#/components/schemas/PaginatedResponse"
"400":
description: Bad request (invalid 'limit' or 'page' query parameter)
parameters:
- $ref: "#/components/parameters/limit"
- $ref: "#/components/parameters/page"
Expand Down Expand Up @@ -86,6 +88,8 @@ paths:
items:
$ref: "#/components/schemas/Book"
- $ref: "#/components/schemas/PaginatedResponse"
"400":
description: Bad request (invalid 'limit' or 'page' query parameter)
parameters:
- in: path
name: collectionName
Expand Down Expand Up @@ -137,6 +141,8 @@ paths:
items:
$ref: "#/components/schemas/Chapter"
- $ref: "#/components/schemas/PaginatedResponse"
"400":
description: Bad request (invalid 'limit' or 'page' query parameter)
parameters:
- in: path
name: collectionName
Expand Down Expand Up @@ -201,6 +207,8 @@ paths:
items:
$ref: "#/components/schemas/Hadith"
- $ref: "#/components/schemas/PaginatedResponse"
"400":
description: Bad request (invalid 'limit' or 'page' query parameter)
parameters:
- in: path
name: collectionName
Expand Down Expand Up @@ -258,6 +266,8 @@ paths:
items:
$ref: "#/components/schemas/Hadith"
- $ref: "#/components/schemas/PaginatedResponse"
"400":
description: Bad request (invalid 'limit', 'page', or 'chapterId' query parameter)
parameters:
- in: query
name: collection
Expand Down