Submit report download link as a post request #14351
Conversation
| @@ -75,12 +75,13 @@ | |||
| </td> | |||
| <td> | |||
| {# download link #} | |||
| <a href="{{ linkTo({'module':'API', 'segment': null, 'token_auth':token_auth, | |||
| <a href="#" | |||
There was a problem hiding this comment.
@katebutler random thought to prevent possible XSS etc... we could have for each report a hidden form like <form action="index.php?module=API&..."><input name=token_auth type=hidden value="..." style="display:none"></form> and then simply submit the hidden form when someone clicks on the link? see https://stackoverflow.com/questions/18406732/using-jquery-to-submit-form-from-link-outside-of-form
This way Twig can do all of the complicated escaping and we can still use action="{{linkTo(...)}}
There was a problem hiding this comment.
re the href="#"... I'm not sure if it's still an issue (not tested), but I think it may cause a scroll to the top or so under circumstances (maybe always, or when before a different anchor was active).
I think we usually use href="javascript:void(0)" for this reason.
|
|
||
| // Build and submit the form | ||
| // The target="_blank" only works if the form's id attribute is also set | ||
| $('<form action="' + basePath + '" method="POST" target="_blank" id="downloadReportForm"></form>') |
There was a problem hiding this comment.
be good to set basePath also through attr() otherwise we may run into XSS again (not sure that this would fully prevent it though)
| 'format': (report.format in ['html', 'csv']) ? report.format : false }) }}" | ||
| method="POST" | ||
| target="_blank" | ||
| id="downloadReportForm_{{ report.idreport }}" |
There was a problem hiding this comment.
haven't tested it yet but code looks good overall. It is not really needed since we know the id will be a number but just to be safe in HTML attributes we escape the output of twig variables using html_attr like this:
{{ report.idreport|e('html_attr') }}
instead of the standard escaping functionality (the standard escaping isn't safe for attributes). Not really needed when we know it's a number, but good to have eg if someone else copy/pastes it or changes it from an ID to something else etc.
| target="_blank" | ||
| id="downloadReportForm_{{ report.idreport }}" | ||
| > | ||
| <input type="hidden" name="token_auth" value="{{ token_auth }}"> |
There was a problem hiding this comment.
same here, in theory it will always be an MD5 and safe. but just to be extra safe be good to use {{ token_auth|e('html_attr') }}
|
I'm pretty confused right now :) For some reason I'm getting the error
Where it expects the report to have a https://github.com/matomo-org/matomo/blob/3.x-dev/plugins/ScheduledReports/Model.php#L74 So wondering how this ever worked? There's something I'm not seeing. Any idea @katebutler @diosmosis ? It's not related to this PR as the PR didn't change anything. Just my scheduled reports seem to play up? |
|
I looked into the code, but I have no idea why it would work. It does work on demo.matomo.org though, so it must've been a recent regression (though the name code has been around for a long time). |
|
@tsteur Oh, I see it's not actually the scheduled report's name, but the individual report in the list of reports to include in the scheduled report (like "Visits Over Time"). |
|
thanks very much @diosmosis that's it! I think it's actually cohorts that doesn't have a name defined, might be an old version not sure...
disabled the plugin and works now |
|
Probably Cohorts as I was affected too, I'll add it. |
|
Works 👍 Will merge now @katebutler |
mattab
added
the
not-in-changelog
Fixes #12721