[webkit-reviews] review denied: [Bug 79704] Restrict access to notifications for unique origins and file URLs with no local file access : [Attachment 132555] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Jon Lee <jonlee at apple.com>'s request
for review:
Bug 79704: Restrict access to notifications for unique origins and file URLs
with no local file access
https://bugs.webkit.org/show_bug.cgi?id=79704

Attachment 132555: Patch
https://bugs.webkit.org/attachment.cgi?id=132555&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=132555&action=review


> Source/WebCore/notifications/NotificationCenter.cpp:62
> +    if (scriptExecutionContext()->securityOrigin()->isLocal())
> +	   return NotificationClient::PermissionAllowed;

Why should local URLs be automatically whitelisted to have PermissionAllowed? 
That sounds like a security decision that should either be made by
SecurityOrigin or the client (probably the client).

> Source/WebCore/notifications/NotificationCenter.cpp:70
> +    if (!scriptExecutionContext()->securityOrigin()->canShowNotifications()
|| scriptExecutionContext()->securityOrigin()->isLocal()) {

We shouldn't be magically whitelisting isLocal.

> Source/WebCore/notifications/NotificationCenter.cpp:71
> +	   callback->handleEvent();

Does this mean we're calling the callback synchronously when before we called
it async?

> Source/WebCore/page/SecurityOrigin.cpp:360
> +    return !isUnique() && (m_universalAccess ||
!m_enforceFilePathSeparation);

I'm not sure I understand this condition.  Is this the same thing we use for,
e.g., localStorage?

> Source/WebCore/page/SecurityOrigin.h:117
>      bool canAccessCookies() const { return !isUnique(); }
>      bool canAccessPasswordManager() const { return !isUnique(); }
>      bool canAccessFileSystem() const { return !isUnique(); }

Why is canShowNotifications different from the above?  Should we change the
above?	I think it make sense allow access to these things when
m_universalAccess is true, but I don't understand the connection with
m_enforceFilePathSeparation.