Allow render HTML with css/js external links by lunny · Pull Request #19017 · go-gitea/gitea

lunny · 2022-03-07T12:01:15Z

This PR allows to render some HTMLs which has <style>, <script> tags when SANITIZER disabled. This also makes it simpler when configuring some external renderers.

techknowlogick

I'm concerned with this PR because someone could link to /user/repo/render/.... and have XSS execute. I know that they chose to do this by disabling SANITIZER, but this seems especially dangerous. If merged, we should at least create a big warning in docs, and perhaps in admin saying the config is insecure.

lunny · 2022-03-13T04:32:44Z

I'm concerned with this PR because someone could link to /user/repo/render/.... and have XSS execute. I know that they chose to do this by disabling SANITIZER, but this seems especially dangerous. If merged, we should at least create a big warning in docs, and perhaps in admin saying the config is insecure.

Warning added in the docs.

techknowlogick · 2022-05-03T19:56:30Z

Could we use Content Security Policy headers to prevent any XSS?

lunny · 2022-05-31T13:26:00Z

Could we use Content Security Policy headers to prevent any XSS?

Done.

modules/markup/renderer.go

routers/web/repo/render.go

routers/web/repo/view.go

routers/web/repo/render.go

Co-authored-by: delvh <dev.lh@web.de>

delvh

Apart from the deprecation issue where we have yet to find a solution LGTM.

modules/markup/renderer.go

routers/web/repo/render.go

delvh · 2022-06-14T13:49:10Z

#19017 (comment):

DISABLE_SANITIZER is not in 1.16. It's a good chance to make it clear.

So it is not breaking for users who are on a stable version, which should be almost everyone.

lunny · 2022-06-14T13:58:32Z

#19017 (comment):

DISABLE_SANITIZER is not in 1.16. It's a good chance to make it clear.

So it is not breaking for users who are on a stable version, which should be almost everyone.

If that, we need a migration and deprecated warning in setting.

wxiaoguang · 2022-06-14T14:39:01Z

Beside the option, still one more thing (sorry for bothering ...)

Now the iframe is using sandbox="allow-same-origin allow-scripts", then the rendered JS can access to the window.config which contains CSRF token, then the JS can make requests as the current users. Is it fine?

Details

update: fixed.

delvh · 2022-06-14T14:45:04Z

#19017 (comment):
Now we certainly know that we cannot allow that.

If we allow that, we have an obvious and unintended privilege escalation.
I am strongly against allowing the scripts now.

wxiaoguang · 2022-06-14T14:46:03Z

#19017 (comment): Now we certainly know that we cannot allow that.

If we allow that, we have an obvious and unintended privilege escalation. I am strongly against allowing the scripts now.

Only allow-scripts can be used here. I am working on it.

techknowlogick

temporarily blocking per my discussion w/ delvh

wxiaoguang · 2022-06-14T15:22:10Z

OK, all fixed. Now the iframe is protected by browser's same-origin policy. @techknowlogick

Uncaught DOMException: Blocked a frame with origin "null" from accessing a cross-origin frame.

Refer: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe#attr-sandbox

If this token (allow-same-origin) is not used, the resource is treated as being from a special origin that always fails the same-origin policy (potentially preventing access to data storage/cookies and some JavaScript APIs).

custom/conf/app.example.ini

docs/content/doc/advanced/config-cheat-sheet.en-us.md

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

codecov-commenter · 2022-06-16T03:29:52Z

Codecov Report

Merging #19017 (c7df5bf) into main (7d1770c) will decrease coverage by 0.18%.
The diff coverage is 19.25%.

@@            Coverage Diff             @@
##             main   #19017      +/-   ##
==========================================
- Coverage   47.36%   47.18%   -0.19%     
==========================================
  Files         967      968       +1     
  Lines      134131   134224      +93     
==========================================
- Hits        63530    63332     -198     
- Misses      62876    63178     +302     
+ Partials     7725     7714      -11

Impacted Files	Coverage Δ
modules/markup/console/console.go	`38.46% <ø> (+2.74%)`	⬆️
modules/markup/csv/csv.go	`29.16% <ø> (+0.88%)`	⬆️
modules/markup/external/external.go	`1.28% <0.00%> (-0.04%)`	⬇️
modules/markup/markdown/markdown.go	`58.78% <ø> (-0.55%)`	⬇️
modules/markup/orgmode/orgmode.go	`52.88% <ø> (+0.99%)`	⬆️
modules/setting/markup.go	`4.90% <0.00%> (-0.72%)`	⬇️
routers/web/repo/compare.go	`46.95% <0.00%> (ø)`
routers/web/repo/render.go	`0.00% <0.00%> (ø)`
modules/markup/renderer.go	`57.80% <15.15%> (-10.48%)`	⬇️
routers/web/repo/view.go	`42.07% <73.68%> (+0.80%)`	⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d1770c...c7df5bf. Read the comment docs.

