CacheKey string is not sanitized

[gcacace]

There is no sanitization of cacheKey string when is used as filename: using special chars, the filename may be invalid, so no cache is written on disk.

[rciovati]

We could implement some kind of hash function but this would lead to a read/write overhead.
I'm wondering if it would be easier checking on your side to use a proper string as cache key.

[stephanenicolas]

I agree with gcacace, the cachekey, when converted to String should be
sanitized to get sure it can be used safely to create a cacheFile.
If you know of any light library that can help to achieve this quickly,
that could help implementing this feature.

Thanks for reporting that bug.

S.

2013/5/15 Riccardo Ciovati [email protected]

We could implement some kind of hash function but this would lead to a
read/write overhead.
I'm wondering if it would be easier checking on your side to use a proper
string as cache key.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/97#issuecomment-17933769
.

Stéphane NICOLAS,
OCTO Technology
Développeur & Consultant Android / Java
..........................................................
50, Avenue des Champs-Elysées
75008 Paris
+33 (0)6.26.32.34.09
www.octo.com - blog.octo.com
www.usievents.com
...........................................................

[StingerAJ]

This is definitively needed, as for example, in our app, the cacheKey would be part of an URL, and right now it seems that searchrequest-URL-parts seem to be ivnalid as filenames (and I don't know why, yet), so they don't ever get cached (which is not a big problem, since they are supposed to be cached max for 15m).

[mdumrauf]

How about using UrlQuerySanitizer? It is a utility class provided by android sdk.

[gcacace]

@mdumrauf I think that this way of sanitization can produce collisions (for example, what happens when I'm using "cache/key" and "cache_key"? Maybe a simple chars replacement it's not enough to guarantee an unique association between cacheKey and the real filename on disk. A hybrid way can be to use a string sanitizer with a hash (SHA1 or MD5) appended in the filename (example: sanitized_cache_key_b7dba5a1bc3605a87b59ac8147512c97)

[stephanenicolas]

+1 for gcacace.

I just read this :
stackoverflow.com/questions/1184176/how-can-i-safely-encode-a-string-in-java-to-use-as-a-filename

And I would Aldo favor a SHA-1 encoded cache key. Thought, this should be
optional as it makes debugging much harder.

This should be released pretty soon.

If any one is interested to contribute this feature plus tests, that would
be awesome.

We are still recruiting...
;)

Thx all for this collective problem solving.
Le 16 mai 2013 15:08, "gcacace" [email protected] a écrit :

@mdumrauf https://github.com/mdumrauf I think that this way of
sanitization can produce collisions (for example, what happens when I'm
using "cache/key" and "cache_key"? Maybe a simple chars replacement it's
not enough to guarantee an unique association between cacheKey and the real
filename on disk. A hybrid way can be to use a string sanitizer with a hash
(SHA1 or MD5) appended in the filename (example:
sanitized_cache_key_b7dba5a1bc3605a87b59ac8147512c97)

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/97#issuecomment-17999709
.

[mdumrauf]

Totally agree with you @gcacace. 👍

[dpsm]

Hi All,

I would like to contribute to this feature. What do you guys think about something like:

FILE_NAME = (SANITIZE(KEY) + HASH(KEY))

This would avoid conflicts since the HASH(KEY) portion of the file name would generate different suffixes even though the sanitized prefix is the same!?

[dpsm]

@stephanenicolas what do you think about my proposal above?

[stephanenicolas]

Quite a good idea. :)

I understand that it would allow a more readable cache_key that just using a hash. If they become readable this way, then it could not be optional.

Do you want to do it by your self (fork, code, test, pull request ) ?
I think it should be implemented around :

//com.octo.android.robospice.persistence.file.InFileObjectPersister<T> : line 110
public File getCacheFile(Object cacheKey) {
    return new File(getCacheFolder(), getCachePrefix() + cacheKey.toString());
}

We should only make the change for file based ObjectPersisters and not interact with DataBase or InMemory persisters.

[dpsm]

Will do. You'll receive a pull request soon :)

[stephanenicolas]

Eager to :)
With tests please !! That's quite a core feature.

[dpsm]

@stephanenicolas I have found a small issue and wanted some insight from you on a possible solution:

If we follow the approach discussed above, CacheManager.getAllCacheKeys() will return sanitized keys which might not match the expectations (knows keys) of the caller. All other APIs seem to work fine since we sanitize the keys on the way into the InFilePersisters but since after sanitizing it does no longer remember the dirty cache key it can not return the user expected set of keys.

Thoughts?

[stephanenicolas]

The only solutions are :

• to save the original (dirty) keys on disk
• or to have a bijective way to sanitize keys
• or to lock that feature (throw an exception) when the key sanitation has been activated.

I would prefer a bijection, but is that possible ?

[dpsm]

