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

[#1624] Escape html of product content field #718

Merged
merged 3 commits into from
Sep 7, 2023

Conversation

pi-sigma
Copy link
Contributor

Taiga: #1624

@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2023

Codecov Report

Merging #718 (4b1fa79) into develop (ceacfe5) will increase coverage by 0.09%.
Report is 90 commits behind head on develop.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #718      +/-   ##
===========================================
+ Coverage    96.20%   96.30%   +0.09%     
===========================================
  Files          642      683      +41     
  Lines        22933    24202    +1269     
===========================================
+ Hits         22063    23308    +1245     
- Misses         870      894      +24     
Files Changed Coverage Δ
src/open_inwoner/pdc/models/product.py 90.97% <100.00%> (+0.39%) ⬆️
src/open_inwoner/pdc/tests/test_product.py 100.00% <100.00%> (ø)
src/open_inwoner/utils/ckeditor.py 88.63% <100.00%> (+0.26%) ⬆️

... and 70 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pi-sigma pi-sigma marked this pull request as ready for review August 1, 2023 13:01
@pi-sigma pi-sigma requested a review from alextreme August 1, 2023 13:01
@Bartvaderkin Bartvaderkin self-assigned this Aug 28, 2023
@Bartvaderkin Bartvaderkin changed the title [#1624] escape html of product content field [#1624] Escape html of product content field Aug 28, 2023
@Bartvaderkin
Copy link
Contributor

The double slashes come from CKeditor or its markdown plugin trying to a some sort of html escape by putting double backslash in front of the angle bracket. If I use the same javascript libraries to render the string it does escape the html-tags, so this feels like a marked/gfmarkdown thing. But I cannot find where this happens.

So I appended a commit with a slightly different approach to clean this up: we undo the weird escape and then strip_tags.

@Bartvaderkin Bartvaderkin self-requested a review August 28, 2023 13:09
@alextreme
Copy link
Member

@Bartvaderkin please doublecheck your solution together with the ckeditor-table option.

I've checked on production where I could find HTML-tags, and noticed that this was the case with 4 products on MG:

https://mijn.groningen.nl/onderwerpen/producten/bijstand-voor-tweedehands-computer-aanvragen/

If we strip tags I'd prefer not to remove (from the editor) valid content. I now also suspect that these tables are the reason why HTML-content is allowed...

@Bartvaderkin
Copy link
Contributor

@alex I updated this PR: we no longer do html escape or strip tags, and only remove the weird slashes at render time as they are needed to be able to edit the html again (I guess the slashes are escaping html in ckeditor).

Copy link
Member

@alextreme alextreme left a comment

Choose a reason for hiding this comment

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

Prima voor nu, graag merge conflicts verhelpen en een aparte issue maken voor ckeditor+tables

@alextreme alextreme merged commit b6482c9 into develop Sep 7, 2023
10 checks passed
@alextreme alextreme deleted the fix/1624-html-escape branch September 7, 2023 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants