Bug 83281 – [EFL] Add setting API to enable/disable XSSAuditor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Sudarsana Nagineni (babu)]

Add API to enable/disable XSSAuditor.

[Sudarsana Nagineni (babu)]

Created attachment 135831 [details]
Patch

setting API to enable/disable XSSAuditor

[Raphael Kubo da Costa (:rakuco)]

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.

[Gyuyoung Kim]

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.

[Sudarsana Nagineni (babu)]

(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.

[Sudarsana Nagineni (babu)]

(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

[Raphael Kubo da Costa (:rakuco)]

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).

[Sudarsana Nagineni (babu)]

(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.

[Sudarsana Nagineni (babu)]

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

[Raphael Kubo da Costa (:rakuco)]

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.

[Sudarsana Nagineni (babu)]

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.

[Raphael Kubo da Costa (:rakuco)]

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?

[Sudarsana Nagineni (babu)]

(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);

[Sudarsana Nagineni (babu)]

Created attachment 136713 [details]
Patch

Fixed review comments

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 136713 [details]
Patch

Informal r+ from my side.

[Sudarsana Nagineni (babu)]

Created attachment 137550 [details]
Patch

rebased after r114386

[Sudarsana Nagineni (babu)]

*** Bug 83163 has been marked as a duplicate of this bug. ***

[WebKit Review Bot]

Comment on attachment 137550 [details]
Patch

Clearing flags on attachment: 137550

Committed r114419: <http://trac.webkit.org/changeset/114419>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.