Security feature external link warning#7397
Conversation
decidim-admin/app/views/layouts/decidim/admin/settings.html.erb
Outdated
Show resolved
Hide resolved
ahukkanen
left a comment
There was a problem hiding this comment.
We could remove the excess styling from the CSS by utilizing the card element already provided by Foundation.
decidim-core/app/assets/stylesheets/decidim/extras/_external-links.scss
Outdated
Show resolved
Hide resolved
decidim-core/db/migrate/20210210114657_add_external_domain_whitelist_to_organization.rb
Show resolved
Hide resolved
|
We should also make sure this logic works when new comments are added dynamically to the page. I believe when new comments are loaded right now to the page, this logic won't apply to them. Same thing for the "opens in new tab" link accessibility icons, I think we could fix them at the same time. |
ahukkanen
left a comment
There was a problem hiding this comment.
This fails in an exception right now if something unexpected in passed in the external_link parameter. Let's fix that.
|
@mrcasals Done |
|
@lahdeero some conflicts appeared, sorry about that 🙈 could you check them and I'll merge the PR? 🙏 |
decidim-admin/app/packs/src/decidim/admin/external_domain_whitelist.js
Outdated
Show resolved
Hide resolved
|
@mrcasals Done again |
There was a problem hiding this comment.
Great job @lahdeero! 👍
Sorry for asking for changes again, but we want to avoid issues with migrations and to enforce consistency on that critical part for the apps upgrades. I promess you that I will merge this as soon as you change the migration. 🙏
(The change request appeas as solved, but it's not)
decidim-core/db/migrate/20210210114657_add_external_domain_whitelist_to_organization.rb
Outdated
Show resolved
Hide resolved
|
Seems like update_all cant cast array(?) |
You can try with raw SQL: Decidim::Organization.update_all("external_domain_whitelist = ARRAY['decidim.org', 'github.com']") |
|
@leio10 Can you restart blogs test? I don't believe this pr is causing it to fail |
* develop: (59 commits) Update supported versions in docs (#8079) Meetings merge minutes and close actions (#7968) Meeting calendars providers (#7944) Fix broken test on meetings after merging PR without rebase (#8076) Show participants list in meetings (#7933) Security feature external link warning (#7397) Add missing tests for scope types admin page (#8053) Use symbols for polymorphic route arguments (#8052) Mockup design for Participation statistics tables in Votings (#7879) Fix boolean fields for .reported? and .hidden? which is nil if no report exists (#7990) Fix redirects broken by Terms and Conditions redirect (#8036) Amend CSS overwritting (#8007) New Crowdin updates (#8048) Fix undetected broken tests because of missing dependencies (#8050) Validate results by Monitoring Committee Members (#7899) Electoral certificate validation by Monitoring Committee Members (#7871) Publish and unpublish a meeting (#7893) New Crowdin updates (#8005) Polling station closure attach the physical electoral closure certificate (#7929) Fix attachment title migration generating possibly invalid values (#8020) ...
🎩 What? Why?
There are many places(e.g. profile and proposals) where registered users can insert links in Decidim. Since Decidim has lot of non-digi-native users we should have clear warning page when they are clicking external or potentially dangerous links. This pull request replaces all links which have external domain to redirect into our warning page. There is also new section in admin panel where admins can add domains to whitelist (so we don't show warning when users click whitelisted links).
📌 Related Issues
https://meta.decidim.org/processes/roadmap/f/122/proposals/15681
Testing
📋 Checklist
🚨 Please review the guidelines for contributing to this repository.
docs/.📷 Screenshots