Skip to content

Reject an oversized statuses filter before it reaches Firestore - #10038

Merged
kodjima33 merged 1 commit into
BasedHardware:mainfrom
ZachL111:zach/integration-statuses-cap
Jul 20, 2026
Merged

Reject an oversized statuses filter before it reaches Firestore#10038
kodjima33 merged 1 commit into
BasedHardware:mainfrom
ZachL111:zach/integration-statuses-cap

Conversation

@ZachL111

@ZachL111 ZachL111 commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

GET /v2/integrations/{app_id}/conversations declares

statuses: List[str] = Query([]),

with no length cap, and passes it straight to conversations_db.get_conversations, which builds

conversations_ref.where(filter=FieldFilter('status', 'in', statuses))

Firestore rejects an in filter with more than 30 values. This repo already knows that and
guards for it in two other places:

  • database/apps.py: # Firestore 'in' limited to 30 items
  • database/chat.py: # Firestore IN operator supports max 30 values, so chunk the queries

This route has no such guard, and nothing wraps the stream() call, so a request repeating the
query key more than thirty times (?statuses=a&statuses=b&...) raises out of the Firestore client
and surfaces as an unhandled HTTP 500 rather than a 4xx.

ConversationStatus has five legitimate values (in_progress, processing, merging,
completed, failed), so no real client is anywhere near this. It is malformed-input surface
reachable by any caller holding a valid per-app integration API key.

Two deliberate choices

Reject rather than clamp. That matches this file's own idiom — every other invalid-input case
raises HTTPException — and the convention for oversized list inputs elsewhere
(action_items.py bounds its id lists with Field(..., max_length=...) and rejects). With only
five valid values, a cap of twenty cannot reject anything a real client would send.

In the handler body, not Query(..., max_length=20). Declaring it on the parameter would
change the generated OpenAPI, and docs/api-reference is a compatibility boundary per
backend/AGENTS.md; the Query([]) declaration is left byte-identical so no spec or generated
client moves. It also keeps the bound testable — a Query-level constraint is enforced by
FastAPI's request-parsing layer, which a direct handler call bypasses entirely, so an in-body
check is the only form a hermetic unit test can actually exercise.

Tests

tests/unit/test_integration_conversations_statuses_cap.py:

  • an oversized statuses list is rejected with 400 before reaching the database
  • a normal statuses list reaches the database unmodified

The fake standing in for the database seam mirrors real Firestore behaviour by raising above
thirty values, so the test asserts what the DB layer actually received rather than only the
response shape.

Verification

Run in backend/:

  • pytest tests/unit/test_integration_conversations_statuses_cap.py: 2 passed
  • prove-fail: with routers/integration.py reverted to origin/main and confirmed byte-identical
    (git diff origin/main empty), the oversized case fails with
    expected a clean HTTPException(400), got Exception("400 Bad Request: 'in' filters support a maximum of 30 elements."), while the normal-path test passes both ways. Re-applied: 2 passed
  • pytest with test_integration_malformed_records.py: 10 passed
  • the test imports routers.integration at module scope, so the heavy router import is collection
    cost rather than per-test CPU; both tests are 0.02s or less in the call phase, and the file
    passes under the strict duration guard (BACKEND_FAST_UNIT_FAIL_SECONDS=0.12, exit 0)
  • black --line-length 120 --skip-string-normalization --check: clean
  • scripts/check_module_stub_pollution.py: 736 files, 0 violations
  • scripts/scan_import_time_side_effects.py: 717 files, 0 violations
  • routers/integration.py is 740 lines and is not in the product file line-count ratchet baseline

Related, not fixed here

routers/conversations.py's /v1/conversations has the same underlying exposure through a
comma-separated statuses: Optional[str] parameter. Different parameter shape and a different
route, so I left it alone rather than widen this PR; worth a follow-up.

Review in cubic

GET /v2/integrations/{app_id}/conversations declares

    statuses: List[str] = Query([]),

with no length cap, and passes it straight to conversations_db.get_conversations, which builds

    conversations_ref.where(filter=FieldFilter('status', 'in', statuses))

