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

Add simple xlsx file render support #4106

Closed
wants to merge 2 commits into from

Conversation

lunny
Copy link
Member

@lunny lunny commented Jun 3, 2018

partial fix #4104

image

@lunny lunny added type/proposal The new feature has not been accepted yet but needs to be discussed first. type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels Jun 3, 2018
@codecov-io
Copy link

Codecov Report

Merging #4106 into master will increase coverage by <.01%.
The diff coverage is 8%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4106      +/-   ##
==========================================
+ Coverage   19.97%   19.97%   +<.01%     
==========================================
  Files         153      153              
  Lines       30494    30497       +3     
==========================================
+ Hits         6091     6093       +2     
- Misses      23489    23490       +1     
  Partials      914      914
Impacted Files Coverage Δ
routers/repo/view.go 0% <0%> (ø) ⬆️
modules/markup/sanitizer.go 84% <100%> (+1.39%) ⬆️

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 fb1daad...e37414d. Read the comment docs.

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 3, 2018
@@ -0,0 +1,84 @@
// Copyright 20178 The Gitea Authors. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

typo, 2018

@lafriks lafriks added this to the 1.6.0 milestone Jun 4, 2018
if i == 0 {
active = "active"
}
tmpBlock.WriteString(fmt.Sprintf(`<a class="%s item" data-tab="%d">%s</a>`, active, i, sheetMap[i+1]))
Copy link

Choose a reason for hiding this comment

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

Shouldn't sheetMap[i+1] be escaped?

@jonasfranz
Copy link
Member

IMHO a xlsx render may be out of scope of the gitea project. Could this be implemented as external renderer? This would reduce the amount of maintenance required.

@lafriks
Copy link
Member

lafriks commented Jul 5, 2018

It's actually quite small code on Gitea side so I would not mind it being included

@jvoisin
Copy link

jvoisin commented Jul 5, 2018

Well, it's adding an external library as a dependency, increasing the attack surface of gitea by several thousands line of code.

@lafriks
Copy link
Member

lafriks commented Jul 5, 2018

It should not be increasing attack vector as it just reads file. Stack/heap overflows should not be possible in Go and used lib is pure go code so it should be quite safe

@jvoisin
Copy link

jvoisin commented Jul 5, 2018

It might add XSS, CSRF, arbitrary file inclusion, …

And yes, you can get arbitrary remote code execution via go, even in its stdlib :)

@lafriks
Copy link
Member

lafriks commented Jul 5, 2018

There is risk but as we are using same sanitizer as markdown it should not be too high to not implement this feature

@jvoisin
Copy link

jvoisin commented Jul 5, 2018

Currently it's not.

But to be honest, I'm more worried about adding an other repository for an attacker to take over to introduce malicious code into gitea. But I guess that it's how go is working 🤷‍♂️

@lafriks
Copy link
Member

lafriks commented Jul 5, 2018

@jvoisin I was talking about library not current PR code. What I was trying to say that library code should not be problem in this case, current PRs code of course needs to be fixed in multiple places.

@bkcsoft
Copy link
Member

bkcsoft commented Jul 8, 2018

I'm with @jvoisin on this one... At least put this behind a build tag as it might introduce attack-vectors 😐 (In before we need a build-service like Caddy 😂 )

@techknowlogick techknowlogick modified the milestones: 1.6.0, 1.7.0 Aug 28, 2018
@techknowlogick
Copy link
Member

Closing due to external rendering available. Re-open if needed.

@lafriks lafriks removed this from the 1.7.0 milestone Oct 30, 2018
@lunny lunny deleted the lunny/render_xlsx branch November 18, 2020 04:34
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/proposal The new feature has not been accepted yet but needs to be discussed first.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default Excel/CSV files render supports
8 participants