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

Update v2 #99

Merged
merged 35 commits into from
Jan 24, 2023
Merged

Update v2 #99

merged 35 commits into from
Jan 24, 2023

Conversation

Cimbali
Copy link
Owner

@Cimbali Cimbali commented Jan 17, 2023

This is a PR rebasing all changes from #98:

  • render in extension page
  • can force rendering an online document
  • ext+view-markdown scheme
  • security improvements (mostly through sandboxed iframe, also CSP, stopped inline CSS injections)

And includes more:

  • less permissions, dynamically granted
  • can open local files without changing mime type
  • render in worker (out of main context for responsiveness and long-term memory footprint, etc.)
  • docs update for coherence, especially wrt page download, view source, etc.
  • “onboarding” page to present new features (for updates) or general workflow (new installs), especially with respect to optional permissions
  • waiting visual feedback (spinner) for slow rendering pages (e.g. 7MB use case from Process markdown in a worker #77)
  • updates code linting

If you can have a look at it too @KeithLRobertson that would be great.

Cimbali and others added 30 commits January 2, 2023 15:44
Now that we fetch() resources directly from the add-on rather than
injecting URLs into the page (since 42ce896) we don’t need
web_accessible_resources anymore.
- builder builds the final page
- inject is the part specific to on-page injection
Allows to make it explicit which document is being rendered or modified
by functions.
Uses an fetch() to get the source and render the document into an
iframe. This allows amongst other things to override target page CSPs.
Note that this declaring of protocols is only supported by firefox.
Rather than directly in the document, which should allow for more security.
Note that we use (a lot of) inline CSS, which allows to
- save the document as HTML,
- insert user-defined CSS,
- add auto light/dark scheme support

All these would need to be covered to tighten the style-src '*' CSP
Work around the fact that our iframe sandboxing does not allow blob URLs
to be opened, by opening them in the top document instead.

Also release Blob URLs
- Move rendering into its own class to prepare for workers
- builder.js does not require all the markdownit scripts any more, only
  a “Renderer” class with a “render” function
- renderer.js can do rendering given plugin prefs and text without all
  the page building logic
Should allow for much lighter pages.

On injected scripts, breaks with:
Security Error: Content at {url} may not load data from moz-extension://{id}/ext/worker.js

Some sources say scripts need to be web-accessible as the worker thread
does not inherit our context, but that does not seem to be the case?
As described in mozilla’s doc “Working with local files”
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Working_with_files#open_files_in_an_extension_using_a_file_picker

It’s a little cumbersome but will likely avoid many issues, such as the
usual linux/iOS file-handling mess.
Also hide the style-selection options as we don’t inline all
possibilities, only current selection.

Essentially finish reverting 42ce896.
Intercept F5 and Ctrl+r, so Ctrl+F5 and Ctrl+Shift+r naturally bypass
our mechanism.

Thanks to this mechanism we don’t need the scrollLock hack anymore.
- relative links need to have the original URL as base, not the extension page
- all non-anchor links should target the top frame

Also refactor node loop a little.
Also change all other options from callback to promise.
Accessible through toolbar button (hideable) and keyboard shortcut.
Especially useful for users that can’t access the injected local page
due to mime-type issues.

Uses a background page, make it non-persistent (event-based).
Could be made optional? This avoids a lot of script injections however

Keeping injected scripts for local files, as those have slightly
different capabilities to extension-page viewed local files. Notably:
- no need for file picker
- access to full paths
- display of relative-path images
@Cimbali
Copy link
Owner Author

Cimbali commented Jan 23, 2023

I’ve been trying to get a signed beta version of the add-on to upload here for people to test, but so far I’ve been waiting on AMO reviews for over 4 days now…

@KOLANICH
Copy link
Contributor

KOLANICH commented Jan 23, 2023

AMO reviews

I have heard Mozilla has eliminated review process for the addons that don't have a "recommended" shield (this one doesn't)...

@Cimbali
Copy link
Owner Author

Cimbali commented Jan 23, 2023

I think not being “recommended” means we don’t get a manual review with extra scrutiny. There still is a process for other addons, it’s mostly automatic, and if it fails (which it likely did due to updated dependencies) then we have to wait for someone to validate it…

The few beta versions I made are in the queue. They sent an automatic email that says “Your submission is taking longer than usual to be reviewed and signed. This process could take a few extra hours and up to a couple of days.”

@Cimbali
Copy link
Owner Author

Cimbali commented Jan 24, 2023

Here is the v2 beta (numbered 1.8.1.44) markdown_viewer_webext-1.8.1.44.zip

The “legacy behaviour” box is not 100% accurate (mentioning permissions instead of the issues downloading .md files). Any further feedback welcome.

@Cimbali
Copy link
Owner Author

Cimbali commented Jan 24, 2023

Merging as it seems this is the way forward anyway. Still tweaks to happen before releasing publicly. Thanks a lot @KOLANICH for all the work and new solutions.

@Cimbali
Copy link
Owner Author

Cimbali commented Jan 25, 2023

New beta available in this github artifact

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.

2 participants