* giteaofficial/main: Allow render HTML with css/js external links (go-gitea#19017) Use correct count for `NumOpenIssues` (go-gitea#19980) In code search, get code unit accessible repos in one (main) query (go-gitea#19764) [skip ci] Updated translations via Crowdin Always try to fetch repo for mirrors (go-gitea#19975) Remove tab/TabName usage where it's not needed (go-gitea#19973) Fix cli command restore-repo: "units" should be parsed as StringSlice (go-gitea#19953) Uppercase first languages letters (go-gitea#19965) Move tests as seperate sub packages to reduce duplicated file names (go-gitea#19951) Replace unstyled meter with progress (go-gitea#19968) [skip ci] Updated translations via Crowdin [skip ci] Updated translations via Crowdin Remove singuliere from MAINTAINERS (go-gitea#19883) Fix aria for logo (go-gitea#19955) Fix mirror template bug (go-gitea#19959)

* Allow render HTML with css/js external links * Fix bug because of filename escape chars * Fix lint * Update docs about new configuration item * Fix bug of render HTML in sub directory * Add CSP head for displaying iframe in rendering file * Fix test * Apply suggestions from code review Co-authored-by: delvh <dev.lh@web.de> * Some improvements * some improvement * revert change in SanitizerDisabled of external renderer * Add sandbox for iframe and support allow-scripts and allow-same-origin * refactor * fix * fix lint * fine tune * use single option RENDER_CONTENT_MODE, use sandbox=allow-scripts * fine tune CSP * Apply suggestions from code review Co-authored-by: wxiaoguang <wxiaoguang@gmail.com> Co-authored-by: delvh <dev.lh@web.de> Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

It was always attempted to read the file to be rendered as UTF-8. The encoding was determined heuristically, regardless of the file type. Sometimes binary files were not recognized as binary. Renderers of binary formats were then fed with corrupted data: binary stream treated as text and encoded in UTF-8. Only apply heuristics for textual formats, read other formats as-is. Fixes: b01dce2 ("Allow render HTML with css/js external links (go-gitea#19017)") Cc: xiaolunwen@gmail.com Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

lunny added the type/enhancement An improvement of existing functionality label Mar 7, 2022

6543 added this to the 1.17.0 milestone Mar 7, 2022

lunny force-pushed the lunny/render_iframe branch from d363a7f to 2f4b6cb Compare March 7, 2022 23:09

techknowlogick requested changes Mar 8, 2022

View reviewed changes

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 8, 2022

lunny requested review from delvh, techknowlogick and zeripath April 8, 2022 01:44

lunny force-pushed the lunny/render_iframe branch from ffb905f to 1a908ce Compare May 31, 2022 13:01

lunny added 7 commits June 1, 2022 14:55

Allow render HTML with css/js external links

581e415

Fix bug because of filename escape chars

c727f99

Fix lint

b04430d

Update docs about new configuration item

d980996

Fix bug of render HTML in sub directory

6e012d9

Add CSP head for displaying iframe in rendering file

7a4a58f

Fix test

c3bfd5b

lunny force-pushed the lunny/render_iframe branch from 447316b to c3bfd5b Compare June 1, 2022 06:59

lunny requested review from lafriks and wxiaoguang June 1, 2022 11:42

merge

7ec5ac6

delvh reviewed Jun 11, 2022

View reviewed changes

Apply suggestions from code review

2006683

Co-authored-by: delvh <dev.lh@web.de>

delvh approved these changes Jun 11, 2022

View reviewed changes

GiteaBot 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 Jun 11, 2022

wxiaoguang reviewed Jun 12, 2022

View reviewed changes

modules/markup/renderer.go Outdated Show resolved Hide resolved

wxiaoguang reviewed Jun 12, 2022

View reviewed changes

routers/web/repo/render.go Outdated Show resolved Hide resolved

techknowlogick requested changes Jun 14, 2022

View reviewed changes

use single option RENDER_CONTENT_MODE, use sandbox=allow-scripts

d9679ce

fine tune CSP

a1cac2a

wxiaoguang requested a review from techknowlogick June 15, 2022 01:05

lunny commented Jun 15, 2022

View reviewed changes

custom/conf/app.example.ini Outdated Show resolved Hide resolved

wxiaoguang reviewed Jun 15, 2022

View reviewed changes

docs/content/doc/advanced/config-cheat-sheet.en-us.md Outdated Show resolved Hide resolved

lunny and others added 4 commits June 15, 2022 11:16

Apply suggestions from code review

cc18ebf

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

Merge branch 'main' into lunny/render_iframe

65443b2

Merge branch 'main' into lunny/render_iframe

6c81597

Merge branch 'main' into lunny/render_iframe

631fea3

techknowlogick approved these changes Jun 15, 2022

View reviewed changes

wxiaoguang and others added 2 commits June 15, 2022 23:59

Merge branch 'main' into lunny/render_iframe

169362d

Merge branch 'main' into lunny/render_iframe

c7df5bf

lunny merged commit b01dce2 into go-gitea:main Jun 16, 2022

lunny deleted the lunny/render_iframe branch June 16, 2022 03:33

wxiaoguang mentioned this pull request Jun 16, 2022

README rendering broken #19988

Closed

wxiaoguang mentioned this pull request Jul 5, 2022

Don't strip HTML classes/style from custom markup #4935

Closed

7 tasks

editfund-founder mentioned this pull request Apr 16, 2023

[FEAT] + View MarkMap [Solved] #24139

Closed

go-gitea locked and limited conversation to collaborators May 3, 2023

Uh oh!

Conversation

lunny commented Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

techknowlogick left a comment

Choose a reason for hiding this comment

Uh oh!

lunny commented Mar 13, 2022

Uh oh!

techknowlogick commented May 3, 2022

Uh oh!

lunny commented May 31, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

delvh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

delvh commented Jun 14, 2022

Uh oh!

lunny commented Jun 14, 2022

Uh oh!

wxiaoguang commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

delvh commented Jun 14, 2022

Uh oh!

wxiaoguang commented Jun 14, 2022

Uh oh!

techknowlogick left a comment

Choose a reason for hiding this comment

Uh oh!

wxiaoguang commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Jun 16, 2022

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

lunny commented Mar 7, 2022 •

edited

Loading

delvh left a comment •

edited

Loading

wxiaoguang commented Jun 14, 2022 •

edited

Loading

wxiaoguang commented Jun 14, 2022 •

edited

Loading