Skip to content
This repository was archived by the owner on Sep 21, 2026. It is now read-only.

fix: handle ValueError on invalid ?limit= query parameter - #617

Closed
velezf wants to merge 4 commits into
mainfrom
fix/api-limit-valueerror
Closed

velezf wants to merge 4 commits into
mainfrom
fix/api-limit-valueerror

Conversation

@velezf

@velezf velezf commented May 4, 2026

Copy link
Copy Markdown

Problem

_get_limit() in api.py passed the ?limit= query parameter directly
to int() without error handling. A non-integer value like ?limit=abc
or ?limit= caused an unhandled ValueError, returning a 500 server error
instead of a meaningful 400 Bad Request.

This also bypassed the TRACK_LIMIT_MAX check entirely, creating a potential
DoS vector noted in issue #310.

Fix

  • Wrapped int(limit) in try/except ValueError, returning abort(400)
  • Added explicit check rejecting non-positive values (limit < 1)
  • Removed redundant int() call on the return statement

Testing

Tested locally with curl against a running dev server:

  • ?limit=50 → 200 OK
  • ?limit=abc → 400 Bad Request
  • ?limit=0 → 400 Bad Request
  • `?l

The limit parameter in _get_limit() was passed directly to int() without
error handling, causing an unhandled 500 error when a non-integer value
like 'abc' or '' was provided.

Changes:
- Wrap int(limit) in try/except ValueError, returning 400 Bad Request
- Add explicit check for non-positive values (limit < 1)
- Remove redundant int() call on return statement

Fixes the DoS concern noted in issue #310.
@velezf velezf self-assigned this May 4, 2026
@velezf velezf added the bug label May 4, 2026
@github-project-automation github-project-automation Bot moved this to In progress in Freezing Saddles May 4, 2026
merlinorg added a commit to freezingsaddles/freezing that referenced this pull request Sep 19, 2026
freezingsaddles/freezing-web#617 by velezf: ?limit= on the track endpoints went
straight into int(), so anything that is not a number was a 500 with a traceback
rather than a refusal. A negative one got through the check and reached SQL as a
negative LIMIT, which was a 500 by another route. Both are 400s now, and the
function returns the limit it validated instead of taking the minimum again. The
rest of that pull request is repository furniture that the monorepo already has
its own versions of.

freezingsaddles/freezing-web#503 by obscurerichard: the beginning of year
procedure never said to clear the activity and weather caches, so last season's
cached responses carried into the new one. The script it calls is already here
as deploy/bin/clear-cache-directories.sh.

Checked against the running app: a non-numeric, empty, zero or negative limit is
a 400 where it used to be a 500, a limit past TRACK_LIMIT_MAX is still a 400,
and 1, the maximum itself and no limit at all still answer.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@merlinorg

Copy link
Copy Markdown
Contributor

Thank you — this is a real fix and it has been carried into the monorepo at freezingsaddles/freezing#49, which is what deploys now. ?limit=abc was a 500 with a traceback there too, and a negative limit reached SQL as a negative LIMIT; both are 400s now, with your credit in the commit message.

Closing here because this repository is no longer the one that ships. Sorry it sat for so long.

@merlinorg merlinorg closed this Sep 19, 2026
@github-project-automation github-project-automation Bot moved this from In progress to Done in Freezing Saddles Sep 19, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants