Bug 82396

Summary: Reinforce Geolocation to prevent accidental leak of the user position
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: WebCore Misc.Assignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ddkilzer
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch abarth: review+

Description Benjamin Poulain 2012-03-27 16:42:31 PDT
Some simple changes can be done to enhance Geolocation in case of programming mistake.
Comment 1 Benjamin Poulain 2012-03-27 16:42:58 PDT
Related to <rdar://problem/11106417>
Comment 2 Benjamin Poulain 2012-03-27 16:58:20 PDT
Created attachment 134171 [details]
Patch
Comment 3 Adam Barth 2012-03-27 17:03:57 PDT
Comment on attachment 134171 [details]
Patch

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

> Source/WebCore/Modules/geolocation/Geolocation.cpp:128
> +    // If we are here and the Geolocation permission is not approved, something has
> +    // gone horribly wrong.
> +    // We bail out to avoid any privacy issue.
> +    ASSERT(m_geolocation->isAllowed());
> +    if (!m_geolocation->isAllowed())
> +        return;

Should we CRASH() rather than return here?
Comment 4 Benjamin Poulain 2012-03-27 17:10:08 PDT
> > Source/WebCore/Modules/geolocation/Geolocation.cpp:128
> > +    // If we are here and the Geolocation permission is not approved, something has
> > +    // gone horribly wrong.
> > +    // We bail out to avoid any privacy issue.
> > +    ASSERT(m_geolocation->isAllowed());
> > +    if (!m_geolocation->isAllowed())
> > +        return;
> 
> Should we CRASH() rather than return here?

I have considered that but that seemed a bit extreme.

Do you have any input on what is bad enough that we should take down the whole browser? It is difficult to know it an error comes from a security breach or a programming mistake.
Comment 5 Adam Barth 2012-03-27 17:45:03 PDT
> Do you have any input on what is bad enough that we should take down the whole browser? It is difficult to know it an error comes from a security breach or a programming mistake.

It's up to you.  The nice thing about crashing in impossible situations is that we'll get crash reports and we can fix the issue.  :)
Comment 6 Benjamin Poulain 2012-03-27 17:58:54 PDT
Committed r112347: <http://trac.webkit.org/changeset/112347>