Skip to content

fix(admin): reject values that break the app - #886

Merged
ajslater merged 1 commit into
feat/choices-limitsfrom
fix/validation-server-bounds
Sep 21, 2026
Merged

ajslater merged 1 commit into
feat/choices-limitsfrom
fix/validation-server-bounds

Conversation

@ajslater

Copy link
Copy Markdown
Owner

Implements P3c of tasks/followups-implementation-plan.md §5.3, including the "items per page" rider.

Stacked on #885 (limits.py), which it consumes. Retarget to develop once that merges.

Three holes where a direct API call saves a value the app cannot survive. The client's rules were the only thing in the way, and a rule in a browser is not validation.

Two live breakages

Items per page (MP) = 0 is a 500. get_browser_max_obj_per_page honours "0" — only an empty or unparseable value falls back to the default — and num_pages = ceil(full_count / …) raises ZeroDivisionError. The same division appears in paginate.py, both OPDS feeds, publications.py, groups.py and the search parser, so browsing stops working entirely until an admin fixes the flag from a UI they can no longer reach.

Upload cap (CM) = -1 makes every custom-cover upload fail with "Upload exceeds -1 MB limit".

Both are stored in a CharField shared with every other flag, so a MinValueValidator cannot live on the field — AdminFlagSerializer.validate is the only place the bound can be expressed. It gains a per-key range map beside the existing BROWSER_DEFAULT_COLLECTION branch, and validate splits into two named helpers rather than growing a third arm inline.

One corrected premise

D6 called the SMTP save path "unbounded". It is not literally unbounded — Django derives a range from the integer type and DRF lifts it into the serializer field. But measured here:

django derived range for port: (0, 9223372036854775807)

So port 0 and port 70000 both save, while negatives are refused. The practical gap stands; the wording should not say "enforces nothing".

Bounds are declared at the serializer level, not as model validators=[...] — validators deconstruct, so those would cost an AlterField migration for no schema change. required=False mirrors what ModelSerializer already inferred from the model defaults, so declaring the fields changes the bounds and nothing else.

The client needed a gate, not a rule

flag-card.vue has no v-form and changeCol PATCHes on every keystroke — so a :rules binding displays a message while the write goes through anyway. The rule is added and changeCol refuses an out-of-range value before calling updateRow.

Both client controls now read their bounds from the generated limits.json instead of retyping them: flag-card.vue for the page size, and custom-covers-tab.vue's hardcoded MAX_UPLOAD_MIN / MAX_UPLOAD_MAX for the upload cap. The static rule arrays live in data, not computed.

Tests

tests/test_admin_value_bounds.py, 11 cases. Verified load-bearing by removing each bound and re-running:

Pre-fix
page size 0 rejected ❌ saved
page size 999999 rejected ❌ saved
upload cap -1 rejected ❌ saved
non-numeric flag value rejected ❌ saved
port 0 rejected ❌ saved
port 70000 rejected ❌ saved
timeout 0 / 6000 rejected ❌ saved

Plus four that must keep passing: a valid page size, a valid port, and — importantly — test_a_text_flag_is_unaffected, so the int bounds cannot leak onto flags holding strings.

The rejecting cases also assert the stored value is unchanged and that the runtime getter still returns something usable, rather than only checking the status code.

make fix && make lint && make ty clean. Full make test green: 1275 pytest (+11), 583 vitest.

NEWS, under Fixes: "Invalid admin settings values are rejected instead of breaking browsing."

🤖 Generated with Claude Code

Three holes where a direct API call could save a value the app cannot
survive. The client's rules were the only thing in the way, and a rule
in a browser is not validation.

A saved page size of 0 is a live 500: `get_browser_max_obj_per_page`
honours "0" -- only an empty or unparseable value falls back -- and
`ceil(count / 0)` raises ZeroDivisionError, so browsing stops working
entirely until an admin fixes the flag from a UI they can no longer
reach. A saved upload cap of -1 makes every custom-cover upload fail
with "Upload exceeds -1 MB limit". Neither value is rejected anywhere
today.

Both live in a text column shared with every other flag, so a
MinValueValidator cannot go on the field; `AdminFlagSerializer.validate`
is the only place the bound can be expressed. It grows a per-key range
map beside the existing per-key branch, and `validate` splits into two
named helpers rather than growing a third arm inline.

The SMTP port and timeout are not literally unbounded -- Django derives
a range from the integer type and DRF lifts it into the field -- but
the derived range is (0, 9223372036854775807), measured here, so port 0
and port 70000 both save while negatives are refused. Explicit
serializer-level bounds, not model `validators=[...]`: validators
deconstruct, so those would cost an AlterField migration for no schema
change.

On the client, `flag-card.vue` has no v-form and PATCHes on every
keystroke, so a rule displays a message without stopping the write. The
write itself is gated. Both client controls now read their bounds from
the generated limits file rather than retyping them.

Each test fails against the unbounded code.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant