Bug 177829 - XMLHttpRequest.setRequestHeader() should allow Content-Transfer-Encoding header; remove duplicate logic to check for a forbidden XHR header field
Summary: XMLHttpRequest.setRequestHeader() should allow Content-Transfer-Encoding head...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar, WebExposed
Depends on:
Blocks:
 
Reported: 2017-10-03 12:08 PDT by Daniel Bates
Modified: 2017-10-04 09:58 PDT (History)
3 users (show)

See Also:


Attachments
Patch (15.47 KB, patch)
2017-10-03 12:20 PDT, Daniel Bates
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2017-10-03 12:08:34 PDT
Remove duplicate logic to check for a forbidden XHR header field.
Comment 1 Daniel Bates 2017-10-03 12:20:02 PDT
Created attachment 322561 [details]
Patch
Comment 2 Alexey Proskuryakov 2017-10-03 12:53:43 PDT
Comment on attachment 322561 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=322561&action=review

> LayoutTests/fast/xmlhttprequest/set-dangerous-headers-in-dashboard.html:26
> +    // CONTENT-TRANSFER-ENCODING is no longer forbidden since <https://www.w3.org/TR/2012/WD-XMLHttpRequest-20121206/>.
>      req.setRequestHeader("CONTENT-TRANSFER-ENCODING", "foobar");

It is strange to keep this header filed name tested here - we don't test other safe names in these tests. It would be cleaner to remove it from these tests, and to add a new one for this fix.

This is a suggested change, not a condition for r+.
Comment 3 Daniel Bates 2017-10-03 13:08:43 PDT
(In reply to Alexey Proskuryakov from comment #2)
> Comment on attachment 322561 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322561&action=review
> 
> > LayoutTests/fast/xmlhttprequest/set-dangerous-headers-in-dashboard.html:26
> > +    // CONTENT-TRANSFER-ENCODING is no longer forbidden since <https://www.w3.org/TR/2012/WD-XMLHttpRequest-20121206/>.
> >      req.setRequestHeader("CONTENT-TRANSFER-ENCODING", "foobar");
> 
> It is strange to keep this header filed name tested here - we don't test
> other safe names in these tests. 

Notice that all these layout tests test the now-considered safe header AUTHORIZATION, e.g. <https://trac.webkit.org/browser/trunk/LayoutTests/http/tests/xmlhttprequest/set-dangerous-headers.html?rev=163915#L16>.

I thought to keep the test for Content-Type-Encoding for similar historical preservation.
Comment 4 Daniel Bates 2017-10-03 13:53:18 PDT
(In reply to Daniel Bates from comment #3)
> (In reply to Alexey Proskuryakov from comment #2)
> > Comment on attachment 322561 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=322561&action=review
> > 
> > > LayoutTests/fast/xmlhttprequest/set-dangerous-headers-in-dashboard.html:26
> > > +    // CONTENT-TRANSFER-ENCODING is no longer forbidden since <https://www.w3.org/TR/2012/WD-XMLHttpRequest-20121206/>.
> > >      req.setRequestHeader("CONTENT-TRANSFER-ENCODING", "foobar");
> > 
> > It is strange to keep this header filed name tested here - we don't test
> > other safe names in these tests. 
> 
> Notice that all these layout tests test the now-considered safe header
> AUTHORIZATION, e.g.
> <https://trac.webkit.org/browser/trunk/LayoutTests/http/tests/xmlhttprequest/
> set-dangerous-headers.html?rev=163915#L16>.
> 
> I thought to keep the test for Content-Type-Encoding for similar historical
> preservation.

Will remove test for Content-Transfer-Encoding before landing.
Comment 5 Daniel Bates 2017-10-03 13:56:09 PDT
Committed r222807: <http://trac.webkit.org/changeset/222807>
Comment 6 Radar WebKit Bug Importer 2017-10-03 13:56:45 PDT
<rdar://problem/34798441>
Comment 7 Daniel Bates 2017-10-03 16:02:18 PDT
Forgot to mention this change also removed User-Agent from the list of forbidden headers. This header is no longer forbidden in the XHR standard, <https://xhr.spec.whatwg.org>, last updated 09/08/2017.
Comment 8 Daniel Bates 2017-10-03 16:17:17 PDT
Updated tests and expected results now that we allow setting the Content-Transfer-Encoding and User-Agent headers in <http://trac.webkit.org/changeset/222817>.
Comment 9 Ryan Haddad 2017-10-04 09:58:25 PDT
One more test re-baseline in https://trac.webkit.org/changeset/222852/webkit to account for a change made to the tests themselves in http://trac.webkit.org/changeset/222817.