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

On opt-out page move js to separate file to fix inline-script (CSP) #12873

Merged
merged 5 commits into from May 30, 2018
Merged

On opt-out page move js to separate file to fix inline-script (CSP) #12873

merged 5 commits into from May 30, 2018

Conversation

Skywalker-11
Copy link
Contributor

This PR moves the javascript part of the opt-out page that can be loaded via an iframe to a separate file. This allows the usage of CSP without the need to allow inline scripts or using hashes

@tsteur
Copy link
Member

tsteur commented May 9, 2018

FYI: Merging this PR would break matomo-org/tracker-proxy#37 which we finished yesterday cc @diosmosis

@Skywalker-11 Skywalker-11 mentioned this pull request May 14, 2018
Merged
@Skywalker-11
Copy link
Contributor Author

This PR should now also work with the patched tracker-proxy which PR is referenced above. To accieve this an additional get parameter (matomoproxy) is added on the proxy. If it doesn't exist in the request of the opt-out iframe the normal path plugins/CoreAdminHome/javascripts/optOut.js is used.
If the parameter matomoproxy exists it will use ?module=CoreAdminHome&action=getOptOutJs as path for the script which is translated by the proxy to the normal path which then will be returned.

I'm not that familiar with the matomo routing so if there is a better way to do this please tell me.

@diosmosis
Copy link
Member

diosmosis commented May 17, 2018 •
edited

I don't think using a special URL that doesn't actually work in matomo (ie, the ?module=CoreAdminHome&action=getOptOutJs) as a sort of message passing between matomo and the tracker-proxy is a good idea. It's not really clear from looking at the matomo code that this is what that special URL is for and it couples two entire repos together.

This should ideally be handled entirely in the tracker-proxy (so matomo doesn't have to know the tracker-proxy exists). Something like, in the tracker-proxy for matomo-proxy.php, rewriting URLs to JS/CSS files in the output to point to matomo-proxy.php instead (eg, tracker-proxy/matomo-proxy.php?file=plugins/CoreAdminHome/javascripts/optOut.js). Would probably need to add a new global variable for this that is set in matomo-proxy.php & handled in proxy.php.

@mattab mattab closed this May 23, 2018
@Skywalker-11
Copy link
Contributor Author

Skywalker-11 commented May 23, 2018 •
edited

@mattab Can you take an other look at the branches. I changed the proxy.php to replace the path in the content to point to the matomo-proxy.php?file=<path>.
https://github.com/Skywalker-11/tracker-proxy/commits/master
https://github.com/Skywalker-11/matomo/commits/optOutInlineScript

If that is ok with you I would create a new PR for the proxy and master

@tsteur
Copy link
Member

tsteur commented May 23, 2018

Was this closed by accident @mattab ?

@mattab mattab reopened this May 23, 2018
diosmosis
diosmosis reviewed May 28, 2018
View reviewed changes
Copy link
Member

@diosmosis diosmosis left a comment

Choose a reason for hiding this comment

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

Thanks for updating @Skywalker-11! There's an extra file that was added to the PR and tmp/.gitkeep shouldn't have it's file mode changed. I'll review the tracker-proxy changes next and we'll merge that first when everything's ready.

misc/user/.htaccess Outdated
Require all granted
</IfModule>
</IfModule>
</Files>
Copy link
Member

Choose a reason for hiding this comment

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

This file was added by accident I think.

@diosmosis
Copy link
Member

BTW, if you could create a PR for your tracker-proxy changes (which in general look good), that would be helpful.

@Skywalker-11
Copy link
Contributor Author

Yes files shouldn't be there. Maybe leftovers from setup or phpunit.
PR 42 of matomo-proxy referenced above should contain that changes. Do you want to reopen that PR or shall I create a new one?

@diosmosis
Copy link
Member

I'll reopen it.

@diosmosis diosmosis added this to the 3.6.0 milestone May 29, 2018
Skywalker-11 and others added 2 commits May 29, 2018 09:08
@Skywalker-11
revert unwanted changes of /tmp/.gitkeep and created .htaccess
c3ae647
@diosmosis
In optout form, clear new window closed check interval after new wind…
6588517 
…ow is closed.

Otherwise, if reload takes more than 1s, the interval will run again and try another reload, cancelling the pending one. Which results in no reload occuring.
@diosmosis diosmosis merged commit 5524432 into matomo-org:3.x-dev May 30, 2018
@Skywalker-11 Skywalker-11 deleted the optOutInlineScript branch May 30, 2018 09:10
@mattab mattab added the Bug For errors / faults / flaws / inconsistencies etc. label Aug 28, 2018
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
@Skywalker-11 @InfinityVoid
On opt-out page move js to separate file to allow not specifying inli…
a6cf0ce 
…ne-script CSP (matomo-org#12873)

* on opt-out page move js to separate file to fix inline-script (CSP)

* Compatibility of separate optOutJs with tracker-proxy

* remove destinction between proxy and normal request

* revert unwanted changes of /tmp/.gitkeep and created .htaccess

* In optout form, clear new window closed check interval after new window is closed.

Otherwise, if reload takes more than 1s, the interval will run again and try another reload, cancelling the pending one. Which results in no reload occuring.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diosmosis diosmosis diosmosis left review comments

Assignees
No one assigned
Labels
Bug For errors / faults / flaws / inconsistencies etc.
Projects
None yet
Milestone
3.6.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Skywalker-11 @tsteur @diosmosis @mattab