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.

Sign up for GitHub

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

Jump to bottom

[9.0][FIX] Mail tracking bugfixes #199

Merged
merged 3 commits into from
Oct 10, 2017
Merged

[9.0][FIX] Mail tracking bugfixes #199

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3bee337
[FIX] Only one data-odoo-tracking-email tag in each email
antespi Jan 26, 2017
0e17f91
[FIX] Get status even in multicompany instances
antespi Sep 6, 2017
2563896
Upgrade version
yajo Oct 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion mail_tracking/models/ir_mail_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ def _tracking_email_id_body_get(self, body):
tracking_email_id = False
# https://regex101.com/r/lW4cB1/2
match = re.search(
r'<img [^>]* data-odoo-tracking-email=["\']([0-9]*)["\']', body)
r'<img[^>]*data-odoo-tracking-email=["\']([0-9]*)["\']', body)
Copy link
Member

Choose a reason for hiding this comment

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

I find all of this would be much easier to maintain and understand by using lxml.html to parse the email body, and then XPath to find/remove the node. This regexp for instance would produce false positives with i.e. <img src="i'm a hacker>"/>, and given those come from the outside world, it's a good attack vector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is another way to do it.
I think regex is faster and consume less memory. BTW, that vector is not matching the regex, see https://regex101.com/r/lW4cB1/3

Copy link
Member

@yajo yajo Sep 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed, I was wrong, but see:

  • <img data-odoo-tracking-email="0'/>
  • <imgdata-odoo-tracking-email="0'/>
  • <img <broken syntax!! data-odoo-tracking-email="0'/>
  • <img <broken syntax!! data-odoo-tracking-email="0' muahahahaa>
  • <img valid-tag=">" data-odoo-tracking-email="1999"/>

want more? 😏

Copy link
Contributor Author

@antespi antespi Sep 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Well, in those cases lxml will break, isn't it?

This fix first remove any tag that match with this regex, then inject the tag and after read the tag, so if there is some broken tag then it's not a security problem.

Using lxml, what would the code have to do if no valid HTML is read?

Copy link
Member

Choose a reason for hiding this comment

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

I think it should except and pass in such case, don't you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that using lxml and then except and pass when invalid tags in the email will miss a tracking for some emails. In my approach (btw, the current one) those corner cases will be tracked without any problem.

So, I prefer to keep the current implementation because:

  • It's more resilience
  • It has a better CPU & RAM performance
  • It's simpler
  • It's the current implementation, less code changes
  • It hasn't any security issue

Any other opinions here?

if match:
try:
tracking_email_id = int(match.group(1))
Expand Down
2 changes: 1 addition & 1 deletion mail_tracking/models/mail_message.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def tracking_status(self):
partner_trackings = []
partners_already = self.env['res.partner']
partners = self.env['res.partner']
trackings = self.env['mail.tracking.email'].search([
trackings = self.env['mail.tracking.email'].sudo().search([
('mail_message_id', '=', message.id),
])
# Search all trackings for this message
Expand Down
6 changes: 5 additions & 1 deletion mail_tracking/models/mail_tracking_email.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,8 +222,12 @@ def tracking_img_add(self, email):
self.ensure_one()
tracking_url = self._get_mail_tracking_img()
if tracking_url:
content = email.get('body', '')
content = re.sub(
r'<img[^>]*data-odoo-tracking-email=["\'][0-9]*["\'][^>]*>',
'', content)
body = tools.append_content_to_html(
email.get('body', ''), tracking_url, plaintext=False,
content, tracking_url, plaintext=False,
container_tag='div')
email['body'] = body
return email
Expand Down