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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
638dd24
Fix HTTP headers for issue attachment download
andreynering Nov 26, 2016
d647d02
Fix Chrome not liking commas
andreynering Nov 26, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions cmd/web.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,11 +336,7 @@ func runWeb(ctx *cli.Context) error {
}
defer fr.Close()

ctx.Header().Set("Cache-Control", "public,max-age=86400")
ctx.Header().Set("Content-Disposition", fmt.Sprintf(`inline; filename="%s"`, attach.Name))
// 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.Handle(500, "ServeData", err)
return
}
Expand Down
20 changes: 13 additions & 7 deletions routers/repo/download.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@
package repo

import (
"fmt"
"io"
"path"
"strings"

"code.gitea.io/git"

Expand All @@ -22,14 +23,19 @@ func ServeData(ctx *context.Context, name string, reader io.Reader) error {
buf = buf[:n]
}

if !base.IsTextFile(buf) {
if !base.IsImageFile(buf) {
ctx.Resp.Header().Set("Content-Disposition", "attachment; filename=\""+path.Base(ctx.Repo.TreePath)+"\"")
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


// 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...

Copy link

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.


if base.IsTextFile(buf) || ctx.QueryBool("render") {
ctx.Resp.Header().Set("Content-Type", "text/plain; charset=utf-8")
} else if base.IsImageFile(buf) || base.IsPDFFile(buf) {
ctx.Resp.Header().Set("Content-Disposition", fmt.Sprintf(`inline; filename="%s"`, name))
} else {
ctx.Resp.Header().Set("Content-Disposition", fmt.Sprintf(`attachment; filename="%s"`, name))
}

ctx.Resp.Write(buf)
_, err := io.Copy(ctx.Resp, reader)
return err
Expand Down