From 1e783a8c992992c2e677bbb6b6ad1621c9b8809e Mon Sep 17 00:00:00 2001 From: hns Date: Wed, 28 Nov 2007 15:32:09 +0000 Subject: [PATCH] * Apply better protection against SQL injection following bug 577 by introducing DbMapping.checkNumber() to catch anything not looking like a number literal from being used as such. I't quite likely this breaks for some SQL type and app out there, but it's better to be careful here. * Introduce new DbMapping.virtual/DbMapping.isVirtual() flag that easily lets us know whether a Node/DbMapping is a virtual one (collection, mountpoint etc) Using this in Node.getNonVirtualParent() fixes bug 566 . * Remove obsolete and unused Node.setParent(Node, String). * Set parentHandle directly instead of calling setParent(Node) in Node.getParent(). --- src/helma/objectmodel/db/DbMapping.java | 62 ++++++++++++---- src/helma/objectmodel/db/Node.java | 73 ++----------------- src/helma/objectmodel/db/Relation.java | 20 ++--- .../objectmodel/db/UpdateableSubnodeList.java | 8 +- 4 files changed, 69 insertions(+), 94 deletions(-) diff --git a/src/helma/objectmodel/db/DbMapping.java b/src/helma/objectmodel/db/DbMapping.java index 0ba6c47e..4aa9ff58 100644 --- a/src/helma/objectmodel/db/DbMapping.java +++ b/src/helma/objectmodel/db/DbMapping.java @@ -123,11 +123,22 @@ public final class DbMapping { // Set of mappings that depend on us and should be forwarded last data change events HashSet dependentMappings = new HashSet(); + // does this DbMapping describe a virtual node (collection, mountpoint, groupnode)? + private boolean virtual = false; + /** - * Create an empty DbMapping + * Create an internal DbMapping used for "virtual" mappings aka collections, mountpoints etc. */ - public DbMapping(Application app, String typename) { - this(app, typename, null); + public DbMapping(Application app, String parentTypeName) { + this(app, parentTypeName, null); + // DbMappings created with this constructor always define virtual nodes + virtual = true; + if (parentTypeName != null) { + parentMapping = app.getDbMapping(parentTypeName); + if (parentMapping == null) { + throw new IllegalArgumentException("Unknown parent mapping: " + parentTypeName); + } + } } /** @@ -764,12 +775,7 @@ public final class DbMapping { // if mapping doesn' exist or isn't defined, create a new (anonymous internal) one groupbyMapping = new DbMapping(app, subRelation.groupbyPrototype); - // If a mapping is defined, make the internal mapping inherit from - // the defined named prototype. - if (subRelation.groupbyPrototype != null) { - groupbyMapping.parentMapping = app.getDbMapping(subRelation.groupbyPrototype); - } - + // set subnode and property relations groupbyMapping.subRelation = subRelation.getGroupbySubnodeRelation(); if (propRelation != null) { @@ -1503,13 +1509,13 @@ public final class DbMapping { for (int i = 0; i < values.length; i++) { if (i > 0) q.append(", "); - q.append("'").append(escape(values[i])).append("'"); + q.append("'").append(escapeString(values[i])).append("'"); } } else { for (int i = 0; i < values.length; i++) { if (i > 0) q.append(", "); - q.append(values[i]); + q.append(checkNumber(values[i])); } } q.append(")"); @@ -1531,9 +1537,9 @@ public final class DbMapping { q.append(column).append(" = "); if (needsQuotes(column)) { - q.append("'").append(escape(val)).append("'"); + q.append("'").append(escapeString(val)).append("'"); } else { - q.append(val); + q.append(checkNumber(val)); } } @@ -1544,7 +1550,8 @@ public final class DbMapping { * @param str the string to escape * @return the escaped string */ - static String escape(String str) { + static String escapeString(Object value) { + String str = value == null ? null : value.toString(); if (str == null) { return null; } else if (str.indexOf("'") < 0) { @@ -1564,4 +1571,31 @@ public final class DbMapping { } return sbuf.toString(); } + + /** + * Utility method to check whether the argument is a number literal. + * @param str a string representing a number literal + * @return the argument, if it conforms to the number literal syntax + * @throws IllegalArgumentException if the argument does not represent a number + */ + static String checkNumber(Object value) throws IllegalArgumentException { + String str = value == null ? null : value.toString(); + if (str == null) { + return null; + } else { + str = str.trim(); + if (str.matches("(?:\\+|\\-)??\\d+(?:\\.\\d+)??")) { + return str; + } + } + throw new IllegalArgumentException("Illegal numeric literal: " + str); + } + + /** + * Find if this DbMapping describes a virtual node (collection, mountpoint, groupnode) + * @return true if this instance describes a virtual node. + */ + public boolean isVirtual() { + return virtual; + } } \ No newline at end of file diff --git a/src/helma/objectmodel/db/Node.java b/src/helma/objectmodel/db/Node.java index da1f9d16..f0685abb 100644 --- a/src/helma/objectmodel/db/Node.java +++ b/src/helma/objectmodel/db/Node.java @@ -691,64 +691,6 @@ public final class Node implements INode, Serializable { parentHandle = parent; } - /** - * This version of setParent additionally marks the node as anonymous or non-anonymous, - * depending on the string argument. This is the version called from the scripting framework, - * while the one argument version is called from within the objectmodel classes only. - */ - public void setParent(Node parent, String propertyName) { - // we only do that for relational nodes. - if (!isRelational()) { - return; - } - - NodeHandle oldParentHandle = parentHandle; - - parentHandle = (parent == null) ? null : parent.getHandle(); - - // mark parent as set, otherwise getParent will try to - // determine the parent again when called. - lastParentSet = System.currentTimeMillis(); - - if ((parentHandle == null) || parentHandle.equals(oldParentHandle)) { - // nothing changed, no need to find access property - return; - } - - if ((parent != null) && (propertyName == null)) { - // see if we can find out the propertyName by ourselfes by looking at the - // parent's property relation - String newname = null; - DbMapping parentmap = parent.getDbMapping(); - - if (parentmap != null) { - // first try to retrieve name via generic property relation of parent - Relation prel = parentmap.getSubnodeRelation(); - - if ((prel != null) && (prel.otherType == dbmap) && - (prel.accessName != null)) { - // reverse look up property used to access this via parent - Relation proprel = dbmap.columnNameToRelation(prel.accessName); - - if ((proprel != null) && (proprel.propName != null)) { - newname = getString(proprel.propName); - } - } - } - - // did we find a new name for this - if (newname == null) { - this.anonymous = true; - } else { - this.anonymous = false; - this.name = newname; - } - } else { - this.anonymous = false; - this.name = propertyName; - } - } - /** * Get parent, retrieving it if necessary. */ @@ -794,7 +736,7 @@ public final class Node implements INode, Serializable { } else if (pn2.equals(this)) { // a special case we want to support: virtualname is actually // a reference to this node, not a collection containing this node. - setParent(pn); + parentHandle = pn.getHandle(); name = pinfo.virtualname; anonymous = false; return pn; @@ -813,7 +755,7 @@ public final class Node implements INode, Serializable { } if (pn != null) { - setParent(pn); + parentHandle = pn.getHandle(); lastParentSet = System.currentTimeMillis(); return pn; @@ -826,7 +768,7 @@ public final class Node implements INode, Serializable { if (i == parentInfo.length-1) { // if we came till here and we didn't find a parent. // set parent to null. - setParent(null); + parentHandle = null; lastParentSet = System.currentTimeMillis(); } } @@ -836,11 +778,9 @@ public final class Node implements INode, Serializable { } } - // fall back to heuristic parent (the node that fetched this one from db) if (parentHandle == null) { return null; } - return parentHandle.getNode(nmgr); } @@ -890,7 +830,7 @@ public final class Node implements INode, Serializable { node.checkWriteLock(); } - // if subnodes are defined via realation, make sure its constraints are enforced. + // if subnodes are defined via relation, make sure its constraints are enforced. if ((dbmap != null) && (dbmap.getSubnodeRelation() != null)) { dbmap.getSubnodeRelation().setConstraints(this, node); } @@ -1203,6 +1143,7 @@ public final class Node implements INode, Serializable { // but it currently isn't supported by NodeManager. // if (dbmap != null && dbmap.getSubnodeRelation () != null) // retval = nmgr.getNode (this, subid, dbmap.getSubnodeRelation ()); + if ((retval != null) && (retval.parentHandle == null) && !nmgr.isRootNode(retval)) { retval.setParent(this); @@ -2531,7 +2472,7 @@ public final class Node implements INode, Serializable { */ public String toString() { try { - // We need to reach deap into helma.framework.core to invoke onInit(), + // We need to reach deap into helma.framework.core to invoke toString(), // but the functionality is really worth it. RequestEvaluator reval = getApp().getCurrentRequestEvaluator(); if (reval != null) { @@ -2675,7 +2616,7 @@ public final class Node implements INode, Serializable { if (node.getState() == Node.TRANSIENT) { DbMapping map = node.getDbMapping(); - if (map == null || map.getTypeName() != null) + if (map == null || !map.isVirtual()) return node; } else if (node.getState() != Node.VIRTUAL) { return node; diff --git a/src/helma/objectmodel/db/Relation.java b/src/helma/objectmodel/db/Relation.java index c5485dab..e8a3af61 100644 --- a/src/helma/objectmodel/db/Relation.java +++ b/src/helma/objectmodel/db/Relation.java @@ -734,18 +734,18 @@ public final class Relation { return null; } - // if the collection node is prototyped, return the app's DbMapping - // for that prototype - if (prototype != null) { - return otherType; - } - // create a synthetic DbMapping that describes how to fetch the // collection's child objects. if (virtualMapping == null) { - virtualMapping = new DbMapping(ownType.app, null); - virtualMapping.subRelation = getVirtualSubnodeRelation(); - virtualMapping.propRelation = getVirtualPropertyRelation(); + // if the collection node is prototyped (a mountpoint), create + // a virtual sub-mapping from the app's DbMapping for that prototype + if (prototype != null) { + virtualMapping = new DbMapping(ownType.app, prototype); + } else { + virtualMapping = new DbMapping(ownType.app, null); + virtualMapping.subRelation = getVirtualSubnodeRelation(); + virtualMapping.propRelation = getVirtualPropertyRelation(); + } } return virtualMapping; @@ -924,7 +924,7 @@ public final class Relation { } // end column version if (value != null) { - q.append(DbMapping.escape(value.toString())); + q.append(DbMapping.escapeString(value.toString())); } else { q.append("NULL"); } diff --git a/src/helma/objectmodel/db/UpdateableSubnodeList.java b/src/helma/objectmodel/db/UpdateableSubnodeList.java index b7ec9bb3..2c5f8e03 100644 --- a/src/helma/objectmodel/db/UpdateableSubnodeList.java +++ b/src/helma/objectmodel/db/UpdateableSubnodeList.java @@ -149,9 +149,9 @@ public class UpdateableSubnodeList extends OrderedSubnodeList { DbColumn dbc = rel.otherType.getColumn(updateCriteria[idx]); if (rel.otherType.getIDField().equalsIgnoreCase(updateCriteria[idx])) { if (rel.otherType.needsQuotes(updateCriteria[idx])) { - sb.append ("'").append (values[idx]).append("'"); + sb.append("'").append(DbMapping.escapeString(values[idx])).append("'"); } else { - sb.append (values[idx]); + sb.append(DbMapping.checkNumber(values[idx])); } return; } @@ -192,9 +192,9 @@ public class UpdateableSubnodeList extends OrderedSubnodeList { case Types.CLOB: default: if (rel.otherType.needsQuotes(updateCriteria[idx])) { - sb.append ("'").append (strgVal).append ("'"); + sb.append("'").append(DbMapping.escapeString(strgVal)).append("'"); } else { - sb.append (strgVal); + sb.append(DbMapping.checkNumber(strgVal)); } } }