Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Single quote in value results in SQLITE_ERROR #256

Closed
dshepsis opened this issue Mar 8, 2022 · 8 comments
Closed

Single quote in value results in SQLITE_ERROR #256

dshepsis opened this issue Mar 8, 2022 · 8 comments

Comments

@dshepsis
Copy link

dshepsis commented Mar 8, 2022

Problem

When using Keyv#set, an error is thrown if the value contains a string containing a single quote.

For example, if the string is simply a single quote, the error is:

node:internal/process/promises:246
          triggerUncaughtException(err, true /* fromPromise */);
          ^

[Error: SQLITE_ERROR: near "","": syntax error] {
  errno: 1,
  code: 'SQLITE_ERROR'
}

How to reproduce

Run this demonstration code in a directory with Keyv added as a dependency:

const Keyv = require('keyv');

const testDB = new Keyv('sqlite://test.sqlite', { namespace: 'test' });
testDB.on('error', err => console.log('Connection Error', err));

const value = "'";
testDB.set('key', value);

This will result in the above error.

It seems that #210 was supposed to address this issue, but it's either not part of the current release, or failed to fix the issue. Either way, a test case should probably be added for this.

Expected behavior

Values containing single quotes in strings should be serialized normally, like any other values.

Version

4.1.1

@jaytist
Copy link
Contributor

jaytist commented Mar 8, 2022

@dshepsis can you pas it like this"\'" or '\''

@dshepsis
Copy link
Author

dshepsis commented Mar 8, 2022

@dshepsis can you pas it like this"\'" or '\''

This doesn't work for a few reasons. Of course, just using a single backslash has no effect because the single quote is already a literal in this case. In other words, the following code produces exactly the same result as the code in the original report:

const Keyv = require('keyv');

const testDB = new Keyv('sqlite://test.sqlite', { namespace: 'test' });
testDB.on('error', err => console.log('Connection Error', err));

const value = "\'";
testDB.set('key', value);

If you instead use a double backslash "\\'", it produces the same error yet again (not really sure why. Maybe backslash isn't a valid escape character in this SQL implementation?). Using a double single quote "''" does actually work, with the saved result having just a single single quote, since a double single quote is a valid escape sequence for a single single quote in this case.

This isn't an acceptable workaround either, because I'm trying to store objects instead of strings. To escape everything would require iterating through every single key and value of the object, recursively, to double-up every single quote. Also, this means that anyone trying to pass in strings/objects which already contain double single quotes (for whatever reason) are having their data silently modified by keyv.

It's also worth mentioning that the key passed into the .set method also can't contain single quotes:

const Keyv = require('keyv');

const testDB = new Keyv('sqlite://test.sqlite', { namespace: 'test' });
testDB.on('error', err => console.log('Connection Error', err));

const value = 'foo';
testDB.set('\'', value);

Results in the error:

node:internal/process/promises:246
          triggerUncaughtException(err, true /* fromPromise */);
          ^

[Error: SQLITE_ERROR: unrecognized token: "{"] {
  errno: 1,
  code: 'SQLITE_ERROR'
}

This can be avoided using double single quotes in the key (both when using .set and .get), but this has the same issues as before.

@jaredwray jaredwray added the bug label Mar 8, 2022
dshepsis added a commit to dshepsis/Autoride that referenced this issue Mar 13, 2022
Implemented pkgRelPath to make sure that paths to local package
resources are consistent and reliable regardless of node's current
working directory. guildConfig is meant to mimic the get and set
API of Keyv, but write to a directory of JSON files instead of a single
sqlite database. This may cost performance, but should make things
easier to maintain, as well as avoid the bug with single quote
sanitization (jaredwray/keyv#256).
@dshepsis
Copy link
Author

Why was this closed?

@jaredwray
Copy link
Owner

Why was this closed?

Sorry as this was a mistake and we are looking into it how to test for this scenario and resolve it. :-)

@jaredwray jaredwray reopened this Mar 24, 2022
@jaredwray
Copy link
Owner

@dshepsis - wanted to update that we are working on moving back to sqlite3 which should help resolve this issue.

@jaredwray
Copy link
Owner

We will have a newer version of sqlite coming in the next week or so that should be based on sqlite3. FYI.

@jaredwray
Copy link
Owner

@dshepsis - this is being resolved in this pull request: #290

@jaredwray
Copy link
Owner

This should be resolved with the following version v3.5.0: https://www.npmjs.com/package/@keyv/sqlite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants