Skip to content
This repository was archived by the owner on Mar 14, 2024. It is now read-only.

Improvements to messaging article. #4275

Merged
merged 10 commits into from
Dec 2, 2022
Merged

Improvements to messaging article. #4275

merged 10 commits into from
Dec 2, 2022

Conversation

jpmedley
Copy link
Contributor

@jpmedley jpmedley commented Nov 11, 2022

@dotproto, I realized this morning that I needed to understand messaging better. If I'm reading something, the cost of improving it is marginal. I made some V3-specific updates to this article. You can find them at lines 27, 33, 43, and 274. Please ignore the other stuff. I'll ask @AmySteam to do a proper copy edit before I merge.

Staged link
Message passing

@jpmedley jpmedley added the extensions Issues related to extensions documentation. label Nov 11, 2022
@netlify
Copy link

netlify bot commented Nov 11, 2022

Deploy Preview for developer-chrome-com ready!

Name Link
🔨 Latest commit 9b71a63
🔍 Latest deploy log https://app.netlify.com/sites/developer-chrome-com/deploys/638a2291a80bee0008f4888b
😎 Deploy Preview https://deploy-preview-4275--developer-chrome-com.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@@ -265,7 +271,7 @@ scripts.
When receiving a message from a content script or another extension, your scripts should be careful
not to fall victim to [cross-site scripting][39]. This advice applies to scripts running inside the
extension background page as well as to content scripts running inside other web origins.
Specifically, avoid using dangerous APIs such as the ones below:
Specifically, avoid using dangerous APIs, specificaslly `chrome.scripting.executeScript()`, `eval()`, and `new Function()`. For example:
Copy link
Contributor

Choose a reason for hiding this comment

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

This section is a bit odd now because of CSP changes in Manifest V3. Ian is working on #3837 to update our CSP docs, but the short version is that Chrome has a minimum CSP for extension contexts that does not allow developers to call eval() or new Function(). For reference, the minimum CSP is script-src 'self' 'wasm-unsafe-eval'; object-src 'self';.

It may be best to use a slightly different approach for this section. Since cross-site scripting isn't as much a concern, we could lean into "cross-site attacks," where a compromised render could use the message channel to send the extension messages in order to extract data or make the extension do something dangerous.

Cross-site scripting is still a concern for content scripts. This is another method by which a content script could potentially be hijacked into sending dangerous messages back to the extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to remove the new text so that we can merge this. I'll circle back when I have more information.

Copy link
Collaborator

@AmySteam AmySteam left a comment

Choose a reason for hiding this comment

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

@jpmedley Good improvements.

Since you are making this article MV3 compatible, can you change the mention of background pages to extension service worker?

For example, an RSS reader extension might use content scripts to detect the presence of an RSS feed on a page, then notify the background page in order to display a page action icon for that page.

This sentence mentions a background page and a page action. Both are not supported in MV3.

@AmySteam AmySteam assigned jpmedley and unassigned dotproto and AmySteam Nov 14, 2022
@jpmedley
Copy link
Contributor Author

@dotproto, I've removed the bit about eval() et al, restoring the original text until I can do more research. That way we can merge this PR.

PTAL

@jpmedley jpmedley requested review from dotproto and AmySteam December 1, 2022 23:55
@jpmedley jpmedley assigned dotproto and AmySteam and unassigned jpmedley Dec 1, 2022
Copy link
Collaborator

@AmySteam AmySteam left a comment

Choose a reason for hiding this comment

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

LGTM 👍 I just have some small change requests. Thanks!

@jpmedley jpmedley requested a review from AmySteam December 2, 2022 15:54
@jpmedley
Copy link
Contributor Author

jpmedley commented Dec 2, 2022

@AmySteam Thank you for the review. Are there any fact that you think @dotproto needs to verify before we merge this?

@AmySteam
Copy link
Collaborator

AmySteam commented Dec 2, 2022

@jpmedley Nope, I think this can be merged as is 👍

@jpmedley jpmedley merged commit e6b0068 into main Dec 2, 2022
@jpmedley jpmedley deleted the messages branch December 2, 2022 16:32
console.log(response.farewell);
});
chrome.tabs.query({active: true, lastFocusedWindow: true}, function(tabs) {
await response = chrome.tabs.sendMessage(tabs[0].id, {greeting: "hello"});
Copy link

Choose a reason for hiding this comment

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

This is invalid JS syntax.

chrome.runtime.sendMessage({greeting: "hello"}, function(response) {
console.log(response.farewell);
});
await response = chrome.runtime.sendMessage({greeting: "hello"});
Copy link

Choose a reason for hiding this comment

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

This is invalid JS syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feedback. We had an off day there.

See #4518

@tophf
Copy link

tophf commented Dec 5, 2022

Fixing the snippets is a little tricky because you need to show a complete async function, otherwise people who only learn JS will waste time (probably) a lot.

The first snippet:

(async () => {
  const response = await chrome.runtime.sendMessage({greeting: "hello"});
  // do something with response here, not outside the function
  console.log(response);
})();

The second snippet should be rewritten:

(async () => {
  const [tab] = await chrome.tabs.query({active: true, lastFocusedWindow: true});
  const response = await chrome.tabs.sendMessage(tab.id, {greeting: "hello"});
  // do something with response here, not outside the function
  console.log(response);
})();

Generally you should convert all function declarations to await as shown above or to arrow functions - it is important in modern frameworks that need this to be bound to the app instance also inside the API callback.

Also make sure to at least verify each JS snippet you modify or add in devtools console, especially if JS is not your forte.

@jpmedley
Copy link
Contributor Author

jpmedley commented Dec 5, 2022

We know that. I had an off day.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
extensions Issues related to extensions documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants