-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Sanitation fix from Gogs #1461
Sanitation fix from Gogs #1461
Conversation
LGTM |
LGTM |
modules/markdown/sanitizer.go
Outdated
|
||
"github.com/microcosm-cc/bluemonday" | ||
|
||
"github.com/gogits/gogs/modules/setting" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
github.com/go-gitea/gitea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually "code.gitea.io/gitea/modules/setting"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted
modules/markdown/sanitizer_test.go
Outdated
import ( | ||
"testing" | ||
|
||
. "github.com/smartystreets/goconvey/convey" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
goconvey has been removed before. I think change this test file to github.com/stretchr/testify
is better
I'm getting nil-panics in some tests 🙄 |
modules/markdown/sanitizer.go
Outdated
@@ -48,10 +48,19 @@ func NewSanitizer() { | |||
|
|||
// Sanitize takes a string that contains a HTML fragment or document and applies policy whitelist. | |||
func Sanitize(s string) string { | |||
if sanitizer == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a race problem?
modules/markdown/sanitizer.go
Outdated
|
||
"github.com/microcosm-cc/bluemonday" | ||
|
||
"code.gitea.io/gitea/modules/setting" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merge the gitea internal packages.
|
||
// Sanitize takes a string that contains a HTML fragment or document and applies policy whitelist. | ||
func Sanitize(s string) string { | ||
if sanitizer.policy == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about
func init() {
NewSanitizer(0
}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't wanna initialize it unless we need to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this maybe a race problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It uses sync.Once
so there's no race-condition that I can see...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
let L-G-T-M work |
@bkcsoft could you send a backport to v1.1.1? |
No description provided.