indexeddbshim / IndexedDBShim

A polyfill for IndexedDB using WebSql
Other
968 stars 191 forks source link

Add further escaping to SQL for the sake of Node #274

Closed brettz9 closed 7 years ago

brettz9 commented 7 years ago

To-do summary

~(Note that checked items are currently only added within https://github.com/brettz9/IndexedDBShim/tree/store-idx-clones which also introduced some regressions which I'm trying to resolve; however, the code for the items below should be sound.)~ Those checked off below are now available (and passing tests) in master

Moved the other (lower priority) to-dos to #278.

Background

Regarding database names, the IndexedDB spec indicates:

The name can be any string value, including the empty string, and stays constant for the lifetime of the database. Database names are always compared in a case-sensitive manner, as opaque sequences of 16-bit code units. Implementations must support all names.

If an implementation uses a storage mechanism which can’t handle arbitrary database names, the implementation must use an escaping mechanism or something similar to map the provided name to a name that it can handle.

The WebSQL spec too indicates that:

All strings including the empty string are valid database names. Database names must be compared in a case-sensitive manner.

Implementations can support this even in environments that only support a subset of all strings as database names by mapping database names (e.g. using a hashing algorithm) to the supported set of names.

However, the node-sqlite3 API, which is used by node-websql and which we are in turn using for Node support, indicates limitations on database names:

"Valid values are filenames, ":memory:" for an anonymous in-memory database and an empty string for an anonymous disk-based database"

Since node-websql is not aiming to address this, we might need do our own mapping to avoid conflicts.

Since our code in master already maps database file names by adding "D_" and prefixing uppercase letters with ^ (and escaping ^ and NUL), however, we are already:

  1. avoiding user databases being given special treatment and leading to a non-permanent database if the empty string or the string ":memory:" is used. (We might offer configuration though, to allow Node consumers to benefit from these temporary databases.)
  2. allowing expression of otherwise identical database names with different casing on case insensitive file systems

However, I think we also ought to ensure that we are escaping characters which are otherwise potentially invalid filenames across systems (bearing in mind though that file systems may limit filename length and escaping may worsen that, so we should at least throw a meaningful and relevant DOMException if the user supplies too long of a name (or save the file with an ID and map this ID to an internal database), including optionally in the browser as well if Node compatibility or mirroring later on the server may be desired).

Also, although neither the WebSQL spec nor SQLite speaks to this matter, node-sqlite3 has the following additional limitations which we do not wish to surface for our users: namely, that statements will only be executed up to the first NUL byte and SQL comments will lead to runtime errors.

While we are not crafting SQL with comments nor allowing for arbitrary SQL injections by our users, we are not currently escaping (and allowing back) NUL bytes in every single context, so I believe that ought to be reviewed, for security/stability reasons if nothing else.