From b900ca9285c8f7393c252d25da38dda2d6071bf8 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Wed, 28 Oct 2026 22:00:10 +0000 Subject: [PATCH] fix(api): Scope item and user lists and issue linking to an organization GET /api/items/ and GET /api/users/ returned rows from every organization when the organization query parameter was missing or did not match an organization. Both backends now return 400 when it is missing and 404 when it is unknown, and only ever return the requested organization's rows. In the Python backend the user filter on /api/items/ is now also applied when items are enriched with Sentry data. The issue-link 'link' endpoint looked up the selected item by ID alone, so an installation could link a Sentry issue to another organization's item. It now only matches items in the installation's organization and returns 404 otherwise. Refs IE-93 Co-Authored-By: Matt Duncan --- backend-py/src/api/endpoints/items.py | 29 +++++++++-------- .../src/api/endpoints/sentry/issue_link.py | 9 +++++- backend-py/src/api/endpoints/users.py | 21 ++++++------ backend-py/tests/api/items/test_index.py | 26 ++++++++++----- .../tests/api/sentry/test_issue_link.py | 12 +++++++ backend-py/tests/api/users/test_index.py | 8 +++-- backend-ts/src/api/items.ts | 32 +++++++++++-------- backend-ts/src/api/sentry/issueLink.ts | 12 ++++++- backend-ts/src/api/users.ts | 24 ++++++++------ backend-ts/tests/api/items.spec.ts | 12 +++++-- backend-ts/tests/api/sentry/issueLink.spec.ts | 12 +++++++ backend-ts/tests/api/users.spec.ts | 12 +++++-- 12 files changed, 143 insertions(+), 66 deletions(-) diff --git a/backend-py/src/api/endpoints/items.py b/backend-py/src/api/endpoints/items.py index ae305f6b..63a8e270 100644 --- a/backend-py/src/api/endpoints/items.py +++ b/backend-py/src/api/endpoints/items.py @@ -2,7 +2,7 @@ from flask import jsonify, request, Response from flask.views import MethodView -from werkzeug.exceptions import NotFound +from werkzeug.exceptions import BadRequest, NotFound from src.api.endpoints.base import register_api from src.api.serializers import serialize @@ -40,24 +40,27 @@ def _get_item_or_404(self, item_id: int) -> Item: return item def index(self) -> Response: + # Always scope items to a single organization so one tenant can never + # list another tenant's items. organization_slug = request.args.get("organization") - user_id = request.args.get("user") - - query = Item.query + if not organization_slug: + raise BadRequest("The 'organization' query parameter is required.") + organization = Organization.query.filter( + Organization.slug == organization_slug + ).first() + if not organization: + raise NotFound - if organization_slug: - organization_option = Organization.query.filter( - Organization.slug == organization_slug - ).first() - if organization_option: - query = query.filter(Item.organization_id == organization_option.id) - linked_query = query.filter(Item.sentry_id is not None) - if linked_query.count() > 0: - return add_sentry_api_data(organization_option, query) + query = Item.query.filter(Item.organization_id == organization.id) + user_id = request.args.get("user") if user_id is not None: query = query.filter(Item.assignee_id == user_id) + linked_query = query.filter(Item.sentry_id is not None) + if linked_query.count() > 0: + return add_sentry_api_data(organization, query) + return jsonify(serialize(query.all())) def get(self, item_id: int) -> Response: diff --git a/backend-py/src/api/endpoints/sentry/issue_link.py b/backend-py/src/api/endpoints/sentry/issue_link.py index 3853dbee..ecbf05a4 100644 --- a/backend-py/src/api/endpoints/sentry/issue_link.py +++ b/backend-py/src/api/endpoints/sentry/issue_link.py @@ -88,7 +88,14 @@ def link_issue_link() -> Response: user_link_data: IssueLinkSettings = request.json.get("fields") # Associate the Sentry Issue with the item from our application the user selected. - item = Item.query.filter(Item.id == user_link_data.get("itemId")).first() + # Only look up items in this installation's organization, so an installation + # can't link to another organization's items. + item = Item.query.filter( + Item.id == user_link_data.get("itemId"), + Item.organization_id == organization.id, + ).first() + if not item: + return Response("", 404) item.sentry_id = request.json.get("issueId") db_session.commit() app.logger.info("Linked item through Sentry Issue Link UI Component") diff --git a/backend-py/src/api/endpoints/users.py b/backend-py/src/api/endpoints/users.py index 80f1f5c4..9457b796 100644 --- a/backend-py/src/api/endpoints/users.py +++ b/backend-py/src/api/endpoints/users.py @@ -2,7 +2,7 @@ from flask import jsonify, request, Response from flask.views import MethodView -from werkzeug.exceptions import NotFound +from werkzeug.exceptions import BadRequest, NotFound from src.api.endpoints.base import register_api from src.api.serializers import serialize @@ -19,17 +19,18 @@ def _get_user_or_404(self, user_id: int) -> User: return user def index(self) -> Response: + # Always scope users to a single organization so one tenant can never + # list another tenant's users. organization_slug = request.args.get("organization") + if not organization_slug: + raise BadRequest("The 'organization' query parameter is required.") + organization = Organization.query.filter( + Organization.slug == organization_slug + ).first() + if not organization: + raise NotFound - query = User.query - - if organization_slug is not None: - organization_option = Organization.query.filter( - Organization.slug == organization_slug - ).first() - if organization_option: - query = query.filter(User.organization_id == organization_option.id) - + query = User.query.filter(User.organization_id == organization.id) return jsonify(serialize(query.all())) def get(self, user_id: int) -> Response: diff --git a/backend-py/tests/api/items/test_index.py b/backend-py/tests/api/items/test_index.py index 40975b62..6ff59add 100644 --- a/backend-py/tests/api/items/test_index.py +++ b/backend-py/tests/api/items/test_index.py @@ -16,18 +16,28 @@ def setUp(self): super().setUp() other_organization = self.create_organization("other") - other_user = self.create_user(other_organization, "other user") - self.create_item(other_organization, other_user) + self.other_user = self.create_user(other_organization, "other user") + self.create_item(other_organization, self.other_user) - def test_index(self): - response = self.get_success_response() - assert len(response.json) == 2 + def test_index_requires_organization(self): + self.get_error_response(status_code=400) + + def test_index_unknown_organization(self): + self.get_error_response(organization="does-not-exist", status_code=404) def test_index_user(self): - response = self.get_success_response(user=self.user.id) + self.create_item(self.organization) + response = self.get_success_response( + organization=self.organization.slug, user=self.user.id + ) - assert len(response.json) == 1 - assert response.json[0]["assigneeId"] == self.user.id + assert [item["id"] for item in response.json] == [self.item.id] + + def test_index_user_from_other_organization(self): + response = self.get_success_response( + organization=self.organization.slug, user=self.other_user.id + ) + assert response.json == [] def test_index_organization(self): response = self.get_success_response(organization=self.organization.slug) diff --git a/backend-py/tests/api/sentry/test_issue_link.py b/backend-py/tests/api/sentry/test_issue_link.py index a8f894bd..88ef5ebb 100644 --- a/backend-py/tests/api/sentry/test_issue_link.py +++ b/backend-py/tests/api/sentry/test_issue_link.py @@ -57,3 +57,15 @@ def test_link_issue_link(self): assert response.json.get("webUrl") is not None assert response.json.get("project") == "ACME" assert response.json.get("identifier") == str(self.item.id) + + def test_link_issue_link_other_organization_item(self): + other_organization = self.create_organization("other") + other_item = self.create_item(other_organization) + db_session.commit() + payload = { + **self.payload, + "fields": {**self.payload["fields"], "itemId": other_item.id}, + } + + self.get_error_response(data=payload, status_code=404) + assert other_item.sentry_id is None diff --git a/backend-py/tests/api/users/test_index.py b/backend-py/tests/api/users/test_index.py index 0094dac3..0dffb451 100644 --- a/backend-py/tests/api/users/test_index.py +++ b/backend-py/tests/api/users/test_index.py @@ -8,9 +8,11 @@ def setUp(self): other_organization = self.create_organization("other") self.create_user(other_organization, "other user") - def test_index(self): - response = self.get_success_response() - assert len(response.json) == 2 + def test_index_requires_organization(self): + self.get_error_response(status_code=400) + + def test_index_unknown_organization(self): + self.get_error_response(organization="does-not-exist", status_code=404) def test_index_organization(self): response = self.get_success_response(organization=self.organization.slug) diff --git a/backend-ts/src/api/items.ts b/backend-ts/src/api/items.ts index 16b5273b..a67d7357 100644 --- a/backend-ts/src/api/items.ts +++ b/backend-ts/src/api/items.ts @@ -29,22 +29,26 @@ async function addSentryAPIData( } router.get('/', async (request, response) => { + // Always scope items to a single organization so one tenant can never + // list another tenant's items. const {organization: slug} = request.query; - if (slug) { - const organization = await Organization.findOne({ - include: Item, - where: {slug}, - }); - if (organization) { - return response.send( - organization.items.some(item => !!item.sentryId) - ? await addSentryAPIData(organization) - : organization.items - ); - } + if (!slug) { + return response + .status(400) + .send({message: "The 'organization' query parameter is required"}); } - const items = await Item.findAll(); - return response.send(items); + const organization = await Organization.findOne({ + include: Item, + where: {slug}, + }); + if (!organization) { + return response.sendStatus(404); + } + return response.send( + organization.items.some(item => !!item.sentryId) + ? await addSentryAPIData(organization) + : organization.items + ); }); router.post('/', async (request, response) => { diff --git a/backend-ts/src/api/sentry/issueLink.ts b/backend-ts/src/api/sentry/issueLink.ts index 3206d1f3..83201830 100644 --- a/backend-ts/src/api/sentry/issueLink.ts +++ b/backend-ts/src/api/sentry/issueLink.ts @@ -57,7 +57,17 @@ router.post('/link', async (request, response) => { const issueLinkData = request.body as IssueLinkRequestData; // Associate the Sentry Issue with the item from our application the user selected. - const item = await Item.findByPk(issueLinkData.fields.itemId); + // Only look up items in this installation's organization, so an installation + // can't link to another organization's items. + const item = await Item.findOne({ + where: { + id: issueLinkData.fields.itemId, + organizationId: response.locals.organization.id, + }, + }); + if (!item) { + return response.sendStatus(404); + } await item.update({sentryId: issueLinkData.issueId}); console.info('Linked item through Sentry Issue Link UI Component'); diff --git a/backend-ts/src/api/users.ts b/backend-ts/src/api/users.ts index 5cffb6e1..2ce600f8 100644 --- a/backend-ts/src/api/users.ts +++ b/backend-ts/src/api/users.ts @@ -6,18 +6,22 @@ import User from '../models/User.model'; const router = express.Router(); router.get('/', async (request, response) => { + // Always scope users to a single organization so one tenant can never + // list another tenant's users. const {organization: slug} = request.query; - if (slug) { - const organization = await Organization.findOne({ - include: User, - where: {slug}, - }); - if (organization) { - return response.send(organization.users); - } + if (!slug) { + return response + .status(400) + .send({message: "The 'organization' query parameter is required"}); } - const users = await User.findAll(); - return response.send(users); + const organization = await Organization.findOne({ + include: User, + where: {slug}, + }); + if (!organization) { + return response.sendStatus(404); + } + return response.send(organization.users); }); router.post('/', async (request, response) => { diff --git a/backend-ts/tests/api/items.spec.ts b/backend-ts/tests/api/items.spec.ts index 62ebfdb3..c3a37ea8 100644 --- a/backend-ts/tests/api/items.spec.ts +++ b/backend-ts/tests/api/items.spec.ts @@ -26,10 +26,16 @@ describe(path, () => { afterAll(async () => await closeTestServer()); - it('handles GET all', async () => { + it('rejects GET without an organization', async () => { const response = await request(server).get(path); - assert.equal(response.body.length, itemTitles.length); - assert.equal(response.statusCode, 200); + assert.equal(response.statusCode, 400); + }); + + it('returns 404 for GET with an unknown organization', async () => { + const response = await request(server) + .get(path) + .query({organization: 'does-not-exist'}); + assert.equal(response.statusCode, 404); }); it('handles GET by organization', async () => { diff --git a/backend-ts/tests/api/sentry/issueLink.spec.ts b/backend-ts/tests/api/sentry/issueLink.spec.ts index 983d33b7..01b66a58 100644 --- a/backend-ts/tests/api/sentry/issueLink.spec.ts +++ b/backend-ts/tests/api/sentry/issueLink.spec.ts @@ -55,4 +55,16 @@ describe(`GET ${path}`, () => { assert.equal(response.body.project, 'ACME'); assert.equal(response.body.identifier, item.id); }); + + it("doesn't link to another organization's items", async () => { + const otherItem = await createItem(); + const payload = { + ...MOCK_ISSUE_LINK, + fields: {...MOCK_ISSUE_LINK.fields, itemId: otherItem.id}, + }; + const response = await request(server).post(`${path}/link`).send(payload); + assert.equal(response.statusCode, 404); + await otherItem.reload(); + assert.equal(otherItem.sentryId, null); + }); }); diff --git a/backend-ts/tests/api/users.spec.ts b/backend-ts/tests/api/users.spec.ts index a21afe3c..a0ea298e 100644 --- a/backend-ts/tests/api/users.spec.ts +++ b/backend-ts/tests/api/users.spec.ts @@ -21,10 +21,16 @@ describe(path, () => { afterAll(async () => await closeTestServer()); - it('handles GET', async () => { + it('rejects GET without an organization', async () => { const response = await request(server).get(path); - assert.equal(response.body.length, userNames.length); - assert.equal(response.statusCode, 200); + assert.equal(response.statusCode, 400); + }); + + it('returns 404 for GET with an unknown organization', async () => { + const response = await request(server) + .get(path) + .query({organization: 'does-not-exist'}); + assert.equal(response.statusCode, 404); }); it('handles GET by organization', async () => {