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 HTTP headers for issue attachment download #270

Merged
merged 2 commits into from
Nov 27, 2016
Merged

Fix HTTP headers for issue attachment download #270

merged 2 commits into from
Nov 27, 2016

Conversation

andreynering
Copy link
Contributor

This fixes:

  • Download filename was wrong for files other than images. Example: It was download instead of file.pdf
  • PDF was downloading instead of showing on browser

@andreynering andreynering added the type/enhancement An improvement of existing functionality label Nov 26, 2016
@andreynering andreynering added this to the 1.0.0 milestone Nov 26, 2016
- Download filename was wrong for files other than images. Example: It was `download` instead of `file.pdf`
- PDF was downloading instead of showing on browser
// Fix #312. Attachments with , in their name are not handled correctly by Google Chrome.
// We must put the name in " manually.
if err = repo.ServeData(ctx, "\""+attach.Name+"\"", fr); err != nil {
if err = repo.ServeData(ctx, attach.Name, fr); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Does this re-introduce the referenced issue ?
gogs/gogs#312

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the quote is preserved here and here

Copy link
Member

Choose a reason for hiding this comment

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

Did you test the issue explained in gogits#312 ?
I'm not sure the bug was related to the quotes in Content-Disposition rather than in repo.ServeData argument

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... In fact there's a problem, thanks for checking.

But I don't understand why it happen. I just moved the quotes, but it's there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@strk It's fixed now. It's was not actually fixed before, because the header was not actually set for these cases.

ctx.Resp.Header().Set("Content-Transfer-Encoding", "binary")
}
} else if !ctx.QueryBool("render") {
ctx.Resp.Header().Set("Cache-Control", "public,max-age=86400")
Copy link
Member

Choose a reason for hiding this comment

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

This cache lifetime change seems to be unrelated to the scope of this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The cache was there before, I just moved it to another place

Copy link
Member

Choose a reason for hiding this comment

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

Ah, now I see, thanks for double-checking

ctx.Resp.Header().Set("Cache-Control", "public,max-age=86400")

// Google Chrome dislike commas in filenames, so let's change it to a space
name = strings.Replace(name, ",", " ", -1)
Copy link
Member

Choose a reason for hiding this comment

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

I dislike spaces in filenames, how about a dash ? :)
Actually, how about a sanitizing function to generalize the issue ? Like, I'm thinking slashes might be impractical to have...

Choose a reason for hiding this comment

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

i like underscores....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, how about a sanitizing function to generalize the issue ? Like, I'm thinking slashes might be impractical to have...

I did research on this and tried few approaches. Unfortunately there isn't how to sanitize it, it's actually a Chrome bug. And only commas trigger this AFAIK.

I don't have strong feeling for comma vs space.

@strk
Copy link
Member

strk commented Nov 26, 2016 via email

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 26, 2016
@strk
Copy link
Member

strk commented Nov 26, 2016

LGTM - the systems I use don't choke on spaces (it's just me, but I can blame the people who attach files with spaces or commas in their filenames)

@tboerger tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 26, 2016
@thibaultmeyer
Copy link
Contributor

LGTM

@tboerger tboerger added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Nov 27, 2016
@andreynering andreynering merged commit c664ffd into go-gitea:master Nov 27, 2016
@andreynering andreynering deleted the gitea/http-headers-download branch November 27, 2016 10:48
@tboerger tboerger removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Nov 27, 2016
richmahn added a commit to richmahn/gitea that referenced this pull request Apr 11, 2019
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants