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.

Sign up for GitHub

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

Jump to bottom

Use encodeURIComponent everywhere #280

Merged
merged 3 commits into from
Jul 22, 2022
Merged

Use encodeURIComponent everywhere #280

merged 3 commits into from
Jul 22, 2022

Conversation

lukaw3d
Copy link
Member

@lukaw3d lukaw3d commented Jul 21, 2022 •
edited
Loading

Just best practice; I don't think any of those parameters can be user-controlled

Fixes #25

@lukaw3d lukaw3d requested a review from buberdds July 21, 2022 14:13
@lukaw3d lukaw3d marked this pull request as ready for review July 21, 2022 14:13
pro-wh
pro-wh approved these changes Jul 21, 2022
View reviewed changes
Copy link
Contributor

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

I'm in favor of this

pro-wh
pro-wh reviewed Jul 21, 2022
View reviewed changes
src/background/service/APIService.js Outdated
@@ -926,9 +926,9 @@ class APIService {
if(myNotificationID === clickId){
let url
if(runtimeId){
url = getExplorerUrl() + "paratimes/transactions/" + clickId
url = getExplorerUrl() + "paratimes/transactions/" + encodeURIComponent(clickId)
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the format of clickId?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh no. It can be hash?runtime=runtimeId

Copy link
Member Author

@lukaw3d lukaw3d Jul 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Refactored into

oasis-wallet-ext/src/background/service/APIService.js

Lines 921 to 938 in d6e14de

notification = (hash,runtimeId) => {
const notificationLinkAsId = runtimeId
? getExplorerUrl() + "paratimes/transactions/" + encodeURIComponent(hash) + "?runtime=" + encodeURIComponent(runtimeId)
: getExplorerUrl() + "transactions/" + encodeURIComponent(hash)
const notificationListener = (notificationId) => {
if(notificationLinkAsId !== notificationId) return
extension.notifications.onClicked.removeListener(notificationListener)
openTab(notificationLinkAsId)
}
extension.notifications.onClicked.addListener(notificationListener)
extension.notifications.create(notificationLinkAsId, {
title: getLanguage('notificationTitle'),
message: getLanguage('notificationContent'),
iconUrl: '/img/oasis.png',
type: 'basic'
});
return

Copy link
Contributor

Choose a reason for hiding this comment

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

what am I looking at here

Copy link
Member Author

Choose a reason for hiding this comment

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

url = getExplorerUrl() + "paratimes/transactions/" + encodeURIComponent(clickId) was wrong because encodeURIComponent('hash?runtime=runtimeId') === 'hash%3Fruntime%3DruntimeId' (notifyId -> myNotificationID -> clickId)

let notifyId = runtimeId ? hash +"?runtime="+runtimeId : hash
let myNotificationID
extension.notifications.create(notifyId, {...},(notificationItem)=>{ myNotificationID = notificationItem })
extension.notifications.onClicked.addListener(function (clickId) { if(myNotificationID === clickId){
  url = getExplorerUrl() + "paratimes/transactions/" + encodeURIComponent(clickId)

Force-pushed fix: https://github.com/oasisprotocol/oasis-wallet-ext/compare/6975b3c83c1110225738478e0a83428191576ccc..b07d9481e5e88f2834d53770913aff678ed92cbc

Additionally refactored:

  • myNotificationID wasn't needed
  • why create part of the url in two places: 
    let notifyId = runtimeId ?  encodeURIComponent(hash) +"?runtime="+encodeURIComponent(runtimeId) : encodeURIComponent(hash)
notifyId -> myNotificationID -> clickId
  if(runtimeId){
      url = getExplorerUrl() + "paratimes/transactions/" + clickId
  }else{
      url = getExplorerUrl() + "transactions/" + clickId
  }
    Combined into 
    const notificationLinkAsId = runtimeId
    ? getExplorerUrl() + "paratimes/transactions/" + encodeURIComponent(hash) + "?runtime=" + encodeURIComponent(runtimeId)
    : getExplorerUrl() + "transactions/" + encodeURIComponent(hash)
notificationLinkAsId -> notificationId
  • removeListener was missing

Copy link
Contributor

Choose a reason for hiding this comment

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

o nice

pro-wh
pro-wh approved these changes Jul 22, 2022
View reviewed changes
Copy link
Contributor

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

more lg

src/background/service/APIService.js
iconUrl: '/img/oasis.png',
type: 'basic'
},(notificationItem)=>{
Copy link
Contributor

Choose a reason for hiding this comment

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

lol what even was this callback

https://developer.chrome.com/docs/extensions/reference/notifications/#method-create

the docs don't give any info on when the browser will call it

src/background/service/APIService.js Outdated
@@ -919,7 +919,7 @@ class APIService {
}
}
notification = (hash,runtimeId) => {
let notifyId = runtimeId ? hash +"?runtime="+runtimeId : hash
let notifyId = runtimeId ? encodeURIComponent(hash) +"?runtime="+encodeURIComponent(runtimeId) : encodeURIComponent(hash)
Copy link
Contributor

Choose a reason for hiding this comment

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

😆 he even preserved asymmetric operator spacing

src/utils/extensionizer.d.ts
@@ -0,0 +1,3 @@
declare module 'extensionizer' {
export default chrome
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this do?

Copy link
Member Author

Choose a reason for hiding this comment

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

extensionizer doesn't have types, but it essentially does: globalThis.chrome || globalThis.browser https://github.com/MetaMask/extensionizer/blob/main/extension-instance.js#L39-L57. Those APIs are nearly identical https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Chrome_incompatibilities#javascript_apis, so I'm just using chrome's.

That makes all our extension.tabs.create({ calls typed!

I guess I should explicitly install @types/chrome tho.

@lukaw3d lukaw3d merged commit ef30177 into master Jul 22, 2022
@lukaw3d lukaw3d deleted the lw/encode-uri branch July 22, 2022 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pro-wh pro-wh pro-wh approved these changes

@buberdds buberdds Awaiting requested review from buberdds buberdds is a code owner

@peterjgilbert peterjgilbert Awaiting requested review from peterjgilbert peterjgilbert is a code owner

@kostko kostko Awaiting requested review from kostko kostko is a code owner

@tjanez tjanez Awaiting requested review from tjanez tjanez is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Notification click listener doesn't unsubscribe
2 participants
@lukaw3d @pro-wh