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 missing variable escaping in the JS tracking code generator #8109

Merged
merged 1 commit into from Jun 16, 2015

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented Jun 15, 2015

The tracking code generator was not escaping quotes in string values (e.g. tracking custom variables).

Was reported in #8035

JS Tracking Code > Advanced > "Track Custom variables for this visitor" -> set Name to hello"world -> in the JS code Expected to get "hello\"world", Got instead: "hello"world"

Also the automatic HTML entities encoding for API parameters was messing things up (yet another #4231 win), I added a temporary fix to remove later.

Reviewers: please give another look for XSS issues.

The tracking code generator was not escaping quotes in string values (e.g. tracking custom variables).
@mnapoli mnapoli added Bug For errors / faults / flaws / inconsistencies etc. Needs Review PRs that need a code review labels Jun 15, 2015
@mnapoli mnapoli added this to the 2.14.0 milestone Jun 15, 2015
@mnapoli mnapoli mentioned this pull request Jun 15, 2015
3 tasks
' _paq.push(["setCustomVariable", %d, %s, %s, "visit"]);%s',
$index++,
json_encode($visitorCustomVariable[0]),
json_encode($visitorCustomVariable[1]),
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible for $visitorCustomVariable to have non-string elements? If so, perhaps they should be casted to strings.

EDIT: Same for page custom variables.

EDIT2: I think using json_encode is ok, actually, though it might result in incorrect tracking code in some cases. Though those cases may never occur in production.

@diosmosis
Copy link
Member

The JS tracking code in jsTrackingGenerator.js uses .val not .html to insert the code, so no XSS issues there.

I see that TrackingCodeGenerator is used when rendering the _displayJavascriptCode template, but the template outputs the tracker code via |raw. There might be an XSS issue here, though it will only be triggered if somehow the other generate() parameters are used.

I think after the TODO that's in the code is dealt w/, and the above XSS is verified to be a non-issue, this can be merged.

@diosmosis
Copy link
Member

Created new issue here: #8123

diosmosis added a commit that referenced this pull request Jun 16, 2015
Fix missing variable escaping in the JS tracking code generator, use json_encode to output proper JavaScript string values for tracking code options.
@diosmosis diosmosis merged commit 7111d0d into master Jun 16, 2015
@diosmosis diosmosis deleted the fix-tracker-generator-encoding branch June 16, 2015 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants