Summary: | Store non-standard HTTP headers in history | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Marshall Greenblatt <magreenblatt> | ||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED INVALID | ||||||||||||
Severity: | Enhancement | CC: | annevk, ap, magreenblatt | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Marshall Greenblatt
2009-07-06 09:43:21 PDT
Created attachment 40005 [details] Patch #1 to resolve bug 26994. 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. (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. Created attachment 40013 [details] Patch #2 to resolve bug 26994. This update fixes startsWith issue and ignores case for "X-". 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. (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? 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. 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?) (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? Created attachment 40068 [details] Patch #4 to resolve bug 26994 Use a fixed list of excluded headers in HistoryItem. Also exclude the Origin header. 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. Closing as there was no follow-up and this also doesn't seem like something we'd want. |