@stephanenicolas another thought would be use BASE 64 FILE name safe (http://developer.android.com/reference/android/util/Base64.html#URL_SAFE)

This would get us a reversible key name:

FILE_NAME = BASE64(KEY);

What do you think?

[stephanenicolas]

That's fine for me. This should be optionally set at the SpiceService level. The implementation should check if SDK is > 8 as Base64 is not available under this SDK. An other option is to include our own base 64 impl, such implementations are available on the net. RS is still compatible with SDK 1.6, but it tends to be old now.

[dpsm]

@stephanenicolas http://developer.android.com/reference/android/util/Base64.html is available on API level 8 as well. Is API 8 the minimun we need to support?

If so we should be fine with the android default BASE64 API and IMHO compromising the usage of the android APIs in ordert to support API < 8 does not worth it since it is such a small portion of the user base?

[stephanenicolas]

Wiki has been updated to include that feature.

Testers are welcome, issue is closed.

[gcacace]

The DefaultKeySanitizer.java implements a base64 encoding that, in some conditions, could fail because the filename is too long:

04-01 10:05:29.504    2182-2253/it.subito.debugVinz E//ManagedPersister.java:54﹕ 10:05:29.506 Thread-178 An error occured on saving request .image_/storage/emulated/0/Android/data/it.subito.debugVinz/cache/c00d21a9_9051_4072_8c8c_209fe6be8590.jpg.user_logged_false.user_pro_false data asynchronously
    java.io.FileNotFoundException: /data/data/it.subito.debugVinz/cache/robospice-cache/ManagedObjectPersisterFactory_ManagedPersister_AdInsertAddImageResponse_LmltYWdlXy9zdG9yYWdlL2VtdWxhdGVkLzAvQW5kcm9pZC9kYXRhL2l0LnN1Yml0by5kZWJ1Z1ZpbnovY2FjaGUvYzAwZDIxYTlfOTA1MV80MDcyXzhjOGNfMjA5ZmU2YmU4NTkwLmpwZy51c2VyX2xvZ2dlZF9mYWxzZS51c2VyX3Byb19mYWxzZQ: open failed: ENAMETOOLONG (File name too long)
            at libcore.io.IoBridge.open(IoBridge.java:409)
            at java.io.FileOutputStream.<init>(FileOutputStream.java:88)
            at java.io.FileOutputStream.<init>(FileOutputStream.java:73)
            at it.subito.services.ManagedPersister$1.run(ManagedPersister.java:52)
     Caused by: libcore.io.ErrnoException: open failed: ENAMETOOLONG (File name too long)
            at libcore.io.Posix.open(Native Method)
            at libcore.io.BlockGuardOs.open(BlockGuardOs.java:110)
            at libcore.io.IoBridge.open(IoBridge.java:393)
            at java.io.FileOutputStream.<init>(FileOutputStream.java:88)
            at java.io.FileOutputStream.<init>(FileOutputStream.java:73)
            at it.subito.services.ManagedPersister$1.run(ManagedPersister.java:52)

[stephanenicolas]

Do you have any working workaround ?

S.

2014-04-01 13:00 GMT+02:00 gcacace [email protected]:

The DefaultKeySanitizer.java implements a base64 encoding that, in some
conditions, could fail because the filename is too long:

04-01 10:05:29.504 2182-2253/it.subito.debugVinz E//ManagedPersister.java:54﹕ 10:05:29.506 Thread-178 An error occured on saving request .image_/storage/emulated/0/Android/data/it.subito.debugVinz/cache/c00d21a9_9051_4072_8c8c_209fe6be8590.jpg.user_logged_false.user_pro_false data asynchronously
java.io.FileNotFoundException: /data/data/it.subito.debugVinz/cache/robospice-cache/ManagedObjectPersisterFactory_ManagedPersister_AdInsertAddImageResponse_LmltYWdlXy9zdG9yYWdlL2VtdWxhdGVkLzAvQW5kcm9pZC9kYXRhL2l0LnN1Yml0by5kZWJ1Z1ZpbnovY2FjaGUvYzAwZDIxYTlfOTA1MV80MDcyXzhjOGNfMjA5ZmU2YmU4NTkwLmpwZy51c2VyX2xvZ2dlZF9mYWxzZS51c2VyX3Byb19mYWxzZQ: open failed: ENAMETOOLONG (File name too long)
at libcore.io.IoBridge.open(IoBridge.java:409)
at java.io.FileOutputStream.(FileOutputStream.java:88)
at java.io.FileOutputStream.(FileOutputStream.java:73)
at it.subito.services.ManagedPersister$1.run(ManagedPersister.java:52)
Caused by: libcore.io.ErrnoException: open failed: ENAMETOOLONG (File name too long)
at libcore.io.Posix.open(Native Method)
at libcore.io.BlockGuardOs.open(BlockGuardOs.java:110)
at libcore.io.IoBridge.open(IoBridge.java:393)
at java.io.FileOutputStream.(FileOutputStream.java:88)
at java.io.FileOutputStream.(FileOutputStream.java:73)
at it.subito.services.ManagedPersister$1.run(ManagedPersister.java:52)

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/97#issuecomment-39193040
.