Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Geolocation audit: warn if page is non-secure origin #1679

Merged
merged 1 commit into from
Feb 10, 2017
Merged

Conversation

ebidel
Copy link
Contributor

@ebidel ebidel commented Feb 10, 2017

R: all

Fixes #1676

throw new Error('Unable to determine if this permission was requested on page load ' +
'because it had already been set. Try resetting the permission and running ' +
'Lighthouse again.');
return options.driver.evaluateAsync('(function(){return window.isSecureContext;})()')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: isSecureContext returns true for https:// and http://localhost. The latter is a secure context for testing purposes. We may want to consider moving the HTTPs gatherer to this or supplementing it with this check for #1175.

.then(permissionState => {
if (permissionState === 'granted' || permissionState === 'denied') {
throw new Error('Unable to determine if the Geolocation permission was ' +
'requested on page load because it was already granted/denied. ' +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we have the permissionState and we know it was either "granted" or "denied", should we just inject it into the string instead of saying "granted/denied"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🌍 🎯 🙅

@brendankenny brendankenny merged commit 5503505 into master Feb 10, 2017
@brendankenny brendankenny deleted the 1676 branch February 10, 2017 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants