Custom widgets: warn user it's risky #1423

manuhabitela · 2025-02-05T12:18:25Z

Context

Proposed solution

I allowed myself to add a somewhat related commit for small issues I stumbled upon while working on this. So that we can't add custom url widgets without filling the url input correctly. Hope it's okay!

edit: the check preventing empty-url custom widgets has been removed. Only the check verifying valid URL string is still there

Here is a preview in video:

custom.widget.warning.mp4

edit: now the image used is specific to dark theme for better contrast

Here is what it looks like in dark mode:

Has this been tested?

👍 yes, I added tests to the test suite
💭 no, because this PR is a draft and still needs work
🙅 no, because this is not relevant here
🙋 no, because I need help

app/client/ui/userTrustsCustomWidget.ts

static/img/security-alert.png

manuhabitela · 2025-02-05T17:44:58Z

I need to update the tests but the implementation is already reviewable if anyone wanna check early!

manuhabitela · 2025-02-05T18:04:18Z

@lusebille you'll notice a few things don't exactly match your figma design, for example the ordering of confirm / cancel buttons. I did it that way because modals in grist usually have those buttons rendered like that (I actually discovered that when seeing how modals were built in the code). Is this okay or should I do some specific design here?

lusebille · 2025-02-06T09:48:47Z

@manuhabitela ok I still think it's really more natural to have validation button on the right ( and it's much more common) but I understand we need to keep Grist homegenous ( the solution might to change it everywhere but should be another ticket anyway )

manuhabitela · 2025-02-07T10:22:22Z

After discovering the tests suite more I ended up removing my small change of making the URL input required for custom url widgets. Looks like it's actually a feature and not a bug to allow empty strings and overall it implied a bit more change than I thought that I'm not even sure is welcomed haha.

I left the change to force entering a valid URL if you input something though.

This is good to review for me!

Appart the code review, on the design part, question remains about the placement of confirm/save button: either specifically put them on the right as per Lucie's mockup, or leave them as is (on the left) to follow usual modal buttons placement in Grist, and maybe challenge this UI choice in a global way later.

paulfitz · 2025-02-19T17:07:07Z

hi @manuhabitela, thanks for this, could you resolve the conflicts that have accumulated so I can stick up a preview of this to show around?

manuhabitela · 2025-02-19T17:24:13Z

Oops sorry, done.

I realize with the merge conflict, that this commit seems to have introduced a somewhat similar idea I had on the markdown side. Maybe it could be reused. Seeing the code though it seems the linked commit doesn't actually returns html not wrapped in p tag but just visually removes the margins. While here the important part was removing the tag. I'll check it out further.

github-actions · 2025-02-19T17:26:42Z

Deployed commit e58c14f36accc8943d296c43c2cdd39bcdd5de58 as https://grist-manuhabitela-grist-core-issue-1338-custom-widget-warning.fly.dev (until 2025-03-21T17:25:01.794Z)

paulfitz · 2025-02-20T17:23:49Z

(just adding a screenshot from the preview, for context)

berhalak · 2025-02-20T17:40:27Z

app/client/ui/userTrustsCustomWidget.ts

