Closed Bug 568091 Opened 9 years ago Closed 9 years ago

DOM Storage cannot store data for about: pages

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b1

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(1 file, 3 obsolete files)

In Bug 563738 i'm adding contents to about:home using localStorage. Chrome context writes and content context reads data, since this page is going to contain a mix of local and remote content we felt like makes sense to increase its security.

To "pipe" the data I need localStorage to be able to store data for about: pages, these have a scheme and a path, no host.

I will start proposing using the path for URIs with empty hosts, gather feedback, then finalize what I get with a test.
Attached patch patch v1.0 (obsolete) — Splinter Review
Attachment #447386 - Flags: feedback?
Attachment #447386 - Flags: feedback? → feedback?(honzab.moz)
Flags: in-testsuite?
I just noticed Bug 357323 that looks similar and has some insight, will read through it.
Notice about: pages are really simple and have only scheme and path that are more than enough to identify each one of them. So maybe I could restrict my solution to about: scheme.
Btw, reading.
Summary: DOM Storage cannot store data for URIs without a host → DOM Storage cannot store data for URIs without a host (about: pages)
Hm, reading through that bug looks like specs are vague about that, and the greates issue is with file:// acces...
actually would it be problem if only for about: pages I'd use ascii path as domain? they are served by our redirector, there should be no risk of another external page looking into their content (unless I miss something in the middle).
Comment on attachment 447386 [details] [diff] [review]
patch v1.0

Would be great to limit this to about: scheme only, the question of files has not been solved so far.  Then, would be good to append a suffix to the path that would protect the scope from potentially overlapping with other TLDs and would keep the system consistent.  Suffixes to select from could be ".about" or better ".about.mozilla" or something like that.
Attachment #447386 - Flags: feedback?(honzab.moz) → feedback-
Taking a look at the db I've noticed this is saved as "emoh.:moz-safe-about"
So if I read correctly your suggestion is to have "emoh.about.mozilla:moz-safe-about"? I thought the part after the colon was already there to protect against that kind of overlapping
Attached patch patch v2.0 (obsolete) — Splinter Review
first try at what you suggested, with a small unit test.
Attachment #447386 - Attachment is obsolete: true
Attachment #448188 - Flags: review?(honzab.moz)
(In reply to comment #5)
> Taking a look at the db I've noticed this is saved as "emoh.:moz-safe-about"
> So if I read correctly your suggestion is to have
> "emoh.about.mozilla:moz-safe-about"? I thought the part after the colon was
> already there to protect against that kind of overlapping

You are absolutely right.  I completely forgot that for localStorage we used the OriginScopeDBKey that also includes the schema.  Sorry for bad advise with the suffix.  Your patch then just needed to have an artificial limitation to work just for the 'about:' schema.

You can probably use aURI->SchemeIs("about") to check.

I don't think that in case of applying this just to the about: protocol needs to use IDN conversion.  If you see a reason to, feel free to oppose.  BTW - another case to add asciiPath on nsIURI...
Summary: DOM Storage cannot store data for URIs without a host (about: pages) → DOM Storage cannot store data for about: pages
Attached patch patch v3.0 (obsolete) — Splinter Review
Yeah, effectively I don't expect us to use any idn feature internally, thus using the path as it is should be fine. Also removed other redundant code based on your comment.
Attachment #448188 - Attachment is obsolete: true
Attachment #449225 - Flags: review?(honzab.moz)
Attachment #448188 - Flags: review?(honzab.moz)
Comment on attachment 449225 [details] [diff] [review]
patch v3.0

gah, this should also handle moz-safe-about scheme
Attachment #449225 - Attachment is obsolete: true
Attachment #449225 - Flags: review?(honzab.moz)
Attached patch patch v3.1Splinter Review
Here it is, but there is something strange, the test was passing also without the change, and it was not complaining about being unable to create a persistent database (i missed do_get_profile();) there must be code handling this case and falling back to memory hash
(In reply to comment #10)
> Here it is, but there is something strange, the test was passing also without
> the change, and it was not complaining about being unable to create a
> persistent database (i missed do_get_profile();) there must be code handling
> this case and falling back to memory hash

It is: http://mxr.mozilla.org/mozilla-central/source/dom/src/storage/nsDOMStorage.cpp#665

W/o "the change" the scope key is empty (fails to create) so, the storage will use just the in-mem db.  You can see NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file d:/mozilla/mozilla-central/dom/src/storage/nsDOMStorageDBWrapper.cpp, line 324 in the console, the place the scope key is being created.


...would be cool to preserve the profile after the test and run a second test (after shell restart) to check the value is still there, but xpc-shell test doesn't provide (AFAIK) such functionality.
Comment on attachment 449447 [details] [diff] [review]
patch v3.1

r=me
Attachment #449447 - Flags: review+
thanks
http://hg.mozilla.org/mozilla-central/rev/60a14de5241c
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a6
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.