Added comprehensive Gerrit HTTP authentication support #366
Conversation
WalkthroughAdds Gerrit HTTP Basic authentication across docs, schemas, types, and backend; enforces non-empty Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin (Config)
participant Backend as Sourcebot Backend
participant DB as Database (Prisma)
participant Gerrit as Gerrit Server
Admin->>Backend: Submit Gerrit connection (url + optional auth)
alt auth present
Backend->>DB: getTokenFromConfig(auth.password, orgId, db)
DB-->>Backend: resolved password
Backend->>Gerrit: GET /a/projects/ (Authorization: Basic)
else public
Backend->>Gerrit: GET /projects/ (no auth)
end
Gerrit-->>Backend: projects response (may include XSSI prefix)
Backend->>Backend: strip XSSI, parse JSON, filter include/exclude
Backend->>Admin: return repository list / compiled config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
docs/snippets/schemas/v3/index.schema.mdx (1)
1-1: Do not modify auto-generated files directlyThis file is marked as auto-generated and should not be modified manually. Any changes made here will likely be overwritten when the file is regenerated. Please update the source files that generate this schema instead.
To verify the source of this auto-generated file and find the correct location to make changes:
#!/bin/bash # Description: Find the source files and generation scripts for the schema # Search for schema generation scripts echo "=== Searching for schema generation scripts ===" fd -t f -e js -e ts -e json . | xargs rg -l "index.schema.(mdx|ts)" | head -20 # Look for build or generation commands in package.json files echo -e "\n=== Checking package.json for generation scripts ===" fd package.json | xargs rg -A2 -B2 "schema|generate" | grep -E "(script|generate|schema)" | head -20 # Search for the source schema definitions echo -e "\n=== Looking for source schema files ===" fd -t f "schema" -e json -e ts -e js | grep -v "index.schema" | grep -v node_modules | head -20 # Check for any schema-related configuration files echo -e "\n=== Searching for schema configuration ===" rg -g "!node_modules" -g "!*.test.*" "indexSchema|schema.*generate" --type ts --type js | head -20
♻️ Duplicate comments (1)
packages/schemas/src/v3/index.schema.ts (1)
1-1: Do not modify auto-generated files directlyThis TypeScript schema file is auto-generated and should not be modified manually. Changes should be made to the source schema definition files instead. Any manual modifications will be lost when this file is regenerated.
🧹 Nitpick comments (4)
docs/snippets/schemas/v3/gitea.schema.mdx (1)
20-23: Security consideration for direct token values.The addition of direct string token support improves flexibility, but the security warning could be more specific about the risks. Consider expanding the description to mention credential exposure in configuration files, version control, and logs.
The current implementation is acceptable, but consider enhancing the warning:
- "description": "Direct token value (not recommended for production)" + "description": "Direct token value (not recommended for production due to credential exposure risks in configuration files, version control, and logs)"docs/snippets/schemas/v3/github.schema.mdx (1)
20-23: Consistent implementation across platforms.The direct string token support is implemented identically to other platforms (Gitea, Bitbucket), maintaining consistency. The same security considerations apply regarding the production usage warning.
Consider standardizing the security warning across all platform schemas to be more specific about credential exposure risks.
packages/backend/src/gerrit.test.ts (1)
509-528: Consider adding a test for missing XSSI prefixWhile you test the response without XSSI prefix later (lines 709-731), this test assumes the XSSI prefix is always present. Consider making the XSSI prefix stripping more robust by checking if it exists before stripping.
packages/backend/src/gerrit.ts (1)
110-192: Robust authenticated API implementation.The implementation correctly:
- Switches between authenticated (
/a/) and public endpoints based on auth presence- Constructs HTTP Basic Auth headers securely
- Handles Gerrit's XSSI protection prefix
- Provides detailed error logging with authentication context
Consider adding rate limiting or retry logic specifically for authentication failures to prevent credential brute-forcing:
if (!response.ok) { const errorText = await response.text().catch(() => 'Unknown error'); logger.error(`Failed to fetch projects from Gerrit at ${endpointWithParams} with status ${response.status}: ${errorText}`); + // Add specific handling for 401 Unauthorized + if (response.status === 401 && auth) { + logger.warn('Authentication failed - verify credentials are correct'); + } const e = new BackendException(BackendError.CONNECTION_SYNC_FAILED_TO_FETCH_GERRIT_PROJECTS, {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
.gitignore(1 hunks)docs/docs/connections/gerrit-troubleshooting.mdx(1 hunks)docs/docs/connections/gerrit.mdx(1 hunks)docs/snippets/schemas/v3/bitbucket.schema.mdx(1 hunks)docs/snippets/schemas/v3/connection.schema.mdx(5 hunks)docs/snippets/schemas/v3/gerrit.schema.mdx(1 hunks)docs/snippets/schemas/v3/gitea.schema.mdx(1 hunks)docs/snippets/schemas/v3/github.schema.mdx(1 hunks)docs/snippets/schemas/v3/gitlab.schema.mdx(1 hunks)docs/snippets/schemas/v3/index.schema.mdx(5 hunks)docs/snippets/schemas/v3/shared.schema.mdx(1 hunks)packages/backend/src/connectionManager.ts(1 hunks)packages/backend/src/gerrit.test.ts(1 hunks)packages/backend/src/gerrit.ts(6 hunks)packages/backend/src/github.ts(1 hunks)packages/backend/src/repoCompileUtils.ts(1 hunks)packages/backend/src/repoManager.ts(3 hunks)packages/backend/src/utils.ts(2 hunks)packages/crypto/src/tokenUtils.ts(1 hunks)packages/schemas/src/v3/bitbucket.schema.ts(1 hunks)packages/schemas/src/v3/bitbucket.type.ts(1 hunks)packages/schemas/src/v3/connection.schema.ts(5 hunks)packages/schemas/src/v3/connection.type.ts(5 hunks)packages/schemas/src/v3/gerrit.schema.ts(1 hunks)packages/schemas/src/v3/gerrit.type.ts(1 hunks)packages/schemas/src/v3/gitea.schema.ts(1 hunks)packages/schemas/src/v3/gitea.type.ts(1 hunks)packages/schemas/src/v3/github.schema.ts(1 hunks)packages/schemas/src/v3/github.type.ts(1 hunks)packages/schemas/src/v3/gitlab.schema.ts(1 hunks)packages/schemas/src/v3/gitlab.type.ts(1 hunks)packages/schemas/src/v3/index.schema.ts(5 hunks)packages/schemas/src/v3/index.type.ts(5 hunks)packages/schemas/src/v3/shared.schema.ts(1 hunks)packages/schemas/src/v3/shared.type.ts(1 hunks)packages/web/src/actions.ts(1 hunks)schemas/v3/gerrit.json(1 hunks)schemas/v3/shared.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*`: Filenames should always be camelCase. Exception: if there are filenames in the same directory with a format other than camelCase, use that format to keep things consistent.
**/*: Filenames should always be camelCase. Exception: if there are filenames in the same directory with a format other than camelCase, use that format to keep things consistent.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/style.mdc)
List of files the instruction was applied to:
packages/schemas/src/v3/github.schema.tspackages/crypto/src/tokenUtils.tspackages/backend/src/github.tspackages/backend/src/connectionManager.tsdocs/snippets/schemas/v3/gitlab.schema.mdxpackages/schemas/src/v3/bitbucket.schema.tspackages/web/src/actions.tspackages/schemas/src/v3/bitbucket.type.tsdocs/snippets/schemas/v3/shared.schema.mdxpackages/schemas/src/v3/gitea.schema.tsschemas/v3/shared.jsonpackages/schemas/src/v3/shared.schema.tsdocs/snippets/schemas/v3/bitbucket.schema.mdxdocs/snippets/schemas/v3/gitea.schema.mdxpackages/backend/src/repoManager.tsdocs/snippets/schemas/v3/gerrit.schema.mdxdocs/snippets/schemas/v3/github.schema.mdxpackages/schemas/src/v3/gerrit.type.tspackages/schemas/src/v3/github.type.tspackages/schemas/src/v3/gitlab.schema.tspackages/schemas/src/v3/gitlab.type.tsschemas/v3/gerrit.jsonpackages/backend/src/utils.tspackages/schemas/src/v3/gitea.type.tspackages/schemas/src/v3/shared.type.tspackages/backend/src/repoCompileUtils.tspackages/backend/src/gerrit.test.tspackages/schemas/src/v3/gerrit.schema.tspackages/schemas/src/v3/index.type.tspackages/backend/src/gerrit.tsdocs/snippets/schemas/v3/index.schema.mdxpackages/schemas/src/v3/index.schema.tsdocs/docs/connections/gerrit-troubleshooting.mdxpackages/schemas/src/v3/connection.type.tsdocs/snippets/schemas/v3/connection.schema.mdxpackages/schemas/src/v3/connection.schema.tsdocs/docs/connections/gerrit.mdx
🪛 LanguageTool
docs/docs/connections/gerrit.mdx
[grammar] ~278-~278: The verb form ‘clone’ does not seem to be suitable in this context.
Context: ...ect Permissions**: Ensure your user has clone permissions 4. **Validate Configuration...
(HAVE_VB)
🪛 Gitleaks (8.26.0)
docs/docs/connections/gerrit.mdx
90-90: Discovered a potential basic authorization token provided in a curl command, which could compromise the curl accessed resource.
(curl-auth-user)
🔇 Additional comments (47)
schemas/v3/shared.json (1)
7-10: LGTM: Enhanced token flexibility with appropriate security warning.The addition of direct string token support provides useful flexibility for development and testing scenarios while maintaining the existing secure token options. The warning message appropriately discourages production usage of direct string tokens.
.gitignore (1)
168-170: LGTM: Good security practice to prevent credential leakage.Adding ignore patterns for test files containing real credentials is an essential security measure. The patterns appropriately cover both root-level and nested files.
packages/schemas/src/v3/shared.type.ts (1)
8-8: LGTM: Auto-generated type definition correctly reflects schema changes.The addition of the string type to the Token union type properly aligns with the JSON schema changes, enabling direct string token support.
packages/backend/src/github.ts (1)
69-69: LGTM: Defensive programming to handle new token schema.The type check ensures safe access to the
secretproperty whenconfig.tokenis an object, preventing runtime errors with the new string token support. This is proper defensive programming.packages/backend/src/connectionManager.ts (1)
175-175: LGTM: Consistent database client passing for Gerrit authentication.Adding the database client parameter aligns the Gerrit case with other connection types and enables authenticated Gerrit connections by providing access to secure token storage.
packages/crypto/src/tokenUtils.ts (1)
6-9: LGTM! Clean implementation of direct string token support.The early return for string tokens is well-implemented and maintains backward compatibility while supporting the new schema variant. The type check is appropriate and the logic is clear.
packages/schemas/src/v3/shared.schema.ts (1)
8-11: Appropriate schema extension for direct string tokens.The addition of string as a valid token type with a production warning is well-implemented. The anyOf structure maintains backward compatibility while enabling the new authentication options described in the PR objectives.
packages/schemas/src/v3/gitea.type.ts (1)
12-12: Consistent type definition update for string token support.The addition of
stringto the token union type properly reflects the schema changes and maintains type safety across the authentication system.docs/snippets/schemas/v3/shared.schema.mdx (1)
9-12: Documentation schema properly mirrors code changes.The schema documentation correctly reflects the new string token support with appropriate production warnings, maintaining consistency between code and documentation.
packages/web/src/actions.ts (1)
2099-2099: Excellent defensive programming to handle new string token type.The explicit
typeofcheck prevents runtime errors whentokenis a string rather than an object. This properly handles the new token format while maintaining safe property access.docs/snippets/schemas/v3/gitea.schema.mdx (1)
1-1: Note: This is an auto-generated file.Ensure that any future modifications are made to the source templates rather than directly to this file, as changes will be overwritten during regeneration.
packages/schemas/src/v3/bitbucket.type.ts (2)
1-1: Note: This is an auto-generated file.Ensure that modifications are made to the source generators rather than directly editing this file.
16-16: Type definition correctly extends token flexibility.The addition of
stringas a union type for the token property aligns with the schema changes and maintains type safety while providing backward compatibility.docs/snippets/schemas/v3/github.schema.mdx (1)
1-1: Note: This is an auto-generated file.Ensure modifications are made to source templates to maintain consistency across all platform schemas.
packages/schemas/src/v3/gitea.schema.ts (2)
1-1: Note: This is an auto-generated file.Modifications should be made to the source generators to ensure consistency between documentation and implementation files.
19-22: Schema implementation correctly mirrors documentation.The TypeScript schema implementation accurately reflects the changes in the corresponding .mdx documentation file, maintaining consistency between documentation and code.
packages/schemas/src/v3/github.schema.ts (2)
1-1: Note: This is an auto-generated file.Ensure source generators maintain consistency across all platform schema implementations.
19-22: Consistent schema implementation across platforms.The GitHub schema implementation follows the same pattern as all other platforms (Gitea, Bitbucket), ensuring consistency in token handling across the entire authentication system.
docs/snippets/schemas/v3/gitlab.schema.mdx (1)
20-23: Documentation correctly reflects schema changes.The documentation file properly mirrors the schema changes from the TypeScript schema file, maintaining consistency between implementation and documentation.
packages/schemas/src/v3/bitbucket.schema.ts (1)
23-26: Consistent implementation with other connection schemas.The direct string token support follows the same pattern as GitLab and other connection schemas, maintaining consistency across the platform. The same security considerations mentioned in the GitLab schema review apply here.
packages/schemas/src/v3/gerrit.type.ts (1)
12-37: Excellent implementation of Gerrit authentication structure.The new
authproperty is well-designed with several strong points:
- Clear documentation: The JSDoc comments clearly explain that this requires Gerrit HTTP passwords, not account passwords, with specific guidance on generation
- Flexible password specification: Supports direct strings, secrets, and environment variables consistently with other connection types
- Backward compatibility: Optional property maintains compatibility with existing configurations
- Type safety: Proper TypeScript union types for password variants
This implementation directly addresses the PR objective for comprehensive Gerrit HTTP authentication support.
packages/schemas/src/v3/github.type.ts (1)
12-12: Completes consistent token type support across connection types.The addition of
stringas a token type option maintains consistency with the schema changes across GitLab, Bitbucket, and other connection types, ensuring uniform authentication flexibility throughout the platform.docs/snippets/schemas/v3/bitbucket.schema.mdx (1)
24-27: LGTM - Consistent token support addition.The addition of direct string token support follows the same pattern as other connection types and includes appropriate production usage warnings.
packages/schemas/src/v3/gitlab.type.ts (1)
12-12: LGTM - Type definition correctly updated.The addition of
stringto the union type aligns with the schema changes and maintains type safety.schemas/v3/gerrit.json (1)
19-48: LGTM - Well-structured authentication schema.The auth object is properly designed with:
- Required username and password fields
- Reference to shared Token definition for consistency
- Comprehensive examples showing different configuration methods
- Appropriate constraints with
additionalProperties: falsepackages/backend/src/repoManager.ts (3)
5-5: LGTM - Import correctly added.The GerritConnectionConfig import is properly added to support the new authentication functionality.
224-233: LGTM - Gerrit authentication properly implemented.The Gerrit connection type handling correctly:
- Checks for auth object existence
- Retrieves password token using the existing token utility
- Returns properly structured credentials with username and password
274-277: LGTM - Security enhancement with credential encoding.The addition of
encodeURIComponentfor both username and password is a crucial security improvement that prevents injection attacks and properly handles special characters in credentials.docs/snippets/schemas/v3/gerrit.schema.mdx (1)
21-81: LGTM - Comprehensive authentication schema documentation.The auth object documentation is well-structured with:
- Clear descriptions for username and password fields
- Inline token type definitions consistent with other schema docs
- Helpful examples showing different configuration approaches
- Proper constraints and validation rules
packages/backend/src/repoCompileUtils.ts (2)
249-252: LGTM: Proper parameter threading for authentication support.The addition of the
db: PrismaClientparameter and its propagation togetGerritReposFromConfigcorrectly enables credential retrieval for authenticated Gerrit access.
259-263: LGTM: Correct authenticated clone URL construction.The conditional logic properly constructs authenticated clone URLs by:
- Adding the
/a/path segment whenconfig.authis present- Using
encodeURIComponentfor proper URL encoding of project names- Falling back to public URLs when no authentication is configured
This aligns with Gerrit's authenticated API access patterns.
packages/backend/src/utils.ts (2)
24-26: LGTM: Simple and effective utility function.The
isRemotePathfunction correctly identifies remote URLs by checking for HTTP/HTTPS protocols.
28-32: LGTM: Excellent backward compatibility for direct string tokens.The type guard correctly handles direct string tokens before falling back to the existing secret/environment variable logic. This maintains backward compatibility while supporting the new Gerrit authentication flow.
packages/schemas/src/v3/gerrit.schema.ts (1)
20-80: LGTM: Comprehensive and well-structured authentication schema.The auth object properly supports multiple authentication methods:
- Username/password structure: Clear separation of concerns
- Flexible password formats: Direct string, secret references, and environment variables
- Security guidance: Excellent documentation about HTTP passwords vs account passwords
- Validation: Proper required fields and
additionalProperties: falseconstraintsThe schema design aligns well with the broader token standardization across other connection types.
docs/docs/connections/gerrit-troubleshooting.mdx (1)
1-496: Excellent comprehensive troubleshooting documentation.This troubleshooting guide is exceptionally well-crafted and provides significant value:
Strengths:
- Practical quick diagnosis: Clear checklist for immediate issue identification
- Scenario-based organization: Real-world error patterns with actionable solutions
- Debug tooling: Complete TypeScript debug script for automated testing
- Security best practices: Proper credential management guidance
- Progressive complexity: From basic checks to advanced network troubleshooting
Coverage highlights:
- Authentication errors (401, incorrect credentials)
- Project discovery issues (permissions, filtering)
- Git clone failures (URL encoding, credential helpers)
- Configuration schema validation
- Network connectivity problems
The accordion structure and code examples make this highly usable for both developers and administrators troubleshooting Gerrit integration issues.
packages/schemas/src/v3/index.type.ts (2)
119-119: LGTM: Consistent token type standardization across connections.The token type updates properly standardize authentication across all connection types (GitHub, GitLab, Gitea, Bitbucket) by allowing both direct string tokens and object references to secrets/environment variables. This creates a unified authentication experience.
Also applies to: 209-209, 277-277, 389-389
331-356: LGTM: Well-structured Gerrit authentication interface.The new
authobject for Gerrit connections properly defines:
- Required fields:
usernameandpasswordappropriately marked as required- Flexible password types: Supports direct strings, secret references, and environment variables
- Type consistency: Matches the schema definition and follows the same pattern as other connection token types
The interface correctly enables the authenticated Gerrit functionality described in the PR objectives.
packages/backend/src/gerrit.test.ts (1)
884-884: Good test coverage for special characters in passwordsExcellent test case for verifying that special characters like
/,+, and=are properly handled in passwords. This aligns well with the PR objectives mentioning automatic URL encoding for these characters.docs/snippets/schemas/v3/connection.schema.mdx (2)
24-27: Consistent token schema enhancement across all providers.The addition of direct string token support is implemented consistently across GitHub, GitLab, Gitea, and Bitbucket connection types. The "not recommended for production" warning is appropriately included for all.
Also applies to: 240-243, 446-449, 742-745
612-671: Well-structured Gerrit authentication schema.The authentication structure is properly designed with:
- Clear distinction between HTTP password and account password in the description
- Flexible password storage options (direct string, secret, environment variable)
- Appropriate security warning for direct token usage
- Required fields properly enforced
packages/schemas/src/v3/connection.schema.ts (1)
23-26: Auto-generated schema correctly reflects source changes.The TypeScript schema properly mirrors all the authentication enhancements from the source schema file.
Also applies to: 240-243, 446-449, 633-664, 742-745
packages/backend/src/gerrit.ts (2)
1-9: Appropriate imports and well-defined interfaces.The addition of PrismaClient and authentication-related imports properly support the new authentication functionality. The GerritProject interface export and internal auth config interface are well-structured.
Also applies to: 25-41
44-67: Secure credential retrieval implementation.The authentication credential retrieval properly:
- Uses the existing secure token retrieval utility
- Includes appropriate error handling and logging
- Maintains credential security by not logging the password
docs/docs/connections/gerrit.mdx (3)
9-74: Excellent authentication documentation.The authentication section provides:
- Clear status of authentication support
- Multiple authentication method options
- Proper security warnings about HTTP passwords
- Clear examples for both public and authenticated connections
207-280: Comprehensive troubleshooting guide.The troubleshooting section effectively covers common issues with clear, actionable solutions. The manual testing commands and debug steps will be valuable for users encountering problems.
282-342: Well-documented implementation details and security practices.The implementation details clearly explain:
- Gerrit's URL structure for authenticated access
- Credential flow through the system
- Important security considerations for credential management
The manual testing script provides a helpful reference for debugging authentication issues.
packages/schemas/src/v3/connection.type.ts (1)
20-32: Type definitions correctly reflect schema enhancements.The TypeScript type definitions properly implement:
- Direct string token support for all providers (GitHub, GitLab, Gitea, Bitbucket)
- Comprehensive Gerrit authentication structure with flexible password storage options
- Proper union types maintaining backward compatibility
Also applies to: 110-122, 178-190, 232-257, 290-302
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
docs/snippets/schemas/v3/index.schema.mdx (3)
480-483: Same empty-string loophole for GitLab tokens
See previous comment – identical fix required.
686-689: Same empty-string loophole for Gitea tokens
See previous comment – identical fix required.
982-985: Same empty-string loophole for Bitbucket tokens
See previous comment – identical fix required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
docs/snippets/schemas/v3/bitbucket.schema.mdx(1 hunks)docs/snippets/schemas/v3/connection.schema.mdx(5 hunks)docs/snippets/schemas/v3/gerrit.schema.mdx(1 hunks)docs/snippets/schemas/v3/gitea.schema.mdx(1 hunks)docs/snippets/schemas/v3/github.schema.mdx(1 hunks)docs/snippets/schemas/v3/gitlab.schema.mdx(1 hunks)docs/snippets/schemas/v3/index.schema.mdx(5 hunks)docs/snippets/schemas/v3/shared.schema.mdx(1 hunks)packages/schemas/src/v3/bitbucket.schema.ts(1 hunks)packages/schemas/src/v3/connection.schema.ts(5 hunks)packages/schemas/src/v3/gerrit.schema.ts(1 hunks)packages/schemas/src/v3/gitea.schema.ts(1 hunks)packages/schemas/src/v3/github.schema.ts(1 hunks)packages/schemas/src/v3/gitlab.schema.ts(1 hunks)packages/schemas/src/v3/index.schema.ts(5 hunks)packages/schemas/src/v3/shared.schema.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- docs/snippets/schemas/v3/gitea.schema.mdx
- packages/schemas/src/v3/github.schema.ts
- packages/schemas/src/v3/bitbucket.schema.ts
- packages/schemas/src/v3/gitea.schema.ts
- packages/schemas/src/v3/gerrit.schema.ts
- packages/schemas/src/v3/shared.schema.ts
- docs/snippets/schemas/v3/shared.schema.mdx
- docs/snippets/schemas/v3/bitbucket.schema.mdx
- docs/snippets/schemas/v3/gerrit.schema.mdx
- packages/schemas/src/v3/gitlab.schema.ts
- docs/snippets/schemas/v3/github.schema.mdx
- docs/snippets/schemas/v3/gitlab.schema.mdx
- docs/snippets/schemas/v3/connection.schema.mdx
- packages/schemas/src/v3/index.schema.ts
- packages/schemas/src/v3/connection.schema.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*`: Filenames should always be camelCase. Exception: if there are filenames in the same directory with a format other than camelCase, use that format to keep things consistent.
**/*: Filenames should always be camelCase. Exception: if there are filenames in the same directory with a format other than camelCase, use that format to keep things consistent.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/style.mdc)
List of files the instruction was applied to:
docs/snippets/schemas/v3/index.schema.mdx
🔇 Additional comments (1)
docs/snippets/schemas/v3/index.schema.mdx (1)
851-911:authobject improvements & spec/summary mismatch
usernameand direct‐stringpasswordvariants can be empty – addminLength: 1.- The PR summary states the
authblock is required, but the schema’srequiredlist (lines 955-958) omits it. Either update the summary or makeauthmandatory to avoid silent mis-configurations.- Minor: consider re-using a shared “Token” schema to avoid 4× duplication (future-proofing).
@@ - "username": { - "type": "string", - "description": "Gerrit username for authentication", + "username": { + "type": "string", + "minLength": 1, + "description": "Gerrit username for authentication", @@ - "description": "Direct token value (SECURITY RISK: not recommended for production - use secrets or environment variables instead)" + "description": "Direct token value (SECURITY RISK: not recommended for production - use secrets or environment variables instead)", + "minLength": 1Outside this hunk, extend the
requiredarray to include"auth"if the intention is to make authentication mandatory:"required": ["type", "url", "auth"]Likely an incorrect or invalid review comment.
|
@brendan-kellam Thank you for the thorough review! I've addressed all your feedback with the following commits: Changes Made🔒 Security
📝 Documentation
🛠️ Code Improvements
✅ Testing
The branch has been rebased on main and all requested changes have been implemented. Ready for re-review when you have a chance! |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
docs/docs/connections/gerrit.mdx (2)
43-65: Good use of environment variables as suggested in past reviews.The authenticated connection example uses environment variables for the password, which aligns with the previous feedback about using
envinstead of secrets (which are deprecated/unsupported).
73-127: Consider consolidating the authentication setup guide.While the step-by-step guide is helpful, it might be overly detailed. Based on past feedback suggesting some sections weren't necessary, consider consolidating this into the main "Authenticated Connection" section.
packages/backend/src/gerrit.ts (2)
113-113: Documentation link addresses previous review comment.The inline comment references the Gerrit REST API documentation as requested in the previous review.
168-171: Clear documentation for XSSI protection handling.The regex explanation addresses the previous review comment about understanding the XSSI protection pattern removal. The comment clearly explains the purpose and pattern.
🧹 Nitpick comments (1)
packages/crypto/src/tokenUtils.test.ts (1)
14-24: Address performance issue with delete operator.The test setup is good, but the delete operator usage can impact performance.
Apply this diff to improve performance:
- delete process.env.TEST_TOKEN; - delete process.env.EMPTY_TOKEN; + process.env.TEST_TOKEN = undefined; + process.env.EMPTY_TOKEN = undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (33)
.gitignore(1 hunks)docs/docs/connections/gerrit.mdx(1 hunks)docs/snippets/schemas/v3/bitbucket.schema.mdx(2 hunks)docs/snippets/schemas/v3/connection.schema.mdx(9 hunks)docs/snippets/schemas/v3/gerrit.schema.mdx(1 hunks)docs/snippets/schemas/v3/gitea.schema.mdx(2 hunks)docs/snippets/schemas/v3/github.schema.mdx(2 hunks)docs/snippets/schemas/v3/gitlab.schema.mdx(2 hunks)docs/snippets/schemas/v3/index.schema.mdx(9 hunks)docs/snippets/schemas/v3/shared.schema.mdx(2 hunks)packages/backend/src/gerrit.test.ts(1 hunks)packages/backend/src/gerrit.ts(6 hunks)packages/backend/src/utils.ts(2 hunks)packages/crypto/package.json(2 hunks)packages/crypto/src/tokenUtils.test.ts(1 hunks)packages/crypto/src/tokenUtils.ts(1 hunks)packages/crypto/vitest.config.ts(1 hunks)packages/schemas/package.json(1 hunks)packages/schemas/src/v3/bitbucket.schema.ts(2 hunks)packages/schemas/src/v3/connection.schema.ts(9 hunks)packages/schemas/src/v3/connection.type.ts(1 hunks)packages/schemas/src/v3/gerrit.schema.ts(1 hunks)packages/schemas/src/v3/gerrit.type.ts(1 hunks)packages/schemas/src/v3/gitea.schema.ts(2 hunks)packages/schemas/src/v3/github.schema.ts(2 hunks)packages/schemas/src/v3/gitlab.schema.ts(2 hunks)packages/schemas/src/v3/index.schema.ts(9 hunks)packages/schemas/src/v3/index.type.ts(1 hunks)packages/schemas/src/v3/shared.schema.test.ts(1 hunks)packages/schemas/src/v3/shared.schema.ts(2 hunks)packages/schemas/vitest.config.ts(1 hunks)schemas/v3/gerrit.json(1 hunks)schemas/v3/shared.json(2 hunks)
✅ Files skipped from review due to trivial changes (5)
- .gitignore
- packages/crypto/vitest.config.ts
- packages/schemas/vitest.config.ts
- packages/schemas/src/v3/bitbucket.schema.ts
- packages/schemas/src/v3/github.schema.ts
🚧 Files skipped from review as they are similar to previous changes (21)
- packages/schemas/src/v3/index.type.ts
- docs/snippets/schemas/v3/gitlab.schema.mdx
- schemas/v3/shared.json
- docs/snippets/schemas/v3/bitbucket.schema.mdx
- packages/schemas/src/v3/shared.schema.ts
- docs/snippets/schemas/v3/shared.schema.mdx
- docs/snippets/schemas/v3/gitea.schema.mdx
- packages/schemas/src/v3/gitea.schema.ts
- packages/backend/src/utils.ts
- schemas/v3/gerrit.json
- docs/snippets/schemas/v3/github.schema.mdx
- packages/schemas/src/v3/gerrit.schema.ts
- packages/schemas/src/v3/connection.type.ts
- packages/schemas/src/v3/gerrit.type.ts
- packages/schemas/src/v3/gitlab.schema.ts
- docs/snippets/schemas/v3/gerrit.schema.mdx
- packages/schemas/src/v3/index.schema.ts
- docs/snippets/schemas/v3/index.schema.mdx
- docs/snippets/schemas/v3/connection.schema.mdx
- packages/schemas/src/v3/connection.schema.ts
- packages/crypto/src/tokenUtils.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/crypto/src/tokenUtils.test.ts (1)
packages/crypto/src/tokenUtils.ts (1)
getTokenFromConfig(5-37)
🪛 Biome (1.9.4)
packages/crypto/src/tokenUtils.test.ts
[error] 22-22: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 23-23: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (25)
packages/crypto/package.json (2)
8-9: LGTM! Clean test infrastructure setup.The addition of the test script with cross-env for cross-platform environment variable handling is well-implemented. The trailing comma addition maintains consistency with the new script entry.
18-20: Good dependency choices for testing.The devDependencies additions are appropriate:
cross-envfor cross-platform environment variable handlingvitestfor modern testing framework- TypeScript update to 5.7.3 for latest features
packages/schemas/package.json (2)
9-10: LGTM! Consistent test infrastructure setup.The test script setup mirrors the crypto package with appropriate cross-platform environment variable handling using cross-env.
14-21: Well-chosen dependencies for schema validation testing.The devDependencies additions are appropriate for a schemas package:
ajvfor JSON schema validation testingcross-envfor cross-platform compatibilityvitestfor testing framework- TypeScript 5.7.3 update for latest features
packages/crypto/src/tokenUtils.test.ts (4)
1-8: LGTM! Well-structured test setup with proper mocking.The test imports and mock setup are cleanly organized. The decrypt function mock with a fixed return value is appropriate for testing the token retrieval logic.
26-46: Excellent test coverage for secret-based tokens.The test correctly verifies both the return value and the Prisma client method call with the expected parameters. The mock setup and assertions are comprehensive.
57-62: Important security test for preventing direct string tokens.This test ensures that direct string tokens are rejected, which is a crucial security measure to prevent accidental credential exposure.
87-94: Good coverage for empty environment variable handling.The test correctly verifies that empty environment variables are treated as missing, which aligns with the implementation logic and prevents security issues.
packages/schemas/src/v3/shared.schema.test.ts (4)
1-10: LGTM! Proper schema validation test setup.The test setup with Ajv and strict mode disabled is appropriate for testing the schema definitions. The imports are clean and well-organized.
35-45: Excellent security test for preventing direct string tokens.This test ensures that direct string tokens are rejected by the schema, which is a crucial security measure to enforce the object-based token structure.
80-100: Important validation tests for empty string security.These tests verify that the minLength constraints are working correctly to prevent empty secret names and environment variable names, which is essential for security.
114-154: Comprehensive GitRevisions schema validation.The GitRevisions tests provide good coverage of valid scenarios, empty objects, and proper rejection of additional properties. The test structure is consistent and thorough.
docs/docs/connections/gerrit.mdx (2)
9-24: LGTM! Clear authentication status and methods.The documentation clearly states that Gerrit authentication is supported and provides a comprehensive overview of available authentication methods. This addresses any previous confusion about authentication support.
205-271: Excellent troubleshooting section with actionable solutions.The troubleshooting section provides comprehensive coverage of common issues with specific solutions and test commands. This is valuable for users experiencing authentication or connection problems.
packages/backend/src/gerrit.test.ts (5)
1-46: Well-structured test setup with proper security considerations.The test file demonstrates excellent practices:
- Comprehensive mocking of all dependencies
- Security-focused token validation that rejects string tokens
- Proper cleanup with beforeEach/afterEach hooks
- Realistic mock implementations that mirror actual behavior
58-300: Comprehensive test coverage for project exclusion logic.The
shouldExcludeProjecttests thoroughly cover:
- Special Gerrit projects exclusion
- State-based exclusion (READ_ONLY, HIDDEN)
- Glob pattern matching for project names
- Complex scenarios with multiple exclusion criteria
- Edge cases and case sensitivity
The test cases are well-organized and demonstrate good understanding of the business logic.
304-476: Excellent authentication test coverage.The authentication tests effectively validate:
- Public access without authentication (proper endpoint usage)
- HTTP Basic Auth with username/password
- Environment variable and secret-based password retrieval
- Proper Authorization header formatting
- Authentication failure scenarios
The tests correctly verify that public endpoints don't include
/a/prefix while authenticated endpoints do.
510-571: Good handling of Gerrit-specific protocol details.The tests properly validate:
- XSSI protection prefix removal
- Pagination handling with
_more_projectsflag- Sequential API calls with proper offset parameters
955-981: Important security validation tests.The security tests correctly validate rejection of:
- Direct string passwords (enforcing secure credential storage)
- Malformed token configurations
This aligns with the security-focused approach mentioned in the PR objectives.
packages/backend/src/gerrit.ts (6)
3-8: Good import organization with proper typing.The imports are well-organized and include all necessary dependencies for authentication support.
25-40: Proper interface definitions for authentication.The exported
GerritProjectinterface and newGerritAuthConfiginterface are well-defined and support the authentication functionality.
44-67: Secure credential retrieval implementation.The authentication logic properly:
- Uses
getTokenFromConfigfor secure credential retrieval- Handles both environment variables and secrets
- Provides proper error handling and logging
- Maintains security by not logging sensitive information
110-129: Proper HTTP Basic Authentication implementation.The authentication implementation correctly:
- Uses authenticated endpoint (
/a/projects/) when credentials are provided- Implements standard HTTP Basic Authentication with Base64 encoding
- Includes proper headers and user agent
141-148: Enhanced error handling with better context.The error handling improvements include:
- More detailed error messages with response text
- Better logging context
- Proper exception propagation
- Authentication status in error context
197-243: Well-implemented project exclusion logic.The
shouldExcludeProjectfunction is now properly exported and includes:
- Clear exclusion logic for special Gerrit projects
- State-based exclusion (READ_ONLY, HIDDEN)
- Glob pattern matching
- Proper logging of exclusion reasons
zuharz
force-pushed
the
feature/gerrit-authentication
branch
from
333d45b to
a81ec65
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (38)
.gitignore(1 hunks)docs/docs/connections/gerrit.mdx(1 hunks)docs/snippets/schemas/v3/bitbucket.schema.mdx(2 hunks)docs/snippets/schemas/v3/connection.schema.mdx(9 hunks)docs/snippets/schemas/v3/gerrit.schema.mdx(1 hunks)docs/snippets/schemas/v3/gitea.schema.mdx(2 hunks)docs/snippets/schemas/v3/github.schema.mdx(2 hunks)docs/snippets/schemas/v3/gitlab.schema.mdx(2 hunks)docs/snippets/schemas/v3/index.schema.mdx(9 hunks)docs/snippets/schemas/v3/shared.schema.mdx(2 hunks)packages/backend/src/connectionManager.ts(1 hunks)packages/backend/src/gerrit.test.ts(1 hunks)packages/backend/src/gerrit.ts(6 hunks)packages/backend/src/github.ts(1 hunks)packages/backend/src/repoCompileUtils.ts(1 hunks)packages/backend/src/repoManager.ts(3 hunks)packages/backend/src/utils.ts(2 hunks)packages/crypto/package.json(2 hunks)packages/crypto/src/tokenUtils.test.ts(1 hunks)packages/crypto/src/tokenUtils.ts(1 hunks)packages/crypto/vitest.config.ts(1 hunks)packages/schemas/package.json(1 hunks)packages/schemas/src/v3/bitbucket.schema.ts(2 hunks)packages/schemas/src/v3/connection.schema.ts(9 hunks)packages/schemas/src/v3/connection.type.ts(1 hunks)packages/schemas/src/v3/gerrit.schema.ts(1 hunks)packages/schemas/src/v3/gerrit.type.ts(1 hunks)packages/schemas/src/v3/gitea.schema.ts(2 hunks)packages/schemas/src/v3/github.schema.ts(2 hunks)packages/schemas/src/v3/gitlab.schema.ts(2 hunks)packages/schemas/src/v3/index.schema.ts(9 hunks)packages/schemas/src/v3/index.type.ts(1 hunks)packages/schemas/src/v3/shared.schema.test.ts(1 hunks)packages/schemas/src/v3/shared.schema.ts(2 hunks)packages/schemas/vitest.config.ts(1 hunks)packages/web/src/actions.ts(1 hunks)schemas/v3/gerrit.json(1 hunks)schemas/v3/shared.json(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/snippets/schemas/v3/gitlab.schema.mdx
- packages/schemas/package.json
🚧 Files skipped from review as they are similar to previous changes (32)
- packages/crypto/vitest.config.ts
- .gitignore
- packages/schemas/vitest.config.ts
- packages/backend/src/github.ts
- packages/backend/src/utils.ts
- packages/crypto/src/tokenUtils.ts
- docs/snippets/schemas/v3/bitbucket.schema.mdx
- packages/schemas/src/v3/shared.schema.ts
- packages/schemas/src/v3/gitlab.schema.ts
- schemas/v3/gerrit.json
- schemas/v3/shared.json
- packages/schemas/src/v3/gitea.schema.ts
- docs/snippets/schemas/v3/github.schema.mdx
- packages/schemas/src/v3/github.schema.ts
- packages/backend/src/connectionManager.ts
- docs/snippets/schemas/v3/shared.schema.mdx
- docs/snippets/schemas/v3/gitea.schema.mdx
- packages/schemas/src/v3/connection.type.ts
- packages/web/src/actions.ts
- packages/schemas/src/v3/gerrit.schema.ts
- packages/schemas/src/v3/index.type.ts
- packages/backend/src/repoManager.ts
- packages/schemas/src/v3/bitbucket.schema.ts
- packages/crypto/package.json
- docs/docs/connections/gerrit.mdx
- packages/schemas/src/v3/gerrit.type.ts
- packages/backend/src/repoCompileUtils.ts
- docs/snippets/schemas/v3/index.schema.mdx
- packages/schemas/src/v3/index.schema.ts
- docs/snippets/schemas/v3/connection.schema.mdx
- docs/snippets/schemas/v3/gerrit.schema.mdx
- packages/schemas/src/v3/connection.schema.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/crypto/src/tokenUtils.test.ts
[error] 22-22: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 23-23: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (8)
packages/crypto/src/tokenUtils.test.ts (1)
1-96: Excellent comprehensive test coverage!This test suite provides thorough coverage of the
getTokenFromConfigfunction, including:
- Secret-based token retrieval
- Environment variable token retrieval
- Security measures (rejecting string tokens)
- Error handling for missing secrets/environment variables
- Edge cases with empty values
The test structure is well-organized and follows good testing practices.
packages/schemas/src/v3/shared.schema.test.ts (1)
1-155: Excellent schema validation test coverage!This test suite thoroughly validates the JSON schema definitions with comprehensive coverage of:
- Token schema validation with minLength constraints
- Security measures (rejecting string tokens and empty values)
- GitRevisions schema validation
- Proper error handling and validation messages
- Edge cases and malformed inputs
The use of Ajv with proper setup and the structured test organization demonstrate good testing practices.
packages/backend/src/gerrit.test.ts (1)
1-1011: Outstanding comprehensive test coverage for Gerrit integration!This test suite provides exceptional coverage of the Gerrit backend functionality, including:
Authentication Testing:
- Public access without credentials
- HTTP Basic Auth with username/password
- Environment variable and secret-based password retrieval
- Authentication error handling
- Security measures (rejecting invalid token formats)
Project Management:
- Project filtering and exclusion logic
- Pagination handling
- Special character handling in project names
- XSSI prefix stripping
- Large response handling
Edge Cases:
- Network errors
- Malformed JSON responses
- Empty responses
- Concurrent requests
- Mixed authentication scenarios
The test structure is excellent with proper mocking, isolation, and comprehensive assertions. The security testing is particularly noteworthy.
packages/backend/src/gerrit.ts (5)
52-67: Excellent secure authentication credential handling!The implementation properly:
- Uses
getTokenFromConfigfor secure credential retrieval- Handles both secret and environment variable-based passwords
- Includes comprehensive error handling and logging
- Provides clear debugging information about authentication status
The security-first approach of rejecting inline passwords is commendable.
110-129: Proper authentication header implementation!The authentication logic correctly:
- Uses authenticated endpoints (
/a/) when credentials are provided- Implements HTTP Basic Authentication with proper base64 encoding
- Includes appropriate headers and user agent
- Provides clear debugging information
The conditional endpoint selection and header preparation are well-implemented.
168-171: Good documentation addressing past review feedback!The implementation properly addresses the past review comment about XSSI prefix handling with:
- Clear documentation about the purpose of the regex
- Conditional prefix removal that only matches at the start
- Proper explanation of the security measure
This addresses the reviewer's concern about ensuring the pattern only matches at the beginning of the response.
112-113: Documentation link addresses past review feedback!The comment properly addresses the past review request by linking to the Gerrit REST API documentation that explains the authentication endpoint structure (
/a/prefix for authenticated endpoints).
141-148: Comprehensive error handling with authentication context!The error handling properly:
- Captures detailed error information including status and URL
- Includes authentication context in error metadata
- Provides clear logging with endpoint and authentication status
- Uses proper BackendException with structured error data
This will greatly help with debugging authentication issues.
zuharz
force-pushed
the
feature/gerrit-authentication
branch
from
6f40dea to
b4d007d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
docs/docs/connections/gerrit.mdx (3)
19-23: Clarify intro sentence for the auth-method list“Gerrit supports these authentication methods with Sourcebot:” reads a bit awkwardly and puts the subject last.
Suggest rephrasing to active voice:-Gerrit supports these authentication methods with Sourcebot: +Sourcebot supports the following Gerrit authentication methods:
48-55: Add an explicit warning about committing secretsRight after the authenticated JSON example it would help to remind users never to commit plaintext credentials or resolved environment values to source control (even in generated configs). A one-liner beneath the snippet should suffice.
74-85: Document automatic “/a/” insertion for Basic-Auth URLsThe backend now injects the
/a/path segment when credentials are present (compileGerritConfiglogic). Calling this out here will prevent confusion when users notice the modified clone URL.A brief note such as “Sourcebot automatically adds the
/a/prefix required by Gerrit for authenticated endpoints” would cover it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
docs/docs/connections/gerrit.mdx(1 hunks)docs/snippets/schemas/v3/index.schema.mdx(9 hunks)packages/schemas/src/v3/index.schema.ts(9 hunks)packages/schemas/src/v3/index.type.ts(1 hunks)packages/web/src/actions.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/web/src/actions.ts
- packages/schemas/src/v3/index.type.ts
- docs/snippets/schemas/v3/index.schema.mdx
- packages/schemas/src/v3/index.schema.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit Inference Engine (.cursor/rules/style.mdc)
Filenames should always be camelCase. Exception: if there are filenames in the same directory with a format other than camelCase, use that format to keep things consistent.
Files:
docs/docs/connections/gerrit.mdx
- Implement HTTP Basic Auth with username/password credentials - Add support for environment variables and secrets for secure credential storage - Create comprehensive test suite with 32 tests covering all authentication scenarios - Add automatic URL encoding for special characters (/, +, =) in passwords - Include complete documentation with troubleshooting guide - Support project filtering and exclusion rules (hidden, read-only, glob patterns) - Update JSON schemas to support both string and object password formats - Add Gerrit connection support to web UI - Fix logger references and add proper error handling - All tests passing (72 total tests: 55 backend + 17 web) Breaking changes: None Closes: #[issue-number]
Address CodeRabbit feedback by making the security warning more explicit about the risks of using direct string tokens in production environments.
…ct tokens - Regenerate TypeScript schema files from updated shared.json - Apply stronger security warning consistently across all connection types: 'SECURITY RISK: not recommended for production - use secrets or environment variables instead' - Update documentation snippets to reflect the enhanced security warnings - Address CodeRabbit feedback about explicit security risks of hardcoded tokens This change affects all connection types (GitHub, GitLab, Gitea, Bitbucket, Gerrit) to ensure users are properly warned about the security implications of direct token usage.
- Added minLength: 1 constraint to Token schema definition in shared.json - Prevents empty string tokens that would cause runtime HTTP errors - Regenerated all schema documentation files (.mdx) and TypeScript definitions - Ensures consistent validation across all connection types (GitHub, GitLab, Gitea, Bitbucket, Gerrit) This addresses CodeRabbit bot's review comment about preventing zero-length tokens at the schema level rather than failing at runtime during HTTP requests.
- Remove string token support as it's considered a security footgun - V3 intentionally removed this feature from V2 - Tokens must now use secret or env references only Addresses: brendan-kellam's security concern
- Remove entries for test files with real credentials - Integration tests will use environment variables in the future Addresses: brendan-kellam's feedback on test credential handling
- Add comment linking to official Gerrit auth documentation - Clarifies why we use /a/ prefix for authenticated endpoints Addresses: brendan-kellam's request for documentation link
- Use regex to ensure prefix is only removed from start of string - Make removal conditional on prefix presence - Prevents accidental removal from response body Addresses: brendan-kellam's question about regex rationale
- Remove separate troubleshooting file (content already in main doc) - Remove unnecessary sections: Implementation Details, Manual Testing Script, Debug Steps - Remove unnecessary Note at the end - Keep documentation concise and focused Addresses: brendan-kellam's feedback on documentation cleanup
- Add tests for token validation and security constraints - Add tests for XSSI handling - Add schema validation tests - Ensure all edge cases are covered - Remove unused isRemotePath function from utils Builds on: brendan-kellam's positive feedback on mocked tests
- Regenerate TypeScript types from updated schemas - Update schema documentation snippets - Update lockfile with test dependencies Auto-generated changes from schema modifications
- Remove troubleshooting section from Gerrit documentation per review feedback - Replace delete operator with undefined assignment for better performance in tests
- Replace deprecated @testing-library/react-hooks with @testing-library/react - Add @testing-library/react and @testing-library/dom as devDependencies - Update renderHook imports in useExtractReferences.test.ts and useMessagePairs.test.ts - All 115 web package tests now pass successfully 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
zuharz
force-pushed
the
feature/gerrit-authentication
branch
from
b469cb4 to
2a1eff4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/backend/src/gerrit.ts (1)
90-94: Bug: Empty include array filters out all projects. Check for length > 0.An empty array is truthy, so this path executes and filters everything out. Only apply the include filter when there are patterns.
- if (config.projects) { + if (config.projects?.length) { projects = projects.filter((project) => { return micromatch.isMatch(project.name, config.projects!); }); }
♻️ Duplicate comments (1)
packages/schemas/src/v3/connection.type.ts (1)
241-252: Nit: Doc text says “token” but this field is a passwordSame generator text as index.type.ts — consider updating the source schema description to say “password” for clarity.
🧹 Nitpick comments (18)
packages/web/src/features/chat/useMessagePairs.test.ts (1)
4-4: Consistent switch to@testing-library/react’srenderHookLooks correct and consistent with the other test.
Optional: to mirror the style used in
useExtractReferences.test.ts, you could destructure the return and useresultdirectly:const { result } = renderHook(() => useMessagePairs(messages)); expect(result.current).toEqual([...]);packages/web/package.json (2)
191-193: Vitest jsdom environment configured; add Jest-DOM setup if usedThe Testing Library devDependencies look good and Vitest in
packages/web/vitest.config.mtsalready runs withenvironment: 'jsdom'. However, it doesn’t include Jest-DOM matchers by default. If you plan to use@testing-library/jest-dommatchers in your Vitest tests, add them viasetupFiles.Locations to review:
- packages/web/vitest.config.mts
•environment: 'jsdom'is already set
• MissingsetupFiles: ['@testing-library/jest-dom/vitest']Suggested update to
packages/web/vitest.config.mts:import { defineConfig } from 'vitest/config' export default defineConfig({ plugins: [tsconfigPaths()], test: { environment: 'jsdom', + setupFiles: ['@testing-library/jest-dom/vitest'], watch: false, }, })
191-193: Drop@testing-library/react-hooksfrom devDependencies
No remaining imports or requires of@testing-library/react-hookswere found in.ts,.tsx,.js, or.jsxfiles—safe to remove this redundant package.• packages/web/package.json
"devDependencies": { "@tanstack/eslint-plugin-query": "^5.74.7", "@testing-library/dom": "^10.4.1", "@testing-library/jest-dom": "^6.7.0", "@testing-library/react": "^16.3.0", - "@testing-library/react-hooks": "^8.0.1", "@types/micromatch": "^4.0.9",packages/schemas/src/v3/index.type.ts (1)
366-377: Nit: Doc text says “token” but this field is actually a passwordFor clarity, consider updating the source schema description so generated docs read “password” (or “credential”) rather than “token”.
docs/docs/connections/gerrit.mdx (4)
9-23: Tighten wording and punctuation in the “Authentication Methods” listAdd terminal punctuation and clarify phrasing.
Apply:
-1. **Public Access**: For publicly accessible projects (no authentication required) -2. **HTTP Basic Auth**: Using Gerrit username and HTTP password +1. **Public Access**: For publicly accessible projects (no authentication required). +2. **HTTP Basic Auth**: Using your Gerrit username and HTTP password. + +<Note> +Environment variable references (auth.password.env) are only supported in declarative connection configs. +</Note>
58-73: Optional: Mention Docker Compose and quoting caveats for special charsUsers often run via Compose; adding a Compose env example and a short note about quoting can reduce support churn.
Proposed addition after the Docker snippet:
+When using Docker Compose: + +```yaml +services: + sourcebot: + image: sourcebot:latest + environment: + - GERRIT_HTTP_PASSWORD=${GERRIT_HTTP_PASSWORD} +``` + +Note: If your password contains shell-interpreted characters, prefer using environment files or Compose variable substitution to avoid quoting issues.
74-85: Optional: Call out Gerrit “/a/” authenticated prefix behaviorSince the backend uses the /a/ prefix when auth is provided, a brief note helps users correlate logs and behavior with Gerrit docs.
Suggested addition at the end of this section:
+<Info> +When authentication is configured, Sourcebot uses Gerrit’s `/a/` prefix for authenticated endpoints where applicable (per Gerrit REST API docs). +</Info>
134-160: Use “projects” consistently in the heading (Gerrit domain term)The schema and JSON use “projects”; heading says “repos”. Align wording to avoid confusion.
Apply:
- <Accordion title="Exclude repos from syncing"> + <Accordion title="Exclude projects from syncing">packages/schemas/src/v3/index.schema.ts (1)
868-926: Gerrit auth shape looks solid; add minLength on username and fix wordingThe auth block is well-constrained (required username/password, additionalProperties: false, env/secret-only password). Two improvements:
- Enforce username to be non-empty (minLength: 1).
- The password option descriptions say “token”; change to “password” for clarity.
Apply within this block:
"username": { - "type": "string", + "type": "string", + "minLength": 1, "description": "Gerrit username for authentication", "examples": [ "john.doe" ] }, "password": { @@ "secret": { "type": "string", "minLength": 1, - "description": "The name of the secret that contains the token." + "description": "The name of the secret that contains the password." } @@ "env": { "type": "string", "minLength": 1, - "description": "The name of the environment variable that contains the token. Only supported in declarative connection configs." + "description": "The name of the environment variable that contains the password. Only supported in declarative connection configs." }packages/crypto/src/tokenUtils.test.ts (3)
27-46: Assert decrypt is invoked with expected IV/value to fully validate secret pathAdd an assertion to confirm we pass the right data into decrypt, increasing coverage of the secret branch.
const result = await getTokenFromConfig(config, testOrgId, mockPrisma); expect(result).toBe('decrypted-secret-value'); + // Verify we invoked decrypt with the secret's IV and encrypted value + const { decrypt } = await import('./index.js'); + expect(decrypt).toHaveBeenCalledWith('test-iv', 'encrypted-value'); expect(mockPrisma.secret.findUnique).toHaveBeenCalledWith({ where: { orgId_key: { key: 'my-secret', orgId: testOrgId } } });If you prefer a static import for the mock:
// at top of file import { decrypt } from './index.js';
48-55: Optional: assert that DB isn’t queried for env tokensThis makes the env branch expectations explicit.
const config = { env: 'TEST_TOKEN' }; const result = await getTokenFromConfig(config, testOrgId, mockPrisma); expect(result).toBe('env-token-value'); + expect(mockPrisma.secret.findUnique).not.toHaveBeenCalled();
71-78: Optional: guard against accidental decrypt calls on missing secretEnsures we short-circuit before decryption when secret lookup fails.
const config = { secret: 'non-existent-secret' }; await expect(getTokenFromConfig(config, testOrgId, mockPrisma)) .rejects.toThrow('Secret with key non-existent-secret not found for org 1'); + const { decrypt } = await import('./index.js'); + expect(decrypt).not.toHaveBeenCalled();docs/snippets/schemas/v3/index.schema.mdx (2)
873-881: Enforce non-empty Gerrit username (minLength: 1).To mirror the non-empty requirement on password references and avoid empty usernames at validation time, add minLength: 1 to auth.username.
"username": { "type": "string", + "minLength": 1, "description": "Gerrit username for authentication", "examples": [ "john.doe" ] },
894-899: Clarify Gerrit password reference descriptions (token → password).These descriptions still say “token” for Gerrit’s HTTP password references. Suggest updating to reduce confusion.
"secret": { "type": "string", "minLength": 1, - "description": "The name of the secret that contains the token." + "description": "The name of the secret that contains the HTTP password." }"env": { "type": "string", "minLength": 1, - "description": "The name of the environment variable that contains the token. Only supported in declarative connection configs." + "description": "The name of the environment variable that contains the HTTP password. Only supported in declarative connection configs." }Also applies to: 908-913
packages/backend/src/gerrit.ts (2)
169-172: Make XSSI prefix removal resilient to missing newline.Gerrit normally prefixes with “)]}’\n”, but guarding with startsWith and requiring a newline in the regex can break if the newline isn’t present. Use an anchored replace that tolerates any whitespace.
- const jsonText = text.startsWith(")]}'") ? text.replace(/^\)\]\}'\n/, '') : text; + const jsonText = text.replace(/^\)\]\}'\s*/, '');
158-165: Network error handling: err.code may be undefined/non-numeric.Default status to 0 and include the error message to improve diagnostics. Current code can log “status undefined” or a non-numeric code.
- const status = (err as any).code; - logger.error(`Failed to fetch projects from Gerrit at ${endpointWithParams} with status ${status}`); - throw new BackendException(BackendError.CONNECTION_SYNC_FAILED_TO_FETCH_GERRIT_PROJECTS, { - status: status, + const status = (err as any).code ?? 0; + const message = (err as Error)?.message ?? String(err); + logger.error(`Failed to fetch projects from Gerrit at ${endpointWithParams} with status ${status}: ${message}`); + throw new BackendException(BackendError.CONNECTION_SYNC_FAILED_TO_FETCH_GERRIT_PROJECTS, { + status, url: endpointWithParams, authenticated: !!auth });packages/schemas/src/v3/connection.schema.ts (2)
609-616: Enforce non-empty Gerrit username (minLength: 1).Add minLength: 1 to auth.username to keep inputs consistent and avoid empty usernames.
"username": { "type": "string", + "minLength": 1, "description": "Gerrit username for authentication", "examples": [ "john.doe" ] },
631-634: Clarify Gerrit password reference descriptions (token → password).These descriptions still use “token” for the Gerrit HTTP password references. Suggest updating for clarity.
"secret": { "type": "string", "minLength": 1, - "description": "The name of the secret that contains the token." + "description": "The name of the secret that contains the HTTP password." }"env": { "type": "string", "minLength": 1, - "description": "The name of the environment variable that contains the token. Only supported in declarative connection configs." + "description": "The name of the environment variable that contains the HTTP password. Only supported in declarative connection configs." }Also applies to: 645-648
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (41)
.gitignore(1 hunks)docs/docs/connections/gerrit.mdx(1 hunks)docs/snippets/schemas/v3/bitbucket.schema.mdx(2 hunks)docs/snippets/schemas/v3/connection.schema.mdx(9 hunks)docs/snippets/schemas/v3/gerrit.schema.mdx(1 hunks)docs/snippets/schemas/v3/gitea.schema.mdx(2 hunks)docs/snippets/schemas/v3/github.schema.mdx(2 hunks)docs/snippets/schemas/v3/gitlab.schema.mdx(2 hunks)docs/snippets/schemas/v3/index.schema.mdx(9 hunks)docs/snippets/schemas/v3/shared.schema.mdx(2 hunks)packages/backend/src/connectionManager.ts(1 hunks)packages/backend/src/gerrit.test.ts(1 hunks)packages/backend/src/gerrit.ts(6 hunks)packages/backend/src/github.ts(1 hunks)packages/backend/src/repoCompileUtils.ts(1 hunks)packages/backend/src/repoManager.ts(3 hunks)packages/backend/src/utils.ts(2 hunks)packages/crypto/package.json(2 hunks)packages/crypto/src/tokenUtils.test.ts(1 hunks)packages/crypto/src/tokenUtils.ts(1 hunks)packages/crypto/vitest.config.ts(1 hunks)packages/schemas/package.json(1 hunks)packages/schemas/src/v3/bitbucket.schema.ts(2 hunks)packages/schemas/src/v3/connection.schema.ts(9 hunks)packages/schemas/src/v3/connection.type.ts(1 hunks)packages/schemas/src/v3/gerrit.schema.ts(1 hunks)packages/schemas/src/v3/gerrit.type.ts(1 hunks)packages/schemas/src/v3/gitea.schema.ts(2 hunks)packages/schemas/src/v3/github.schema.ts(2 hunks)packages/schemas/src/v3/gitlab.schema.ts(2 hunks)packages/schemas/src/v3/index.schema.ts(9 hunks)packages/schemas/src/v3/index.type.ts(1 hunks)packages/schemas/src/v3/shared.schema.test.ts(1 hunks)packages/schemas/src/v3/shared.schema.ts(2 hunks)packages/schemas/vitest.config.ts(1 hunks)packages/web/package.json(1 hunks)packages/web/src/actions.ts(1 hunks)packages/web/src/features/chat/useExtractReferences.test.ts(1 hunks)packages/web/src/features/chat/useMessagePairs.test.ts(1 hunks)schemas/v3/gerrit.json(1 hunks)schemas/v3/shared.json(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (26)
- packages/schemas/vitest.config.ts
- packages/backend/src/github.ts
- schemas/v3/shared.json
- packages/schemas/src/v3/github.schema.ts
- docs/snippets/schemas/v3/shared.schema.mdx
- packages/schemas/src/v3/shared.schema.test.ts
- packages/crypto/src/tokenUtils.ts
- packages/backend/src/connectionManager.ts
- packages/web/src/actions.ts
- packages/crypto/vitest.config.ts
- docs/snippets/schemas/v3/github.schema.mdx
- packages/backend/src/gerrit.test.ts
- packages/schemas/src/v3/gerrit.type.ts
- packages/backend/src/repoCompileUtils.ts
- docs/snippets/schemas/v3/gitea.schema.mdx
- packages/schemas/src/v3/shared.schema.ts
- docs/snippets/schemas/v3/gitlab.schema.mdx
- docs/snippets/schemas/v3/bitbucket.schema.mdx
- packages/schemas/package.json
- packages/crypto/package.json
- docs/snippets/schemas/v3/gerrit.schema.mdx
- docs/snippets/schemas/v3/connection.schema.mdx
- schemas/v3/gerrit.json
- packages/backend/src/repoManager.ts
- packages/schemas/src/v3/gerrit.schema.ts
- packages/schemas/src/v3/gitlab.schema.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit Inference Engine (.cursor/rules/style.mdc)
Filenames should always be camelCase. Exception: if there are filenames in the same directory with a format other than camelCase, use that format to keep things consistent.
Files:
packages/web/package.jsonpackages/web/src/features/chat/useMessagePairs.test.tspackages/crypto/src/tokenUtils.test.tspackages/schemas/src/v3/gitea.schema.tspackages/schemas/src/v3/bitbucket.schema.tspackages/web/src/features/chat/useExtractReferences.test.tspackages/schemas/src/v3/connection.type.tspackages/schemas/src/v3/index.schema.tspackages/backend/src/utils.tspackages/schemas/src/v3/index.type.tspackages/backend/src/gerrit.tsdocs/snippets/schemas/v3/index.schema.mdxpackages/schemas/src/v3/connection.schema.tsdocs/docs/connections/gerrit.mdx
🧬 Code Graph Analysis (3)
packages/crypto/src/tokenUtils.test.ts (1)
packages/crypto/src/tokenUtils.ts (1)
getTokenFromConfig(5-37)
packages/backend/src/utils.ts (2)
packages/crypto/src/tokenUtils.ts (1)
getTokenFromConfig(5-37)packages/schemas/src/v3/shared.type.ts (1)
Token(7-19)
packages/backend/src/gerrit.ts (3)
packages/schemas/src/v3/index.type.ts (1)
GerritConnectionConfig(345-398)packages/crypto/src/tokenUtils.ts (1)
getTokenFromConfig(5-37)packages/backend/src/utils.ts (1)
getTokenFromConfig(25-39)
🪛 LanguageTool
docs/docs/connections/gerrit.mdx
[grammar] ~21-~21: There might be a mistake here.
Context: ...le projects (no authentication required) 2. HTTP Basic Auth: Using Gerrit username...
(QB_NEW_EN)
🔇 Additional comments (24)
packages/web/src/features/chat/useExtractReferences.test.ts (1)
2-2: Migration Complete – No Remainingreact-hooksImportsAll references to
@testing-library/react-hookshave been removed. The switch torenderHookfrom@testing-library/reactaligns with best practices and is consistent across.ts/.tsxfiles. Approving these changes.packages/schemas/src/v3/gitea.schema.ts (2)
21-26: Add minLength: 1 for token.secret — LGTMThis prevents empty secret names and aligns with the PR objective to enforce non-empty references.
36-41: Add minLength: 1 for token.env — LGTMGood defensive validation for env var names; consistent with other providers.
packages/schemas/src/v3/bitbucket.schema.ts (2)
26-31: Add minLength: 1 for token.secret — LGTMPrevents empty secret identifiers; aligns with cross-provider constraints in this PR.
40-45: Add minLength: 1 for token.env — LGTMEnsures env var name is non-empty; consistent with the shared token validation approach.
packages/schemas/src/v3/index.type.ts (2)
354-378: Gerrit auth type addition — LGTMOptional auth with username + secret/env password is clearly modeled and documented.
354-378: JSON schema already enforcesminLengthandadditionalPropertiesThe Gerrit JSON schemas already include the requested constraints:
• In schemas/v3/shared.json, the
Tokendefinition (used byauth.password) has both
–"minLength": 1onsecretandenv
–"additionalProperties": falsefor each branch of theanyOf.
• In packages/schemas/src/v3/gerrit.schema.ts and schemas/v3/gerrit.json, theauthobject itself declares
–"additionalProperties": false
–passwordreferencing theTokenschema above (so it inherits the sameminLengthandadditionalPropertiesrules).No changes are needed.
Likely an incorrect or invalid review comment.
packages/schemas/src/v3/connection.type.ts (1)
229-253: Gerrit auth type addition — LGTMMatches the structure added in index.type.ts; consistent and forward-compatible.
docs/docs/connections/gerrit.mdx (5)
28-38: Public connection example — LGTMClear and minimal; matches the schema.
40-56: Authenticated connection example — LGTMUses env for the password (preferred), aligns with the schema and PR guidelines.
89-113: Examples (public/private) — LGTMGood breadth and consistent with the schema and backend behavior.
115-132: Glob pattern example — LGTMIncludes auth and realistic patterns; looks good.
162-170: Schema reference section — LGTMAccurate link and embedded snippet; matches repo structure.
packages/backend/src/utils.ts (3)
6-6: Type import aligns with shared schema Token shapeImporting Token directly from the shared types keeps the wrapper’s signature precise and consistent with schema constraints.
24-24: No comment necessary on whitespace-only change
25-39: Ensure wrapper is used everywhere and refine its signatureThe ripgrep results reveal that many modules still import
getTokenFromConfigdirectly from@sourcebot/crypto, bypassing your new wrapper inpackages/backend/src/utils.ts. To centralize error mapping/logging, you’ll need to migrate these call sites to use the wrapper (once it’s moved into a shared utility package), and update its signature and error‐code logic as discussed.Call sites to update:
- packages/web/src/actions.ts
- packages/web/src/features/chat/actions.ts
- packages/crypto/src/tokenUtils.test.ts
- (Plus any other direct imports of
getTokenFromConfigfrom@sourcebot/crypto)Backend modules already using the wrapper (imported from
packages/backend/src/utils.ts):
- packages/backend/src/repoManager.ts
- packages/backend/src/github.ts
- packages/backend/src/gerrit.ts
- packages/backend/src/gitlab.ts
- packages/backend/src/gitea.ts
- packages/backend/src/bitbucket.ts
Proposed diff for the wrapper (
packages/backend/src/utils.ts):-export const getTokenFromConfig = async (token: Token, orgId: number, db: PrismaClient, logger?: Logger) => { +export const getTokenFromConfig = async ( + token: Token, + orgId: number, + db: PrismaClient, + logger?: Logger +): Promise<string> => { try { return await getTokenFromConfigBase(token, orgId, db); } catch (error: unknown) { if (error instanceof Error) { - const e = new BackendException(BackendError.CONNECTION_SYNC_SECRET_DNE, { - message: error.message, - }); + // Derive more accurate error codes and preserve cause + const isSecret = typeof token !== 'string' && 'secret' in token; + const code = + /Invalid token configuration/i.test(error.message) + ? BackendError.CONNECTION_SYNC_INVALID_TOKEN_CONFIG + : isSecret + ? BackendError.CONNECTION_SYNC_SECRET_DNE + : BackendError.CONNECTION_SYNC_ENVVAR_DNE; + const e = new BackendException(code, { + message: error.message, + orgId, + tokenSource: isSecret ? 'secret' : 'env', + } /*, { cause: error }*/); Sentry.captureException(e); - logger?.error(error.message); + logger?.error(e.message, { orgId, tokenSource: isSecret ? 'secret' : 'env' }); throw e; } throw error; } };Please migrate all direct imports of
getTokenFromConfigto this wrapper (or its extracted shared location), then verify that no call site is left pointing at@sourcebot/crypto.packages/schemas/src/v3/index.schema.ts (1)
291-292: Good tightening: enforce non-empty secret/env strings for tokensAdding minLength: 1 to token.secret and token.env across providers prevents silent misconfigurations and aligns with the PR’s removal of inline tokens.
Also applies to: 305-306, 506-507, 520-521, 710-711, 724-725, 1043-1044, 1057-1058
packages/crypto/src/tokenUtils.test.ts (2)
5-8: Mocking decrypt is appropriate and keeps tests hermeticMocking ./index.js to control decrypt makes the secret flow deterministic.
14-24: Nice: environment cleanup aligns with lint guidanceSwitching from delete to assigning undefined resolves the earlier noDelete lint and avoids perf pitfalls. This also makes test isolation explicit.
docs/snippets/schemas/v3/index.schema.mdx (1)
292-307: LGTM: Non-empty token/env guards added across providers.The added minLength: 1 constraints for token.secret and token.env are correct and align with prior review feedback.
Also applies to: 507-522, 711-726, 1044-1059
packages/backend/src/gerrit.ts (3)
52-66: Auth flow, headers, endpoint selection, and error modeling look solid.
- Graceful credential retrieval with proper logging.
- Correct Basic header assembly and switch to /a/projects/.
- Good contextual logging and Sentry capture.
Also applies to: 118-129, 130-133, 141-149, 193-195
197-243: Exclusion logic reads well and covers common Gerrit project sentinels.Special projects, read-only, hidden, and glob exclusions are handled cleanly, with useful debug messages.
185-191: Pagination sentinel semantics verifiedTests in packages/backend/src/gerrit.test.ts (around lines 540–570) cover multi-page fetching and confirm that when a project entry’s
_more_projectsflag istrue,fetchAllProjectsissues a second request with?S=1. The Gerrit REST API uses a per-entry_more_projectssentinel (no top-level flag), so the existing loop and offset logic are correct.packages/schemas/src/v3/connection.schema.ts (1)
28-43: LGTM: Added minLength: 1 to secret/env across providers.These guards prevent empty token names at validation time and are consistent across GitHub, GitLab, Gitea, and Bitbucket.
Also applies to: 243-258, 447-462, 780-795
|
@brendan-kellam , pls, have a look. |
- Fix critical bug: empty projects array filtering out all repos - Add minLength validation for Gerrit usernames - Update Gerrit auth docs from "token" to "HTTP password" - Improve network error handling with better logging - Make XSSI prefix removal more resilient - Remove unused @testing-library/react-hooks dependency - Enhance test coverage with better assertions 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/snippets/schemas/v3/index.schema.mdx (1)
1-10: Regenerate the auto-generated schema artifacts to include your schema changesWe’ve confirmed that your source schema at
packages/schemas/src/v3/gerrit.schema.tsalready defines the newusernameproperty under theauthobject, but the MDX snippet indocs/snippets/schemas/v3/index.schema.mdx(and the corresponding TS artifacts) hasn’t been updated to reflect this change. Since these files are generated, please:
- Ensure any intended schema edits (e.g., adding/removing fields) are applied in the source schema (e.g.,
packages/schemas/src/v3/gerrit.schema.ts).- Re-run the project’s schema generation command (for example,
npm run generate-schemasoryarn schemas:generate) to regenerate both:
- The MDX documentation snippets under
docs/snippets/schemas/v3/…- The TypeScript definitions under
packages/schemas/src/v3/…Committing the updated generated files will keep your TS types and MDX docs in sync with the source schemas.
♻️ Duplicate comments (1)
packages/crypto/src/tokenUtils.test.ts (1)
21-24: Kudos for replacing delete with undefined assignment (matches earlier review).The env cleanup now avoids the
deleteperf hit flagged previously. Nothing else to do here.
🧹 Nitpick comments (11)
packages/schemas/src/v3/languageModel.schema.ts (1)
170-178: Consider deduplicating with a shared Token/$ref in the generator.There is a lot of repeated
anyOffor{ secret } | { env }with identicalminLengthand descriptions. If feasible in the schema generator, referencing a single shared definition (e.g.,#/definitions/TokenNonEmpty) would reduce drift and shrink output.If you want, I can sketch a generator-side change to emit
$refinstead of repeating the inlineanyOf.Also applies to: 338-346
packages/schemas/src/v3/shared.schema.ts (1)
23-29: Optional: tighten env var naming with a regex pattern.If you want additional guardrails, consider a conservative pattern for
env(e.g.,^[A-Za-z_][A-Za-z0-9_]*$) so obviously-invalid names are caught at validation time. Leave as-is if you intentionally allow broader names."properties": { "env": { "type": "string", + "pattern": "^[A-Za-z_][A-Za-z0-9_]*$", "minLength": 1, "description": "The name of the environment variable that contains the token. Only supported in declarative connection configs." } },packages/crypto/src/tokenUtils.test.ts (2)
5-9: Stabilize mock import path across TS/ESM builds.Using
./index.jscan be brittle depending on module resolution. Prefer extensionless./indexin bothvi.mockand the dynamic import to match howtokenUtilsimports it.-vi.mock('./index.js', () => ({ +vi.mock('./index', () => ({ decrypt: vi.fn().mockReturnValue('decrypted-secret-value') })); ... -const { decrypt } = await import('./index.js'); +const { decrypt } = await import('./index');Also applies to: 39-40
75-85: Optional: add a test for decrypt failure propagation.Mock
decryptto throw (e.g., missing key) and assert the error bubbles as expected. This ensures we don’t accidentally swallow crypto errors later.packages/backend/src/gerrit.ts (3)
130-139: Reduce pagination round-trips by requesting a larger page size.Gerrit supports the
nquery param to limit results per page. Default pages can be small; fetching e.g. 500 at a time cuts API calls and speeds syncs.- const endpointWithParams = `${projectsEndpoint}?S=${start}`; + const PAGE_SIZE = 500; // tune as appropriate, Gerrit may cap this + const endpointWithParams = `${projectsEndpoint}?S=${start}&n=${PAGE_SIZE}`;
168-173: Align comment with implemented regex for XSSI prefix.The code strips
^\)\]\}'\s*, not strictly a single newline. Update the comment to avoid confusion.-// The regex /^\)\]\}'\n/ matches the literal string ")]}'" at the start of the response -// followed by a newline character, which Gerrit adds to prevent JSON hijacking +// The regex /^\)\]\}'\s*/ matches the literal string ")]}'" at the start of the response, +// followed by any whitespace Gerrit may add to prevent JSON hijacking
186-192: Tighten typing for _more_projects probe (optional).You can avoid the
as anyby extending the project info type to include_more_projects?: boolean, matching Gerrit’s pagination marker.-interface GerritProjects { - [projectName: string]: GerritProjectInfo; -} +interface GerritProjectInfoWithMore extends GerritProjectInfo { + _more_projects?: boolean; +} +interface GerritProjects { + [projectName: string]: GerritProjectInfoWithMore; +} ... -hasMoreProjects = Object.values(data).some( - (project) => (project as any)._more_projects === true -); +hasMoreProjects = Object.values(data).some( + (project) => project._more_projects === true +);packages/schemas/src/v3/index.schema.ts (2)
893-899: Wording nit: say “password” instead of “token” in Gerrit password refs.The description strings under auth.password.{secret,env} say “contains the token,” which conflicts with the surrounding “HTTP password” wording. Prefer “contains the HTTP password.”
- "description": "The name of the secret that contains the token." + "description": "The name of the secret that contains the HTTP password." ... - "description": "The name of the environment variable that contains the token. Only supported in declarative connection configs." + "description": "The name of the environment variable that contains the HTTP password. Only supported in declarative connection configs."Also applies to: 907-913
872-920: Optional: tighten username format (no whitespace/colon).Basic Auth encodes "username:password". Colons or whitespace in the username can be problematic with some clients and server configs. If you want to prevent these outright at config time, add a conservative pattern.
"username": { "type": "string", "minLength": 1, + "pattern": "^[^:\\s]+$", "description": "Gerrit username for authentication",If you intentionally allow these characters, ignore this.
docs/snippets/schemas/v3/index.schema.mdx (2)
895-899: Docs wording nit: use “HTTP password” instead of “token”.Update the descriptions under auth.password.{secret,env} to say “contains the HTTP password” for consistency with the main description.
- "description": "The name of the secret that contains the token." + "description": "The name of the secret that contains the HTTP password." ... - "description": "The name of the environment variable that contains the token. Only supported in declarative connection configs." + "description": "The name of the environment variable that contains the HTTP password. Only supported in declarative connection configs."Also applies to: 909-913
869-927: Docs: optional format tightening example for username.If you adopt the optional username pattern constraint in the schema, reflect it here too.
"username": { - "type": "string", - "minLength": 1, + "type": "string", + "minLength": 1, + "pattern": "^[^:\\s]+$", "description": "Gerrit username for authentication",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
docs/snippets/schemas/v3/index.schema.mdx(109 hunks)docs/snippets/schemas/v3/languageModel.schema.mdx(100 hunks)docs/snippets/schemas/v3/shared.schema.mdx(4 hunks)packages/backend/src/gerrit.ts(7 hunks)packages/crypto/src/tokenUtils.test.ts(1 hunks)packages/schemas/src/v3/connection.schema.ts(9 hunks)packages/schemas/src/v3/gerrit.schema.ts(1 hunks)packages/schemas/src/v3/index.schema.ts(109 hunks)packages/schemas/src/v3/languageModel.schema.ts(100 hunks)packages/schemas/src/v3/shared.schema.ts(4 hunks)packages/web/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/schemas/src/v3/gerrit.schema.ts
- docs/snippets/schemas/v3/shared.schema.mdx
- packages/schemas/src/v3/connection.schema.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit inference engine (.cursor/rules/style.mdc)
Filenames should always be camelCase. Exception: if there are filenames in the same directory with a format other than camelCase, use that format to keep things consistent.
Files:
packages/schemas/src/v3/languageModel.schema.tspackages/crypto/src/tokenUtils.test.tspackages/backend/src/gerrit.tspackages/schemas/src/v3/index.schema.tspackages/web/package.jsonpackages/schemas/src/v3/shared.schema.tsdocs/snippets/schemas/v3/languageModel.schema.mdxdocs/snippets/schemas/v3/index.schema.mdx
🧬 Code graph analysis (2)
packages/crypto/src/tokenUtils.test.ts (2)
packages/crypto/src/tokenUtils.ts (1)
getTokenFromConfig(5-37)packages/crypto/src/index.ts (1)
decrypt(52-68)
packages/backend/src/gerrit.ts (3)
packages/schemas/src/v3/index.type.ts (1)
GerritConnectionConfig(345-398)packages/crypto/src/tokenUtils.ts (1)
getTokenFromConfig(5-37)packages/backend/src/utils.ts (1)
getTokenFromConfig(25-39)
🔇 Additional comments (10)
packages/schemas/src/v3/languageModel.schema.ts (1)
27-33: Non-empty secret/env enforcement is a solid hardening. LGTM.Adding
minLength: 1to token references and header token references across providers closes off accidental empty identifiers and aligns behavior with shared schema and docs. Looks consistent in bothdefinitionsandoneOfsections.Also applies to: 41-47, 116-121, 130-135, 1421-1426, 1435-1440
packages/schemas/src/v3/shared.schema.ts (1)
11-15: Shared non-empty constraints look correct and consistent.Adding
minLength: 1toToken.secret,Token.env, and header token references will prevent empty identifiers from slipping into runtime. This matches the language-model schemas and tests.Also applies to: 25-29, 93-97, 107-111
packages/crypto/src/tokenUtils.test.ts (1)
26-101: Great coverage of secret/env and error paths.Tests validate decryption path, Prisma lookup, env flow, and error messages. Clear alignment with runtime behavior.
docs/snippets/schemas/v3/languageModel.schema.mdx (1)
28-31: Docs accurately reflect non-empty secret/env constraints.The snippet mirrors the schema changes, including nested header credentials. Good consistency signal for users.
Also applies to: 42-45
packages/backend/src/gerrit.ts (3)
44-66: Auth retrieval flow and error wrapping look good.
- Uses shared
getTokenFromConfigwith orgId/db and wraps with logger/Sentry on failure.- Username is logged, password is not—good from a security standpoint.
110-129: Request headers and Basic auth construction are correct.
Accept+User-AgentplusAuthorization: Basic <b64(username:password)>is appropriate for Gerrit. No secrets leak to logs.
44-48: Verified: all call sites updated to new getGerritReposFromConfig signatureSearch across the backend code confirms every invocation now passes three arguments—
config,orgId, anddb(or their test equivalents)—matching the updated function signature. No outdated calls remain.packages/schemas/src/v3/index.schema.ts (2)
291-306: Good hardening: non-empty secret/env constraints added consistently.Adding minLength: 1 to credential references prevents silent misconfigurations and pushes failures to validation-time. This is applied consistently across providers and nested header credentials. LGTM.
Also applies to: 505-521, 710-725, 1043-1058, 1387-1403, 1447-1463, 1557-1573, 1610-1626
1011-1015: Confirm GerritConnectionConfig required fields remain correct
The GerritConnectionConfig schema in packages/schemas/src/v3/index.schema.ts intentionally makes only “type” and “url” required (auth stays optional), preserving support for unauthenticated public Gerrit instances. No changes are needed, but please verify the “required” array does not include “auth.”• File: packages/schemas/src/v3/index.schema.ts
• Lines 1011–1015:"required": [ "type", "url" ], "additionalProperties": falsePlease double-check that only “type” and “url” appear under “required.”
docs/snippets/schemas/v3/index.schema.mdx (1)
291-307: Docs: minLength hardening reflected correctly.The generated docs mirror the non-empty secret/env constraints across providers and header credentials. LGTM.
Also applies to: 505-522, 710-726, 1043-1059, 1388-1404, 1447-1464, 1558-1574, 1611-1627
| "type": "string", | ||
| "description": "Gerrit username for authentication", | ||
| "examples": [ | ||
| "john.doe" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Docs: require non-empty Gerrit username.
Mirror the schema change by adding minLength: 1 to auth.username in the docs snippet.
"username": {
- "type": "string",
+ "type": "string",
+ "minLength": 1,
"description": "Gerrit username for authentication",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "type": "string", | |
| "description": "Gerrit username for authentication", | |
| "examples": [ | |
| "john.doe" | |
| ] | |
| }, | |
| "username": { | |
| "type": "string", | |
| "minLength": 1, | |
| "description": "Gerrit username for authentication", | |
| "examples": [ | |
| "john.doe" | |
| ] | |
| }, |
🤖 Prompt for AI Agents
In docs/snippets/schemas/v3/index.schema.mdx around lines 874 to 879, the Gerrit
auth.username schema lacks a minLength constraint; update the "auth.username"
property to include "minLength: 1" so the docs reflect the non-empty username
requirement (keep the existing type, description and examples intact).
| "username": { | ||
| "type": "string", | ||
| "description": "Gerrit username for authentication", | ||
| "examples": [ | ||
| "john.doe" | ||
| ] | ||
| }, | ||
| "password": { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Require non-empty Gerrit username.
auth.username is currently just "type": "string". Allowing empty strings will result in confusing 401s at runtime. Add minLength: 1.
Apply this diff (update the schema generator so both TS and MDX artifacts get regenerated accordingly):
"username": {
- "type": "string",
+ "type": "string",
+ "minLength": 1,
"description": "Gerrit username for authentication",
"examples": [
"john.doe"
]
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "username": { | |
| "type": "string", | |
| "description": "Gerrit username for authentication", | |
| "examples": [ | |
| "john.doe" | |
| ] | |
| }, | |
| "password": { | |
| "username": { | |
| "type": "string", | |
| "minLength": 1, | |
| "description": "Gerrit username for authentication", | |
| "examples": [ | |
| "john.doe" | |
| ] | |
| }, |
🤖 Prompt for AI Agents
In packages/schemas/src/v3/index.schema.ts around lines 872 to 879 the Gerrit
auth.username is declared only as "type": "string" which permits empty strings;
change the schema to require non-empty usernames by adding "minLength": 1 to the
username property, and update the schema generator so both the TypeScript and
MDX artifacts are regenerated to reflect this constraint.
| "@testing-library/dom": "^10.4.1", | ||
| "@testing-library/jest-dom": "^6.7.0", | ||
| "@testing-library/react": "^16.3.0", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Switch to Testing Library stack looks good; add Vitest setup for jsdom + matchers, and verify no stale react-hooks imports.
These devDeps align with React 18 and Vitest. To fully wire them:
- Ensure Vitest runs with the jsdom environment and extends expect with jest-dom matchers.
- Confirm all tests were migrated off the deprecated
@testing-library/react-hooks.
Recommended setup file (create packages/web/vitest.setup.ts if not already present):
// packages/web/vitest.setup.ts
import { expect, afterEach } from 'vitest'
import matchers from '@testing-library/jest-dom/matchers'
import { cleanup } from '@testing-library/react'
expect.extend(matchers)
afterEach(() => {
cleanup()
})And configure Vitest (either in packages/web/vitest.config.ts or the workspace-level config):
// packages/web/vitest.config.ts
import { defineConfig } from 'vitest/config'
export default defineConfig({
test: {
environment: 'jsdom',
setupFiles: ['./vitest.setup.ts'],
// if using TS path aliases in this package:
// alias: { /* ...via vite-tsconfig-paths or manual */ },
},
})Verification script to catch lingering imports and confirm direct DOM utility usage is intentional:
🏁 Script executed:
#!/bin/bash
# 1) Ensure no deprecated react-hooks testing lib remains
rg -nP "from\s+['\"]@testing-library/react-hooks['\"]|require\(['\"]@testing-library/react-hooks['\"]\)" -C2 || echo "OK: no @testing-library/react-hooks imports found"
# 2) List direct @testing-library/dom imports (keep only if truly needed)
rg -nP "from\s+['\"]@testing-library/dom['\"]" -C2 || echo "NOTE: no direct @testing-library/dom imports; could remove the devDep if unused"
# 3) Check for jest-dom matchers setup in tests (expects vitest.setup.ts or similar)
rg -nP "@testing-library/jest-dom|jest-dom/matchers" -C2 || echo "WARN: no jest-dom setup detected; add vitest.setup.ts and setupFiles"Length of output: 1460
🏁 Script executed:
#!/bin/bash
# 1) Check for vitest setup file in packages/web
fd vitest.setup.ts packages/web
# 2) Check for vitest.config.ts in packages/web
fd vitest.config.ts packages/web
# 3) Search for jsdom environment in Vitest config files under packages/web
rg -nP "environment\\s*:\\s*['\\\"]jsdom['\\\"]" -g "packages/web/**/*.{ts,js}"
# 4) Search for setupFiles in Vitest config files under packages/web
rg -nP "setupFiles" -g "packages/web/**/*.{ts,js}"Length of output: 201
🏁 Script executed:
#!/bin/bash
# Search for Vitest setup file across the entire repo
fd vitest.setup.ts .
# Search for Vitest config file at any level
fd vitest.config.ts .
# Look for jsdom environment declaration in any config
rg -nP "environment\s*:\s*['\"]jsdom['\"]" -g "*.{ts,js}"
# Look for setupFiles field in any config
rg -nP "setupFiles" -g "*.{ts,js}"Length of output: 252
Add Vitest config + setup for the web package
Confirmed:
- No lingering
@testing-library/react-hooksimports found in source (rgscan passed) - No direct
@testing-library/domimports—if you’re not using DOM utilities directly, you can drop that devDep frompackages/web/package.json
What’s missing: Vitest isn’t yet wired up in packages/web. Please add the following:
• Create packages/web/vitest.setup.ts
import { expect, afterEach } from 'vitest'
import matchers from '@testing-library/jest-dom/matchers'
import { cleanup } from '@testing-library/react'
expect.extend(matchers)
afterEach(() => {
cleanup()
})• Create packages/web/vitest.config.ts
import { defineConfig } from 'vitest/config'
export default defineConfig({
test: {
environment: 'jsdom',
setupFiles: ['./vitest.setup.ts'],
// add path aliases here if you use them, e.g. via vite-tsconfig-paths
},
})• Update packages/web/package.json devDeps (optional):
- Remove
"@testing-library/dom"if unused - Add
"vitest"and"@vitest/ui"(or your preferred reporters)
• Reinstall and verify:
cd packages/web
yarn install
yarn test # ensure Vitest runs in jsdom and picks up jest-dom matchers🤖 Prompt for AI Agents
In packages/web/package.json around lines 191-193, Vitest isn't configured and
the devDeps include @testing-library/dom which may be unused; add a new file
packages/web/vitest.setup.ts that imports vitest's expect and afterEach, extends
expect with @testing-library/jest-dom matchers and calls cleanup from
@testing-library/react after each test, add a new packages/web/vitest.config.ts
that exports defineConfig with test.environment set to 'jsdom' and setupFiles
pointing to ['./vitest.setup.ts'], update packages/web/package.json
devDependencies to remove "@testing-library/dom" if not used and add "vitest"
(and optionally "@vitest/ui") and add a test script (e.g., vitest) if missing,
then run yarn install in packages/web and run yarn test to verify Vitest runs in
jsdom and picks up the jest-dom matchers.
P.s. coded in Cursor
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Tests
Documentation
Chores