Mforns has uploaded a new change for review. https://gerrit.wikimedia.org/r/196599
Change subject: Add size limit to event url ...................................................................... Add size limit to event url Today, when the event url is too long, the query string gets truncated to its first 1014 chars by varnishncsa. This results in validation errors that can be frequent if the schema is sufficiently big. The consequence is unnecessary overhead in the EventLogging back-end and unpredictible variations in the validation alerting system. This change adds a check on the client-side to avoid sending events that are too long. In that case an error is logged in the console if available. It also adds a check in the dev-server. Bug: T91918 Change-Id: Ie164a9357cdd599823f64122dcc79fec2198832f --- M modules/ext.eventLogging.core.js M server/eventlogging/parse.py M tests/ext.eventLogging.tests.js 3 files changed, 82 insertions(+), 3 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/EventLogging refs/changes/99/196599/1 diff --git a/modules/ext.eventLogging.core.js b/modules/ext.eventLogging.core.js index 6ab72c3..ddf339b 100644 --- a/modules/ext.eventLogging.core.js +++ b/modules/ext.eventLogging.core.js @@ -46,6 +46,16 @@ schemas: {}, /** + * Maximum length in chars that a beacon url can have. + * If a url is longer than that, the log may be truncated + * by varnishncsa and result in validation problems. + * + * @property maxUrlSize + * @type Number + */ + maxUrlSize: 1000, + + /** * Load a schema from the schema registry. * If the schema does not exist, it will be initialised. * @@ -204,6 +214,23 @@ }, /** + * Checks whether a beacon url is short enough, + * so that it does not get truncated by varnishncsa. + * + * @param {String} schemaName Canonical schema name. + * @param {String} url Beacon url. + * @return {String|undefined} The error message in case of error. + * + */ + checkUrlSize: function ( schemaName, url ) { + if ( url.length > self.maxUrlSize ) { + var message = 'Url exceeds maximum length'; + mw.track( 'eventlogging.error', mw.format( '[$1] $2', schemaName, message ) ); + return message; + } + }, + + /** * Transfer data to a remote server by making a lightweight HTTP * request to the specified URL. * @@ -228,9 +255,18 @@ * @return {jQuery.Promise} jQuery Promise object for the logging call. */ logEvent: function ( schemaName, eventData ) { - var event = self.prepare( schemaName, eventData ); - self.sendBeacon( self.makeBeaconUrl( event ) ); - return $.Deferred().resolveWith( event, [ event ] ).promise(); + var event = self.prepare( schemaName, eventData ), + url = self.makeBeaconUrl( event ), + sizeError = self.checkUrlSize( schemaName, url ), + deferred = $.Deferred(); + + if ( !sizeError ) { + self.sendBeacon( url ); + deferred.resolveWith( event, [ event ] ); + } else { + deferred.rejectWith( event, [ event, sizeError ] ); + } + return deferred.promise(); } }; diff --git a/server/eventlogging/parse.py b/server/eventlogging/parse.py index c0f8be4..c41136c 100644 --- a/server/eventlogging/parse.py +++ b/server/eventlogging/parse.py @@ -65,6 +65,11 @@ # used to anonymize IP addresses. KEY_LIFESPAN = datetime.timedelta(days=90) +# Limit to the query string size. +# If a log has a longer query string, the developer will see an error. +# This will help the developer avoid log truncation by varnishncsa. +MAX_QS_SIZE = 900 + def capsule_uuid(capsule): """Generate a UUID for a capsule object. @@ -100,6 +105,11 @@ """Decodes a QSON (query-string-encoded JSON) object. :param qs: Query string. """ + # Raises an error when the query string is longer than MAX_QS_SIZE. + if len(qson) > MAX_QS_SIZE: + raise ValueError( + 'Query string size (%d) is greater than max size (%d)' % + (len(qson), MAX_QS_SIZE)) return json.loads(unquote_plus(qson.strip('?;'))) diff --git a/tests/ext.eventLogging.tests.js b/tests/ext.eventLogging.tests.js index 4678110..b0279d9 100644 --- a/tests/ext.eventLogging.tests.js +++ b/tests/ext.eventLogging.tests.js @@ -115,6 +115,39 @@ } ); } ); + + $.each( { + 'URL size is ok': { + size: mw.eventLog.maxUrlSize, + expected: undefined + }, + 'URL size is not ok': { + size: mw.eventLog.maxUrlSize + 1, + expected: 'Url exceeds maximum length' + } + }, function ( name, params ) { + QUnit.test( name, 1, function ( assert ) { + var url = new Array( params.size + 1 ).join( 'x' ), + result = mw.eventLog.checkUrlSize( 'earthquake', url ); + assert.deepEqual( result, params.expected, name ); + } ); + } ); + + QUnit.asyncTest( 'logTooLongEvent', 1, function ( assert ) { + var event = { + epicenter: 'Valdivia', + magnitude: 9.5, + article: new Array( mw.eventLog.maxUrlSize + 1).join( 'x' ) + }; + + mw.eventLog.logEvent( 'earthquake', event ).always( function ( e, error ) { + QUnit.start(); + assert.deepEqual( error, 'Url exceeds maximum length', + 'logEvent promise resolves with error' ); + } ); + } ); + + QUnit.test( 'setDefaults', 1, function ( assert ) { var prepared, defaults; -- To view, visit https://gerrit.wikimedia.org/r/196599 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ie164a9357cdd599823f64122dcc79fec2198832f Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/EventLogging Gerrit-Branch: master Gerrit-Owner: Mforns <mfo...@wikimedia.org> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits