explicitly check viewer access to settings in GraphQL API#63945
Conversation
Previously, a user's or org's settings were protected from unauthorized access in the GraphQL API by access checks far from the actual `SettingCascade` and `LatestSettings` implementations in most cases. This did not present a security issue because on Sourcegraph.com we prevented users from getting a reference to an org they aren't in, and user settings had the right manual access checks. But the access checks are too far away from the actual resolver methods (so it'd be easy to make a mistake) and were not consistently implemented. One substantive security change is that now site admins may NOT view the settings of an org they are not a member of. This is in line with site admins on dotcom not being able to see user settings. This is important because settings might contain secrets in the future (e.g., OpenCtx provider config). Site admins may still add themselves to the org and then view the settings, but that creates more of an audit trail and and we may lock down that as well.
| // 🚨 SECURITY: Check that the viewer can access these settings. | ||
| subject, err := settingsSubjectForNodeAndCheckAccess(ctx, o) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &settingsCascade{db: o.db, subject: subject}, nil |
Check notice
Code scanning / Semgrep OSS
Semgrep Finding: security-semgrep-rules.semgrep-rules.generic.comment-tagging-rule
| // 🚨 SECURITY: Check that the viewer can access these settings. | ||
| subject, err := settingsSubjectForNodeAndCheckAccess(ctx, r) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| settings := &api.Settings{ | ||
| Subject: api.SettingsSubject{Default: true}, | ||
| Contents: `{"experimentalFeatures": {}}`, | ||
| } | ||
| return &settingsResolver{db: r.db, subject: &settingsSubjectResolver{defaultSettings: r}, settings: settings}, nil | ||
| return &settingsResolver{db: r.db, subject: subject, settings: settings}, nil |
Check notice
Code scanning / Semgrep OSS
Semgrep Finding: security-semgrep-rules.semgrep-rules.generic.comment-tagging-rule
| // 🚨 SECURITY: Check that the viewer can access these settings. | ||
| subject, err := settingsSubjectForNodeAndCheckAccess(ctx, r) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &settingsCascade{db: r.db, subject: subject}, nil |
Check notice
Code scanning / Semgrep OSS
Semgrep Finding: security-semgrep-rules.semgrep-rules.generic.comment-tagging-rule
| // 🚨 SECURITY: Check that the viewer can access these settings. | ||
| subject, err := settingsSubjectForNodeAndCheckAccess(ctx, o) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| settings, err := o.db.Settings().GetLatest(ctx, o.settingsSubject()) | ||
| settings, err := o.db.Settings().GetLatest(ctx, subject.toSubject()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if settings == nil { | ||
| return nil, nil | ||
| } | ||
|
|
||
| return &settingsResolver{db: o.db, subject: &settingsSubjectResolver{org: o}, settings: settings}, nil | ||
| return &settingsResolver{db: o.db, subject: subject, settings: settings}, nil |
Check notice
Code scanning / Semgrep OSS
Semgrep Finding: security-semgrep-rules.semgrep-rules.generic.comment-tagging-rule
| // 🚨 SECURITY: Only the settingsSubjectForNodeAndCheckAccess function can set this. It is used | ||
| // to ensure that access checks have been run on this value, so that we don't leak settings to | ||
| // an unauthorized viewer by an accidental bypass of access checks. This struct type is | ||
| // naturally constructed all over the place (because many types of nodes have settings), and it | ||
| // was too easy to bypass the access check accidentally. | ||
| checkedAccess_DO_NOT_SET_THIS_MANUALLY_OR_YOU_WILL_LEAK_SECRETS bool |
Check notice
Code scanning / Semgrep OSS
Semgrep Finding: security-semgrep-rules.semgrep-rules.generic.comment-tagging-rule
|
|
||
| return &subject, nil | ||
| } | ||
|
|
Check notice
Code scanning / Semgrep OSS
Semgrep Finding: security-semgrep-rules.semgrep-rules.generic.comment-tagging-rule
| // 🚨 SECURITY: Check that the viewer can access these settings. | ||
| subject, err := settingsSubjectForNodeAndCheckAccess(ctx, r) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| settings, err := r.db.Settings().GetLatest(ctx, subject.toSubject()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if settings == nil { | ||
| return nil, nil | ||
| } | ||
| return &settingsResolver{db: r.db, subject: &settingsSubjectResolver{site: r}, settings: settings}, nil | ||
| return &settingsResolver{db: r.db, subject: subject, settings: settings}, nil |
Check notice
Code scanning / Semgrep OSS
Semgrep Finding: security-semgrep-rules.semgrep-rules.generic.comment-tagging-rule
| // 🚨 SECURITY: Check that the viewer can access these settings. | ||
| subject, err := settingsSubjectForNodeAndCheckAccess(ctx, r) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &settingsCascade{db: r.db, subject: subject}, nil |
Check notice
Code scanning / Semgrep OSS
Semgrep Finding: security-semgrep-rules.semgrep-rules.generic.comment-tagging-rule
| // 🚨 SECURITY: Check that the viewer can access these settings. | ||
| subject, err := settingsSubjectForNodeAndCheckAccess(ctx, r) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| settings, err := r.db.Settings().GetLatest(ctx, r.settingsSubject()) | ||
| settings, err := r.db.Settings().GetLatest(ctx, subject.toSubject()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if settings == nil { | ||
| return nil, nil | ||
| } | ||
| return &settingsResolver{db: r.db, subject: &settingsSubjectResolver{user: r}, settings: settings}, nil | ||
| return &settingsResolver{db: r.db, subject: subject, settings: settings}, nil |
Check notice
Code scanning / Semgrep OSS
Semgrep Finding: security-semgrep-rules.semgrep-rules.generic.comment-tagging-rule
| // 🚨 SECURITY: Check that the viewer can access these settings. | ||
| subject, err := settingsSubjectForNodeAndCheckAccess(ctx, r) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &settingsCascade{db: r.db, subject: subject}, nil |
Check notice
Code scanning / Semgrep OSS
Semgrep Finding: security-semgrep-rules.semgrep-rules.generic.comment-tagging-rule
evict
left a comment
There was a problem hiding this comment.
Nice! Looks good from security POV. 👍
Previously, a user's or org's settings were protected from unauthorized access in the GraphQL API by access checks far from the actual
SettingCascadeandLatestSettingsimplementations in most cases. This did not present a security issue because on Sourcegraph.com we prevented users from getting a reference to an org they aren't in, and user settings had the right manual access checks.But the access checks are too far away from the actual resolver methods (so it'd be easy to make a mistake) and were not consistently implemented. Now, all checks for view-settings-of-subject access go through the same function,
settingsSubjectForNodeAndCheckAccess.One substantive security change is that now site admins may NOT view the settings of an org they are not a member of. This is in line with site admins on dotcom not being able to see user settings. This is important because settings might contain secrets in the future (e.g., OpenCtx provider config). Site admins may still add themselves to the org and then view the settings, but that creates more of an audit trail and and we may lock down that as well.
Test plan
Unit tests, also browse around the UI and ensure there are no code paths where our assertion triggers.