Possible to bypass holds placement limits via direct API calls

Bug #1017990 reported by Bill Erickson on 2012-06-26
262
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Evergreen
Medium
Unassigned

Bug Description

The holds placement permit and creation steps are handled by two different API calls:

open-ils.circ.title_hold.is_possible
open-ils.circ.holds.create[.override]

This means Evergreen clients have the ability to bypass the "permit" step and go straight to the holds placement step, ignoring any limits in place with the hold matrix. This problem was brought to my attention because there is apparently an "unauthorized" IOS app that some patrons are using to place holds and it's letting them place as many holds as they want without limit.

As to why/how, I believe the "is_possible" call started out as a way to answer the question, "are there any targets for this hold / should I bother placing it?". Over time, it evolved into the primary location to check for permissibility. It's time to rectify that.

My suggestion is to deprecate both of the above methods and move clients toward using open-ils.circ.holds.test_and_create[.batch]. It combines the permission check with the holds placement. Fortunately, there are few interfaces that actually call the open-ils.circ.holds.create[.override] methods, since the catalog is the only place holds are placed. TPAC is already using open-ils.circ.holds.test_and_create.batch We just need to teach JSPAC how to do the same and thoroughly document/announce the change so that 3rd-party applications can start using the correct API.

JFYI, holds can also be created with place_hold.xul (e.g. Request
Items in Copy Buckets), but it's already using
open-ils.circ.holds.test_and_create.batch

Thomas Berezansky (tsbere) wrote :

For note, I believe that even the test and create batch method suffers from this.

If you call the override variant *first* the title hold is possible checks don't run.

I have plans on changing how a lot of that works. They include making the non-batch create function go away as a call of its own....but I have reasons to want the title hold is possible check to continue to exist. "Could this hold be placed, and if not, why not?" type interfaces could be very useful to allow staff to test without placing actual holds, for example.

Jason Stephenson (jstephenson) wrote :

The changes that I am planning for https://bugs.launchpad.net/evergreen/+bug/1076062 will most likely address this issue. I'm still working on a complete description of the problems in that bug and the plans to address them via code. I'm going to send that to the dev list when I've completed the description. I won't mention this bug in that email since it is a private, security issue and since fixing this one is actually incidental to fixing 1076062.

Changed in evergreen:
status: New → Confirmed
importance: Undecided → Medium
assignee: nobody → Jason Stephenson (jstephenson)
milestone: none → 2.4.0-alpha
Jason Stephenson (jstephenson) wrote :

I set the priority to medium I don't think this is that big of an issue as far as security issues go. It does not warrant a special security fix notice in my opinion.

Jason Stephenson (jstephenson) wrote :

Not really sure if my branch for bug 1076062 addresses this at all.

I didn't test for it, so we'll consider this bug still open and separate from that one.

Changed in evergreen:
assignee: Jason Stephenson (jstephenson) → nobody
Ben Shum (bshum) on 2013-03-03
Changed in evergreen:
milestone: 2.4.0-alpha1 → 2.4.0-beta
Ben Shum (bshum) on 2013-03-17
Changed in evergreen:
milestone: 2.4.0-beta → 2.4.0-rc
Ben Shum (bshum) on 2013-04-27
Changed in evergreen:
milestone: 2.4.0-rc → none
Kathy Lussier (klussier) wrote :

I don't know if this is the same issue, but we are currently seeing a problem where the kenstir Android app is placing holds without first going through the permit test. I've looked through the code, but I'm unfamiliar with it and didn't see which call is being used. I've posted a comment to the Android app's LP bug, but for the long-term, I think we need to prevent these calls from occurring, because the next person who builds an app could use them.

The system where this occurred is on 2.8.x. I know a lot of jspac code was removed in 2.9. Is it possible it also removed the ability to make those calls, making this a non-issue?

Kathy Lussier (klussier) wrote :

After a bit of digging, I've found that the Android app is indeed using open-ils.circ.holds.create call.

I performed a grep on the code, aside from Holds.pm, I found that open-ils.circ.holds.create and open-ils.circ.holds.create.override are only used internally in Open-ILS/web/opac/common/js/config.js

 var CREATE_HOLD = "open-ils.circ:open-ils.circ.holds.create";
 var CREATE_HOLD_OVERRIDE = "open-ils.circ:open-ils.circ.holds.create.override";

Grepping CREATE_HOLD turns up ilsperm.CREATE_HOLD in some of the dtd files, but it doesn't look like the call is being used internally now that the jspac files have been removed.

I found the same to be true for open-ils.circ.title_hold.is_possible.

I would like to follow through with Bill's original suggestion to 1) deprecate these methods and then 2)entirely remove them from the code so that third-party apps can no longer use them. Actually, I wouldn't mind jumping to #2 right away, but I'm guessing clients using these API's need some forewarning.

What needs to be done to deprecate these methods for 2.10. Is it primarily a documentation/PR change? If so, I'm happy to do the legwork on this. I also could working on removing these calls from Holds.pm, but I probably will need a little assistance/direction in doing so.

Jason Stephenson (jstephenson) wrote :

Well, you're supposed to run open-ils.circ.title_hold.is_possible before doing open-ils.circ.holds.create, but that said using open-ils.circ.holds.test_and_create.batch is better.

I would be for deprecating open-ils.circ.holds.create in 2.10, and removing it in the next release.

A non-batch version of open-ils.circ.holds.test_and_create.batch might be good to add when you only want to do 1 hold at a time. The batch on the end may throw some developers who don't realize that a list with one entry is still a batch.

I think open-ils.circ.title_hold.is_possible should definitely stay. There are times I've used that method to find out if holds can be placed or not without actually creating the hold.

Jason Stephenson (jstephenson) wrote :

BTW, I removed the 2.3 and 2.4 series targets because there is no way it's getting fixed there. :)

no longer affects: evergreen/2.3
no longer affects: evergreen/2.4
information type: Private Security → Public Security
Bill Erickson (berick) wrote :

For added fun, the code we want to encourage using (open-ils.circ.holds.test_and_create[.batch]) calls the code we want to deprecate (open-ils.circ.holds.create[.override]) internally. (Code re-use, FTW). If we deprecate open-ils.circ.holds.create[.override], we'll need to retain its code in a non-published API / utility function.

Jeff Davis (jdavis-sitka) wrote :

First crack at a fix:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/jeffdavis/lp1017990-deprecate-hold-create

This logs a warning about the deprecated API calls, moves the bulk of the create_hold method to a separate implementation method, and makes the open-ils.circ.holds.test_and_create.batch[.override] calls use the implementation method directly.

To post a comment you must log in.
This report contains Public Security information  Edit
Everyone can see this security related information.

Other bug subscribers