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 invalid Content-Disposition #357

Merged
merged 3 commits into from
Jul 21, 2022

Conversation

spinillos
Copy link
Member

@spinillos spinillos commented Jul 13, 2022

It uses content-disposition library to accept non-ascii characters in Content-Disposition header. Right now its failing when it's happening and we aren't creating the csv.

Related issue: https://github.com/grafana/grafana-enterprise/issues/3576

@CLAassistant
Copy link

CLAassistant commented Jul 13, 2022

CLA assistant check
All committers have signed the CLA.

@AgnesToulet
Copy link
Contributor

To give a bit more context on this header, it's useful when having template variables in panel titles. Grafana backend can't interpolate template variables so if we send an empty file name, it will work fine for panel titles that don't contain template variable but will show something like My panel ${myVariable}-2022-07-19.csv when there is a variable.

Also, I noticed the regex you used is a bit too broad. I couldn't find a rule but not all non-ascii characters are causing the error, german characters like in Rückgängig are working fine.

However, when researching for the characters that were actually breaking the Content-Disposition header, I found out that there was a library made to handle this header: https://www.npmjs.com/package/content-disposition

I tested this change and it seems to work fine:
image

What do you think?

@spinillos
Copy link
Member Author

Thanks Agnès!

I was more scared about breaking things and I didn't notice about variables in titles. I'm going to test it.

@@ -234,7 +235,7 @@ export class HttpServer {
const result = await this.browser.renderCSV(options);

if (result.fileName) {
res.setHeader('Content-Disposition', `attachment; filename="${result.fileName}"`);
res.setHeader('Content-Disposition', contentDisposition(result.fileName));
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the file name is expected to contains special characters - will they be simply removed?

@ArturWierzbicki ArturWierzbicki self-requested a review July 21, 2022 10:22
@spinillos spinillos merged commit 4aaf2f9 into master Jul 21, 2022
@spinillos spinillos deleted the encode-content-disposition-filename branch July 21, 2022 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants