From 4a9352ab5b85c5cf5d2318cbb6e75494cac057dd Mon Sep 17 00:00:00 2001 From: ankurjuneja Date: Tue, 19 Oct 2021 22:15:34 -0700 Subject: [PATCH 1/2] log row values when exception during script execution --- api/src/org/labkey/api/data/triggers/ScriptTrigger.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/org/labkey/api/data/triggers/ScriptTrigger.java b/api/src/org/labkey/api/data/triggers/ScriptTrigger.java index ba0a126182a..015f49d4a91 100644 --- a/api/src/org/labkey/api/data/triggers/ScriptTrigger.java +++ b/api/src/org/labkey/api/data/triggers/ScriptTrigger.java @@ -195,7 +195,7 @@ private T _invokeTableScript(Container c, User user, Class resultType, St } catch (NoSuchMethodException | ScriptException e) { - throw new UnexpectedException(e); + throw UnexpectedException.wrap(e, "Script execution failed for row with values: " + Arrays.toString(args)); } return null; From 1271ed61b01a58415130ed46c60fe7cf589acdd3 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Tue, 2 Nov 2021 10:59:31 -0700 Subject: [PATCH 2/2] Improve error messages, and avoid leaking PHI --- .../api/data/triggers/ScriptTrigger.java | 52 +++++++++++++------ 1 file changed, 37 insertions(+), 15 deletions(-) diff --git a/api/src/org/labkey/api/data/triggers/ScriptTrigger.java b/api/src/org/labkey/api/data/triggers/ScriptTrigger.java index 015f49d4a91..9aabe1f2309 100644 --- a/api/src/org/labkey/api/data/triggers/ScriptTrigger.java +++ b/api/src/org/labkey/api/data/triggers/ScriptTrigger.java @@ -19,6 +19,7 @@ import org.jetbrains.annotations.Nullable; import org.labkey.api.data.Container; import org.labkey.api.data.DbScope; +import org.labkey.api.data.PHI; import org.labkey.api.data.TableInfo; import org.labkey.api.query.BatchValidationException; import org.labkey.api.query.ValidationException; @@ -35,6 +36,7 @@ import java.util.HashMap; import java.util.Map; import java.util.Objects; +import java.util.function.Supplier; /** * Implements a trigger for table operations backed by JavaScript code. @@ -67,16 +69,29 @@ public boolean canStream() return false; } + /** + * To avoid leaking PHI through log files, avoid including the full row info in the error detail when any of the + * columns in the target table is considered PHI + */ + private Supplier filterErrorDetailByPhi(TableInfo table, Supplier errorDetail) + { + if (table.getMaxPhiLevel() != PHI.NotPHI) + { + return () -> null; + } + return errorDetail; + } + @Override public void init(TableInfo table, Container c, User user, TableInfo.TriggerType event, BatchValidationException errors, Map extraContext) { - invokeTableScript(table, c, user, "init", errors, extraContext, event.name().toLowerCase()); + invokeTableScript(table, c, user, "init", errors, extraContext, () -> null, event.name().toLowerCase()); } @Override public void complete(TableInfo table, Container c, User user, TableInfo.TriggerType event, BatchValidationException errors, Map extraContext) { - invokeTableScript(table, c, user, "complete", errors, extraContext, event.name().toLowerCase()); + invokeTableScript(table, c, user, "complete", errors, extraContext, () -> null, event.name().toLowerCase()); } @@ -85,7 +100,14 @@ public void beforeInsert(TableInfo table, Container c, User user, @Nullable Map newRow, ValidationException errors, Map extraContext) { - invokeTableScript(table, c, user, "beforeInsert", errors, extraContext, newRow); + invokeTableScript(table, + c, + user, + "beforeInsert", + errors, + extraContext, + filterErrorDetailByPhi(table, () -> "New row data: " + newRow), + newRow); } @Override @@ -93,7 +115,7 @@ public void beforeUpdate(TableInfo table, Container c, User user, @Nullable Map newRow, @Nullable Map oldRow, ValidationException errors, Map extraContext) { - invokeTableScript(table, c, user, "beforeUpdate", errors, extraContext, newRow, oldRow); + invokeTableScript(table, c, user, "beforeUpdate", errors, extraContext, filterErrorDetailByPhi(table, () -> "New row: " + newRow + ". Old row: " + oldRow), newRow, oldRow); } @Override @@ -101,7 +123,7 @@ public void beforeDelete(TableInfo table, Container c, User user, @Nullable Map oldRow, ValidationException errors, Map extraContext) { - invokeTableScript(table, c, user, "beforeDelete", errors, extraContext, oldRow); + invokeTableScript(table, c, user, "beforeDelete", errors, extraContext, filterErrorDetailByPhi(table, () -> "Old row: " + oldRow), oldRow); } @Override @@ -109,7 +131,7 @@ public void afterInsert(TableInfo table, Container c, User user, @Nullable Map newRow, ValidationException errors, Map extraContext) { - invokeTableScript(table, c, user, "afterInsert", errors, extraContext, newRow); + invokeTableScript(table, c, user, "afterInsert", errors, extraContext, filterErrorDetailByPhi(table, () -> "New row: " + newRow), newRow); } @Override @@ -117,7 +139,7 @@ public void afterUpdate(TableInfo table, Container c, User user, @Nullable Map newRow, @Nullable Map oldRow, ValidationException errors, Map extraContext) { - invokeTableScript(table, c, user, "afterUpdate", errors, extraContext, newRow, oldRow); + invokeTableScript(table, c, user, "afterUpdate", errors, extraContext, filterErrorDetailByPhi(table, () -> "New row: " + newRow + ". Old row: " + oldRow), newRow, oldRow); } @Override @@ -125,15 +147,15 @@ public void afterDelete(TableInfo table, Container c, User user, @Nullable Map oldRow, ValidationException errors, Map extraContext) { - invokeTableScript(table, c, user, "afterDelete", errors, extraContext, oldRow); + invokeTableScript(table, c, user, "afterDelete", errors, extraContext, filterErrorDetailByPhi(table, () -> "Old row: " + oldRow), oldRow); } - protected void invokeTableScript(TableInfo table, Container c, User user, String methodName, BatchValidationException errors, Map extraContext, Object... args) + protected void invokeTableScript(TableInfo table, Container c, User user, String methodName, BatchValidationException errors, Map extraContext, Supplier errorDetail, Object... args) { Object[] allArgs = Arrays.copyOf(args, args.length+1); allArgs[allArgs.length-1] = errors; - Boolean success = _invokeTableScript(c, user, Boolean.class, methodName, extraContext, allArgs); + Boolean success = _invokeTableScript(c, user, Boolean.class, methodName, extraContext, errorDetail, allArgs); if (success != null && !success) errors.addRowError(new ValidationException(methodName + " validation failed")); if (isConnectionClosed(table.getSchema().getScope())) @@ -141,11 +163,11 @@ protected void invokeTableScript(TableInfo table, Container c, User user, String } - protected void invokeTableScript(TableInfo table, Container c, User user, String methodName, ValidationException errors, Map extraContext, Object... args) + protected void invokeTableScript(TableInfo table, Container c, User user, String methodName, ValidationException errors, Map extraContext, Supplier errorDetail, Object... args) { Object[] allArgs = Arrays.copyOf(args, args.length+1); allArgs[allArgs.length-1] = errors; - Boolean success = _invokeTableScript(c, user, Boolean.class, methodName, extraContext, allArgs); + Boolean success = _invokeTableScript(c, user, Boolean.class, methodName, extraContext, errorDetail, allArgs); if (success != null && !success) errors.addGlobalError(methodName + " validation failed"); if (isConnectionClosed(table.getSchema().getScope())) @@ -153,7 +175,7 @@ protected void invokeTableScript(TableInfo table, Container c, User user, String } - private T _invokeTableScript(Container c, User user, Class resultType, String methodName, Map extraContext, Object... args) + private T _invokeTableScript(Container c, User user, Class resultType, String methodName, Map extraContext, Supplier errorDetail, Object... args) { try { @@ -163,7 +185,6 @@ private T _invokeTableScript(Container c, User user, Class resultType, St // Push a view context if we don't already have one available. It will be pulled if labkey.js // is required by the trigger script being invoked, via the call to PageFlowUtil.jsInitObject() in // server/modules/core/resources/scripts/labkey/init.js - //noinspection deprecation viewContextResetter = ViewContext.pushMockViewContext(user, c, new ActionURL("dummy", "dummy", c)); } try @@ -195,7 +216,8 @@ private T _invokeTableScript(Container c, User user, Class resultType, St } catch (NoSuchMethodException | ScriptException e) { - throw UnexpectedException.wrap(e, "Script execution failed for row with values: " + Arrays.toString(args)); + String extraErrorMessage = errorDetail.get(); + throw UnexpectedException.wrap(e, "Script execution failed for " + methodName + "()" + (extraErrorMessage == null ? "" : " " + extraErrorMessage)); } return null;