Closed Bug 1150505 Opened 8 years ago Closed 8 years ago

Cache API should sanitize Request objects before working with them

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1120501

People

(Reporter: bkelly, Unassigned, Mentored)

References

Details

(Whiteboard: [good first bug][lang=c++])

The spec has changed:

  https://github.com/slightlyoff/ServiceWorker/issues/664

When passed a Request object the Cache should now sanitize it by passing it to the Request constructor.

This is probably pretty straightforward to do in dom/cache/TypeUtils.cpp's ToInternalRequest() in these places:

  https://dxr.mozilla.org/mozilla-central/source/dom/cache/TypeUtils.cpp?from=TypeUtils.cpp&case=true#158
  https://dxr.mozilla.org/mozilla-central/source/dom/cache/TypeUtils.cpp?from=TypeUtils.cpp&case=true#177

Before returning the Request objects InternalRequest, first construct a new Request object wrapper.

This would probably need a slight refactor in dom/fetch/Request as well.  The TypeUtils class only has an nsIGlobalObject, but the constructor needs a GlobalObject.  This can be fixed by factoring out most of the constructor into a Request::Create() that takes the nsIGlobalObject.
Actually, the sanitize should only be done for Cache .add() and .addAll().  So TypeUtils is probably the wrong place.  Instead we should just run the constructor in Cache::Add() and Cache::AddAll().
Hey Ben,
I would like to work on this bug. Also, can you please explain what needs to be done here. I could not understand much from the above two comments.
Flags: needinfo?(bkelly)
Sorry, I thought I replied to this yesterday, but apparently I forgot to save the comment to the bug.

So, we need to run the Request::Constructor() logic in Cache::Add() and Cache::AddAll() if a Request is passed in.  This should happen before or as part of the ToInternalRequest() calls here:

  https://dxr.mozilla.org/mozilla-central/source/dom/cache/Cache.cpp?from=Cache.cpp#192
  https://dxr.mozilla.org/mozilla-central/source/dom/cache/Cache.cpp?from=Cache.cpp#234

In comment 1 was trying to say you could just do it before these calls, but after looking closer I don't think thats a good idea.  You would have to duplicate the type conversion code that ToInternalRequest() already does.

So what I now recommend is adding a new enumeration to control the behavior of ToInternalRequest().  Something like BodyAction:

  https://dxr.mozilla.org/mozilla-central/source/dom/cache/TypeUtils.h#41

Maybe SanitizeAction or something.

You then need to add the SanitizeAction enum to ToInternalRequest() calls so you can check it here:

  https://dxr.mozilla.org/mozilla-central/source/dom/cache/TypeUtils.cpp#155
  https://dxr.mozilla.org/mozilla-central/source/dom/cache/TypeUtils.cpp#174

If the SanitizeAction is set to sanitize, then run the logic in Request::Constructor to create a new Request object:

  https://dxr.mozilla.org/mozilla-central/source/dom/fetch/Request.h#115

Note, the Constructor takes a GlobalObject, but all you have available is the nsIGlobalObject returned from TypeUtils::GetGlobalObject().  If you look at the Request::Constructor() method, though, you can see it just immediately converts to an nsIGlobalObject before doing any work.  So you can easily split the Request::Constructor logic out into a separate method call Request::Create() that takes nsIGlobalObject.  The Constructor() can then call Create().

Since the Request::Constructor will effectively do the same logic as CheckAndSetBodyUsed(), I think you can remove that function completely.  That means you can also remove the BodyAction arg sent to TonInternalRequest().

Does that make sense?  Please don't hesitate to ask for clarification.  Thanks for taking this on!
Flags: needinfo?(bkelly)
Vaibhav, are you working on it? Or is it okay for me to take and work on it?
Dhananjay, yes I am working on it.
Actually, since I'm now actively rewriting Add/AddAll for bug 1120501 any patches you have here will very shortly be bit-rotted.  I'm very sorry!

I think its best just to dupe this to bug 1120501 for now.  I will just include the Request constructor call there.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1120501
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.