Bug 26994 – Store non-standard HTTP headers in history

• Component: WebCore Misc.
• Status: RESOLVED INVALID

[Marshall Greenblatt]

Non-standard HTTP headers, usually prefixed with 'X-', are used by a number of server-based applications. It is currently possible to create an NSMutableURLRequest that contains non-standard HTTP headers and load that request via a call to WebFrame::loadRequest(). If the request is then re-loaded via a call to WebFrame::reload() the non-standard HTTP headers are remembered and re-submitted. However, if the request is loaded by navigating the history then the non-standard HTTP headers will have been lost and will not be re-submitted. To address what I believe is an API omission I propose that WebCore::HistoryItem be modified to store non-standard HTTP headers so that they can be re-submitted when the user navigates back to the request via the history.

[Marshall Greenblatt]

Created attachment 40005 [details]
Patch #1 to resolve bug 26994.

[Darin Adler]

Comment on attachment 40005 [details]
Patch #1 to resolve bug 26994.

> +        if (it->first.find("X-") == 0)

This is not the correct way to check for a prefix. It wastefully searches the entire string for "X-". There's a startsWith function that can be used for this purpose. But also, should this be a case-sensitive check or case-insensitive?

Why keep the extra headers in a map instead of just in a vector of pairs? Do we need efficient key lookup for some reason.

I don't understand the concept of this patch; why are headers the start with "x-" different from others? I suspect this is the wrong approach.

[Marshall Greenblatt]

(In reply to comment #2)
> (From update of attachment 40005 [details])
> > +        if (it->first.find("X-") == 0)
> 
> This is not the correct way to check for a prefix. It wastefully searches the
> entire string for "X-". There's a startsWith function that can be used for this
> purpose. But also, should this be a case-sensitive check or case-insensitive?

I'll make the changes that you suggest. Case-insensitive matching is not a requirement but would be acceptable.

> 
> Why keep the extra headers in a map instead of just in a vector of pairs? Do we
> need efficient key lookup for some reason.

Efficient key lookup is not a priority for this usage case. I chose this particular implementation to be consistent with the ResourceRequest implementation.  Perhaps a better overall approach would be having HistoryItem store a ResourceRequest object internally instead of duplicating many of the same members.

> 
> I don't understand the concept of this patch; why are headers the start with
> "x-" different from others? I suspect this is the wrong approach.

Some web applications use custom headers to extend an HTTP request. The convention is to prefix the header name with X- to indicate that it is non-standard.  This is a reasonably well-documented approach -- see, for instance, section 2.1 of the following (expired) draft specification:

http://www.mnot.net/drafts/draft-nottingham-hdrreg-http/draft-nottingham-hdrreg-http-00.txt

Searching only for headers that begin with X- implicitly excludes all standard headers.  An alternative approach would be explicitly excluding all standard headers that are either stored separately or should not be stored as part of the history such as Host, Cookie, Content-Length, Accept, User-Agent, Referrer, Content-Type, etc.  Either approach would be valid but I think the current implicit approach is more manageable from both an implementation and documentation standpoint.

[Marshall Greenblatt]

Created attachment 40013 [details]
Patch #2 to resolve bug 26994.

This update fixes startsWith issue and ignores case for "X-".

[Darin Adler]

Comment on attachment 40013 [details]
Patch #2 to resolve bug 26994.

This patch is going to need a regression test -- should be easy to make one of our http tests to test this.

Also, it would be good to have an example of a website where the change makes a WebKit-based browser work better with the site.

it->first.startsWith("x-", false) is what you want for case sensitivity.

I don't think that using "x-" prefixed headers only is the right way to go about this. Something that instead involved some sort of whitelist would be better.

[Marshall Greenblatt]

(In reply to comment #5)
> (From update of attachment 40013 [details])
> This patch is going to need a regression test -- should be easy to make one of
> our http tests to test this.

I agree, I'll work on that.

> 
> Also, it would be good to have an example of a website where the change makes a
> WebKit-based browser work better with the site.

Normal web browsing won't cause non-standard HTTP request headers to be sent so this capability would be used mainly by applications embedding WebKit or by extensions loaded into a WebKit-based browser.  A good use-case for this capability would be an extension implementing a WebDAV report viewer. WebDAV uses a number of non-standard HTTP header fields:

http://www.webdav.org/specs/rfc2518.html#http.headers.for.distributed.authoring

The extension would modify the display of the WebDAV report, perhaps adding a search form and/or hyperlinks.  When the user interacts with these elements the extension sets the extra HTTP headers and submits the request.  WebKit can then transparently handle history navigation by storing those extra HTTP header values.

> 
> it->first.startsWith("x-", false) is what you want for case sensitivity.
> 
> I don't think that using "x-" prefixed headers only is the right way to go
> about this. Something that instead involved some sort of whitelist would be
> better.

The WebDAV header fields don't begin with "X-" so that's a good argument for not restricting the use to "X-" headers only.  How would you want to see the whitelist concept implemented?

[Marshall Greenblatt]

Created attachment 40031 [details]
Patch #3 to resolve bug 26994

This patch addresses the HTTP header whitelist issue by offloading the test to the client.  It adds a new FrameLoaderClient::canStoreHTTPHeaderFieldInHistory() method.

[Adam Barth]

Comment on attachment 40031 [details]
Patch #3 to resolve bug 26994

Why would different clients want to store different headers in the history item?  It seems like we should store all the headers except some specific list of headers that are problematic.  (Which are the problematic headers?)

[Marshall Greenblatt]

(In reply to comment #8)
> (From update of attachment 40031 [details])
> Why would different clients want to store different headers in the history
> item?  It seems like we should store all the headers except some specific list
> of headers that are problematic.  (Which are the problematic headers?)

Headers in any of the following categories should not be stored in the history.

1. Transfer and/or content negotiation.

Accept-Ranges, Content-Encoding, Content-Length, Content-Location, Content-MD5, Content-Range, Content-Type, If-Range, Range, TE, Trailer, Transfer-Encoding, Via

2. Managed state information.

Authorization, Cookie, Host, Proxy-Authorization, Referer.

3. Browser capabilities.

Accept, Accept-Charset, Accept-Encoding, Accept-Language, Expect, Upgrade, User-Agent.


Am I missing any?

[Marshall Greenblatt]

Created attachment 40068 [details]
Patch #4 to resolve bug 26994

Use a fixed list of excluded headers in HistoryItem.  Also exclude the Origin header.

[Adam Barth]

Comment on attachment 40068 [details]
Patch #4 to resolve bug 26994

Darin asked for a regression test, but you did not provide one.  Also, this patch is missing a ChangeLog.

In general, this header list seems to overlap significantly with the XMLHttpRequest black list.  Are the lists actually the same?  Is one a subset of the other?  I'd like to see an explanation of any differences.

[Anne van Kesteren]

Closing as there was no follow-up and this also doesn't seem like something we'd want.