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
Only allow widgetize and token usage for view users #16263
Conversation
build is failing, looks like this has some other side effects |
@diosmosis tests should pass now |
core/FrontController.php
Outdated
Request::reloadAuthUsingTokenAuth(); | ||
if (($module !== 'API' || ($action && $action !== 'index')) && Piwik::isUserHasSomeWriteAccess() && !Common::isPhpCliMode()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be a good idea to move ($module !== 'API' || ($action && $action !== 'index'))
and && !Common::isPhpCliMode()
to another method? something like isTokenAuthLimitedToWriteAccess()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 changed it. will merge if tests pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left one comment, otherwise looks good to me
@tsteur Can you summarize what security impact this has, or what privileges can be gained by exploiting this? |
@attritionorg I assume you refer to when someone uses the token of a write, admin or super user? If the widgetized URL is shared with other users and that URL includes the token, then people who that token is shared with would gain the same privileges over the API. As a best practice we're now enforcing to use only tokens for view users. So if someone was to embed the widget into an internal wiki or so for all employees then they only have "view" access. Not sure if this roughly answers the question? |
I think it does, thank you @tsteur. The initial language in this PR made it sound like this could be an exploitable security issue so was looking for clarity. |
fyi added mention in the guide: https://matomo.org/docs/embed-matomo-reports/
|
Not really needed this change I guess but might be better to allow authentication using tokens only for users when the used token has only view access. This may be helpful if otherwise a user tries to embed some screens using token_auth.
Should there be any regressions that we can't fix we could undo it again.