[open-ils-commits] r18321 - trunk/Open-ILS/src/c-apps (scottmk)

[svn at svn.open-ils.org]

Author: scottmk
Date: 2010-10-13 14:53:56 -0400 (Wed, 13 Oct 2010)
New Revision: 18321

Modified:
   trunk/Open-ILS/src/c-apps/oils_sql.c
Log:
Changes to the treatment of ORDER BY:

1. For json_query: when ORDER BY is expressed as an object keyed on class
(instead of an array of field specifications), and the class is not in
scope, error out instead of silently ignoring the class.

The other changes affect only methods other than json_query:

2. When the ORDER BY list is provided as a raw text string: block any
string containing a semicolon, in order to block simple SQL injections.
For now we make no exceptions for quoted semicolons, which are not
likely ever to appear an an ORDER BY clause.

3. Keep virtual fields out of the ORDER BY clause.  For now we silently
ignore them, as we ignore non-existent fields.  In both cases we should
perhaps error out.

4. Don't require that a class referenced in the ORDER BY clause also be
referenced in the SELECT clause.  Just make sure it's in scope.

M    Open-ILS/src/c-apps/oils_sql.c


Modified: trunk/Open-ILS/src/c-apps/oils_sql.c
===================================================================
--- trunk/Open-ILS/src/c-apps/oils_sql.c	2010-10-13 17:34:13 UTC (rev 18320)
+++ trunk/Open-ILS/src/c-apps/oils_sql.c	2010-10-13 18:53:56 UTC (rev 18321)
@@ -4416,10 +4416,26 @@
 
 				ClassInfo* order_class_info = search_alias( class_itr->key );
 				if( ! order_class_info ) {
-					osrfLogInternal( OSRF_LOG_MARK,
-						"%s: Invalid class \"%s\" referenced in ORDER BY clause, skipping it",
+					osrfLogError( OSRF_LOG_MARK,
+						"%s: Invalid class \"%s\" referenced in ORDER BY clause",
 						modulename, class_itr->key );
-					continue;
+					if( ctx )
+						osrfAppSessionStatus(
+							ctx->session,
+							OSRF_STATUS_INTERNALSERVERERROR,
+							"osrfMethodException",
+							ctx->request,
+							"Invalid class referenced in ORDER BY clause -- "
+								"see error log for more details"
+						);
+					jsonIteratorFree( class_itr );
+					buffer_free( order_buf );
+					free( having_buf );
+					buffer_free( group_buf );
+					buffer_free( sql_buf );
+					if( defaultselhash )
+						jsonObjectFree( defaultselhash );
+					return NULL;
 				}
 
 				osrfHash* field_list_def = order_class_info->fields;
@@ -5094,10 +5110,11 @@
 				jsonIterator* class_itr = jsonNewIterator( order_by );
 				while( (snode = jsonIteratorNext( class_itr )) ) {  // For each class:
 
-					if( !jsonObjectGetKeyConst( selhash, class_itr->key ))
-						continue;    // class not referenced by SELECT clause?  Ignore it.
+					ClassInfo* order_class_info = search_alias( class_itr->key );
+					if( ! order_class_info )
+						continue;    // class not referenced by FROM clause?  Ignore it.
 
-					if( snode->type == JSON_HASH ) {
+					if( JSON_HASH == snode->type ) {
 
 						// If the data for the current class is a JSON_HASH, then it is
 						// keyed on field name.
@@ -5106,10 +5123,12 @@
 						jsonIterator* order_itr = jsonNewIterator( snode );
 						while( (onode = jsonIteratorNext( order_itr )) ) {  // For each field
 
-							osrfHash* field_def = oilsIDLFindPath( "/%s/fields/%s",
-								class_itr->key, order_itr->key );
+							osrfHash* field_def = osrfHashGet(
+								order_class_info->fields, order_itr->key );
 							if( !field_def )
 								continue;    // Field not defined in IDL?  Ignore it.
+							if( str_is_true( osrfHashGet( field_def, "virtual")))
+								continue;    // Field is virtual?  Ignore it.
 
 							char* field_str = NULL;
 							char* direction = NULL;
@@ -5174,10 +5193,33 @@
 
 						jsonIteratorFree( order_itr );
 
-					} else {
-						// The data for the current class is not a JSON_HASH, so we expect
-						// it to be a JSON_STRING with a single field name.
+					} else if( JSON_STRING == snode->type ) {
+						// We expect a comma-separated list of sort fields.
 						const char* str = jsonObjectGetString( snode );
+						if( strchr( str, ';' )) {
+							// No semicolons allowed.  It is theoretically possible for a
+							// legitimate semicolon to occur within quotes, but it's not likely
+							// to occur in practice in the context of an ORDER BY list.
+							osrfLogError( OSRF_LOG_MARK, "%s: Possible attempt at SOL injection -- "
+								"semicolon found in ORDER BY list: \"%s\"", modulename, str );
+							if( ctx ) {
+								osrfAppSessionStatus(
+									ctx->session,
+									OSRF_STATUS_INTERNALSERVERERROR,
+									"osrfMethodException",
+									ctx->request,
+									"Possible attempt at SOL injection -- "
+										"semicolon found in ORDER BY list"
+								);
+							}
+							jsonIteratorFree( class_itr );
+							buffer_free( order_buf );
+							buffer_free( sql_buf );
+							if( defaultselhash )
+								jsonObjectFree( defaultselhash );
+							clear_query_stack();
+							return NULL;
+						}
 						buffer_add( order_buf, str );
 						break;
 					}