Bug 111964 – Explicitly send only one report via XSSAuditorDelegate.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mike West]

We want to send only one report when blocking script/pages via XSSAuditor. Currently, we do this by zeroing out the report URL after creating an XSSInfo object. I'd like to move this responsibility to the XSSAuditorDelegate instead, and do it explicitly via a "m_numberOfReportsSent" counter or "m_reportSent" boolean. That seems simpler to understand.

[Mike West]

Created attachment 192446 [details]
Patch

[Adam Barth]

Comment on attachment 192446 [details]
Patch

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

> Source/WebCore/html/parser/XSSAuditor.cpp:-335
> -        m_reportURL = KURL();

Won't m_reportURL now be referenced on multiple threads at the same time?

[Mike West]

(In reply to comment #2)
> (From update of attachment 192446 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192446&action=review
> 
> > Source/WebCore/html/parser/XSSAuditor.cpp:-335
> > -        m_reportURL = KURL();
> 
> Won't m_reportURL now be referenced on multiple threads at the same time?

Ugh. Yes. r-.

I think it's more elegant to allow the delegate to determine what the reporting policy should be. I'm not sure that elegance is worth making multiple isolated copies of the report URL.

Perhaps it would be possible to send the report URL to the delegate separately, rather than passing it around on an XSSInfo object. How would you feel about exposing the report URL via a public method on XSSAuditor, and adding it as a property to the XSSAuditorDelegate? We could populate it from HTMLDocumentParser::pumpTokenizer directly after initializing the Auditor. This would allow us to remove the property from XSSInfo objects entirely, which would also allow us to drop the thread-safe checks, as it would consist entirely of booleans (and a TextPosition, which might or might not be safe to pass across threads? (it isn't being checked in the existing code)).

WDYT?

[Adam Barth]

Yeah, there don't seem to be much reason to send the report URL to the background thread.  We should give it to the delegate on the main thread somehow.

[Mike West]

(In reply to comment #4)
> Yeah, there don't seem to be much reason to send the report URL to the background thread.  We should give it to the delegate on the main thread somehow.

Forked this into https://bugs.webkit.org/show_bug.cgi?id=112179 for clarity. Mind taking a look?

[Mike West]

Created attachment 192901 [details]
Patch

[Adam Barth]

Comment on attachment 192901 [details]
Patch

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

> Source/WebCore/html/parser/XSSAuditorDelegate.cpp:111
> +            RefPtr<FormData> report = generateViolationReport();
> +            PingLoader::sendViolationReport(m_document->frame(), m_reportURL, report);

report.release()

Actually, I would just combine these lines.

[Mike West]

Created attachment 193098 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 193098 [details]
Patch for landing

Clearing flags on attachment: 193098

Committed r145801: <http://trac.webkit.org/changeset/145801>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.