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

Fixes possible vulnerabilities with keyword hijacking #20

Merged
merged 3 commits into from Nov 12, 2016
Merged

Fixes possible vulnerabilities with keyword hijacking #20

merged 3 commits into from Nov 12, 2016

Conversation

ghost
Copy link

@ghost ghost commented Nov 3, 2016

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
Copy link
Member

tboerger commented Nov 3, 2016

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
Copy link
Member

bkcsoft commented Nov 4, 2016

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
Copy link
Member

strk commented Nov 4, 2016

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
Copy link
Member

bkcsoft commented Nov 4, 2016

Same request as @strk, we need tests 😄

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

@bkcsoft
Copy link
Member

bkcsoft commented Nov 4, 2016

ooh, and rebase 😒

@ghost
Copy link
Author

ghost commented Nov 4, 2016

@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
Copy link

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

@@ -518,7 +518,7 @@ func isUsableName(names, patterns []string, name string) error {
}

func IsUsableUsername(name string) error {
Copy link
Member

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 🙂

Copy link
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
Copy link
Member

strk commented Nov 4, 2016

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
Copy link
Member

strk commented Nov 6, 2016 via email

@thibaultmeyer
Copy link
Contributor

LGTM

@thibaultmeyer thibaultmeyer merged commit 3ef022b into go-gitea:master Nov 12, 2016
@tboerger
Copy link
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
Copy link
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
* rename utlis.go to utils.go

* TreeEntry IsLink function
lunny referenced this pull request in lunny/gitea Feb 7, 2019
@camlafit
Copy link
Contributor

camlafit commented Aug 31, 2019

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

@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
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
Development

Successfully merging this pull request may close these issues.

FreeBSD problem
7 participants