fix(knowledge): scope keyword detail/delete to the caller's tenant (cross-org IDOR)#68
Open
dmitrymaranik wants to merge 1 commit into
Open
Conversation
GET /v1/knowledge/keywords/detail and DELETE /v1/knowledge/keywords were not scoped to the caller's org/user, so any authenticated user could read or permanently delete another organization's KB keyword by its enumerable integer id. The sibling list/repeat-check helpers already scope via WithPermit(org,user) and the other /knowledge/* routes carry AuthKnowledge; these two were the exception. - GetKnowledgeKeywordsDetail: verify the loaded keyword belongs to the caller (req.Identity) before returning. - bff DeleteDocCategoryKeywords: gate the delete on the now tenant-scoped detail lookup (the delete RPC carries no Identity).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The keyword ("专名词") endpoints
GET /v1/knowledge/keywords/detailandDELETE /v1/knowledge/keywordswere not scoped to the caller's tenant (org_id+user_id), so any authenticated user could read or permanently delete another organization's keyword by its (small, enumerable) integerid. Their sibling endpoints already enforce the boundary — the list / repeat-check helpers usesqlopt.WithPermit(org, user), and the other/knowledge/*routes carrymiddleware.AuthKnowledge(...)— these two were the exception.Root cause:
knowledge-serviceGetKnowledgeKeywordsDetailcallsorm.GetKeywordsById(id)(scoped byidonly) and never checks theIdentityalready on the request.DeleteDocCategoryKeywordsforwards onlyIdtoDeleteKnowledgeKeywords, and the ORMDeleteKeywordsis a bare-idUnscoped()(hard) delete.Change
GetKnowledgeKeywordsDetail— after loading the keyword, verify it belongs to the caller (req.Identity.OrgId/UserId); otherwise return the existing not-found error. Root-cause fix for the read, using theIdentityalready present on the request.DeleteDocCategoryKeywords— gate the delete on that now tenant-scoped detail lookup, so a caller can only delete a keyword that is their own (the delete RPC itself carries noIdentity).No behaviour change for legitimate same-tenant calls. 18 lines, no proto changes.
Defense-in-depth follow-up (not in this PR)
Thread the caller
IdentityintoDeleteKnowledgeKeywordsReqand scope the ORMDeleteKeywordswithWithPermit(org, user)(plus aRowsAffected == 0 → not-foundcheck), and/or addmiddleware.AuthKnowledge(...)to the two routes — so the gRPC delete is safe independent of the bff.Fixes #67
Reported and verified with Sectum AI — open-source multi-tenant isolation verification.