Describe the bug
OED allows sites to limit access to certain items via the displayable attribute. These items are, in general, only available to a properly logged in admin. The OED web page (client code) should already be properly limiting access as desired. However, improper requests can be made directly to the OED server to acquire this information in some routes. It is desired to enforce the restrictions on the server routes to avoid any leakage of information.
The information in this issue builds upon a private security notice received from @anupamme. That person already provided accepted code via PR #1703 to patch for the readings routes. They also provided information on many of the other routes that return readings that are involved.
This issue is being made public to allow for appropriate changes to the code to be readily made and allow others to comment on the ideas. Given PR #1703 already exposed the basic issue, making this public seems an appropriate risk.
Details
Issue #1724 will centralize how routes with optional auth middleware does the check of the token and the type of user. This directly relates to this issue since that will need to be done in many routes. Thus, that issue (or at least the known solution with details) should be done first so the new, centralize way is utilized here to avoid having to change it later. Anyone working on this issue is also welcome to work on that issue if it is available - just put a comment on the issue to claim it.
A review of routes found the following where all are in src/server/routes/.
- There are three main techniques used to limit returned information:
- Uses function to modify data since some info should be returned even if not displayable. This is used in some routes.
- Uses logic in the route to call different functions/SQL to avoid returning non-displayable information to the route so it is not returned to the user. This is useful if no information is returned in this case. This is used in some routes.
- As OED continues to centralize appropriate work, it makes sense to do that here too. There should be a function in the model that takes a parameter about whether to show displayable or not and then does the logic to call other functions in the model to the correct DB calls. All current uses do the logic in the route and should be modified and this idea applied to the new uses of this technique.
- A database function can take an argument to tell if displayable items should be returned or not. This may be esp. useful if a call is made for multiple items in one call and requested as a batch. This is not currently used so it is unclear if it will be used. One example where it might be useful is for line meter readings where this is done.
- groups
and these routes should be checked: /idname, /deep/groups, /children/:group_id, /allChildren/, /idname, /deep/groups/:group_id, /deep/meters/:group_id, /parents/:group_id. It has to be investigated similarly to the / route above.
- maps
- / changes DB call to protect. This differs from some routes, e.g., meters that modify what is returned but still return some information. This method seems fine here since no information should be returned.
- /:map_id has no check but probably should. It is likely to do the same as the / route.
- meters
- Uses function to modify data since some info needed even if not displayable.
- readings
- PR 1703 fixes up so changes DB call to protect as with maps.
- unitReadings
- They do not limit and should for meters & groups.
- compareReadings
- They do not limit and should for meters & groups. They also lack the normal optionalAuthMiddleware.
The client-side restriction should be looked at for each of these cases to see what restrictions are enforced. This will make sure that the client and server are doing the same restrictions and no cases might have been overlooked in the above list. If anyone thinks they find another case or that the restriction is improperly or incorrectly enforces information then they should comment so it can be discussed and addressed.
The test code should also be analyzed and new test cases added to verify correct functionality. PR #1703 did this for the readings route and can serve as an example. Consideration of the ability to centralize the methodology as has been done for other tests should be done.
Additional context
There may be questions about what is appropriate to return and/or methodology used. Anyone is welcome to contact OED to discuss any aspect of this work (as always).
Describe the bug
OED allows sites to limit access to certain items via the displayable attribute. These items are, in general, only available to a properly logged in admin. The OED web page (client code) should already be properly limiting access as desired. However, improper requests can be made directly to the OED server to acquire this information in some routes. It is desired to enforce the restrictions on the server routes to avoid any leakage of information.
The information in this issue builds upon a private security notice received from @anupamme. That person already provided accepted code via PR #1703 to patch for the readings routes. They also provided information on many of the other routes that return readings that are involved.
This issue is being made public to allow for appropriate changes to the code to be readily made and allow others to comment on the ideas. Given PR #1703 already exposed the basic issue, making this public seems an appropriate risk.
Details
Issue #1724 will centralize how routes with optional auth middleware does the check of the token and the type of user. This directly relates to this issue since that will need to be done in many routes. Thus, that issue (or at least the known solution with details) should be done first so the new, centralize way is utilized here to avoid having to change it later. Anyone working on this issue is also welcome to work on that issue if it is available - just put a comment on the issue to claim it.
A review of routes found the following where all are in src/server/routes/.
-
-
and these routes should be checked: /idname, /deep/groups, /children/:group_id, /allChildren/, /idname, /deep/groups/:group_id, /deep/meters/:group_id, /parents/:group_id. It has to be investigated similarly to the / route above./: Does not check. Unsure if information is needed for nested groups on client. This needs to be checked/analyzed to see if they can be hidden or have partial information as is done for meters. Anyone with questions should contact OED.
The file has this TODO:
The client-side restriction should be looked at for each of these cases to see what restrictions are enforced. This will make sure that the client and server are doing the same restrictions and no cases might have been overlooked in the above list. If anyone thinks they find another case or that the restriction is improperly or incorrectly enforces information then they should comment so it can be discussed and addressed.
The test code should also be analyzed and new test cases added to verify correct functionality. PR #1703 did this for the readings route and can serve as an example. Consideration of the ability to centralize the methodology as has been done for other tests should be done.
Additional context
There may be questions about what is appropriate to return and/or methodology used. Anyone is welcome to contact OED to discuss any aspect of this work (as always).