Mercurial > hg > orthanc

changeset 2324:53df86a17e99

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
fix issue #55 (prevent anonymization/modification to unexpectingly break the database model)
author Sebastien Jodogne <s.jodogne@gmail.com>
date Fri, 14 Jul 2017 11:17:56 +0200
parents f5fc61337bdf
children 8198a9fa2553
files NEWS OrthancServer/OrthancRestApi/OrthancRestAnonymizeModify.cpp
diffstat 2 files changed, 64 insertions(+), 13 deletions(-) [+]
line wrap: on
line diff
--- a/NEWS	Fri Jul 14 10:50:00 2017 +0200
+++ b/NEWS	Fri Jul 14 11:17:56 2017 +0200
@@ -34,12 +34,14 @@
 
 * Ability to retrieve raw frames encoded as unsigned 32-bits integers
 * Fix issue 29 (more consistent handling of the "--upgrade" argument)
-* Fix issue 31 (Create new modality types for Philips ADW, GE Xeleris, GE AWServer)
+* Fix issue 31 (create new modality types for Philips ADW, GE Xeleris, GE AWServer)
 * Fix issue 35 (AET name is not transferred to Orthanc using DCMTK 3.6.0)
-* Fix issue 44 (Bad interpretation of photometric interpretation MONOCHROME1)
+* Fix issue 44 (bad interpretation of photometric interpretation MONOCHROME1)
 * Fix issue 45 (crash when providing a folder to "--config" command-line option)
 * Fix issue 46 (PHI remaining after anonymization)
-* Fix issue 49 (Worklists: accentuated characters are removed from C-Find responses)
+* Fix issue 49 (worklists: accentuated characters are removed from C-Find responses)
+* Fix issue 55 (modification/anonymization of tags that might break the database
+  model now require the "Force" parameter to be set to "true" in the query)
 * Fix Debian #865606 (orthanc FTBFS with libdcmtk-dev 3.6.1~20170228-2)
 * Fix XSS inside DICOM in Orthanc Explorer (as reported by Victor Pasnkel, Morphus Labs)
 * Upgrade forthcoming DCMTK 3.6.1 to snapshot 20170228
--- a/OrthancServer/OrthancRestApi/OrthancRestAnonymizeModify.cpp	Fri Jul 14 10:50:00 2017 +0200
+++ b/OrthancServer/OrthancRestApi/OrthancRestAnonymizeModify.cpp	Fri Jul 14 11:17:56 2017 +0200
@@ -52,9 +52,18 @@
     TagOperation_Remove
   };
 
+  static bool IsDatabaseKey(const DicomTag& tag)
+  {
+    return (tag == DICOM_TAG_PATIENT_ID ||
+            tag == DICOM_TAG_STUDY_INSTANCE_UID ||
+            tag == DICOM_TAG_SERIES_INSTANCE_UID ||
+            tag == DICOM_TAG_SOP_INSTANCE_UID);
+  }
+
   static void ParseListOfTags(DicomModification& target,
                               const Json::Value& query,
-                              TagOperation operation)
+                              TagOperation operation,
+                              bool force)
   {
     if (!query.isArray())
     {
@@ -67,6 +76,14 @@
 
       DicomTag tag = FromDcmtkBridge::ParseTag(name);
 
+      if (!force && IsDatabaseKey(tag))
+      {
+        LOG(ERROR) << "Marking tag \"" << name << "\" as to be "
+                   << (operation == TagOperation_Keep ? "kept" : "removed")
+                   << " requires the \"Force\" option to be set to true";
+        throw OrthancException(ErrorCode_BadRequest);
+      }
+
       switch (operation)
       {
         case TagOperation_Keep:
@@ -87,7 +104,8 @@
 
 
   static void ParseReplacements(DicomModification& target,
-                                const Json::Value& replacements)
+                                const Json::Value& replacements,
+                                bool force)
   {
     if (!replacements.isObject())
     {
@@ -101,6 +119,14 @@
       const Json::Value& value = replacements[name];
 
       DicomTag tag = FromDcmtkBridge::ParseTag(name);
+
+      if (!force && IsDatabaseKey(tag))
+      {
+        LOG(ERROR) << "Marking tag \"" << name << "\" as to be replaced "
+                   << "requires the \"Force\" option to be set to true";
+        throw OrthancException(ErrorCode_BadRequest);
+      }
+
       target.Replace(tag, value, false);
 
       VLOG(1) << "Replace: " << name << " " << tag 
@@ -116,25 +142,46 @@
   }
 
 
+  static bool GetBooleanValue(const std::string& member,
+                              const Json::Value& json,
+                              bool defaultValue)
+  {
+    if (!json.isMember(member))
+    {
+      return defaultValue;
+    }
+    else if (json[member].type() == Json::booleanValue)
+    {
+      return json[member].asBool();
+    }
+    else
+    {
+      LOG(ERROR) << "Member \"" << member << "\" should be a Boolean value";
+      throw OrthancException(ErrorCode_BadFileFormat);
+    }
+  }
+
 
   bool OrthancRestApi::ParseModifyRequest(DicomModification& target,
                                           const Json::Value& request)
   {
     if (request.isObject())
     {
-      if (request.isMember("RemovePrivateTags"))
+      bool force = GetBooleanValue("Force", request, false);
+      
+      if (GetBooleanValue("RemovePrivateTags", request, false))
       {
         target.SetRemovePrivateTags(true);
       }
 
       if (request.isMember("Remove"))
       {
-        ParseListOfTags(target, request["Remove"], TagOperation_Remove);
+        ParseListOfTags(target, request["Remove"], TagOperation_Remove, force);
       }
 
       if (request.isMember("Replace"))
       {
-        ParseReplacements(target, request["Replace"]);
+        ParseReplacements(target, request["Replace"], force);
       }
 
       // The "Keep" operation only makes sense for the tags
@@ -144,7 +191,7 @@
       // you're doing!
       if (request.isMember("Keep"))
       {
-        ParseListOfTags(target, request["Keep"], TagOperation_Keep);
+        ParseListOfTags(target, request["Keep"], TagOperation_Keep, force);
       }
 
       return true;
@@ -185,6 +232,8 @@
       return false;
     }
 
+    bool force = GetBooleanValue("Force", request, false);
+      
     // As of Orthanc 1.2.1, the default anonymization is done
     // according to PS 3.15-2017c Table E.1-1 (basic profile)
     DicomVersion version = DicomVersion_2017c;
@@ -203,24 +252,24 @@
     target.SetupAnonymization(version);
     std::string patientName = target.GetReplacementAsString(DICOM_TAG_PATIENT_NAME);
 
-    if (request.isMember("KeepPrivateTags"))
+    if (GetBooleanValue("KeepPrivateTags", request, false))
     {
       target.SetRemovePrivateTags(false);
     }
 
     if (request.isMember("Remove"))
     {
-      ParseListOfTags(target, request["Remove"], TagOperation_Remove);
+      ParseListOfTags(target, request["Remove"], TagOperation_Remove, force);
     }
 
     if (request.isMember("Replace"))
     {
-      ParseReplacements(target, request["Replace"]);
+      ParseReplacements(target, request["Replace"], force);
     }
 
     if (request.isMember("Keep"))
     {
-      ParseListOfTags(target, request["Keep"], TagOperation_Keep);
+      ParseListOfTags(target, request["Keep"], TagOperation_Keep, force);
     }
 
     if (target.IsReplaced(DICOM_TAG_PATIENT_NAME) &&