463327 - Enable stateful SJS handlers somehow other than through universal insta-XSS

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Jeff Walden [:Waldo]]

PUT/DELETE is natural, but without some way to constrain access to it it's a gaping security hole, and I'd rather not see such a thing baked into the server (or even as a part which must be deliberately enabled).  Also, the current method prevents SJS from using PUT/DELETE, which isn't good.

Basically, we have two choices for how to pass state to handlers: add to the properties of the global object in the SJS sandbox, or pass it to the handleRequest method.  The second is an API change, so I think the first would be better.  What object(s) should we pass this way?  Components would grant ultimate power but isn't a very structured way to access it; it could still be a good idea anyway even if not for state-storing purposes.  The server itself is another idea, but it'd be an awkward way to store state -- other than through swapping request handlers.

Any other ideas?

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

One possible API would be something like:

nsIPileOData getDataFor(in string identifier);

This would allow you to use for example the use the test url as identifier to avoid having tests stomp on each others data.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Adds getState and setState functions to SJS global scope and to server objects

Actually, I forgot that the sandbox is being implicitly given the system
principal, so there actually is a usable Components object.  So really we do
provide SJS writers with plenty of rope, and what's needed is some clear way to
expose information to allow different pages to not trample on each other (and
ideally not to have problems if a page forgets to clean up after itself), in a
simple and usable manner.

How's this look for an API?  It's trivial enough to use, and the same state is exposed to handlers and SJS, assuming the handler can access a reference to the server.  As I see it, JSON addresses the need for storing any more structured data, and it's easier as well since the sandbox mechanism prohibits non-primitive values from being returned from imported functions.

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 346820 [details] [diff] [review]
Adds getState and setState functions to SJS global scope and to server objects

I tried to rewrite one of the offline tests using this new feature. I update state of an offline cache manifest using GET XMLHttpRequest with query "?state=somestate". All is more elegant now but I have to internally clear the associated offline cache from the XMLHttpRequest's channel under UniversalXPConnect privilege to allow it.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Just occurred to me that it might be worth clearing all state caches between mochitests to avoid accidentally having tests depend on each other. We can do this by setting up an SJS file that clears all stored state strings, and calling that SJS from mochitest between all tests.

Comment 5

[Honza Bambas (:mayhemer)]

Agree. But, I plan to have an automated cleanup in OfflineTest.teardown() anyway because different tests might refer the same SJS. SJS state update is integrated to OfflineTest helper class. I expect other tests will need to do particularly manually or somehow semi-automatically (as OfflineTest) a cleanup on finish, anyway.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: For checkin

Currently, server-side scripts used in Mochitests have no easy way to remember anything across separate loads, except by unsupported hacks or use of dubious PUT/GET handling built directly into the server (which also prevents any server-side scripts from handling requests which use those HTTP methods -- this bit some of sicking's tests recently).  This patch provides a supported way to save state across loads of a page, and with a few changes to existing tests we'll be able to get rid of the suboptimal PUT/GET functionality.

Comment 7

[Jeff Walden [:Waldo]]

I checked in the patch in mozilla-central, still waiting for a+ to get it on branch so we can make writing stateful handlers simpler...

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 350530 [details] [diff] [review]
For checkin

Actually, this is NPOTB, so it doesn't need approval; will land momentarily...

Comment 9

[Jeff Walden [:Waldo]]

Moving httpd.js bugs to the new Testing :: httpd.js component; filter out this bugmail by searching for "ICanHasHttpd.jsComponent".

Comment 10

[Jeff Walden [:Waldo]]

...and changing the QA contact as well.  Filter on the string "BugzillaQAContactHandlingIsStupid".

Comment 11

[Jeff Walden [:Waldo]]

Landed in the 191 repository, marking FIXED.

Wrt clearing all state between caches, I think 1) that'd be rather slow, and 2) there might be cases where we'd want state to persist across tests.  #2 suggests any such test should be expanded to encompass those smaller tests, but I think the speed issue is probably a bigger issue.  If we have consistent problems with state leaking, we can revisit the issue then.

Comment 12

[Jeff Walden [:Waldo]]

I somehow forgot to document this when I wrote it initially; fixed that now:

https://developer.mozilla.org/En/Httpd.js/HTTP_server_for_unit_tests