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

add file:// protocal exclude option in tracker #19835

Merged
merged 10 commits into from Nov 3, 2022
Merged

add file:// protocal exclude option in tracker #19835

merged 10 commits into from Nov 3, 2022

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Oct 10, 2022

Description:

Fixes: #17017
add file:// protocol exclude option in the tracker

add _paq.push(['enableFileTracking']); will start track protocal file:

Review

add file protocal option in js
@peterhashair
Copy link
Contributor Author

Not too sure if we can write a test for that feature, to change window.location.protocal maybe a problem.

@peterhashair
Copy link
Contributor Author

@sgiehl just checking, if that is the right solution.

@sgiehl
Copy link
Member

sgiehl commented Oct 14, 2022

@peterhashair what should your PR provide? Currently it adds a config option, that can be changed. But the option is not having any effect, so it's kind of useless 🤔

@peterhashair
Copy link
Contributor Author

@sgiehl I thought this would escape the tracking.

                if (!configFileTracking && windowAlias.location.protocol === 'file:') {
                    configDoNotTrack = true;
                }

@sgiehl
Copy link
Member

sgiehl commented Oct 16, 2022

@sgiehl I thought this would escape the tracking.

                if (!configFileTracking && windowAlias.location.protocol === 'file:') {
                    configDoNotTrack = true;
                }

Isn't that variable responsible for something completely different? Didn't have look right now, but iirc that should be the config flag responsible for checking "do-not-track" headers in the browser.

return empty
@peterhashair peterhashair added c: Documentation Improvements to in-app product help messages, or to the Matomo knowledge base. and removed c: Documentation Improvements to in-app product help messages, or to the Matomo knowledge base. labels Oct 17, 2022
move file protocol check after do not track
Copy link
Member

@Findus23 Findus23 left a comment

Choose a reason for hiding this comment

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

Just so we don't forget: Once this feature is finished, we need to mention it as a breaking change in the CHANGELOG.md.
As this would at least stop the tracking of one of my sites by default. (correction: I just remembered that it doesn't use file:// URLs anymore)

@github-actions
Copy link
Contributor

github-actions bot commented Nov 1, 2022

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Nov 1, 2022
@peterhashair peterhashair removed the Stale The label used by the Close Stale Issues action label Nov 1, 2022
@peterhashair peterhashair added this to the 5.0.0 milestone Nov 3, 2022
@peterhashair peterhashair added Needs Review For pull requests 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. labels Nov 3, 2022
add apply first
@peterhashair
Copy link
Contributor Author

build js

Copy link
Contributor

@bx80 bx80 left a comment

Choose a reason for hiding this comment

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

Tested with locally saved html file that loads the Matomo tracking code via https, by default no tracking requests are sent. With the new enableFileTracking config setting enabled the tracking requests are sent 👍

@bx80 bx80 merged commit 4917ae0 into 5.x-dev Nov 3, 2022
@bx80 bx80 deleted the m17017 branch November 3, 2022 23:15
@justinvelluppillai
Copy link
Contributor

@peterhashair did you see/action @Findus23 comment re updating the changelog with this?

@peterhashair
Copy link
Contributor Author

@justinvelluppillai will update changelog shortly

bx80 pushed a commit that referenced this pull request Nov 25, 2022
…by default (#19835)

* add file protocal option in js

add file protocal option in js

* rebuilt piwik.js

* remove function tests

remove function tests

* set DoNotTrack to true if file protocol

* return empty

return empty

* move file protocol check after do not track

move file protocol check after do not track

* rebuilt piwik.js

* fix esjint

fix esjint

* add apply first

add apply first

* rebuilt piwik.js

Co-authored-by: peterhashair <peterhashair@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review For pull requests 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
Development

Successfully merging this pull request may close these issues.

JS tracker should not track anything when the protocol is file:// to avoid tracking personal data by accident
5 participants