Fix possible renderer security problem(#30136) #30315
Conversation
GiteaBot
added
the
lgtm/need 2
pull-request-size
bot
added
the
size/S
GiteaBot
added
lgtm/need 1
lunny
changed the title
| @@ -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") | |||
There was a problem hiding this comment.
Should we also set frame-ancestors 'self'?
There was a problem hiding this comment.
I think so. Maybe we can also update the original PR. Please send commits to this PR directly.
There was a problem hiding this comment.
I think we likely want to set only frame-ancestors, e.g. control where this page is embedded, not which pages it can embed.
There was a problem hiding this comment.
I suggest setting both until someone needs that feature. Currently I don't think anyone has used that feature.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
OK. Let's revert it. 07e1ba9
GiteaBot
added
lgtm/done
lunny
added
the
reviewed/wait-merge
This reverts commit 5b2d7b6.
GiteaBot
removed
the
reviewed/wait-merge
wxiaoguang
added
the
topic/security
backport #30136