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

No protection against use of SQLite keywords #47

Closed
patrickhartling opened this issue Jan 10, 2014 · 13 comments
Closed

No protection against use of SQLite keywords #47

patrickhartling opened this issue Jan 10, 2014 · 13 comments
Assignees

Comments

@patrickhartling
Copy link
Contributor

I am evaluating ECD, and the managed object model that we want to use with it exposed a problem. We have an entity with the name Transaction, and that is a keyword in SQLite. The convention within Core Data's built-in support for using the SQLite store type is to prefix just about every name (tables, columns, possibly other aspects) with "Z". For my case, I can make a quick change to -tableNameForEntity: so that every table name starts with Z. Other people might run into similar problems but not have such an easy time addressing the naming conflicts.

This is a great library, and I hope to put it into production use soon. Thank you!

@gavin-black
Copy link
Member

Hi Patrick, thanks for taking the time to look into ECD. Good point about adding a prefix to every table that gets generated. This seems like a very simple change, and I went ahead and made it in the "name-append" git branch. Would like to test it a bit more though before I merge it and close the issue. If you end up trying it, I'd love to know if it actually fixes your issue.

@patrickhartling
Copy link
Contributor Author

Yes, the change on the name-append branch does fix my issue. I think that there may still need to be similar changes made for column names, but I do not currently have a test case to confirm that.

@gandg
Copy link
Member

gandg commented Jan 14, 2014

Hi Patrick! Thanks a bunch for your interest in iMAS and this security control. As part of our research, we are always interested to hear about how folks are incorporating iMAS into their production environments. When you have a chance do you mind emailing me directly and tell me about your use of iMAS. Ideally, which industry, type of application, iMAS security controls used etc. My email address is gganley@mitre.org With your feedback, we are hoping to garner enough support such that we can successfully propose and have accepted another year of research funding which means continued support by Gavin and myself. :-)

@gavin-black
Copy link
Member

Went ahead and merged in the branch, but will leave this open to remind myself to do the same for the columns.

@patrickhartling
Copy link
Contributor Author

I found a place where a column name that uses a reserved SQLite word causes a problem. This occurs during managed object model migration in the method -newValuesForObjectWithID:withContext:error:. We have an attribute named index, and the following error state is constructed when trying to migrate entities with that attribute:

Error Domain=NSSQLiteErrorDomain Code=1 "The operation couldn’t be completed. (NSSQLiteErrorDomain error 1.)" UserInfo=0x2c74f60 {EncryptedStoreErrorMessage=near "index": syntax error}

For all other cases I have tried with ECD, that column name does not conflict with the INDEX reserved word. This situation is a little different, but I am not (yet) sure why. The most obvious difference is that the attribute column name is not wrapped in single quotes as is done in most (all?) other places where a query string is constructed. I tried that, but doing so led to other failures in cases that had been working correctly. I also tried using the <table name>.<column name> syntax in the hope that that might help the parser avoid ambiguity, but that syntax did not fix the above error.

I should be able to find some time this week to write up a test case that reproduces this issue. The test case that I wrote for #51 captures the managed object model details that pertain to this case, but it lacks the migration component.

@patrickhartling
Copy link
Contributor Author

I updated the gist to include another test method (-testMigration) that reproduces the problems I encountered when migrating from one managed object model version to another. The migration works correctly if the store type is NSSQLiteStoreType. This can be seen by changing the value of the value of the useECD parameter passed to -newCoreDataStackUsingECD: to NO.

@gandg
Copy link
Member

gandg commented Jan 22, 2014

Hi Patrick! Thanks a bunch for your interest and contributions in iMAS and this security control. As part of our research, we are always interested to hear about how folks are incorporating iMAS into their production environments. When you have a chance do you mind emailing me directly and tell me about your use of iMAS. Ideally, which industry, type of application, iMAS security controls used etc. My email address is gganley@mitre.orgmailto:gganley@mitre.org With your feedback, we are hoping to garner enough support such that we can successfully propose and have accepted another year of research funding which means continued support by Gavin and myself. :-)

From: Patrick Hartling [mailto:notifications@github.com]
Sent: Friday, January 10, 2014 1:02 PM
To: project-imas/encrypted-core-data
Subject: [encrypted-core-data] No protection against use of SQLite keywords (#47)

I am evaluating ECD, and the managed object model that we want to use with it exposed a problem. We have an entity with the name Transaction, and that is a keywordhttps://www.sqlite.org/lang_keywords.html in SQLite. The convention within Core Data's built-in support for using the SQLite store type is to prefix just about every name (tables, columns, possibly other aspects) with "Z". For my case, I can make a quick change to -tableNameForEntity: so that every table name starts with Z. Other people might run into similar problems but not have such an easy time addressing the naming conflicts.

This is a great library, and I hope to put it into production use soon. Thank you!


Reply to this email directly or view it on GitHubhttps://github.com//issues/47.

@gavin-black
Copy link
Member

The branch uniq-cols mostly does this now, with a couple of exceptions that I don't think are effecting the migration issue. Once I get to the point where I can merge in that branch I'll probably close this issue and track the migration issues on the newer ticket.

@patrickhartling
Copy link
Contributor Author

With the changes on that branch, use of a transformable attribute extracts only 9 bytes from the column. I am working on a test case for this and will submit that as a pull request.

@patrickhartling
Copy link
Contributor Author

I submitted a pull request that fixes an issue when using the default value transformer. The two new tests also serve to reproduce a problem with transformable values on the uniq-cols branch. If the first commit is cherry picked to uniq-cols, both the default value transformer and the custom value transformer cases fail due to an incomprehensible archive exception being thrown.

Incidentally, while working on the test cases, I realized that I had forgotten to set up the inverse relationships between the Account and Transaction entities. That exposed another issue wherein -foreignKeyColumnForRelationship: will return the string "(null)_id". Failing to define the inverse relationship is not a fatal error in Core Data, but momc will warn about it pretty loudly when compiling a managed object model authored in Xcode. In other words, it's probably an uncommon thing to do, but it is still a more or less valid use of Core Data.

@ghost ghost assigned gavin-black Jan 28, 2014
@gavin-black
Copy link
Member

Did the (null)_id issue only occur in uniq-cols? My kneejerk reaction is that it's related to issue #20 (Many-to-many not creating an intermediate table). But if it's only on uniq-cols then that narrows it down.

@gavin-black
Copy link
Member

After a bit of fiddling the only error I'm seeing in EncryptedCoreDataTests.m that is unique to the column names change is:

error: testFetchObjectWithTemporaryID (EncryptedCoreDataTests) failed: Cannot retrieve referenceObject from an objectID that was not created by this store

There are other errors but I will put them up in their own issue. I went ahead and merged in the unique column changes, but let me know if that causes too many headaches for you and can pull them back out.

Will close this for now since the columns/tables are now at least unique, and address any bugs as their own issues.

@patrickhartling
Copy link
Contributor Author

The (null)_id issue crops up when the inverseRelationship property on an NSAttributeDescription object is nil. My guess is that this isn't necessarily a common case, but it can happen.

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

No branches or pull requests

3 participants