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

Jump to bottom

Avoid double encoding page titles #17470

Merged
merged 1 commit into from Apr 18, 2021
Merged

Avoid double encoding page titles #17470

merged 1 commit into from Apr 18, 2021

Conversation

flamisz
Copy link
Contributor

@flamisz flamisz commented Apr 18, 2021

Description:

fixes: #14065

Review

  • Functional review done
  • Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@flamisz flamisz added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Apr 18, 2021
@flamisz flamisz added this to the 4.3.0 milestone Apr 18, 2021
@flamisz flamisz self-assigned this Apr 18, 2021
@flamisz
Copy link
Contributor Author

flamisz commented Apr 18, 2021 •
edited

The problem what I found was that we use htmlspecialchars on the title before we save it to the db, and we double encode it in the twig file. We put into the DOM at line 83 (but this is hidden on the page), and than we use this already encoded version at line 88.

matomo/plugins/Live/templates/getLastVisitsStart.twig

Lines 83 to 88 in 84b9f9c

{% if action.pageTitle is not empty %}<span class='tooltip-action-page-title'>{{ action.pageTitle }}</span>{% endif %}
<span class="tooltip-action-server-time">{{ action.serverTimePretty }}</span>
{% if action.timeSpentPretty is defined %}<span class='tooltip-time-on-page'>{{ 'General_TimeOnPage'|translate }}: {{ action.timeSpentPretty|raw }}</span>{% endif %}
{%- endset %}
<img class='iconPadding' src="{{ action.iconSVG|default(action.icon) }}" title="{{- title|e('html_attr') -}}"/>

@diosmosis
Copy link
Member

diosmosis commented Apr 18, 2021 •
edited

LGTM and works locally, merging

@diosmosis diosmosis merged commit 3969bb9 into 4.x-dev Apr 18, 2021
@diosmosis diosmosis deleted the 14065-encodig-special-chars branch April 18, 2021 23:41
@tsteur
Copy link
Member

tsteur commented Apr 19, 2021

|raw would that be causing any security issue? Like when title maybe contains {{ and }}? Would we need to use something like |rawSafeDecoded? Or if we ever tracked title not encoded in the past could this cause an issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@flamisz flamisz

Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
4.3.0
Development

Successfully merging this pull request may close these issues.

Avoid double HTML entities / special characters encoding
3 participants
@flamisz @diosmosis @tsteur