orthanc-anon ReceivedInstanceCallback must not allow exceptions to go uncaught or anonymisation will be compromised

[jeremyestein]

Definition of Done / Acceptance Criteria

• [ ] ReceivedInstanceCallback must only contain a big try/except statement, to maximise the chances of intercepting any exception that's thrown (would recommend sticking its current contents into another method)
• [ ] A comment is added to explain why it's very important to keep all contents of this method wrapped in this try/except statement.
• [ ] Probably do the same for the orthanc raw callback to aid in debugging, although I don't believe it's a security issue as we don't tend to reject stuff anyway.

Details and Comments

Repro steps

• Add the following line to simulate a bug in the code that causes an exception to be raised in the ReceivedInstanceCallback of the orthanc anon plugin. (It would have to be an intermittent or data dependent bug to not be caught by system testing).

  
  --- a/orthanc/orthanc-anon/plugin/pixl.py
  +++ b/orthanc/orthanc-anon/plugin/pixl.py
  @@ -281,6 +281,8 @@ def ReceivedInstanceCallback(receivedDicom: bytes, origin: str) -> Any:
   # Read the bytes as DICOM/
   dataset = dcmread(BytesIO(receivedDicom))

• raise ValueError

* Run the system test without performing teardown afterwards (the system test should fail)
* Inspect the orthanc anon server in your browser at http://localhost:7003/
* Navigate to "All studies"

#### Expected
Due to the raised exception, I would expect no data to have reached orthanc anon. And if any data has, it absolutely shouldn't contain any PII.

#### Actual
orthanc-anon contains the same data as orthanc-raw. Ie. two studies with full, original patient name, patient ID, accession number.
Luckily, it seems that FTP upload hasn't happened, but I haven't investigated why.

#### Discussion
It seems that orthanc requires an explicit return value (eg. `return orthanc.ReceivedInstanceAction.DISCARD, None`) to discard or modify a received instance. The default is to accept unmodified.
When anonymising, this "fail dangerous" paradigm is very much not ideal.

I can't be sure that in real life we wouldn't perform the FTP upload. It could be the way the system test is set up to test for success before proceeding that stops it from doing so. A human operator may not have realised this has happened and may manually run the upload command, thus putting patient data at risk.

[stefpiatek]

Is this not just a top level try block catching all exceptions and discarding?

I can't be sure that in real life we wouldn't perform the FTP upload.

We query the hashed_image_idenfifier in pipeline.image table, so if the data hasn't been pseudonymised, then it will fail on that query and not export

[jeremyestein]

Possible duplicate of #122