Firestore rejects an `in` filter with more than 30 values. This repo already knows that and
guards for it in two other places (database/apps.py: "Firestore 'in' limited to 30 items",
database/chat.py: "Firestore IN operator supports max 30 values, so chunk the queries"). This
route has no such guard, and nothing wraps the stream() call, so a request repeating the query
key more than thirty times raises out of the Firestore client and surfaces as an unhandled
HTTP 500 rather than a 4xx.

ConversationStatus has five legitimate values (in_progress, processing, merging, completed,
failed), so no real client is anywhere near this. It is malformed-input surface reachable by any
caller holding a valid per-app integration API key.

The cap rejects rather than clamps, matching this file's own idiom (every other invalid-input
case raises HTTPException) and the convention for oversized list inputs elsewhere in the codebase
(action_items.py bounds its id lists with Field(..., max_length=...) and rejects). With only five
valid values, a cap of twenty cannot reject a request any real client would send.

The check is in the handler body rather than as Query(..., max_length=20) on purpose. Declaring it
on the parameter would change the generated OpenAPI, and docs/api-reference is a compatibility
boundary; the Query([]) declaration is left byte-identical so no spec or generated client moves.
It also keeps the bound testable: a Query-level constraint is enforced by FastAPI's request
parsing, which a direct handler call bypasses entirely, so an in-body check is the only form a
hermetic unit test can actually exercise.

Tests (tests/unit/test_integration_conversations_statuses_cap.py):
- an oversized statuses list is rejected with 400 before reaching the database
- a normal statuses list reaches the database unmodified

The fake standing in for the database seam mirrors real Firestore behaviour by raising above
thirty values, so the test asserts what the DB layer actually received rather than only the
response shape.

Verification (run in backend/):
- pytest tests/unit/test_integration_conversations_statuses_cap.py: 2 passed
- prove-fail: with routers/integration.py reverted to origin/main and confirmed byte-identical
  (git diff origin/main empty), the oversized case fails with
  "expected a clean HTTPException(400), got Exception(\"400 Bad Request: 'in' filters support a
  maximum of 30 elements.\")", while the normal-path test passes both ways. Re-applied: 2 passed
- pytest with test_integration_malformed_records.py: 10 passed
- the test imports routers.integration at module scope, so the heavy router import is collection
  cost rather than per-test CPU; both tests are 0.02s or less in the call phase and the file
  passes under the strict duration guard (BACKEND_FAST_UNIT_FAIL_SECONDS=0.12, exit 0)
- black --line-length 120 --skip-string-normalization --check: clean
- scripts/check_module_stub_pollution.py: 736 files, 0 violations
- scripts/scan_import_time_side_effects.py: 717 files, 0 violations
- routers/integration.py is 740 lines and is not in the product file line-count ratchet baseline

@kodjima33 kodjima33 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Backend hardening (reject oversized statuses filter). Approve-only — Hygiene CI red; author to fix lint.

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Verified this end-to-end and it holds up cleanly. The route was forwarding an unbounded statuses straight into a single Firestore FieldFilter('status','in',...) with no guard, and Firestore rejects in filters over 30 values — so a caller with a valid integration key could force an unhandled 500 where a 400 belonged. The 20-value cap sits well under Firestore's 30 and well above the 5 real ConversationStatus values, so it cannot reject any legitimate request.

I checked each claim in the description against the source:

  • The 30-value Firestore limit is real and already guarded in database/apps.py and database/chat.py, but not on this route.
  • conversations_db.get_conversations does apply statuses as a single unchunked in filter.
  • Doing the check in the handler body rather than via Query(max_length=20) correctly keeps the released integration-public-openapi.json byte-identical (its statuses param has no maxItems), and the Public Developer API contract check is green on this head.

The tests cover the right behavior: an oversized list is rejected as a clean 400 without ever reaching the db seam, and a normal list passes through unmodified.

One small note, not blocking: the os.environ.setdefault(...) env seeding in the new test follows the same fixture pattern as existing unit tests (the same ENCRYPTION_SECRET value appears in test_firestore_di_seam.py and test_review_queue_sort_tz.py), so it's consistent with the suite — worth being aware that this is a pre-existing shared fixture, not something this PR introduces.

Strong positive signal from automated review. Leaving formal approval to a human maintainer per the repo's review policy on integration/data-handling-adjacent endpoints.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

@kodjima33
kodjima33 merged commit a612f54 into BasedHardware:main Jul 20, 2026
29 of 32 checks passed
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.

3 participants