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

Fix proposal etiquette and length validator with base64 images #9639

Merged
merged 4 commits into from
Oct 28, 2022

Conversation

ahukkanen
Copy link
Contributor

🎩 What? Why?

When submitting a proposal and pasting an image to it (using the computer's clipboard), a base 64 encoded image is entered into the view.

This causes the proposal etiquette and length validator to fail as the base 64 encoded image has a lot of characters and particularly uppercase characters.

As a sidenote, these images should not probably be base 64 as we today have the possibility in the editor to upload these properly. This is, however, another separate issue outside of the scope of fixing this bug.

📌 Related Issues

Testing

  • Enable rich text editor for the participants from the organization settings
  • Go to create a proposal in the proposals component
  • Open image to an image editor
  • Copy the image from the image editor using CTRL+C
  • Paste the image to the proposal's description
  • Submit the form

Content e.g. in `<script>` tags should be automatically hidden,
so this should not be included in the validation either.
Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

Just a small detail on the specs and we're good to go

@andreslucena
Copy link
Member

@ahukkanen I was thinking about this issue last night, and I actually think that the problem is on the base64 conversion itself: it shouldn't kick-in.

After #7712 and #7713, these drag&drops shouldn't be base64. For what I've seen this only works on the admin panel, even on this same form. Steps to reproduce this:

  1. Sign in as admin
  2. Enable "Enable rich text editor for participants" on http://localhost:3000/admin/organization/edit?locale=en
  3. Go to a proposals component settings, enable "Participants can create proposals"
  4. Create a new proposal in the admin panel. Drag and drop an image, see with the inspector that the image is uploaded to the server
    Selection_331
  5. Create a new proposal in the frontend. Drag and drop an image, see with the inspector that the image is base64.

So, I would say that the bug shouldn't be "base64 images generate the EtiquetteValidator to run" but "we (still) allow uploading base64 images".

This seems like it originated because in the frontend editor, we don't have the editor image functionality enabled. This is something that in the past I blocked, but only because it uploaded on base64; now I actually can come up with any reason to not have it enabled by default 😅 .

So, I'm up for changing this behavior and enable image uploads on the frontend editor by default. The cleaner way that I see is just to drop this line. Should we handle that in other PR?

@ahukkanen
Copy link
Contributor Author

@andreslucena The original issue was actually that even if we did not allow it, it was possible to paste images to the Quill editor.

I think the last changes we did in the Quill editor actually "fixed" that by accident since last time I tested locally, I could not paste images to the editor.

That said, I still think they should be excluded from these checks, whether they are base64 images or not. I don't think these validators should consider the image content.

@andreslucena
Copy link
Member

I don't think these validators should consider the image content.

Yes, you're right. Even without base64 I think you could have the same problem, like if you have a really long filename with all caps. I still will work on the image upload change though, as base64 shouldn't work anymore.

Lets retake this PR, can you give it an eye to the pending conversations 🙏🏽?

@ahukkanen
Copy link
Contributor Author

Lets retake this PR, can you give it an eye to the pending conversations 🙏🏽?

Done.

Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

👍🏽

@andreslucena andreslucena merged commit 2fa06d4 into decidim:develop Oct 28, 2022
@ahukkanen ahukkanen deleted the fix/8985 branch October 28, 2022 06:22
andreslucena pushed a commit that referenced this pull request Oct 28, 2022
* Fix proposal etiquette and length validator with base64 images

* Use strip_tags instead of nokogiri not to include hidden content

Content e.g. in `<script>` tags should be automatically hidden,
so this should not be included in the validation either.

* Fix the expected base64 mime types
andreslucena pushed a commit that referenced this pull request Oct 28, 2022
* Fix proposal etiquette and length validator with base64 images

* Use strip_tags instead of nokogiri not to include hidden content

Content e.g. in `<script>` tags should be automatically hidden,
so this should not be included in the validation either.

* Fix the expected base64 mime types
entantoencuanto added a commit that referenced this pull request Oct 31, 2022
* develop: (36 commits)
  Fix proposal etiquette and length validator with base64 images (#9639)
  Install turbo-rails (#9881)
  Fix conference invitations (#9664)
  Fix invalid rendering of meeting and proposal body texts (#9764)
  Make documentation site work with multiple versions (#9917)
  Bump versions on install docs (#9916)
  Standardize CSV import formats and fix private users CSV import with invalid file (#9627)
  Fix: The i18n locales selector is showing a dropdown with 3 languages (#9902)
  Make Scopes field in debates translatable (#9903)
  Make ToS agreement translatable (#9909)
  Fix issues with a11y specs (#9929)
  Remove invitations badge (#9906)
  Make initiatives order translatable (#9905)
  Add missing active actions on admin navigation menu (#9904)
  Fix user sign up with invalid name (#9896)
  Remove duplication of LastActivity queries (#9895)
  Rename IgnoredMethods to AllowedMethods in Rubocop configuration (#9893)
  Exclude malformed file from codeclimate configuration (#9910)
  Fix correct resource linking for amendments (#9887)
  Fix superposition in admin's error forms (#9871)
  ...
entantoencuanto added a commit that referenced this pull request Nov 2, 2022
…esign-staging

* feature/redesign-accountability-turbo:
  Ensure the correct use of only one h1 displaying/hidding drawers
  Avoid initialize and open drawer twice
  Display a preview of index on background of results show
  Update close drawer url with filtered path
  Improve drawers navigation
  Initialize drawers dialog with the turbo:frame-render event
  Display results show inside a drawer using a turbo frame
  Allow card_l_cell to open linked resource on a turbo frame
  Fix proposal etiquette and length validator with base64 images (#9639)
entantoencuanto added a commit that referenced this pull request Nov 3, 2022
* develop:
  Ignore the problematics HTML validation checks with hidden inputs (#10020)
  Fix proposal etiquette and length validator with base64 images (#9639)
entantoencuanto added a commit that referenced this pull request Nov 3, 2022
* develop:
  Ignore the problematics HTML validation checks with hidden inputs (#10020)
  Fix proposal etiquette and length validator with base64 images (#9639)
entantoencuanto added a commit that referenced this pull request Nov 3, 2022
* develop:
  Ignore the problematics HTML validation checks with hidden inputs (#10020)
  Fix proposal etiquette and length validator with base64 images (#9639)
ahukkanen added a commit that referenced this pull request Nov 3, 2022
…ges' to v0.26 (#10010)

* Fix proposal etiquette and length validator with base64 images (#9639)

* Fix proposal etiquette and length validator with base64 images

* Use strip_tags instead of nokogiri not to include hidden content

Content e.g. in `<script>` tags should be automatically hidden,
so this should not be included in the validation either.

* Fix the expected base64 mime types

* Add omitted value in keyword argument

* Replace unexisting API on v0.26

Co-authored-by: Antti Hukkanen <antti.hukkanen@mainiotech.fi>
ahukkanen added a commit that referenced this pull request Nov 4, 2022
…ges' to v0.27 (#10009)

* Fix proposal etiquette and length validator with base64 images (#9639)

* Fix proposal etiquette and length validator with base64 images

* Use strip_tags instead of nokogiri not to include hidden content

Content e.g. in `<script>` tags should be automatically hidden,
so this should not be included in the validation either.

* Fix the expected base64 mime types

* Add omitted value in keyword argument

Co-authored-by: Antti Hukkanen <antti.hukkanen@mainiotech.fi>
Quentinchampenois pushed a commit to Quentinchampenois/decidim that referenced this pull request Nov 23, 2022
…im#9639)

* Fix proposal etiquette and length validator with base64 images

* Use strip_tags instead of nokogiri not to include hidden content

Content e.g. in `<script>` tags should be automatically hidden,
so this should not be included in the validation either.

* Fix the expected base64 mime types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: core module: proposals type: fix PRs that implement a fix for a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal is using too many capital letters (over 25% of the text)
2 participants