DBW: Audit for notification permissions on start #1064
Conversation
patrickhulce
force-pushed
the
notification_on_start
branch
from
8c33c16
to
8a88506
Compare
patrickhulce
force-pushed
the
notification_on_start
branch
from
8a88506
to
3e9ece0
Compare
|
Note: this doesn't seem to work when the permissions request comes from a different domain from within an iframe (test url: https://developer.mozilla.org/en-US/docs/Web/API/Notification) |
| }).catch(err => { | ||
| this.artifact = { | ||
| value: -1, | ||
| debugString: err && err.toString() |
There was a problem hiding this comment.
geolocation uses e.message. Can we make them consistent?
There was a problem hiding this comment.
sure, do we want to standardize on message and not include the error name then? would have benefit of accepting non-Error objects I suppose too
There was a problem hiding this comment.
err.toString() might be more noise in the html report. Not sure how that will look.
| afterPass(options) { | ||
| return options.driver.evaluateAsync(`(${queryNotificationPermission.toString()}())`) | ||
| .then(state => { | ||
| if (state === 'granted' || state === 'denied') { |
There was a problem hiding this comment.
Can you add || state === 'denied' to the geolocation audit too? It only checks state === 'granted' atm.
Can you file a bug for that? I suspect that also applies to the geolocation audit and we'll need to think of a solution. |
patrickhulce
changed the title
| /* global navigator */ | ||
|
|
||
| /* istanbul ignore next */ | ||
| function queryNotificationPermission() { |
There was a problem hiding this comment.
yeah, it would be great to pull this out into driver.queryPermissionStatus or whatever
| */ | ||
| queryPermissionState(name) { | ||
| const expressionToEval = ` | ||
| (function () { |
There was a problem hiding this comment.
any reason to wrap? navigator.permissions.query({name: ${name}}).then(result => result.state) is also an expression
There was a problem hiding this comment.
also, any specific reason for the JSON.stringify?
There was a problem hiding this comment.
Eh I guess no wrap necessary, and just in case name contained a ' or " didn't want an inscrutable error message
| @@ -433,12 +433,14 @@ class Driver { | |||
| * See https://developer.mozilla.org/en-US/docs/Web/API/Permissions/query. | |||
| */ | |||
| queryPermissionState(name) { | |||
| if (['geolocation', 'notifications', 'midi', 'push'].indexOf(name) === -1) { | |||
There was a problem hiding this comment.
The API already rejects for cases it doesn't understand:
Can we use that instead of enumerating all the types ourselves?
There was a problem hiding this comment.
yeah @ebidel this was to remove the JSON.stringify in response to @brendankenny's feedback since a malformed name wouldn't ever reach the friendly message. i'd lean toward my original stringify and let Chrome handle the enum check but I don't feel terribly strongly
There was a problem hiding this comment.
I'm not too concerned about protecting against all errors like this. Shouldn't exceptionDetails/when we land #1037 catch syntax errors like that?
There was a problem hiding this comment.
Gotcha. I think leaving off JSON.stringify is ok since we'll be the ones writing the majority of audits.
There was a problem hiding this comment.
Sorry, shuttle wifi catch up! @brendankenny said the same thing :)
There was a problem hiding this comment.
alright fair enough
|
PTAL :) |
|
@patrickhulce whoops, totally missed that the notification-on-start audit doesn't have any tests. Should be fairly fast, can basically just copy the geolocation one |
Addresses #1060