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 possible renderer security problem(#30136) #30315

Merged
merged 3 commits into from
Apr 8, 2024
Merged

Fix possible renderer security problem(#30136) #30315

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1adb00d
Refactor render (#30136)
wxiaoguang Mar 27, 2024
5b2d7b6
Add frame-ancestors 'self'
lunny Apr 8, 2024
07e1ba9
Revert "Add frame-ancestors 'self'"
lunny Apr 8, 2024
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
15 changes: 7 additions & 8 deletions routers/web/repo/render.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"code.gitea.io/gitea/modules/charset"
"code.gitea.io/gitea/modules/context"
"code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/markup"
"code.gitea.io/gitea/modules/typesniffer"
"code.gitea.io/gitea/modules/util"
Expand Down Expand Up @@ -44,20 +45,17 @@ func RenderFile(ctx *context.Context) {
isTextFile := st.IsText()

rd := charset.ToUTF8WithFallbackReader(io.MultiReader(bytes.NewReader(buf), dataRc), charset.ConvertOpts{})
ctx.Resp.Header().Add("Content-Security-Policy", "frame-src 'self'; sandbox allow-scripts")
Copy link
Member

Choose a reason for hiding this comment

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

Should we also set frame-ancestors 'self'?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so. Maybe we can also update the original PR. Please send commits to this PR directly.

Copy link
Member

Choose a reason for hiding this comment

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

I think we likely want to set only frame-ancestors, e.g. control where this page is embedded, not which pages it can embed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I suggest setting both until someone needs that feature. Currently I don't think anyone has used that feature.

Copy link
Member

Choose a reason for hiding this comment

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

Assuming we want to backport this, I'd suggest doing the frame-ancestors change only on v1.23, so this is fine with me without.

Copy link
Member Author

Choose a reason for hiding this comment

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

5b2d7b6

Copy link
Member

@silverwind silverwind Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My only concern about this is in case someone uses this renderer on external websites. Do we support this? I think we shouldn't but not sure if we really should prevent this on the stable branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. Let's revert it. 07e1ba9


if markupType := markup.Type(blob.Name()); markupType == "" {
if isTextFile {
_, err = io.Copy(ctx.Resp, rd)
if err != nil {
ctx.ServerError("Copy", err)
}
return
_, _ = io.Copy(ctx.Resp, rd)
} else {
http.Error(ctx.Resp, "Unsupported file type render", http.StatusInternalServerError)
}
ctx.Error(http.StatusInternalServerError, "Unsupported file type render")
return
}

ctx.Resp.Header().Add("Content-Security-Policy", "frame-src 'self'; sandbox allow-scripts")
err = markup.Render(&markup.RenderContext{
Ctx: ctx,
RelativePath: ctx.Repo.TreePath,
Expand All @@ -71,7 +69,8 @@ func RenderFile(ctx *context.Context) {
InStandalonePage: true,
}, rd, ctx.Resp)
if err != nil {
ctx.ServerError("Render", err)
log.Error("Failed to render file %q: %v", ctx.Repo.TreePath, err)
http.Error(ctx.Resp, "Failed to render file", http.StatusInternalServerError)
return
}
}
Loading