759582 - Content from the HTTP cache may be returned for custom conditional requests (e.g. XMLHttpRequest requests with If-Non-Match, If-Range, If-Modified-Since, etc.) without us first verifying that all conditions are satisfied

Status: NEW

(Core :: Networking: HTTP, defect, P5)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

We do not have any logic that verifies that the response we have cached in the HTTP cache meets all the conditions of a custom conditional request (mCustomConditionalRequest==true in nsHttpChannel). We should not return responses from the HTTP cache unless/until we have verified that all criteria have been met.

In nsHttpChannel::ProcessNotModified, we have this code:

    if (mCustomConditionalRequest) {
        LOG(("Bypassing ProcessNotModified due to custom conditional headers")); 
        return NS_ERROR_FAILURE;
    }

and in nsHttpChanell::ProcessResponse, we have this code:

        rv = ProcessNotModified();
        if (NS_FAILED(rv)) {
            LOG(("ProcessNotModified failed [rv=%x]\n", rv));
            rv = ProcessNormal();
        }
        else {
            successfulReval = true;
        }
        break;

So, we always end up processing 304 responses with custom validators as "normal" responses.

ProcessNormal/ContinueProcessNormal does not do any checking to see whether it is processing a custom conditional request. That means we will return a previously-cached response when we receive a 304 response. This is wrong in two respects:

1. A web developer that sets conditional request headers on an XHR expects to get back the 304 response directly, but he never will.

2. The cached response doesn't necessarily meet all the criteria specified in the XHR request, including conditional request directives, Accept-*, Cache-Control, etc.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I think I misunderstood what ProcessNormal actually does while I was debugging another problem. I do not think ProcessNormal is part of the problem.

I think the problem is actually that we wrongly consider cached content to be fresh (we set mCachedContentIsValid) for custom conditional requests.

Comment 2

[Honza Bambas (:mayhemer)]

(In reply to Brian Smith (:bsmith) from comment #1)
> I think I misunderstood what ProcessNormal actually does while I was
> debugging another problem. I do not think ProcessNormal is part of the
> problem.
> 
> I think the problem is actually that we wrongly consider cached content to
> be fresh (we set mCachedContentIsValid) for custom conditional requests.

Should this bug then be closed as INVALID?

Comment 3

[Honza Bambas (:mayhemer)]

(In reply to Brian Smith (:bsmith) from comment #1)
> I think the problem is actually that we wrongly consider cached content to
> be fresh (we set mCachedContentIsValid) for custom conditional requests.

Where?

Comment 4

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 5

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 6

[Jens Stutte [:jstutte]]

Bulk-downgrade of unassigned, untouched DOM/Storage bug's priority.

If you have reason to believe, this is wrong, please write a comment and ni :jstutte.