fix topic special tags #4031

lunny · 2018-05-24T05:24:05Z

This will filter special tags on topics.

codecov-io · 2018-05-24T05:33:01Z

Codecov Report

❗ No coverage uploaded for pull request base (master@11da1e6). Click here to learn what that means.
The diff coverage is 57.14%.

@@            Coverage Diff            @@
##             master    #4031   +/-   ##
=========================================
  Coverage          ?   19.97%           
=========================================
  Files             ?      153           
  Lines             ?    30484           
  Branches          ?        0           
=========================================
  Hits              ?     6090           
  Misses            ?    23480           
  Partials          ?      914

Impacted Files	Coverage Δ
models/topic.go	`54.78% <57.14%> (ø)`

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 11da1e6...d87510a. Read the comment docs.

jonasfranz · 2018-05-24T10:50:04Z

If I add the topic "><iframe><div class=" it will result in:

If I try to remove the same topic which I added before your PR will result in not removing it.
In the console this error appears:
Error: Syntax error, unrecognized expression: [data-value=""><iframe><div class=""] jquery.min.js:2:12731

axifive · 2018-06-10T03:13:17Z

I think for topics we need to add restrictions on the used symbols like GitHub topics and StackOverflow tags.

Rules:

All topics are in lowercase
Can be used only [a-z0-9_+.#-] symbols
Length limitation - {1, 35} chars
Number of topics added to the repository - max 25 topics
... maybe something else

techknowlogick

I'm approving this because it gets rid of the security issue. A different PR can be created later to limit what can be in a tag.

bkcsoft

Only a few nit-picks

bkcsoft · 2018-06-15T14:00:50Z

models/topic.go

@@ -113,7 +118,8 @@ func SaveTopics(repoID int64, topicNames ...string) error {

 	var addedTopicNames []string
 	for _, topicName := range topicNames {
-		if strings.TrimSpace(topicName) == "" {
+		topicName = validTopic(topicName)
+		if topicName == "" {


if topicName = validTopic(topicName); len(topicName) == 0 {

bkcsoft · 2018-06-15T14:01:00Z

models/topic.go

@@ -133,6 +139,11 @@ func SaveTopics(repoID int64, topicNames ...string) error {
 	for _, t := range topics {
 		var found bool
 		for _, topicName := range topicNames {
+			topicName = validTopic(topicName)
+			if topicName == "" {


same as above

bkcsoft · 2018-06-15T14:01:48Z

public/js/index.js

@@ -2301,12 +2301,23 @@ function initNavbarContentToggle() {
    });
 }

+function pasteFilter(e) {


What does this magical function do? Mind adding that to a comment above the function? :)

It replaces all tags (substrings on the form <...>) with a space, " " when a user pastes, but I agree there should be a comment.

jonasfranz · 2018-06-15T17:15:35Z

@techknowlogick I cannot follow you because the security issue is not resolved as I stated in a previous comment. If you think that this will resolve the problem, let me know it!

Edit: It actually resolves the security problem but introduces a bug as I mentioned in the previous comment.

techknowlogick · 2018-06-15T17:50:50Z

@JonasFranzDEV yes. I give LG-TM because bug happens when a user has the "'s in their tag (at least that's the only way I can trigger it in my build of this branch), which hopefully is an uncommon character for a tag. This is why I am ok with the PR as it resolves the security, and still improves upon existing behavior.

~~It is almost my lunch hour, and so I might be able to send a patch to Lunny.~~ Sadly I wasn't able to accomplish this.

techknowlogick · 2018-06-15T18:05:23Z

@JonasFranzDEV what if in the validTopic function, the list of conditions that @axifive made were added?

axifive · 2018-06-15T18:42:55Z

@techknowlogick, I'm working on the full solution with front/back validation. And I also found repo topics duplication bug. I'll try to finish this on the weekend.

Signed-off-by: Alexey Terentyev <[email protected]>

lunny · 2018-06-17T01:48:26Z

closed by #4258

* Added topics validation, fixed repo topics duplication (#4031) Signed-off-by: Alexey Terentyev <[email protected]> * Added tests Signed-off-by: Alexey Terentyev <[email protected]> * Fixed fmt Signed-off-by: Alexey Terentyev <[email protected]> * Added comments to exported functions Signed-off-by: Alexey Terentyev <[email protected]> * Deleted RemoveDuplicateTopics function Signed-off-by: Alexey Terentyev <[email protected]> * Fixed messages Signed-off-by: Alexey Terentyev <[email protected]> * Added migration Signed-off-by: Alexey Terentyev <[email protected]> * fmt migration file Signed-off-by: Alexey Terentyev <[email protected]> * fixed lint Signed-off-by: Alexey Terentyev <[email protected]> * Added Copyright Signed-off-by: Alexey Terentyev <[email protected]> * Added query solution for duplicates Signed-off-by: Alexey Terentyev <[email protected]> * Fixed migration query Signed-off-by: Alexey Terentyev <[email protected]> * Changed RegExp. Fixed migration Signed-off-by: Alexey Terentyev <[email protected]> * fmt migration file Signed-off-by: Alexey Terentyev <[email protected]> * Fixed test for changed regexp Signed-off-by: Alexey Terentyev <[email protected]> * Removed validation log messages Signed-off-by: Alexey Terentyev <[email protected]> * Renamed migration file Signed-off-by: Alexey Terentyev <[email protected]> * Renamed validate function Signed-off-by: Alexey Terentyev <[email protected]>

fix topic special tag

d87510a

lunny added type/bug topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! labels May 24, 2018

lunny added this to the 1.5.0 milestone May 24, 2018

bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 24, 2018

techknowlogick approved these changes Jun 15, 2018

View reviewed changes

bkcsoft 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 15, 2018

bkcsoft suggested changes Jun 15, 2018

View reviewed changes

axifive added a commit to axifive/gitea that referenced this pull request Jun 16, 2018

Added topics validation, fixed repo topics duplication (go-gitea#4031)

42096c6

Signed-off-by: Alexey Terentyev <[email protected]>

axifive mentioned this pull request Jun 16, 2018

Fix topics addition (Another solution) (#4031) #4258

Merged

lunny removed this from the 1.5.0 milestone Jun 17, 2018

lunny closed this Jun 17, 2018

lunny deleted the lunny/fix_topic branch June 17, 2018 01:49

go-gitea locked and limited conversation to collaborators Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix topic special tags #4031

fix topic special tags #4031

lunny commented May 24, 2018

codecov-io commented May 24, 2018

jonasfranz commented May 24, 2018

axifive commented Jun 10, 2018 •

edited

Loading

techknowlogick left a comment

bkcsoft left a comment

bkcsoft Jun 15, 2018

bkcsoft Jun 15, 2018

bkcsoft Jun 15, 2018 •

edited

Loading

techknowlogick Jun 15, 2018

jonasfranz commented Jun 15, 2018 •

edited

Loading

techknowlogick commented Jun 15, 2018 •

edited

Loading

techknowlogick commented Jun 15, 2018

axifive commented Jun 15, 2018 •

edited

Loading

lunny commented Jun 17, 2018 •

edited

Loading

fix topic special tags #4031

fix topic special tags #4031

Conversation

lunny commented May 24, 2018

codecov-io commented May 24, 2018

Codecov Report

jonasfranz commented May 24, 2018

axifive commented Jun 10, 2018 • edited Loading

techknowlogick left a comment

Choose a reason for hiding this comment

bkcsoft left a comment

Choose a reason for hiding this comment

bkcsoft Jun 15, 2018

Choose a reason for hiding this comment

bkcsoft Jun 15, 2018

Choose a reason for hiding this comment

bkcsoft Jun 15, 2018 • edited Loading

Choose a reason for hiding this comment

techknowlogick Jun 15, 2018

Choose a reason for hiding this comment

jonasfranz commented Jun 15, 2018 • edited Loading

techknowlogick commented Jun 15, 2018 • edited Loading

techknowlogick commented Jun 15, 2018

axifive commented Jun 15, 2018 • edited Loading

lunny commented Jun 17, 2018 • edited Loading

axifive commented Jun 10, 2018 •

edited

Loading

bkcsoft Jun 15, 2018 •

edited

Loading

jonasfranz commented Jun 15, 2018 •

edited

Loading

techknowlogick commented Jun 15, 2018 •

edited

Loading

axifive commented Jun 15, 2018 •

edited

Loading

lunny commented Jun 17, 2018 •

edited

Loading