pop-os / launcher

Modular IPC-based desktop launcher service
Mozilla Public License 2.0
232 stars 48 forks source link

fix(web): Ignore invalid URLs rather than panicking #157

Closed canadaduane closed 1 year ago

canadaduane commented 1 year ago

This fixes an issue where the launcher panics if given an invalid URL. Precondition to PR #156 working as expected.

Note that this fix prevents another (possibly bad) issue: if the launcher panics while fetching an icon image to the cache, a corrupt image may be stored. Subsequent uses of pop-launcher could attempt to display the corrupted (cached) image, resulting in either a garbled image or, perhaps, undefined behavior.

canadaduane commented 1 year ago

Thanks! Much better.

Jan 13, 2023 7:10:56 AM Michael Murphy @.***>:

@.**** commented on this pull request.


In plugins/src/web/mod.rs[https://github.com/pop-os/launcher/pull/157#discussion_r1069506173]:

@@ -130,14 +130,21 @@ impl App { async fn fetch_icon_in_background(&self, def: &Definition, favicon_path: &Path) { let client = self.client.clone();

  • let url = build_query(def, "");
  • let url = Url::parse(&url).expect("invalid url");
  • let query = build_query(def, "");
  • let url = match Url::parse(&query) {
  • Ok(parsed) => parsed,
  • Err(_) => {
  • // return early if passed an invalid url, e.g. 'http://'
  • return;
  • }
  • };

This might be better

let Ok(parsed) = Url::parse(&query) else { return; };

— Reply to this email directly, view it on GitHub[https://github.com/pop-os/launcher/pull/157#pullrequestreview-1247700311], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAAABANCJZINI2EISMHSXW3WSFO6VANCNFSM6AAAAAATZ6SGUY]. You are receiving this because you authored the thread.[Tracking image][https://github.com/notifications/beacon/AAAABAJBAKTNWXPZB7NUJILWSFO6VA5CNFSM6AAAAAATZ6SGU2WGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTSKLZSVO.gif]

canadaduane commented 1 year ago

This PR has been superceded by #156, as it includes this PR's changes, among others.