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] protect jinja2 expressions from being messed with #158

Closed
wants to merge 1 commit into from

Conversation

hbrunn
Copy link
Member

@hbrunn hbrunn commented Jun 30, 2015

fixes #151

// this is '#39' per default which screws up single quoted text in ${}
entities_additional: '',
protectedSource: [
/\n*\s*%.*\n*/gm, // line expressions, keep newline if any
Copy link
Contributor

Choose a reason for hiding this comment

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

@hbrunn I don't know why but this comments aren't minifing correctly and broke JS, check it in runbot.

Copy link
Member Author

Choose a reason for hiding this comment

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

@antespi thanks, should work now (I don't have an instance at hand right now, that's guessing)

Copy link
Contributor

Choose a reason for hiding this comment

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

@hbrunn It's working now (JS doesn't broke when minified). But other issue (Invoice Email Template broken when edited) is still there. Thanks

@antespi
Copy link
Contributor

antespi commented Jun 30, 2015

@hbrunn

This is not fixing this use case:

  1. Install addon web_ckeditor4
  2. Go to Invoicing
  3. Go to a validated invoice
  4. Click on Send by Email
  5. Click on Invoice - Send by Email template to edit it
  6. Add some plain text after A new invoice is available for you
  7. Save template

When return to Compose Email modal, body content has been disappeared.
An example of broken HTML block:

Original:

       % if object.user_id:
       &nbsp;&nbsp;Your contact: <a href="mailto:${object.user_id.email or ''}?subject=Invoice%20${object.number}">${object.user_id.name}</a>
       % endif
    </p>

    % if object.paypal_url:
    <br/>
    <p>It is also possible to directly pay with Paypal:</p>
        <a style="margin-left: 120px;" href="${object.paypal_url}">
            <img class="oe_edi_paypal_button" src="/account/static/src/img/btn_paynowcc_lg.gif"/>
        </a>
    % endif

Modified by ckeditor:

       % if object.user_id:
 &nbsp;&nbsp;Your contact: <a href="mailto:&lt;!--{cke_protected}%24%7Bobject.user_id.email%20or%20''%7D--&gt;?subject=Invoice&lt;!--{cke_protected}%2520%24%7Bobject.number%7D%22%3E%24%7Bobject.user_id.name%7D%3C%2Fa%3E%0A--&gt;&lt;!--{cke_protected}%20%20%20%20%20%20%20%25%20endif%0A--&gt;    &lt;/p&gt;&lt;!--{cke_protected}%20%20%0A%20%20%20%20%0A%20%20%20%20%25%20if%20object.paypal_url%3A%0A--&gt;    &lt;br/&gt;
    &lt;p&gt;It is also possible to directly pay with Paypal:&lt;/p&gt;
        &lt;a style="> <img class="oe_edi_paypal_button" src="/account/static/src/img/btn_paynowcc_lg.gif" /> </a>
    % endif

Pay attention at {cke_protected} mark inserted when variable is inside a href attribute

@legalsylvain legalsylvain added this to the 8.0 milestone Jun 30, 2015
@hbrunn
Copy link
Member Author

hbrunn commented Jul 1, 2015

@antespi thanks for your observation! I won't have time to look into this any time soon, could you try the following:

  • remove lines 128/129 from the patch and readd 125 & 126. Does this work in all cases?
  • try ckeditor 4.5beta maybe?
  • play more with config options... It seems like &lt;!--{cke_protected} should be <!--{cke_protected} and --&gt; should be -->. Could you edit this in source mode and see if it works then?

@RoelAdriaans
Copy link

  • Putting removing 128/129 (variable expressions) and adding entities_additional does not fix the problem.
  • Updating to CKEditor 4.5.1 (revision a513a92) did not solve the problem.
    (The CKEditor 4.5 Beta is older then that one)

@rafaelbn
Copy link
Member

@hbrunn could you check comment of @RoelAdriaans-B-informed #158 (comment). It could be very useful, thanks!

@hbrunn
Copy link
Member Author

hbrunn commented Aug 28, 2015

@rafaelbn I have, but as he stated, my suggestions didn't help. Which is a pity, but that's all I can say currently (no time to look into this deeply). If you need this issue to be fixed, I'd suggest you to look into my comment to try some more guesswork: #158 (comment)
But to me, this seems like a CKEditor bug, so you should probably have a look there and see what their community says. Editing PHP in attributes should break too, and from my understanding, CKEditor is used a lot in the PHP world (drupal, joomla, etc)

@hbrunn
Copy link
Member Author

hbrunn commented Sep 4, 2015

@antespi @rafaelbn @RoelAdriaans-B-informed merge #213 (and if this works, it's worth a thumbs up there ;-) ), then merge this branch with the commit from just now. With this, the editor adds some extra whitespace (which shouldn't matter), causing line statements to be treated properly. And the code update in #213 makes protecting of expressions within attributes work

@RoelAdriaans
Copy link

@hbrunn Too bad, still didn't solve the problem :-(

@hbrunn
Copy link
Member Author

hbrunn commented Sep 4, 2015

@RoelAdriaans-B-informed how does the code look before and after? And did you double check you're running ckeditor 4.5.3?

@hbrunn
Copy link
Member Author

hbrunn commented Sep 4, 2015

oh, and did you also double check you applied commit 8e8508d?

@hbrunn
Copy link
Member Author

hbrunn commented Sep 4, 2015

another thing to check: did you refresh the page after the update? otherwise, you get the old versions of js code

@hbrunn
Copy link
Member Author

hbrunn commented Sep 4, 2015

@RoelAdriaans-B-informed you were right, still a small thing wrong. With my last commit, you get this: https://runbot.odoo-community.org/runbot/build/3114496

In this instance, I installed account, played with the invoice mail template and everything goes fine. Can you test again?

@pedrobaeza
Copy link
Member

@RoelAdriaans-B-informed @antespi can you please check if the patch is correct for merging this?

@pedrobaeza
Copy link
Member

@hbrunn we are not using anymore CKEditor4, but maybe you do. Are you still interested? Is this safe to be merged?

@hbrunn
Copy link
Member Author

hbrunn commented May 5, 2018

it still messes up some jinja expressions, but less bad than before, so yes, I think we can merge this

@pedrobaeza
Copy link
Member

pedrobaeza commented May 5, 2018

OK, please make these 2 changes:

  • Bump module version number.
  • Add in the README in known issues section some words about when this is messed up.

@hbrunn hbrunn force-pushed the 8.0-web_ckeditor_fix_jinja2 branch from 9907b5f to b145b56 Compare May 7, 2018 08:01
@ghost
Copy link

ghost commented May 7, 2018

It's no longer an issue here; Few clients are still using 8.0, and none of them are using this module anymore...

vrenaville pushed a commit to camptocamp/web that referenced this pull request Jul 19, 2018
Update Dockerimage & odoo-template
Copy link
Contributor

@NL66278 NL66278 left a comment

Choose a reason for hiding this comment

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

👍 LGTM and all review remarks taken up and implemented.

@github-actions
Copy link

github-actions bot commented Oct 3, 2021

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Oct 3, 2021
@github-actions github-actions bot closed this Nov 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants