Support using templates with mozStorage. Very simple code is available but it doesn't compile.

neil, if you have a patch (even if it doesn't compile) can you attach here?

This zip file contains the partial storage implementation, which doesn't build. It needs to be made into a component with the id: @mozilla.org/xul/xul-query-processor;1?name=storage Right now it's hard coded to use the profile database. Changes are needed to the template builder to allow it to determine the one to use from the datasources attribute. But that should ideally be done in a generic way.

Comment on attachment 241172 [details] Storage files text/zip -> application/zip

Neil, any chance you might have time to work on this again?

No, I don't plan on working on this.

I will update the current patch and improve it.

Here is updated files. I added also a little patch. It compiles now, but it does not work : the query processor is instancied but never call. Because of bug 321170, we cannot use the datasources attribute, and so it is never modified, and then the query processor is not called. I will take a look for a solution for the bug 321170.

Here is the first patch which works on the trunk. It is based on the new template builder API provided by the patch of bug 321170, so you should apply it before applying this new patch. During the display of a template, there are many assertions because of the GetBindingObjectFor method which returns NS_ERROR_NOT_IMPLEMENTED. I don't really understand the purpose of this method and I don't know if it must be implemented or not. I will investigate later. In the datasources attribute, you should provide an URL which correspond to a local file : "file://", "resource://" etc.. You can also provide a "profile" url. The profile url is a shortcut to the user profile directory, so you can use easily sqlite database stored in a profile. Ex : "profile:places.sqlite". The query will be made on the places.sqlite database.

Here is an example of a template. You should access to this file with a chrome url (so save this file in an extension for example).

(In reply to comment #8) > During the display of a template, there are many assertions because of the > GetBindingObjectFor method which returns NS_ERROR_NOT_IMPLEMENTED. I don't > really understand the purpose of this method and I don't know if it must be > implemented or not. I will investigate later. > It should just return null. It is just like GetBindingFor except returns an nsISupports instead of a string. The template builder only uses this for rdf when sorting.

Laurent, is your latest patch a work in progress, or is it ready for review?

Well, it works for a simple example, but I would like to make more tests. I didn't verify yet if it works well on complex templates. I need also to improved the source code (spaces, indentation etc..). So you can consider the current patch as a work in progress. I think I could propose a new patch ready for a review next week.

A new version of the patch, ready for a review. I made improvements on nsXULTemplateQueryProcessorStorage::CompareResults : now comparisons are made depending of the type of datas (so the template works well when using the sort and sortDirection attributes). I made an other change in order to perform this comparisons : values of each field are stored into a nsIVariant object instead of a simple string.

This zip file contains a directory with a little sqlite database and some XUL files with templates. You should install it into a chrome area.

Comment on attachment 273586 [details] [diff] [review] patch v2 >+++ content/xul/templates/src/nsXULTemplateQueryProcessorStorage.cpp 2007-07-24 16:05:36.000000000 . ... >+ * >+ * The Original Code is Mozilla Communicator client code. This isn't right, I think. >+ * >+ * The Initial Developer of the Original Code is >+ * Netscape Communications Corporation. >+ * Portions created by the Initial Developer are Copyright (C) 1998 >+ * the Initial Developer. All Rights Reserved. Er, it is 2007 now and I think the original code wasn't written by Netscape >+NS_IMETHODIMP >+nsXULTemplateQueryProcessorStorage::GetDatasource(nsIArray* aDataSources, >+ nsIDOMNode* aRootNode, >+ PRBool aIsTrusted, >+ nsIXULTemplateBuilder* aBuilder, >+ PRBool* aShouldDelayBuilding, >+ nsISupports** aReturn) Align rest of the parameters with nsIArray* aDataSources, >+{ >+ *aReturn = nsnull; >+ *aShouldDelayBuilding = PR_FALSE; >+ nsresult rv; >+ >+ PRUint32 length, index; >+ rv = aDataSources->GetLength(&length); nsresult rv = aDataSources->GetLength(&length); >+ NS_ENSURE_SUCCESS(rv,rv); Space after rv, >+ >+ if (length==0 || !aIsTrusted) { space before and after == >+ nsCOMPtr<nsIURI> uri; >+ >+ // we get the first uri we find. this query processor supports >+ // only one database at a time Sentences start with capital letter. >+ for (index =0; index < length; index++) { Space after = I'd prefer ++index >+ uri = do_QueryElementAt(aDataSources,index); space before index >+NS_IMETHODIMP >+nsXULTemplateQueryProcessorStorage::InitializeForBuilding(nsISupports* aDatasource, >+ nsIXULTemplateBuilder* aBuilder, >+ nsIDOMNode* aRootNode) >+{ >+ if (mGenerationStarted) >+ return NS_ERROR_UNEXPECTED; Maybe NS_ENSURE_STATE(!mGenerationStarted); >+NS_IMETHODIMP >+nsXULTemplateQueryProcessorStorage::CompileQuery(nsIXULTemplateBuilder* aBuilder, ... >+ for (PRUint32 n = 0; n < length; n++) >+ { { in the same line as for (;;) >+ // XXX this needs to be query specific What you mean here? Is this something we want to fix later? If it is, file a follow-up bug and add the bug number here. >+nsXULTemplateQueryProcessorStorage::GenerateResults(nsISupports* aDatasource, >+ nsIXULTemplateResult* aRef, >+ nsISupports* aQuery, >+ nsISimpleEnumerator** aResults) Align parameters. >+nsXULTemplateQueryProcessorStorage::AddBinding(nsIDOMNode* aRuleNode, >+ nsIAtom* aVar, >+ nsIAtom* aRef, >+ const nsAString& aExpr) Also here >+NS_IMETHODIMP nsXULTemplateQueryProcessorStorage::TranslateRef(nsISupports* aDatasource, >+ const nsAString& aRefString, >+ nsIXULTemplateResult** aRef) And here. NS_IMETHODIMP should be in it own line - or at least that is the style you have used elsewhere in the file. >+NS_IMETHODIMP nsXULTemplateQueryProcessorStorage::CompareResults(nsIXULTemplateResult* aLeft, >+ nsIXULTemplateResult* aRight, >+ nsIAtom* aVar, >+ PRInt32* aResult) Same here >+ *aResult = ::Compare(nsDependentString(leftVal), >+ nsDependentString(rightVal), >+ nsCaseInsensitiveStringComparator()); Indentation Is :: before Compare really needed? Is CaseInsensitive what we really want here. >--- content/xul/templates.trunk/src/nsXULTemplateQueryProcessorStorage.h 1970-01-01 01:00:00.000000000 +0100 >+++ content/xul/templates/src/nsXULTemplateQueryProcessorStorage.h 2007-07-24 16:20:32.000000000 +0200 Update the license block. >+/** >+ * An single result of an query from mozstorage Spelling About security; can only chrome access mozStorage or also content, or when can content access the data? And which data?

> // XXX this needs to be query specific > What you mean here? It's not a comment from me, but a Neil's comment. I don't know too what does he mean. >About security; can only chrome access mozStorage or also content, or when can content access the data? And which data? Only XUL from chrome can use a template with mozstorage. A security check is done in GetDatasource method (test on aIsTrusted parameter, given by the template builder, which says if the template is in a trusted document or not). And only two kind of URI are allowed for the sqlite database : file URI, or the special "profile:" protocol (see comment above). So all sqlite database on the hard drive are allowed, included all Firefox databases. I think security checks in the GetDatasource() method are enough. Tell me if I forgot something.

This patch include all corrections of errors pointed out by smaug.

Comment on attachment 273757 [details] [diff] [review] patch v2.1 else if (querytype.EqualsLiteral("xml")) { mQueryProcessor = new nsXULTemplateQueryProcessorXML(); NS_ENSURE_TRUE(mQueryProcessor, NS_ERROR_OUT_OF_MEMORY); } + else if (querytype.EqualsLiteral("storage")) { + mQueryProcessor = new nsXULTemplateQueryProcessorStorage(); + NS_ENSURE_TRUE(mQueryProcessor, NS_ERROR_OUT_OF_MEMORY); + } else { nsCAutoString cid(NS_QUERY_PROCESSOR_CONTRACTID_PREFIX); AppendUTF16toUTF8(querytype, cid); mQueryProcessor = do_CreateInstance(cid.get(), &rv); // XXXndeakin log an error here - bug 321169 NS_ENSURE_TRUE(mQueryProcessor, rv); } There was some reason which I can't remember, why I did it that way for xml, but we should just rely on the last else block and create a contractid for the storage query processor rather than having several if blocks. diff -rNu8p -x CVS content/xul/templates.trunk/src/nsXULTemplateQueryProcessorStorage.cpp content/xul/templates/src/nsXULTemplateQueryProcessorStorage.cpp +#include "nsIArray.h" +#include "nsArrayUtils.h" +#include "nsString.h" nsString is already included in the header. Some of the other includes might not be needed either. + + PRUint32 length, index; + nsresult rv = aDataSources->GetLength(&length); + NS_ENSURE_SUCCESS(rv, rv); + + if (length == 0 || !aIsTrusted) { + return NS_OK; + } Could also check aIsTrusted earlier, but not a big deal. + // We get the first uri we find. This query processor supports + // only one database at a time Period at end. + for (index = 0; index < length; ++index) { + uri = do_QueryElementAt(aDataSources, index); + if (uri) { + break; + } + } I think we should just check only the first item, rather than the first item that is valid. + rv = ioservice->NewChannelFromURI(uri, getter_AddRefs(channel)); + NS_ENSURE_SUCCESS(rv,rv); + + if (!channel) + return NS_ERROR_NULL_POINTER; + + nsCOMPtr<nsIFileChannel> fileChannel = do_QueryInterface(channel); + if (!fileChannel) + return NS_ERROR_FAILURE; // not a file url + + nsCOMPtr<nsIFile> file; + rv = fileChannel->GetFile(getter_AddRefs(databaseFile)); + NS_ENSURE_SUCCESS(rv,rv); What happens here if the database file doesn't exist? Does it fail? +NS_IMETHODIMP +nsXULTemplateQueryProcessorStorage::CompileQuery(nsIXULTemplateBuilder* aBuilder, ... + + PRUint16 nodeType; + child->GetNodeType(&nodeType); + + if (nodeType == nsIDOMNode::TEXT_NODE) { Should also allow CDATA_SECTION_NODE types here as well +NS_IMETHODIMP +nsXULTemplateQueryProcessorStorage::CompareResults(nsIXULTemplateResult* aLeft, + nsIXULTemplateResult* aRight, + nsIAtom* aVar, + PRInt32* aResult) +{ + *aResult = 0; + + PRInt32 idx = GetColumnIndex(aVar); + if(idx == -1) + return NS_OK; + + // We're going to see if values are integers or float, to perform + // a suitable comparison + nsCOMPtr<nsISupports> sLeftValue, sRightValue; The s prefix is usually used for static variables. I think just using leftValue and rightValue would be better. + if (vtypeL == vtypeR && vtypeL == nsIDataType::VTYPE_INT32) { ... + else if (vtypeL == vtypeR && vtypeL == nsIDataType::VTYPE_DOUBLE) { Could combine the two a bit better to avoid doing the same comparison twice but no big deal diff -rNu8p -x CVS content/xul/templates.trunk/src/nsXULTemplateResultStorage.cpp content/xul/templates/src/nsXULTemplateResultStorage.cpp + +NS_IMETHODIMP +nsXULTemplateResultStorage::GetBindingFor(nsIAtom* aVar, nsAString& aValue) +{ + PRInt32 idx = mProcessor->GetColumnIndex(aVar); + if (idx >= 0){ + nsCOMPtr<nsIVariant> value; + value = mValues[idx]; + if (value) { + nsresult rv = value->GetAsAString(aValue); + if(NS_SUCCEEDED(rv)) + return NS_OK; We may want to better optimize for strings instead of converting to variants and then back, but that can be a followup bug. Could also just put the NS_SUCCEEDED around the function rather than using a temporary rv. + +NS_IMETHODIMP +nsXULTemplateResultStorage::GetBindingObjectFor(nsIAtom* aVar, nsISupports** aValue) +{ + PRInt32 idx = mProcessor->GetColumnIndex(aVar); + if (idx >= 0){ + *aValue = mValues[idx]; + NS_ADDREF(*aValue); NS_IF_ADDREF would be better here. What happens when GetBindingObjectFor is called from JS? Do the variants get converted into native js types? This would be easier to use, but might be unexpected. I can't decide whether that is a good thing or not.

Comment on attachment 273757 [details] [diff] [review] patch v2.1 >+nsXULTemplateQueryProcessorStorage::CompileQuery(nsIXULTemplateBuilder* aBuilder, ... >+ for (PRUint32 c = 0; c < count; c++) { >+ nsCAutoString name; >+ statement->GetColumnName(c, name); >+ mColumnNames.AppendObject(NS_NewAtom(NS_LITERAL_CSTRING("?") + name)); >+ } I think you're leaking nsIAtom objects. NS_NewAtom AddRefs, so does AppendObject. Use an nsCOMPtr and do_GetAtom. >+nsXULTemplateResultStorage::GetId(nsAString& aId) >+{ >+ const char* uri; initialize to nsnull, just in case GetValueConst starts failing at some point. with those, r=me, but Neil D. should review this.

> // XXX this needs to be query specific > What you mean here? If more than one query is used, the columns used in the queries may be different, so really there should be several column lists.

>There was some reason which I can't remember, why I did it that way for xml, >but we should just rely on the last else block and create a contractid for the >storage query processor rather than having several if blocks. I don't really see the benefit of the use of a contractid since this query processor is included into gecko. For me, the use of a contractid is only useful for those who provide their own query processor in their application or extension. Loading a component by its contractid take more time (during the instantiation) and resources (by its registration in the module), right ? And is there a case where a developer would create himself an instance of the storage queryprocessor ? If the contractid is really needed, where should I declare the component ? In layout/build/nsLayoutModule.cpp ? >What happens here if the database file doesn't exist? Does it fail? Yes, it does (assertion in the console). It fails also when setting a bad "profile:" URI. However a better logging is needed, but I think we could fix that with bug 321169. >We may want to better optimize for strings instead of converting to variants and then back So, should I duplicate values in two arrays, one with nsIVariant objects, and an other with strings ? What do you think about the memory footprint ? Is it acceptable ? >What happens when GetBindingObjectFor is called from JS? Do the variants get >converted into native js types? I think it does because we don't have to convert values from or to nsIVariant when using nsIDOMNode::setUserData or nsIDOMNode::getUserData. I don't know XPConnect source code, but I see in XPCConvert::JSData2Native and XPCConvert::NativeData2JS that it converts values from or to nsIVariant objects when it is needed. >If more than one query is used, the columns used in the queries may be different, so really there should be several column lists. Ok I'm going to make this improvement.

What is this bug/patch waiting for at the moment? I know a bunch of people anxious to see this landing ;-)

This patch fixes all issues pointed by Neil except the adding of a contract ID for the query processor and the optimization on nsXULTemplateResultStorage::GetBindingFor (no response to my questions on this two points). :-)

New version of example files to test templates with mozstorage. Just few changes to test with the v3+ of the patch .

Comment on attachment 280340 [details] [diff] [review] patch v3 >+void >+nsXULTemplateResultSetStorage::FillColumnValues(nsCOMArray<nsIVariant>& aArray) ... >+ >+ for (PRInt32 c = 0; c < count; c++) { >+ >+ nsCOMPtr<nsIWritableVariant> value = new nsVariant(); Remove the extra blank line. >+nsXULTemplateQueryProcessorStorage::CompileQuery(nsIXULTemplateBuilder* aBuilder, ... >+ if (nodeType == nsIDOMNode::TEXT_NODE >+ || nodeType == nsIDOMNode::CDATA_SECTION_NODE) { Put the || at the end of the line. >+class nsXULTemplateResultStorage : public nsIXULTemplateResult ... >+ nsXULTemplateResultSetStorage* mResultSet; >+ Why isn't an nsCOMPtr used here? The constructor and destructor just addref and release. Otherwise, this looks good.

>Remove the extra blank line >[...] >Put the || at the end of the line. Ok I will fix that. >Why isn't an nsCOMPtr used here? The constructor and destructor just addref and release. Because I think it is not necessary to create a new interface for two public methods like GetColumnIndex and FillColumnValues, called only by nsXULTemplateResultStorage.

(In reply to comment #26) > > Because I think it is not necessary to create a new interface for two public > methods like GetColumnIndex and FillColumnValues, called only by > nsXULTemplateResultStorage. > OK, in that case an nsRefPtr should be suitable here so the manual refcounting can be removed. That is: nsRefPtr<nsXULTemplateResultSetStorage> mResultSet

Comment on attachment 280340 [details] [diff] [review] patch v3 >+nsXULTemplateResultSetStorage::nsXULTemplateResultSetStorage(mozIStorageStatement* aStatement) >+ : mStatement(aStatement) >+{ >+ PRUint32 count; >+ aStatement->GetColumnCount(&count); >+ >+ for (PRUint32 c = 0; c < count; c++) { >+ nsCAutoString name; >+ aStatement->GetColumnName(c, name); >+ nsCOMPtr<nsIAtom> columnName = do_GetAtom(NS_LITERAL_CSTRING("?") + name); >+ mColumnNames.AppendObject(columnName); >+ } >+} You should absolutely be checking the return variables of GetColumnCount and GetColumnName. I'm aware that this is a constructor, but if these calls fail, there is no assurance that the variables hold what you might expect. >+NS_IMETHODIMP >+nsXULTemplateResultSetStorage::HasMoreElements(PRBool *aResult) >+{ >+ if (mStatement) { >+ nsresult rv = mStatement->ExecuteStep(aResult); >+ NS_ENSURE_SUCCESS(rv,rv); >+ // Because the nsXULTemplateResultSetStorage is owned by many nsXULTemplateResultStorage objects, >+ // it could live longer than it needed to get results. >+ // So we destroy the statement to free ressources when all results are fetched >+ if (*aResult == PR_FALSE) { >+ mStatement = nsnull; >+ } >+ return NS_OK; >+ } >+ else { >+ *aResult = PR_FALSE; >+ return NS_OK; >+ } >+} code style nit: It'd be cleaner to do if (!mStatement) { *aResult = PR_FALSE; return NS_OK; } at the top of the function, and then not have everything else in an if statement (branching sucks). >+void >+nsXULTemplateResultSetStorage::FillColumnValues(nsCOMArray<nsIVariant>& aArray) >+{ >+ if (!mStatement) >+ return; >+ >+ nsCOMPtr<mozIStorageValueArray> record = do_QueryInterface(mStatement); You don't need to do this - mozIStorageStatement inherits from mozIStorageValueArray. >+ nsCOMPtr<mozIStorageService> storage = >+ do_GetService("@mozilla.org/storage/service;1"); >+ >+ if (!storage) >+ return NS_ERROR_FAILURE; Probably better to use the two param version of do_GetService so you can just NS_ENSURE_SUCCESS the result. >+ nsCAutoString scheme; >+ uri->GetScheme(scheme); you should check the return result >+ if (scheme.EqualsLiteral("profile")) { >+ >+ nsCAutoString path; >+ uri->GetPath(path); ditto >+ if (path.IsEmpty()) { >+ return NS_ERROR_FAILURE; >+ } >+ >+ nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, >+ getter_AddRefs(databaseFile)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ if (!databaseFile) >+ return NS_ERROR_UNEXPECTED; you don't need this - the NS_ENSURE_SUCCESS already checks this >+ databaseFile->AppendNative(path); check result >+ } >+ else { >+ nsCOMPtr<nsIChannel> channel; >+ nsCOMPtr<nsIIOService> ioservice = >+ do_GetService("@mozilla.org/network/io-service;1",&rv); nit: space after comma >+ NS_ENSURE_SUCCESS(rv,rv); ditto, and same for other parts of code >+ rv = ioservice->NewChannelFromURI(uri, getter_AddRefs(channel)); >+ NS_ENSURE_SUCCESS(rv,rv); >+ >+ if (!channel) >+ return NS_ERROR_NULL_POINTER; don't need this (also not the right return type) >+ >+ nsCOMPtr<nsIFileChannel> fileChannel = do_QueryInterface(channel); >+ if (!fileChannel) >+ return NS_ERROR_FAILURE; // not a file url you should use the two param version again with NS_ENSURE_SUCCESS >+ if (!databaseFile) { >+ return NS_ERROR_FAILURE; >+ } This would have already errored out by the time you got here, so you don't need it either. >+ >+ // ok now we have an URI of a sqlite file >+ nsCOMPtr<mozIStorageConnection> connection; >+ rv = storage->OpenDatabase(databaseFile, getter_AddRefs(connection)); >+ NS_ENSURE_SUCCESS(rv,rv); You probably shouldn't get the service up until just before here too. >+ if (!connection) >+ return NS_ERROR_FAILURE; not necessary >+ >+ return CallQueryInterface(connection, aReturn); I don't think this is needed either - it inherits from nsISupports (if anything, a static cast would do if it doesn't compile without). NS_ADDREF(*aResult = connection); >+ ~nsXULTemplateResultSetStorage() >+ { >+ } You don't need to declare an empty destructor, the compiler does it for you if you don't specify one. >+ ~nsXULTemplateQueryProcessorStorage(){}; ditto Nearly there. :)

Updated patch.

Comment on attachment 280451 [details] [diff] [review] patch v3.1 >+#include "nsVariant.h" Actually, you should be #including nsIVariant here I. >+nsXULTemplateResultSetStorage::nsXULTemplateResultSetStorage(mozIStorageStatement* aStatement) >+ : mStatement(aStatement) >+{ >+ PRUint32 count; >+ nsresult rv = aStatement->GetColumnCount(&count); >+ if (NS_FAILED(rv)) { >+ return; I think you need to null out mStatement here too to make sure things work properly if it were to fail. >+ // So we destroy the statement to free ressources when all results are fetched nit: resources >+void >+nsXULTemplateResultSetStorage::FillColumnValues(nsCOMArray<nsIVariant>& aArray) >+{ >+ if (!mStatement) >+ return; >+ >+ PRInt32 count = mColumnNames.Count(); >+ >+ for (PRInt32 c = 0; c < count; c++) { >+ nsCOMPtr<nsIWritableVariant> value = new nsVariant(); do_CreateInstance("@mozilla.org/variant;1") >+ PRInt32 type; >+ mStatement->GetTypeOfIndex(c, &type); check return type >+ >+ if (type == mStatement->VALUE_TYPE_INTEGER) { >+ PRInt32 val; >+ mStatement->GetInt32(c, &val); PRInt32 val = mStatement->AsInt32(c); >+ value->SetAsInt32(val); >+ } >+ else if (type == mStatement->VALUE_TYPE_FLOAT) { >+ double val; >+ mStatement->GetDouble(c, &val); double val = mStatement->AsDouble(c); >+ value->SetAsDouble(val); >+ } >+ else { >+ nsAutoString val; >+ mStatement->GetString(c, val); the return value should be checked here probably >+ value->SetAsAString(val); >+ } >+ aArray.AppendObject(value); Technically, this can fail too (only if we run out of memory though). It returns a PRBool if I recall correctly. General comment: Please note that until you reset the statement, no writing will be able to happen to the database. I'm not 100% sure how templates work, but I want to make sure you are aware of this implication. Otherwise, r=sdwilsh

Updated patch. >Please note that until you reset the statement, no writing will be able to happen to the database. I'm not 100% sure how templates work, but I want to make sure you are aware of this implication. I don't think there is a problem for that, since the statement is used and results are fetched in the content builder, just after the creation of the statement.

+ nsCOMPtr<nsIVariant> value; + value = mValues[idx]; + if (value) { + value->GetAsAString(aValue); + } can't this be a regular nsIVariant*? When you access mValues[idx], these accesses could be out of bounds if mStatement->GetString(c, val) failed in FillColumnValues. In nsXULTemplateQueryProcessorStorage::CompileQuery, wouldn't it make more sense to concatenate the nodes together to form the statement instead of just using the first node? Someone might find it convenient to use DOM APIs to create multiple text nodes to make a query. This approach to building statements seems to be rather prone to SQL injection attacks; all dynamic query parameters have to be correctly quoted and appended into the statement string. This doesn't seem like a good API. Maybe the query DOM should support a SQL statement as text containing ? placeholders, followed by some <param> elements or something that containing parameters that should be substituted for the placeholders?

(In reply to comment #32) > This approach to building statements seems to be rather prone to SQL injection > attacks; all dynamic query parameters have to be correctly quoted and appended > into the statement string. There aren't any dynamic query parameters, the query string is plain text with no substitution. I'm not clear what you think is a problem here. Eventually we may wish to support some parameters to allow better recursion, but that isn't done here.

I think Roc means a situation like that: Imagine an extension which uses a template with this query processor. The extension allows the user to enter a value in a field. Then the extension construct the query with this value (var sql = "select...from...where value='"+theFieldValue+"'";) but without verifying the value, and then passes this sql query in the query element. Imagine now the user enters "foo'; DELETE FROM aTable WHERE ''='"... However, I don't think that is very dangerous. First, because in this case, this user is an idiot, because he delete his own datas. Second, since this query processor can be used only in a chrome file, I think there isn't any consequence if a unprivileged script (in a web page) try to modify the query. Furthermore, perhaps mozstorage cannot execute two queries in a same call. I will verify that.

If we are worried about SQL injection type of attacks, we should be binding any and all parameters, but I'm not sure how we can go about doing that with the current setup.

(In reply to comment #34) > However, I don't think that is very dangerous. First, because in this case, > this user is an idiot, because he delete his own datas. Second, since this > query processor can be used only in a chrome file, I think there isn't any > consequence if a unprivileged script (in a web page) try to modify the query. There is every chance, though, that an extension will pull data from an untrusted source to use as part of a query. Imagine for example that an extension takes text from an untrusted Web page (say, some selected text) and uses it as part of query. It's just good practice to allow --- or better still, require --- parameters to be bound explicitly rather than stuffing them into the query string. This lesson has been learned many times, let's learn from other people's mistakes this time.

Even if we offer some elements like <param> to allow to use dynamic parameters in a safe way, it won't force an extension developer to use them. So he could still generate himself a query with parameters in an unsafe way. A more secured way would be to analyse the SQL query, in the query processor and to keep only SELECT statements, before to pass the SQL query to mozstorage. However, it is not needed, because mozstorage execute only one query when we pass two queries at the same time. For example, in the <query> element, I wrote "select product_id, label, price, cat_id from products; delete from things;". After the display of the xul page, the "things" table still contains its records. So I think there is not security issue.

(In reply to comment #37) > So I think there is not security issue. > So, untrusted content can't inject a second query, but it can still *modify* the query - eg: it won't be able to alter the database structure, but it'll still be able to have data fetched that is not supposed to. Now, about the "injection issue" as a whole, this is not specific to templates: an extension developer who uses mozStorage and build-up queries with unverified untrusted content allows for SQL injection just the same. We don't consider this a security issue - it's the extension developer problem not to shoot himself in the foot... Should we take a different approach here?

(In reply to comment #37) > Even if we offer some elements like <param> to allow to use dynamic parameters > in a safe way, it won't force an extension developer to use them. So he could > still generate himself a query with parameters in an unsafe way. That's true. However, we should still make it easy for the developer to do the right thing. > A more secured > way would be to analyse the SQL query, in the query processor and to keep only > SELECT statements, before to pass the SQL query to mozstorage. This is also a good idea. > However, it is not needed, because mozstorage execute only one query when we > pass two queries at the same time. For example, in the <query> element, I > wrote "select product_id, label, price, cat_id from products; delete from > things;". After the display of the xul page, the "things" table still > contains its records. > > So I think there is not security issue. I don't think we can predict all possible ways this code could be used, and therefore we can't predict all possible ways injection flaws could be abused. In some application, it might matter if someone gets unexpected control over which records are selected.

> In some application, it might matter if someone gets unexpected control over which records are selected. Yes, but there is no solution to prevent for SQL injection like Olivier described as an example. And you have the same problem when the developer use directly mozstorage. So this is the developer who is responsible to verify all untrusted values. I don't know any other framework which prevent 100% of SQL injection, without a work from the developer to take care of this security issues. For me, there aren't more security issues in my query processor than in the case when the developer use the mozstorage API. However, I'm agree to offer an API to support queries with parameters, but I would like to develop it later, in an other patch, in an other bug, and see my current patch landed into the trunk (after doing improvements you want in nsXULTemplateQueryProcessorStorage::CompileQuery). Because, first, it will give me less work (I have no write access in the CVS, and doing a diff with new files sucks with CVS), and second, we have to discuss about this API, so it will take time, and then I'm afraid it will be too late to see this query processor in Gecko 1.9 :-(

Note that templates get built before there is any chance to set any parameters. However, for rebuilding, one could, in a follow up bug, use syntax like: <query> select * from sometable where cat=?dog,bat=?rat <param var="?dog" value="Gnat"/> </query> I assume that named parameters are allowed, using the <param> elements or values from the ref for recursion. And/or you could also support <... ref="?dog=Gnat,?rat=Flea"> Then someone could just set the dom nodes and rebuild as desired.

I haven't looked at the API in detail, so I can't speak to specifics. However, some of the reasoning in comment 40 is faulty: > Yes, but there is no solution to prevent for SQL injection like Olivier > described as an example. And you have the same problem when the developer use > directly mozstorage. So this is the developer who is responsible to verify all > untrusted values. I don't know any other framework which prevent 100% of SQL > injection, without a work from the developer to take care of this security > issues. Preventing 100% of the SQL injection cases isn't the goal. Maximizing the size of the prevented set (or minimizing the size of the set of extensions with security holes) is. > For me, there aren't more security issues in my query processor than in the > case when the developer use the mozstorage API. This may be true, but it's a straw man. The trajectory of the code base should be that new APIs default to maximum safety so that overall, the percentage of unsafe APIs continues to shrink.

(In reply to comment #40) > > In some application, it might matter if someone gets unexpected control over > which records are selected. > > Yes, but there is no solution to prevent for SQL injection like Olivier > described as an example. And you have the same problem when the developer use > directly mozstorage. So this is the developer who is responsible to verify > all untrusted values. I don't know any other framework which prevent 100% of > SQL injection, without a work from the developer to take care of this security > issues. We already agreed that 100% protection is not our goal. All I want is to make it easy for developers to do the right thing. The WHATWG SQL API is a good example: http://www.whatwg.org/specs/web-apps/current-work/multipage/section-sql.html#executing > For me, there aren't more security issues in my query processor than in the > case when the developer use the mozstorage API. We're trying to make things better, not worse. > However, I'm agree to offer an API to support queries with parameters, but I > would like to develop it later, in an other patch, in an other bug, and see my > current patch landed into the trunk (after doing improvements you want in > nsXULTemplateQueryProcessorStorage::CompileQuery). Because, first, it will > give me less work (I have no write access in the CVS, and doing a diff with > new files sucks with CVS), and second, we have to discuss about this API, so > it will take time, and then I'm afraid it will be too late to see this query > processor in Gecko 1.9 :-( Susceptibility to SQL injection attacks will not help it get into 1.9. I think once we've decided on an API this will be easy to implement. > Note that templates get built before there is any chance to set any > parameters. Can't the developer suppress query execution until it's ready? For API, how about <query> select * from sometable where cat=?,bat=? <param id="cat"/><param id="bat"/> </query> ... document.getElementById('cat').textContent = "Hat"; document.getElementById('bat').textContent = "Fat"; ... so the parameters are simply taken in-order from the <query>'s child list, and we use the textContent of each element. An alternative: <query> select * from sometable where cat=<param id="cat"/>,bat=<param id="bat"/> </query>

Ok, I'm going to make this improvements... >Can't the developer suppress query execution until it's ready? I see two solutions : 1) The query processor could support an attribute on the <query> element (holdon="true" ? suspended="true" ?). It it is true, it doesn't execute the query. After the developer set up its param elements, he remove the attribute on the query element, and then can call rebuild(). 2) The value of the parameter is set into an attribute (value=""), not in the content. If the query processor don't see a value attribute, it won't execute the query. So the developer have to set a value attribute on each param element (even if this value is an empty string), and then have to call rebuild() method. I think the second solution is better. For API now. Note that you can have named parameters (see here http://www.sqlite.org/capi3ref.html#sqlite3_bind_blob ). SQL syntaxe for parameters are "?", "?NNN", ":AAA", "@AAA", "$VVV". So I propose <param id="" name="theName">theValue</param> ( or <param id="" name="theName" value="theValue" /> ). The name attribute is of course optional. I think I will add also the support of a "type" attribute, so it binds the value with the given type. Possible values of this type attribute will be "string" (default), "double", "int32", "int64" and "null" (in this case, the given value in the param element will be ignored, and if we choose the solution with the value attribute, this attribute is optional).

> Can't the developer suppress query execution until it's ready? Actually, I'm not sure why I said that, it isn't important. Building can already be delayed by just not setting the 'ref' attribute. Which is why I like the ref="?cat=dog" solution, in addition to the <param> notation for extra arguments. This allows recursion to be done, which none of the other solutions allow, as in: <vbox datasources="..." ref="?id=rootid"> <query> select id, field1, field2 from table where parent=?id </query> ... </vbox>

Thanks Laurent. Since I know next to nothing about SQL and XUL templates, I'll leave the API decisions to you and Neil ... except ... > the ref="?cat=dog" solution Doesn't this open up the possibility of "XUL ref attacks"? We really want the param values to be stand-alone textContent or attribute values. One advantage of textContent over attribute values is that "elem.textContent = ..." is shorter than "elem.setAttribute('value', ...)". But maybe that's only a small advantage.

>Building can already be delayed by just not setting the 'ref' attribute. Ok, let's forget my attribute on <query> or the checking on the value attribute on <param> :-) > the ref="?cat=dog" solution I don't like it very much too. Because the query processor has to parse it (and I don't like to code this sort of parsing :-) ), and because the developer has to construct a string (by escaping specials chars like "?" in his values, by concatenating etc.). For me, it is less sexy and more complex than just setting an attribute or the content on a param element. >One advantage of textContent over attribute values is that "elem.textContent = ..." is shorter than "elem.setAttribute('value', ...)". But maybe that's only a small advantage. Since we can use the ref attribute to delay the building, we can put the value as a content of <param>, and not in an attribute.

Ok now I've made the improvements. I can do this: <rows datasources="tests.sqlite" querytype="storage" id="rows" ref="?"> <query> select product_id, label, price, cat_id from products where price > :price <param id="price-param" name=":price" type="int32"/> </query> And I just have to do this to change the parameter and to refresh the generated content: document.getElementById("price-param").textContent = "270"; document.getElementById("rows").builder.rebuild(); I will post a new version of the patch later, after additionnals tests.

(In reply to comment #48) > Ok now I've made the improvements. I can do this: > > <rows datasources="tests.sqlite" querytype="storage" id="rows" ref="?"> > <query> > select product_id, label, price, cat_id from products where price > :price > <param id="price-param" name=":price" type="int32"/> > </query> Note: the colon isn't necessary inside the "name" attribute, since the attribute identifies its content as a param name. And I think it's clearer to leave it out, since then the colon in the SQL string can mean "the parameter with this name". Leaving it out would also be more consistent with other implementations of named parameters, such as mozIStorageStatementParams, which use parameter markers in the SQL string but not in the API for setting parameters.

Since there are different ways to specify a parameter name in a SQLite query ( like "?5", ":foo", "@foo" or "$foo"), how this first parameter can be guessed by my code ? The implementation of mozIStorageStatementParams doesn't take care of this differents type of parameter, and this is bad IMHO.

Sorry, you should read "how this first character can be guessed..."

I forgot to clarify a thing: the parameter marker is necessary for the sqlite API, and for the mozIStorageStatement API, so name=":foo" is consistent with this APIs ;-)

(In reply to comment #50) > Since there are different ways to specify a parameter name in a SQLite query ( > like "?5", ":foo", "@foo" or "$foo"), how this first parameter can be guessed > by my code ? > > The implementation of mozIStorageStatementParams doesn't take care of this > differents type of parameter, and this is bad IMHO. We don't need to support all of SQLites's parameter name syntaxes, do we? We can just say "if you use this API, you must use :foo syntax".

> We don't need to support all of SQLites's parameter name syntaxes, why not... We should support at least "?", "?5" and ":foo" parameters in the SQL query. So what about a "name" attribute for ":foo", and an "index" attribute for "?x" ? Example: <query>select label from products where price > ?3 and label like :pattern <param name="pattern">a%</param> <param index="2" type="int32">270</param> </query> Note that index value is consistent with the behavior of mozIStorageStatement::bind*parameter methods (because indexes passed to the binding functions count from 0). It will also allow us to do things like that: <query>select label from products where price > ? and label like ? <param index="1">a%</param> <param index="0" type="int32">270</param> </query> So the list of <param> elements haven't to respect the order of declared parameters in the query. Note that name and index attribute are optional. In my current implementation, if the third <param> element have not one of this attribute, the default index value is 2. So we can do this: <query>select label from products where price > ? and label like ? <param type="int32">270</param> <param>a%</param> </query> Are you agree with this behaviors ?

Yeah, OK.

This new version adds the support of <param> element in a query and fixed two bugs pointed by Roc: + nsCOMPtr<nsIVariant> value; + value = mValues[idx]; + if (value) { + value->GetAsAString(aValue); + } value is now a nsIVariant * >When you access mValues[idx], these accesses could be out of bounds if >mStatement->GetString(c, val) failed in FillColumnValues. FillColumnValues now fills mValues with a correct number of values (if mStatement->GetString fails, it adds an empty string into aArray instead of doing nothing).

Added tests with param element.

(In reply to comment #54) > Note that index value is consistent with the behavior of > mozIStorageStatement::bind*parameter methods (because indexes passed to the > binding functions count from 0). It will also allow us to do things like that: Note that there is a bug file to be 1 based (that's what sqlite is). This probably won't happen until moz2 though.

(In reply to comment #58) > (In reply to comment #54) > > Note that index value is consistent with the behavior of > > mozIStorageStatement::bind*parameter methods (because indexes passed to the > > binding functions count from 0). It will also allow us to do things like that: > Note that there is a bug file to be 1 based (that's what sqlite is). This > probably won't happen until moz2 though. Given that the mozIStorageStatement::bind*parameter zero-basedness is a bug, it seems like it would be better to make this interface be one-based now rather than waiting for Moz2, when there will already be a bunch of zero-based code out there that will need to be updated.

The diff includes .#nsXULTemplateBuilder.cpp, which it shouldn't. + else if (nodeType == nsIDOMNode::ELEMENT_NODE) { + hasParameters = PR_TRUE; + break; + } Why not just collect all the text nodes? That's simpler code and lets you put the <param> elements anywhere, which is a simpler API. + if (hasParameters) { We don't need this check or hasParameters at all, right? There's no performance worry here. Simplify the code. + if (child->HasAttr(kNameSpaceID_None, nsGkAtoms::name)) { + nsAutoString name, fullName; + child->GetAttr(kNameSpaceID_None, nsGkAtoms::name, name); Just do GetAttr instead of HasAttr + GetAttr Make your parameters 1-based instead of 0-based like Myk said. Should we do something special to handle invalid parameter indexes? I guess not. + if (child->HasAttr(kNameSpaceID_None, nsGkAtoms::type)) { + nsAutoString type; + child->GetAttr(kNameSpaceID_None, nsGkAtoms::type, type); Use FindAttrValueIn + if (type.EqualsLiteral("int32")) { + PRInt32 val = 0; + PR_sscanf(NS_ConvertUTF16toUTF8(value).get(),"%d",&val); + rv = statement->BindInt32Parameter(index, val); + NS_ENSURE_SUCCESS(rv, rv); + } I think we should fail for things that aren't valid ints etc. Here I think we just bind 0, which could lead to hard-to-figure-out errors. Also, move the NS_ENSURE_SUCCESS outside the type test so it can be shared by all types.

> Use FindAttrValueIn So I should have to create atoms. How should I create them ? by creating them in nsGkAtomList.h or by using NS_NewAtom/do_GetAtom ?

> So I should have to create atoms. How should I create them ? by creating them > in nsGkAtomList.h or by using NS_NewAtom/do_GetAtom ? Just add a new line to nsGkAtomList.h for any that don't already exist, then you can refer to it using nsGkAtom::whatever

Comment on attachment 286283 [details] [diff] [review] patch v3.3 >+nsXULTemplateQueryProcessorStorage::GetDatasource(nsIArray* aDataSources, ... >+ nsCOMPtr<nsIFileChannel> fileChannel = do_QueryInterface(channel, &rv); >+ NS_ENSURE_SUCCESS(rv, rv); // if if fail, not a file url two 'if's here >+ if (child->NodeInfo()->Equals(nsGkAtoms::param, kNameSpaceID_XUL)) { >+ nsAutoString value; >+ nsContentUtils::GetNodeTextContent(child, PR_TRUE, value); Why pass PR_TRUE here. This doesn't seem like something that should be recursive. >+ >+ PRUint32 index = parameterCount++; I think it would be better to only increment this for those that don't specify a name or index. We don't need to be too smart here though.

>I think it would be better to only increment this for those that don't specify a name or index. We don't need to be too smart here though. When there is an index attribute, you're right, but with a name attribute, you are not, because a named parameter has a numerical index. For example, if you have "WHERE foo = :myparam AND bar = ?", The index of the second parameter is 2, not 1 (with a 1-based index). So I need to increment the index to have the good one if the developer put this elements : <param name="myparam" /> <param /> (or <param /><param />).

This fixes all things pointed by Roc and Enn : - All text nodes of the <query> element are collected - removed of the use of the hasParameters variable - use GetAttr instead of HasAttr + GetAttr - parameters index are 1-based - use FindAttrValueIn - check the returned value of PR_sscanf - NS_ENSURE_SUCCESS is shared by all type tests - fixed the comment // if if fail, not a file url - pass PR_FALSE to GetNodeTextContent instead of PR_TRUE - parameterCount is not incremented for <param> elements with index attribute

updated test examples.

Comment on attachment 286687 [details] [diff] [review] patch v3.4 --- content/xul/templates.trunk/src/.#nsXULTemplateBuilder.cpp.1.346 1970-01-01 01:00:00.000000000 +0100 +++ content/xul/templates/src/.#nsXULTemplateBuilder.cpp.1.346 2007-07-19 17:53:26.000000000 +0200 Could you stop this file appearing in your diffs please! cvs remove it or something

Same patch but without the bad file.

Thanks to Neil, Shawn, Olli and Robert for the reviews/superreview. I haven't write access in the CVS repository so I set the checkin-needed keyword. Perhaps there is any other flag/keyword to set, because of the roadmap ? Can this patch be integrated for M9 ? Or should we wait the next milestone ?

This needs approval first

I want endgame drivers to make the call on this one.

this may not affect Firefox to a high degree, but the potential for extension developers and XUL application developers is huge, Huge, HUGE!

SeaMonkey wants this too as it's holding up a key SM bug that depends on this.

I think this should actually be raised at the Gecko 1.9 meeting for approval. Mark, can you push it there?

Have we answered all the concerns around SQL injection and other security issues? What is the risk of this patch?

We have(In reply to comment #75) > Have we answered all the concerns around SQL injection and other security > issues? Yes. > What is the risk of this patch? Low. It's mostly new code that won't affect existing code.

Checking in content/base/src/nsGkAtomList.h; /cvsroot/mozilla/content/base/src/nsGkAtomList.h,v <-- nsGkAtomList.h new revision: 3.86; previous revision: 3.85 done Checking in content/xul/templates/src/Makefile.in; /cvsroot/mozilla/content/xul/templates/src/Makefile.in,v <-- Makefile.in new revision: 1.24; previous revision: 1.23 done Checking in content/xul/templates/src/nsXULTemplateBuilder.cpp; /cvsroot/mozilla/content/xul/templates/src/nsXULTemplateBuilder.cpp,v <-- nsXULTemplateBuilder.cpp new revision: 1.348; previous revision: 1.347 done RCS file: /cvsroot/mozilla/content/xul/templates/src/nsXULTemplateQueryProcessorStorage.cpp,v done Checking in content/xul/templates/src/nsXULTemplateQueryProcessorStorage.cpp; /cvsroot/mozilla/content/xul/templates/src/nsXULTemplateQueryProcessorStorage.cpp,v <-- nsXULTemplateQueryProcessorStorage.cpp initial revision: 1.1 done RCS file: /cvsroot/mozilla/content/xul/templates/src/nsXULTemplateQueryProcessorStorage.h,v done Checking in content/xul/templates/src/nsXULTemplateQueryProcessorStorage.h; /cvsroot/mozilla/content/xul/templates/src/nsXULTemplateQueryProcessorStorage.h,v <-- nsXULTemplateQueryProcessorStorage.h initial revision: 1.1 done RCS file: /cvsroot/mozilla/content/xul/templates/src/nsXULTemplateResultStorage.cpp,v done Checking in content/xul/templates/src/nsXULTemplateResultStorage.cpp; /cvsroot/mozilla/content/xul/templates/src/nsXULTemplateResultStorage.cpp,v <-- nsXULTemplateResultStorage.cpp initial revision: 1.1 done RCS file: /cvsroot/mozilla/content/xul/templates/src/nsXULTemplateResultStorage.h,v done Checking in content/xul/templates/src/nsXULTemplateResultStorage.h; /cvsroot/mozilla/content/xul/templates/src/nsXULTemplateResultStorage.h,v <-- nsXULTemplateResultStorage.h initial revision: 1.1 done

Is it possible to create reftests (with XUL files) in a chrome context ? I didn't see how to do it in the documentation.

This is probably more suitably tested with Mochitests: http://developer.mozilla.org/en/docs/Mochitest The environment used for that testing is such that a call to netscape....enablePrivilege will give you whatever privileges you need to do whatever you have to do. If for some reason you need to do things with chrome: URLs, there's also a variant that will dump the test files and whatever data you have at chrome: URLs; see _CHROME_FILES at the following URL for an example: http://mxr.mozilla.org/mozilla/source/toolkit/components/places/tests/chrome/Makefile.in Basically the chrome Mochitests just copy the test files/data to a different location where they get run with a slightly different setup that dumps the files at some location in the chrome: URI space, but otherwise it's basically the exact same functionality.

That's a shame. Tests on templates could be very easy to write with reftests (for many of them, they are just comparisons between two DOM trees)... And I need chrome privileges because the template query processor for mozstorage works only in a chrome context...

Bug 378893 contains a xul template testing framework, so we should just add additional tests there.

(In reply to comment #80) > (for many of them, they are just comparisons between two DOM trees)... Perhaps Node.isEqualNode would suffice for comparing DOM trees for exactness? You'll get that with any of these harnesses.

for the reference, the docs are here: http://developer.mozilla.org/en/docs/XUL:Template_Guide:SQLite_Templates