Scope laboratory assay protocol lookups to the request container#286
Scope laboratory assay protocol lookups to the request container#286labkey-martyp wants to merge 2 commits into
Conversation
GetImportMethodsAction and four sibling actions in LaboratoryController looked up assay protocols by row id via ExperimentService.getExpProtocol(int), an unscoped global primary-key lookup. Because the actions are guarded only by container-level permissions, a user with read access to any single folder could pass an arbitrary row id and have the server use — and, in GetImportMethodsAction, echo back the name, container, and container path of — a protocol defined in a folder they cannot read, enabling cross-container enumeration of assay designs and folder paths. Each lookup now verifies the protocol is in scope for the request container via AssayService.getAssayProtocols(getContainer()) before use, returning the same generic not-found message whether the row id is unknown or simply out of scope so the response is not an existence oracle. Legitimate same-container callers are unaffected.
|
Hi @labkey-martyp, do you know how assays defined in the shared container are treated under this change? |
|
Also, just curious, is the recent set of permission related PRs AI driven? |
|
@bbimber there should be no change in behavior for assays in the Shared folder. They would be considered in-scope for the current folder. |
Yes we are using AI to do scans for potential security issues. I anticipate there will be communication on that once we've patched any issues. |
And in light of that, the scans will flag potential issues but it doesn't know all the true use cases, so if there are legitimate cross container use cases, we need to make sure we're not blocking those but marking them in the code. I am not as familiar with this code so please let us know if there are any true needs for cross container functionality. |
|
OK. The two things that come to mind are the shared container and scenarios involving workbooks/parent. I am pretty sure the experiment service deals with these, but I'm not in front of my computer and haven't thought about this in a while. If you can wait a day I'll look at this tomorrow. |
No rush. The assay containers that would be in scope and would remain unaffected by these changes as far as I can tell are the 1. the request container, 2. the project root of the request container, 3. the Shared container. |
bbimber
left a comment
bbimber
left a comment
There was a problem hiding this comment.
The AI is technically right here, though the risk is probably not huge. This is a worthwhile change. I cannot think of any cross-container cases that would break from this. AssayService.getAssayProtocols() should handle resolving /Shared and workbook/parent situations.
My only question is around the comments. In general, LK's codebase isnt that verbose with inline comments. This is adding a bunch of redundant explanation. That explanation doesnt really hurt anything. Does LK plan to default to including a lot more comments from these sort of AI PRs?
Also, the AI's 'existence oracle' comment is interesting. There are probably many examples in EHR and related modules that differentiate 'not found' and 'wrong container' in the error messages. It'd be interesting if it finds things like this.
|
Yes, point taken on the comments. Usually AI will try to match the coding style of the project, but comments seem to be the exception. Claude in particular can be very verbose with comments. I shortened these. Yeah we can definitely add the existence Oracle to the list of items to scan across the whole LK database. There will be more checks to come. I'll probably have another PR or two in this area. |
2 participants
Rationale
GetImportMethodsActionand four sibling actions inLaboratoryControllerlooked up assay protocols by row id viaExperimentService.getExpProtocol(int), an unscoped global primary-key lookup. Because these actions are guarded only by container-level permissions, a user with read access to any single folder could supply an arbitrary protocol row id and have the server operate on a protocol defined in a folder they cannot read.GetImportMethodsActionadditionally echoed the protocol's name, container, and container path back in its response, allowing cross-container enumeration of assay design names and folder paths (an IDOR / information-disclosure issue). The unchecked lookup inGetImportMethodsActioncould also NPE on a non-existent row id.Related Pull Requests
None.
Changes
AssayService.getAssayProtocols(getContainer())before use, coveringGetImportMethodsAction,ProcessAssayDataAction,SaveTemplateAction,CreateTemplateAction, andGetAssayImportHeadersAction.Laboratory.Utils.getAssayDetailsfrom the assay import/template panels) are unaffected, since they always pass an in-scope protocol.