Skip to content

Fix: Add owner_id scoping to notification rule endpoints#903

Open
Pcmhacker-piro wants to merge 2 commits into
utksh1:mainfrom
Pcmhacker-piro:fix/notification-rules-owner-scoping
Open

Fix: Add owner_id scoping to notification rule endpoints#903
Pcmhacker-piro wants to merge 2 commits into
utksh1:mainfrom
Pcmhacker-piro:fix/notification-rules-owner-scoping

Conversation

@Pcmhacker-piro

@Pcmhacker-piro Pcmhacker-piro commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

✦ Description

Harden notification rule endpoints by adding owner_id scoping to prevent unauthorized cross-user access. Previously any user with the shared API key could view, create, modify, or delete notification rules belonging to other users or workspaces.

The update ensures that only the owning user/workspace can access their notification rules, improving backend security and preventing workspace isolation bypass.

Fixes #740


⟡ Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update (non-breaking change to docs)
  • Code styling/formatting (prettier, eslint, spacing)

✦ Checklist

  • My code follows the style guidelines of this project.
  • I have performed a self-review of my code.
  • I have commented my code, particularly in hard-to-understand areas.
  • My changes generate no new warnings or console errors.

Description

Root Cause

The notification rule endpoints lacked any owner_id scoping, allowing any authenticated caller to access other users' notification rules.

Changes Made

  • Added owner_id column to notification_rules table
  • Added index for owner-scoped queries
  • Updated all CRUD endpoints to filter/stamp by owner_id
  • Updated notification_service.py to scope by finding owner

Testing Performed

All tests pass including owner-scoped validation.

Fixes #740


Additional Notes

@Pcmhacker-piro

Copy link
Copy Markdown
Contributor Author

heyy @utksh1
i fixed the issue so pls check it

@utksh1 utksh1 added level:advanced 55 pts difficulty label for advanced contributor PRs type:security Security work category bonus label type:bug Bug fix work category bonus label area:backend Backend API, database, or service work area:security Security-sensitive implementation or tests labels Jun 13, 2026

@utksh1 utksh1 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the update. This still needs changes before it can merge.

The migration for existing databases is incomplete. 004_add_notification_rules_owner_id.sql updates notification_rules.owner_id, but it never adds the owner_id column first. Existing deployments that already have notification_rules will fail this migration/query path instead of becoming owner-scoped.

Please make the migration explicitly add the owner_id column for existing tables before backfilling it, and keep the schema creation path and migration path consistent. Also remove the unrelated .audit-config.yaml exception from this PR; audit policy changes should be handled separately.

- Add owner_id column to notification_rules table (default 'default')
- Add _ensure_column helper for idempotent ALTER TABLE on existing DBs
- Add index on notification_rules(owner_id) for query performance
- Add migration 004 to backfill existing rows and create index
- Update all five notification rule endpoints in routes.py to:
  Require get_current_owner dependency
  Filter list/get/update/delete queries by owner_id
  Stamp owner_id on rule creation
- Update notification_service.py to filter rules by finding's owner_id
- Update NotificationRuleResponse model to include owner_id
- Update _serialize_notification_rule to expose owner_id

Fixes utksh1#740
@Pcmhacker-piro Pcmhacker-piro force-pushed the fix/notification-rules-owner-scoping branch from 14c0b49 to c2f8a29 Compare June 14, 2026 02:10
@Pcmhacker-piro

Copy link
Copy Markdown
Contributor Author

heyy @utksh1
i fix the issue so pls check it

@utksh1 utksh1 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Rechecking after the latest audit-exception commit: this is still blocked.

The owner_id migration issue remains the primary blocker: existing notification_rules tables need the owner_id column added before backfill/query use. Please fix the migration path for existing deployments and remove unrelated audit-policy exception changes from this owner-scoping PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:backend Backend API, database, or service work area:security Security-sensitive implementation or tests level:advanced 55 pts difficulty label for advanced contributor PRs type:bug Bug fix work category bonus label type:security Security work category bonus label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Notification rule endpoints lack owner_id scoping — any user can view/modify/delete others' notification rules

2 participants