Skip to content

Fix notification mark-read ID bounds - #718

Merged
JSONbored merged 2 commits into
mainfrom
codex/fix-unbounded-mcp-notification-ids-issue
Jun 14, 2026
Merged

Fix notification mark-read ID bounds#718
JSONbored merged 2 commits into
mainfrom
codex/fix-unbounded-mcp-notification-ids-issue

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • Prevent a denial-of-service/resource-exhaustion path where an unbounded ids array (count or long strings) is forwarded into an SQL IN predicate.
  • Make ids: [] explicit as a no-op instead of being interpreted as omission and clearing all delivered notifications.

Description

  • Added shared limits MAX_NOTIFICATION_MARK_READ_IDS = 100 and MAX_NOTIFICATION_DELIVERY_ID_LENGTH = 128 in src/db/repositories.ts.
  • Enforced repository-level validation in markNotificationDeliveriesRead to return early for an empty ids array and to throw a RangeError when the array is too large or any ID is too long, before building SQL.
  • Changed the SQL predicate usage to only include the IN filter when ids is present (non-omitted) so empty arrays do not clear all notifications.
  • Applied the same bounds to the MCP tool input schema in src/mcp/server.ts using zod to reject oversized client requests early.
  • Added unit test coverage for the new behavior and regressions in test/unit/notifications-service.test.ts and test/unit/mcp-notifications.test.ts (empty-array semantics and oversized-filter rejection).

Testing

  • Ran type checking with npm run typecheck which completed successfully.
  • Ran the modified unit tests with npx vitest run test/unit/notifications-service.test.ts test/unit/mcp-notifications.test.ts and all tests passed (2 files, 15 tests).

Codex Task

@ghost

ghost commented Jun 14, 2026

Copy link
Copy Markdown

Note

Gittensory Gate skipped

PR closed before full evaluation. No late first comment was created.

Signal Result Evidence Action
Gate result ⚠️ Skipped #718 is no longer open. No action.

💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

@ghost ghost added the gittensory:reviewed label Jun 14, 2026
@ghost

ghost commented Jun 14, 2026

Copy link
Copy Markdown

gittensory · advisory review

Reviewed 4 changed file(s) — two independent AI reviewers.

Suggested action:Safe to merge — both reviewers found no blocking issues.

Reviewer A · gpt-oss-120b — recommends ✅ merge
The PR introduces limits for notification mark‑read IDs, adds validation in the repository and API schema, and updates tests to cover the new constraints and empty‑array handling. The changes are well‑scoped, type‑safe, and include corresponding test coverage.

Suggestions

  • Document the new MAX_NOTIFICATION_… constants and limits in the project README or API docs.
  • Search the codebase for any internal calls that previously relied on passing an empty array to clear all notifications and update them if needed.
  • Consider exporting a TypeScript type for the ID array shape to keep validation consistent across modules.

Worth double-checking

  • Behavior change: passing an empty ID array now results in a no‑op instead of clearing all notifications; ensure callers expect this.
  • If other external clients bypass the server schema and call the repository directly, they will now receive RangeError for oversized inputs.

Reviewer B · nemotron-3-120b-a12b — recommends ✅ merge
The PR adds bounds checking for the markNotificationDeliveriesRead function to prevent excessive resource usage: it validates that the ID array length doesn't exceed 100 and individual IDs don't exceed 128 characters. It also fixes a bug where passing an empty ID array would incorrectly mark all notifications as read (now correctly treated as a no-op). Changes are consistent across the repository layer, MCP server validation, and unit tests.

Suggestions

  • Consider adding a brief comment explaining the rationale for the chosen limits (100 IDs, 128 chars) in src/db/repositories.ts for future maintainers.
  • The test names could be slightly more descriptive (e.g., 'throws when ID count exceeds limit' instead of 'rejects oversized mark-read id filters'), but this is minor.

Worth double-checking

  • Verify that the RangeError thrown in the repository is properly caught and converted to a user-friendly error in the MCP layer (though Zod validation should prevent invalid inputs from reaching the repo in normal operation).
  • Ensure the constants (100, 128) align with any existing database constraints or UI expectations (e.g., notification ID length in the schema).

@ghost ghost added the gittensory-review label Jun 14, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

# Conflicts:
#	test/unit/mcp-notifications.test.ts
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jun 14, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
gittensory-ui 2147713 Commit Preview URL

Branch Preview URL
Jun 14 2026, 02:19 PM

@JSONbored
JSONbored merged commit cea76c0 into main Jun 14, 2026
11 checks passed
@JSONbored
JSONbored deleted the codex/fix-unbounded-mcp-notification-ids-issue branch June 14, 2026 14:26
@github-project-automation github-project-automation Bot moved this from Todo to Done in gittensory - v1 roadmap Jun 14, 2026
@github-actions github-actions Bot mentioned this pull request Jun 14, 2026
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant