Closed Bug 1150505 Opened 9 years ago Closed 9 years ago

Cache API should sanitize Request objects before working with them


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

Not set





(Reporter: bkelly, Unassigned, Mentored)



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

The spec has changed:

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:

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:

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:

Maybe SanitizeAction or something.

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

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

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.
Closed: 9 years ago
Resolution: --- → DUPLICATE
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.