From: scottmk <scottmk@dcc99617-32d9-48b4-a31d-7c20da2025e4>
Date: Wed, 13 Oct 2010 18:53:56 +0000 (+0000)
Subject: Changes to the treatment of ORDER BY:
X-Git-Url: https://old-git.evergreen-ils.org/?a=commitdiff_plain;h=5face2671982f6b4749a901e2601e0950258ea52;p=evergreen%2Ftadl.git

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


git-svn-id: svn://svn.open-ils.org/ILS/trunk@18321 dcc99617-32d9-48b4-a31d-7c20da2025e4
---

diff --git a/Open-ILS/src/c-apps/oils_sql.c b/Open-ILS/src/c-apps/oils_sql.c
index 81e3cd8e63..76b99978a8 100644
--- a/Open-ILS/src/c-apps/oils_sql.c
+++ b/Open-ILS/src/c-apps/oils_sql.c
@@ -4416,10 +4416,26 @@ char* SELECT (
 
 				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 @@ static char* buildSELECT ( const jsonObject* search_hash, jsonObject* rest_of_qu
 				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 @@ static char* buildSELECT ( const jsonObject* search_hash, jsonObject* rest_of_qu
 						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 @@ static char* buildSELECT ( const jsonObject* search_hash, jsonObject* rest_of_qu
 
 						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;
 					}