search

hediet / browser-ext-github-monaco

This extension brings the famous Monaco editor to Github
https://chrome.google.com/webstore/detail/monaco-markdown-editor-fo/mmpbdjdnmhgkpligeniippcgfmkgkpnf?hl=de&authuser=0
257 stars 32 forks source link

Enable customizations of the editor #25

Closed ChayimFriedman2 closed 3 years ago

ChayimFriedman2 commented 3 years ago

We provide a settings page, that enables the user to change all of Monaco's options except those that are irrelevant here.

There are two settings that I did not found a good way to express in the settings UI: fontLigatures can be a string or a boolean (currently only a checkbox for boolean), and rulers is an array.

I'm not good with UX and the UI is minimal. If someone more knowledgeable than me would like to style it, I welcome suggestions.

Accessibility isn't good either, and again, if you know better than me what to do, please share it!

This fixes bunch of issues. Here's what I've found:

ChayimFriedman2 commented 3 years ago

These settings are passed directly to monaco without validating them. Does JavaScript Prototype Poising apply? What are the risks that the settings go corrupt and monaco receives garbish, making the text editor unusable?

I've thought about security, but considering that the settings object is both produced and consumed on the client side, I see no problem. The only way to exploit this messaging is by injecting a malware script into the page, and the only bad thing that can happen is corrupted editor, so that's not a problem.

And BTW, if you can inject a script to the page there are worse thing you can do.

hediet commented 3 years ago

@ChayimFriedman2 seems reasonable.

hediet commented 3 years ago

Thank you!

hediet commented 3 years ago

Btw. I think it would be a nice feature to highlight changed settings in bold.

ChayimFriedman2 commented 3 years ago

OK. I'm pretty busy this week, but maybe I'll find time to push this.

hediet commented 3 years ago

Published with v0.5.0!

bbugh commented 3 years ago

Thanks for this @ChayimFriedman2!

hediet commented 3 years ago

Oops, installation count dropped from 530 to 450 after requesting the "storage" permission! I think any future permission should be requested dynamically.