Skip to content

Fixes possible vulnerabilities with keyword hijacking#20

Merged
thibaultmeyer merged 3 commits into
masterfrom
unknown repository
Nov 12, 2016
Merged

Fixes possible vulnerabilities with keyword hijacking#20
thibaultmeyer merged 3 commits into
masterfrom
unknown repository

Conversation

@ghost

@ghost ghost commented Nov 3, 2016

Copy link
Copy Markdown

This fixes #3700. Apparently nothing in the public/ directory is actually filtered out from possible usernames, which means we can have try.gogs.io/css as a possible username. This could be quite dangerous in terms of XSS or some other exploit.

Also @unknwon, how did you derp so hard in variable naming? reversed? really?

@tboerger tboerger closed this Nov 3, 2016
@tboerger

tboerger commented Nov 3, 2016

Copy link
Copy Markdown
Member

Please reopen against master.

@bkcsoft bkcsoft changed the base branch from develop to master November 4, 2016 04:11
@bkcsoft bkcsoft added type/bug topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! issue/critical This issue should be fixed ASAP. If it is a PR, the PR should be merged ASAP and removed reviewed/invalid topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! labels Nov 4, 2016
@bkcsoft bkcsoft reopened this Nov 4, 2016
@bkcsoft

bkcsoft commented Nov 4, 2016

Copy link
Copy Markdown
Contributor

Quick side-note on this one, can't we use the router to check for collisions? instead of having a static (sometimes broken :trollface:) list?

@strk

strk commented Nov 4, 2016

Copy link
Copy Markdown
Member

Let's go there incrementally @bkcsoft -- bugfix is important.
Rather, @LefsFlarey, could you try adding a testcase for checking reserved username handling ?
Other than that, LGTM

@tboerger tboerger added this to the 1.0.0 milestone Nov 4, 2016
@bkcsoft

bkcsoft commented Nov 4, 2016

Copy link
Copy Markdown
Contributor

Same request as @strk, we need tests 😄

ping me when it's done and I'll LG_TM and Merge 😉

@bkcsoft

bkcsoft commented Nov 4, 2016

Copy link
Copy Markdown
Contributor

ooh, and rebase 😒

@ghost

ghost commented Nov 4, 2016

Copy link
Copy Markdown
Author

@strk What do you mean by 'testcase'?

@bkcsoft I don't think a rebase is necessary for a PR, since it merges just fine.

@codecov-io

codecov-io commented Nov 4, 2016

Copy link
Copy Markdown

Current coverage is 2.18% (diff: 0.00%)

Merging #20 into master will not change coverage

@@            master       #20   diff @@
========================================
  Files           31        31          
  Lines         7508      7508          
  Methods          0         0          
  Messages         0         0          
  Branches         0         0          
========================================
  Hits           164       164          
  Misses        7327      7327          
  Partials        17        17

Powered by Codecov. Last update 03902bb...427bd15

bkcsoft
bkcsoft reviewed Nov 4, 2016
View reviewed changes
Comment thread models/user.go
@@ -518,7 +518,7 @@ func isUsableName(names, patterns []string, name string) error {
}

func IsUsableUsername(name string) error {

@bkcsoft bkcsoft Nov 4, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This function can be tested 🙂

@ghost ghost Nov 6, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Wait, why does that even need testing? It's a very straightforward function.

@strk

strk commented Nov 4, 2016

Copy link
Copy Markdown
Member

On Fri, Nov 04, 2016 at 11:52:04AM -0700, LefsFlare wrote:

@strk What do you mean by 'testcase'?

See https://golang.org/pkg/testing/

@strk

strk commented Nov 6, 2016 via email

Copy link
Copy Markdown
Member

@thibaultmeyer

thibaultmeyer commented Nov 12, 2016

Copy link
Copy Markdown
Contributor

LGTM

@thibaultmeyer thibaultmeyer merged commit 3ef022b into go-gitea:master Nov 12, 2016
@tboerger

tboerger commented Nov 13, 2016

Copy link
Copy Markdown
Member

@0xBAADF00D stop that! You can't merge prs when there are pending requests for changes!!! Otherwise I will drop the rights for that.

@thibaultmeyer

thibaultmeyer commented Nov 13, 2016

Copy link
Copy Markdown
Contributor

ok @tboerger, I understand

@ghost ghost deleted the issue/3700 branch November 25, 2016 01:38
@tboerger tboerger added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Nov 29, 2016
@ghost ghost restored the issue/3700 branch December 11, 2016 05:39
lunny referenced this pull request in lunny/gitea Feb 7, 2019
@gronke @lunny
Check if files are a symlink (#20)
d97cb0f 
* rename utlis.go to utils.go

* TreeEntry IsLink function
lunny referenced this pull request in lunny/gitea Feb 7, 2019
@tboerger
Merge pull request #20 from lunny/lunny/islink_for_treeentry
43c086d 
Check if files are a symlink (#20)
@camlafit

camlafit commented Aug 31, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Hello

I've encountered a problem with this patch. With last version 1.9.2, I've tried to rename an organization to "plugins" as it's its final purpose. I've got a simple error 500.

I've check to on gitea.log to see a notice about reserved word without any information. And many thank to our chat as I've got a link to this PR (easier to understand my 500 error :)

I see at least two problems :

  • First we can't provide an error 500 when organization is renamed with one of these reserved words. It's only an action forbidden. At least miss an information about this list of reserved words.
  • Second problem, in my use case "plugins" is really licit, this organization has to purpose to manage a plugins collection.
    I've do a workaround with "plugin" but If I've understood the initial problem this word should be prohibited. And looks difficult to maintain a harcoded list because we must manage any variant word and could be add many conflict with licit organization purpose.

Edit : New issue was opened #8072

Thank a lot

@camlafit camlafit mentioned this pull request Sep 3, 2019
Closed
@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.

Reviewers

No reviews

Assignees

No one assigned

Labels

issue/critical This issue should be fixed ASAP. If it is a PR, the PR should be merged ASAP lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug

Projects

None yet

Milestone

1.0.0

Development

Successfully merging this pull request may close these issues.

FreeBSD problem

7 participants

@tboerger @bkcsoft @strk @codecov-io @thibaultmeyer @camlafit @LefsFlare