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

Sanitize notification message #13510

Merged
merged 19 commits into from
Dec 15, 2022

Conversation

fcollonval
Copy link
Member

References

Follow up of #13365 and JupyterLab weekly meeting

Code changes

Agressively sanitize the notification message

I tried the iframe encapsulation. But it sounds to much to apply it per notification messages. And to have all notifications wrapped within an iframe, this requires quite some rework and it will also bring question about accessibility (how do you notify of a new alert within a iframe).

User-facing changes

Most likely none

Backwards-incompatible changes

None

@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@fcollonval
Copy link
Member Author

Friendly ping to @vidartf and @jasongrout

@jasongrout
Copy link
Contributor

jasongrout commented Nov 30, 2022

If we're okay with starting very conservative, I would suggest having messages be only plain text (set with textContent), with a maximum size, with a single url the user can click on to read more. Starting simple is a feature, in that the simpler the message is forced to be, the less distracting it is in general for users.

@jasongrout
Copy link
Contributor

jasongrout commented Nov 30, 2022

Perhaps RSS is a good model to follow: an item has three required fields:

  • title (plain text)
  • description (plain text)
  • link

I'd encourage that title/descriptions be shortened to a max number of displayed characters to encourage best practices of not having overly long notifications, and to protect user screen space.

@fcollonval
Copy link
Member Author

bot please update snapshots

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2022

Documentation snapshots updated.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2022

Galata snapshots updated.

@fcollonval fcollonval closed this Dec 6, 2022
@fcollonval fcollonval reopened this Dec 6, 2022
@fcollonval
Copy link
Member Author

Thanks for the review @krassowski

The changes made are:

  • Fix code block in the privacy policies page
  • Add display type to the actions that will style the button (available values are: default, accent, warn or link). If the case of link, the button is identical to default but the text is colored and underlined with the link content color. I tested added a icon but if it feels strange.
    I did not change the HTML tree because I want to avoid coding something specific and I want to be sure the notification message is displayed as HTML text node only for security.
  • I added a way to not close the notification if an action is triggered.
  • The privacy policies page is now opened in a tab within JupyterLab. I'm half happy about it as the documentation website looks partly broken (and lots of errors are prompted in the web console) due to the cross-origin restriction.
  • Wording switched
  • Wording for the cross button from Close notification to Hide notification.

@fcollonval
Copy link
Member Author

bot please update galata snapshots

@github-actions
Copy link
Contributor

Galata snapshots updated.

Copy link
Contributor

@JasonWeill JasonWeill left a comment

Choose a reason for hiding this comment

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

Made a couple of small cosmetic changes and opened #13594 to revise privacy docs; otherwise, this looks good

@fcollonval
Copy link
Member Author

Merging this one to move forward with 3.6 beta release (CI failures are not related).

@krassowski if I release the beta this week. The RC will probably land before Christmas. But I won't be doing the final release before next year (probably waiting for the first January 4th meeting). Does that leave enough time for testing the 3.6.0?

@fcollonval fcollonval merged commit c5ebd3a into jupyterlab:master Dec 15, 2022
@fcollonval fcollonval deleted the fix/iframed-notification branch December 15, 2022 08:27
@fcollonval
Copy link
Member Author

@meeseeksdev please backport to 3.6.x

@lumberbot-app
Copy link

lumberbot-app bot commented Dec 15, 2022

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 3.6.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 c5ebd3af006fa4abd0fada9dd114dc28a90d01bc
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #13510: Sanitize notification message'
  1. Push to a named branch:
git push YOURFORK 3.6.x:auto-backport-of-pr-13510-on-3.6.x
  1. Create a PR against branch 3.6.x, I would have named this PR:

"Backport PR #13510 on branch 3.6.x (Sanitize notification message)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

fcollonval added a commit to fcollonval/jupyterlab that referenced this pull request Dec 15, 2022
fcollonval added a commit that referenced this pull request Dec 15, 2022
…13599)

* Backport PR #13510: Sanitize notification message

* Update Playwright Snapshots

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants