fix: no auth and prompt protect, misc#1116
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces significant improvements to the system's authentication and prompt protection capabilities. It provides a new configuration option to enable a "no-auth" API access mode, enhancing deployment flexibility. The prompt protection feature has been re-engineered for more robust and sequential application of rules, including immediate rejection for sensitive content. Additionally, comprehensive documentation for prompt protection has been added, alongside minor database configuration adjustments and general code refinements. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a new AllowNoAuth configuration option to control API authentication behavior, updates the EnsureNoAuthAPIKey logic to check for existing keys and handle project association, and refactors the prompt protection rule application to return a more comprehensive result including all matched rules and a rejection flag. Review comments suggest improving logging for configuration values and cache errors, clarifying why the project is not cached in EnsureNoAuthAPIKey, enhancing the error message for noauth API key usage, and returning all matched prompt protection rules for better auditing.
| if apiKey.Type == apikey.TypeNoauth { | ||
| return nil, fmt.Errorf("noauth api key is only available when api auth is disabled: %w", ErrInvalidAPIKey) | ||
| } |
There was a problem hiding this comment.
This check prevents the use of noauth API keys when API authentication is enabled, which aligns with the intended security policy. However, consider adding a more descriptive error message to guide users on how to properly configure the system to use noauth keys.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| return nil, fmt.Errorf("failed to get owner user for noauth api key: %w", err) | ||
| } | ||
|
|
||
| scopeValues := []string{ | ||
| string(scopes.ScopeReadChannels), | ||
| string(scopes.ScopeWriteRequests), | ||
| } | ||
|
|
||
| existing, err := client.APIKey.Query().Where(apikey.KeyEQ(NoAuthAPIKeyValue)).Only(ctx) | ||
| if err == nil { | ||
| updated, updateErr := client.APIKey.UpdateOneID(existing.ID). | ||
| SetName(NoAuthAPIKeyName). | ||
| SetType(apikey.TypeNoauth). | ||
| SetStatus(apikey.StatusEnabled). | ||
| SetScopes(scopeValues). | ||
| Save(ctx) | ||
| if updateErr != nil { | ||
| return nil, fmt.Errorf("failed to update noauth api key: %w", updateErr) | ||
| } | ||
|
|
||
| s.invalidateAPIKeyCaches(ctx, updated.Key) | ||
|
|
||
| return updated, nil | ||
| } | ||
|
|
||
| if !ent.IsNotFound(err) { | ||
| return nil, fmt.Errorf("failed to query noauth api key: %w", err) | ||
| } | ||
|
|
||
| created, err := client.APIKey.Create(). | ||
| apiKey, err := client.APIKey.Create(). | ||
| SetName(NoAuthAPIKeyName). | ||
| SetKey(NoAuthAPIKeyValue). | ||
| SetUserID(owner.ID). | ||
| SetProjectID(proj.ID). | ||
| SetType(apikey.TypeNoauth). | ||
| SetStatus(apikey.StatusEnabled). | ||
| SetScopes(scopeValues). | ||
| SetScopes([]string{string(scopes.ScopeWriteRequests), string(scopes.ScopeReadChannels)}). | ||
| Save(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create noauth api key: %w", err) | ||
| } | ||
|
|
||
| s.invalidateAPIKeyCaches(ctx, created.Key) | ||
| // DO NOT CACHE PROJECT | ||
| project, err := s.ProjectService.GetProjectByID(ctx, apiKey.ProjectID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get api key project: %w", err) | ||
| } | ||
|
|
||
| apiKey.Edges.Project = project | ||
|
|
||
| return created, nil | ||
| s.invalidateAPIKeyCaches(ctx, apiKey.Key) | ||
|
|
||
| return apiKey, nil |
There was a problem hiding this comment.
🟡 Race condition in EnsureNoAuthAPIKey: concurrent first-time creation causes unique constraint violation
The old EnsureNoAuthAPIKey used an upsert pattern: query the DB directly, update if found, create if not found. The new implementation checks the cache/DB via GetAPIKey (line 580), and if the key doesn't exist (ErrInvalidAPIKey), creates it (line 602-610). However, if multiple concurrent no-auth requests arrive when the key doesn't yet exist, all will miss the cache and DB, and all will attempt Create(). Since the key field has a unique index (internal/ent/schema/api_key.go:33-35), only one will succeed — the rest will fail with a unique constraint violation, returning a 500 error to the client.
This is a regression from the old code which handled the "already exists" case gracefully via update. The new code is now called in the hot path (every request without an API key when AllowNoAuth is true via AuthenticateNoAuth at internal/server/biz/auth.go:222-224), rather than at startup, making the race window more likely to be hit on first use.
(Refers to lines 579-625)
Prompt for agents
In internal/server/biz/api_key.go, the EnsureNoAuthAPIKey function (lines 579-625) has a TOCTOU race condition. When concurrent calls both miss the cache and DB, multiple Create() calls will race and all but one will fail with a unique constraint violation on the key field.
To fix this, after the Create() call fails, catch the unique constraint violation error and retry GetAPIKey. Something like:
1. After the Create() call at line 602-610, if err is a unique constraint violation (ent.IsConstraintError(err)), retry s.GetAPIKey(ctx, NoAuthAPIKeyValue) to get the key that was created by the winning concurrent request.
2. Alternatively, re-introduce the old upsert pattern: query the DB directly with client.APIKey.Query().Where(apikey.KeyEQ(NoAuthAPIKeyValue)).Only(ctx) and update if found, create only if not found.
3. Or use a sync.Once or mutex to ensure only one goroutine creates the key.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Uh oh!
There was an error while loading. Please reload this page.