+    ? 'img/security-alert.png'
+    : 'img/security-alert-dark-theme.png';
+
+  return new Promise<boolean>((resolve) => {


Can you refactor this, and create the save modal outside of the promise? Example of such modal is in modals.ts function invokePrompt.

Done! I'll let you check in case I misunderstood.

app/client/ui2018/alerts.ts

app/client/lib/markdown.ts

nbush · 2025-02-20T19:30:20Z

(just adding a screenshot from the preview, for context)

Is this a universal change that is intended to be shown to all users? If so, the reference to administrator & organization will be confusing to some or inaccurate for individuals.

If this is universal, I feel like this could be greatly condensed. I also think showing the URL in question on this pane is useful.

Something like:

Adding unknown custom widget

Custom widgets have the ability to read and write your document's data, as well as send it elsewhere. Make sure that you trust the resource at this URL before you continue.

https://unknownwidget.com/widget.html

[ ] I understand this warning and accept the risks

[Confirm] [Cancel]

If there are specific requirements that necessitate elements of this dialog, that would be good to know. Apologies if I've missed something!

commit to fix PR feedback, to fixup after review

github-actions · 2025-02-25T10:29:49Z

Deployed commit 7a7244f08e3e8160181cf0a67f2c6c5f4c1b52d1 as https://grist-manuhabitela-grist-core-issue-1338-custom-widget-warning.fly.dev (until 2025-03-27T10:28:07.568Z)

app/client/ui2018/alerts.ts

github-actions · 2025-02-25T10:37:47Z

Deployed commit 61837a6442941b6f05cc89245b6ae2141f784190 as https://grist-manuhabitela-grist-core-issue-1338-custom-widget-warning.fly.dev (until 2025-03-27T10:35:57.310Z)

fflorent

LGTM, thanks @manuhabitela!

app/client/ui/CustomWidgetGallery.ts

app/client/ui/userTrustsCustomWidget.ts

app/client/ui2018/alerts.ts

app/client/ui/userTrustsCustomWidget.ts

app/client/ui2018/modals.ts

github-actions · 2025-02-28T11:26:22Z

Deployed commit a2c044278da61044638335105985f5639fab1824 as https://grist-manuhabitela-grist-core-issue-1338-custom-widget-warning.fly.dev (until 2025-03-30T11:24:39.564Z)

berhalak · 2025-02-28T12:09:13Z

Ok, code looks good, thank you for addressing my comments, but some other UX questions:

It is possible to add Custom URL without an URL, and it still shows the confirmation popup.
After adding a custom widget we allow to change the URL on the right panel: (see screenshot below):

and there is no confirmation check there.
We lately added sanitizeHttpUrl helper to prevent this URL from being iffy. It still works, but maybe we can improve validation on the input element to disallow even pasting it.

We can all do it in a separate PR if you want, just mentioning it, if you want to address it here.

berhalak

Code looks ok, some UX questions above, but this is already a good improvement.
Thank you @manuhabitela.

github-actions · 2025-02-28T13:34:12Z

Deployed commit 75fe467d34dab306e2fc867c26b7c3c2a2aa424e as https://grist-manuhabitela-grist-core-issue-1338-custom-widget-warning.fly.dev (until 2025-03-30T13:32:18.013Z)

manuhabitela · 2025-02-28T13:34:39Z

Thanks for the thorough feedback @berhalak :)

It is possible to add Custom URL without an URL, and it still shows the confirmation popup.

Good catch, I'll fix code and change the tests for that case, you are completely right.

After adding a custom widget we allow to change the URL on the right panel and there is no confirmation check there.

Oh, let me try to handle this quickly. So that the confirm is triggered by the widget customdef url change and not a specific user action. if I see it's too much changes maybe yeah this can be tackled later, I feel the most important part is when first adding the widget.

We lately added sanitizeHttpUrl helper to prevent this URL from being iffy. It still works, but maybe we can improve validation on the input element to disallow even pasting it.

I feel it's not really necessary in my case? And rely on browser native url input check is sufficient to verify user input as this moment.

github-actions · 2025-02-28T13:57:44Z

Deployed commit f42bd9d604d332fb3384a967d1573ba604ca8b2c as https://grist-manuhabitela-grist-core-issue-1338-custom-widget-warning.fly.dev (until 2025-03-30T13:55:35.951Z)

github-actions · 2025-02-28T15:04:27Z

Deployed commit ecf57b9ae0bebb47530b3de8ca709efbf01fee35 as https://grist-manuhabitela-grist-core-issue-1338-custom-widget-warning.fly.dev (until 2025-03-30T15:02:23.882Z)

github-actions · 2025-02-28T15:50:42Z

Deployed commit aa42ddae0278ea1845b2cabbdae3116ce9e3da18 as https://grist-manuhabitela-grist-core-issue-1338-custom-widget-warning.fly.dev (until 2025-03-30T15:48:57.192Z)

manuhabitela · 2025-02-28T16:05:40Z

So I think this is good, besides the wording feedback from @nbush.

I added some code to handle the url change in the panel and prevent showing the warning modal with empty urls. Do you mind checking one last time @berhalak? 🙏

berhalak · 2025-03-03T16:25:46Z

app/client/ui/CustomWidgetGallery.ts

@@ -330,7 +330,8 @@ class CustomWidgetGallery extends Disposable {
  private async _validateSelectedWidget() {
    const isCustomUrlWidget = this._selectedWidgetId.get() === CUSTOM_URL_WIDGET_ID;
    if (isCustomUrlWidget) {
-      return this._customUrlInput?.reportValidity() && await userTrustsCustomWidget();
+      return this._customUrlInput?.reportValidity() &&
+        (!this._customUrl.get().length || await userTrustsCustomWidget());


Nitpicking, ignore if you like:

Hard to read, it would be better if this would look like the comment above

Comment above is outdated
:)

Man I can't figure out what comment you talk about haha. But I updated with something more readable!

The jsdoc for this method. Thanks for updating, it looks very nice :)

berhalak · 2025-03-03T16:27:57Z

app/client/ui/CustomSectionConfig.ts

@@ -558,9 +559,14 @@ export class CustomSectionConfig extends Disposable {
        return cssCustomUrlDetails(
          cssTextInput(
            this._url,
-            async value => this._url.set(value),
+            async (value) => {
+              if ((!value.length || await userTrustsCustomWidget())) {


Too many ((, and shouldn't we check the validity here?

Yeah I originally wanted to use the native reportValidity but for some reason I don't understand, it always returns true here… so the user doesn't get the native form validation with native browser message, unlike in the custom widgets gallery where it works.

So yeah I didn't use it. Sometimes those native apis are only usable if the browser detects we actually are in a user interaction, maybe something like that is the culprit. Really I'm surely missing something but I can't figure out what.

(I removed the extra parentheses, thanks)

Sorry for that. This is caused by the rawTextInput component in editableLabel.ts. It tries to be clever and disable the input before saving, so the reportValidity doesn't work. I'll file an issue for it.

github-actions · 2025-03-03T16:53:03Z

Deployed commit 7a60a4a5ce061f2cfea3f2584378dd5a821c5670 as https://grist-manuhabitela-grist-core-issue-1338-custom-widget-warning.fly.dev (until 2025-04-02T16:48:38.318Z)

berhalak · 2025-03-04T09:51:31Z

app/client/ui/CustomSectionConfig.ts

@@ -558,9 +559,14 @@ export class CustomSectionConfig extends Disposable {
        return cssCustomUrlDetails(
          cssTextInput(
            this._url,
-            async value => this._url.set(value),
+            async (value) => {
+              if ((!value.length || await userTrustsCustomWidget())) {


Sorry for that. This is caused by the rawTextInput component in editableLabel.ts. It tries to be clever and disable the input before saving, so the reportValidity doesn't work. I'll file an issue for it.

manuhabitela self-assigned this Feb 5, 2025

manuhabitela added the gouv.fr label Feb 5, 2025

manuhabitela force-pushed the issue-1338-custom-widget-warning branch from 8739304 to 9758d08 Compare February 5, 2025 17:21

manuhabitela commented Feb 5, 2025

View reviewed changes

app/client/ui/userTrustsCustomWidget.ts Show resolved Hide resolved

app/client/ui/userTrustsCustomWidget.ts Outdated Show resolved Hide resolved

static/img/security-alert.png Outdated Show resolved Hide resolved

manuhabitela requested review from fflorent and hexaltation February 5, 2025 17:45

manuhabitela mentioned this pull request Feb 5, 2025

Display a security disclaimer when user add a widget custom #1338

Closed

manuhabitela force-pushed the issue-1338-custom-widget-warning branch 8 times, most recently from 80bf1b2 to d8b21e4 Compare February 7, 2025 10:07

manuhabitela marked this pull request as ready for review February 7, 2025 10:22

paulfitz added the preview Launch preview deployment of this PR label Feb 13, 2025

manuhabitela force-pushed the issue-1338-custom-widget-warning branch from d8b21e4 to e58c14f Compare February 19, 2025 17:19

berhalak self-assigned this Feb 20, 2025

berhalak self-requested a review February 20, 2025 17:24

berhalak requested changes Feb 20, 2025

View reviewed changes

manuhabitela added 2 commits February 25, 2025 11:22

(widgets) french translation of new custom widget warning modal

340925e

fixup! (widgets) warn the user when he wants to install a custom widget

7a7244f

commit to fix PR feedback, to fixup after review

manuhabitela force-pushed the issue-1338-custom-widget-warning branch from 7e889e4 to 7a7244f Compare February 25, 2025 10:22

fixup! (widgets) warn the user when he wants to install a custom widget

61837a6

fflorent reviewed Feb 25, 2025

View reviewed changes

app/client/ui2018/alerts.ts Show resolved Hide resolved

fflorent approved these changes Feb 25, 2025

View reviewed changes

berhalak requested changes Feb 26, 2025

View reviewed changes

pr feedbacks - commit to fixup

a2c0442

berhalak approved these changes Feb 28, 2025

View reviewed changes

pr feedback, needs fixup

4ebf981

pr feedback, needs fixup (dont show popup when url is empty)

f42bd9d

manuhabitela force-pushed the issue-1338-custom-widget-warning branch from 75fe467 to f42bd9d Compare February 28, 2025 13:50

pr feedback (warn user of risk when modifying widget url in panel)

ecf57b9

to fixup

aa42dda

manuhabitela requested a review from berhalak February 28, 2025 16:05

berhalak reviewed Mar 3, 2025

View reviewed changes

pr feedback, to fixup (format issue + code reading issue)

7a60a4a

berhalak approved these changes Mar 4, 2025

View reviewed changes

berhalak merged commit 11d9dfe into gristlabs:main Mar 4, 2025
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom widgets: warn user it's risky #1423

Custom widgets: warn user it's risky #1423

manuhabitela commented Feb 5, 2025 •

edited

Loading

manuhabitela commented Feb 5, 2025

manuhabitela commented Feb 5, 2025

lusebille commented Feb 6, 2025

manuhabitela commented Feb 7, 2025

paulfitz commented Feb 19, 2025

manuhabitela commented Feb 19, 2025

github-actions bot commented Feb 19, 2025

paulfitz commented Feb 20, 2025

berhalak Feb 20, 2025

manuhabitela Feb 28, 2025

nbush commented Feb 20, 2025

Adding unknown custom widget

github-actions bot commented Feb 25, 2025

github-actions bot commented Feb 25, 2025

fflorent left a comment

github-actions bot commented Feb 28, 2025

berhalak commented Feb 28, 2025 •

edited

Loading

berhalak left a comment

github-actions bot commented Feb 28, 2025

manuhabitela commented Feb 28, 2025

github-actions bot commented Feb 28, 2025

github-actions bot commented Feb 28, 2025

github-actions bot commented Feb 28, 2025

manuhabitela commented Feb 28, 2025

berhalak Mar 3, 2025

manuhabitela Mar 3, 2025

berhalak Mar 4, 2025

berhalak Mar 3, 2025

manuhabitela Mar 3, 2025

manuhabitela Mar 3, 2025

berhalak Mar 4, 2025

github-actions bot commented Mar 3, 2025

berhalak Mar 4, 2025

Custom widgets: warn user it's risky #1423

Custom widgets: warn user it's risky #1423

Conversation

manuhabitela commented Feb 5, 2025 • edited Loading

Context

Proposed solution

Has this been tested?

manuhabitela commented Feb 5, 2025

manuhabitela commented Feb 5, 2025

lusebille commented Feb 6, 2025

manuhabitela commented Feb 7, 2025

paulfitz commented Feb 19, 2025

manuhabitela commented Feb 19, 2025

github-actions bot commented Feb 19, 2025

paulfitz commented Feb 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbush commented Feb 20, 2025

Adding unknown custom widget

github-actions bot commented Feb 25, 2025

github-actions bot commented Feb 25, 2025

fflorent left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 28, 2025

berhalak commented Feb 28, 2025 • edited Loading

berhalak left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 28, 2025

manuhabitela commented Feb 28, 2025

github-actions bot commented Feb 28, 2025

github-actions bot commented Feb 28, 2025

github-actions bot commented Feb 28, 2025

manuhabitela commented Feb 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 3, 2025

Choose a reason for hiding this comment

manuhabitela commented Feb 5, 2025 •

edited

Loading

berhalak commented Feb 28, 2025 •

edited

Loading