-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Ensure that feeds are appropriately restricted #10018
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
Ensure that feeds are appropriately restricted #10018
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10018 +/- ##
==========================================
- Coverage 42.27% 42.26% -0.01%
==========================================
Files 611 611
Lines 80389 80405 +16
==========================================
- Hits 33988 33987 -1
- Misses 42225 42239 +14
- Partials 4176 4179 +3
Continue to review full report at Codecov.
|
@@ -319,7 +319,7 @@ func SearchRepository(opts *SearchRepoOptions) (RepositoryList, int64, error) { | |||
func accessibleRepositoryCondition(user *User) builder.Cond { | |||
var cond = builder.NewCond() | |||
|
|||
if user == nil || !user.IsRestricted { | |||
if user == nil || !user.IsRestricted || user.ID <= 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.
Isn't it strange that user ==nil
enables VisibleTypeLimited
orgs three lines below?
I was under the impression that user == nil (or user.ID <= 0) meant anonymous/unidentified.
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's not enabling it - rather restricting them away further. But I should probably add the user ID <= 0 test to that too - DONE
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.
Not your fault, the code is previous to your PR. I've just noticed it:
cond = cond.Or(builder.And(
builder.Eq{"`repository`.is_private": false},
builder.Or(
// A. Aren't in organisations __OR__
builder.NotIn("`repository`.owner_id", builder.Select("id").From("`user`").Where(builder.Eq{"type": UserTypeOrganization})),
// B. Isn't a private organisation. Limited is OK as long as we're logged in.
builder.NotIn("`repository`.owner_id", builder.Select("id").From("`user`").Where(builder.In("visibility", orgVisibilityLimit))))))
If I read this right, it's: user can see it if "repository is not private or (rest of the condition)". A limited org has public repos that anonymous users should not be able to see. This condition will make those repos pass, since they've got is_private == false
.
I'm pretty tired, so I might be I'm getting this wrong.
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.
On a second look... see my comment about orgs.
Please disregards my comments about the permissions. They were just fine. 😔 |
Fix #9981