Summary: | [EFL] Add setting API to enable/disable XSSAuditor | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sudarsana Nagineni (babu) <naginenis> | ||||||||||||
Component: | WebKit EFL | Assignee: | Sudarsana Nagineni (babu) <naginenis> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | gyuyoung.kim, lucas.de.marchi, rakuco, tonikitoo, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 83030, 84052 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Sudarsana Nagineni (babu)
2012-04-05 07:50:26 PDT
Created attachment 135831 [details]
Patch
setting API to enable/disable XSSAuditor
To summarize what we discussed on #webkit-efl: please implement the FrameLoaderClientEfl::didDetectXSS side by dispatching a signal and explaining this in the documentation, otherwise this feature is only half-implemented. Comment on attachment 135831 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135831&action=review > Source/WebKit/efl/ChangeLog:8 > + Nowadays, some patches have added public APIs for LTC. In my opinion, we should review whether this new APIs are needed surely. It looks this APIs is able to be landed. If you write functionality of this API in ChangeLog as well as in API description, it is a little better. (In reply to comment #3) > Nowadays, some patches have added public APIs for LTC. In my opinion, we should review whether this new APIs are needed surely. It looks this APIs is able to be landed. > If you write functionality of this API in ChangeLog as well as in API description, it is a little better. Thanks for your review. I will update the changelog entry. As we discussed on #webkit-efl, the main purpose of adding these methods to enable/disable WebKit's XSSAuditor to protect from reflective cross-site scripting attacks. (In reply to comment #2) > To summarize what we discussed on #webkit-efl: please implement the FrameLoaderClientEfl::didDetectXSS side by dispatching a signal and explaining this in the documentation, otherwise this feature is only half-implemented. Should I add this signal dispatching in the same patch or separate bug? Looked around the failing test due to this missing implementation and noticed that the test is also dependent on dumpFrameLoadCallbacks (bug 81891). So, fixing didDetectXSS doesn't fully fix the test. I think better to create a separate bug for signal dispatching, catching it in DRT and unskipping the test and make the bug dependent on these two bugs. test: http/tests/security/xssAuditor/script-tag-with-callbacks.html To me, it looks like a single feature being implemented, so I'd rather see it all in a single patch here (it also makes it easier to dig through the commit history later, and to roll back the patch in case it is needed). (In reply to comment #6) > To me, it looks like a single feature being implemented, so I'd rather see it all in a single patch here (it also makes it easier to dig through the commit history later, and to roll back the patch in case it is needed). Ok, I will update the patch. Created attachment 136444 [details]
Patch
Added all changes in a single patch here.
1. Added setting API to enable/disable XSSAuditor
2. Emits signal 'xss,detected' from FrameLoaderClientEfl::didDetectXSS
3. Catch signal in DRT and print the expected output 'didDetectXSS' for script-tag-with-callbacks.html test
4. Added missing implementation setXSSAuditorEnabled to EFL's LayoutTestController and unskiped tests in http/tests/security/xssAuditor
Comment on attachment 136444 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136444&action=review Looks almost there to me. My main grip here is related to what additional information is emitted with the frame and view signals: right now I don't know the URL that caused the issue or whether the whole page was blocked, and in the case of the view signal I only know that "something XSS-related happened", as I don't even know which frame it is related to. > Source/WebKit/efl/ewk/ewk_view.h:2386 > + * The XSSAuditor (cross-site scripting protection) feature provides protection > + * from reflected XSS attacks on vulnerable web sites. This feature is enabled > + * by default. It'd be good to expand this by explaining how XSS attacks are presented. There's no need for a detailed explanation, but from only looking at this paragraph I can't tell if requests are blocked, if the whole page is not loaded etc. Created attachment 136686 [details]
Patch
As discussed in #webkit-efl, providing additional information with the frame signal when XSSAutior notifies that XSS is encountered in the page.
This patch is dependent on 83030, so not requesting r/cq flags now.
Comment on attachment 136686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136686&action=review > Source/WebKit/efl/ewk/ewk_frame.cpp:1787 > +void ewk_frame_xss_detected(Evas_Object* ewkFrame, const Ewk_Frame_Xss_Notification *xssInfo) Style nit: inconsistent positioning of the asterisks. > Source/WebKit/efl/ewk/ewk_view.h:2388 > + * from reflected XSS attacks on vulnerable web sites. It notifies FrameLoaderClient > + * with didDetectXSS when XSS is encountered in the page and provides additional > + * information on whether the entire page was blocked or only injected scripts were > + * removed. This feature is enabled by default. Thanks for rephrasing this. Mentioning FLC and the method name is not necessary; when writing these documentation bits, try to think from the user's point of view: they are using webkit-efl in their program, but probably have no idea of how it works and have never heard of FrameLoaderClient and friends. > Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:108 > + evas_object_smart_callback_add(mainFrame, "xss,detected", onDidDetectXSS, 0); Don't you need to listen for this in all frames? (In reply to comment #11) > > Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:108 > > + evas_object_smart_callback_add(mainFrame, "xss,detected", onDidDetectXSS, 0); > > Don't you need to listen for this in all frames? Already listing for all frames in onFrameCreated. evas_object_smart_callback_add(frame, "xss,detected", onDidDetectXSS, 0); Created attachment 136713 [details]
Patch
Fixed review comments
Comment on attachment 136713 [details]
Patch
Informal r+ from my side.
Created attachment 137550 [details] Patch rebased after r114386 *** Bug 83163 has been marked as a duplicate of this bug. *** Comment on attachment 137550 [details] Patch Clearing flags on attachment: 137550 Committed r114419: <http://trac.webkit.org/changeset/114419> All reviewed patches have been landed. Closing bug. |