Implement OWASP Nest GitHub authentication#1634
Conversation
* implemented authentication using next-auth * update code * type fix * updated migration * added backend test cases * added frontend unit test cases * added e2e test case * pre-commit * fixes e2e test cases * updated ci/cd * updated code * upgraded mutaitons from graphene to strawberry * updated code * Update code * Update tests * fixes * fix test * added relation * Update code * Update pnpm-lock.yaml --------- Co-authored-by: Kate Golovanova <kate@kgthreads.com> Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com> Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org>
…1556) Bumps python from 3.13.3-alpine to 3.13.4-alpine. --- updated-dependencies: - dependency-name: python dependency-version: 3.13.4-alpine dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
) Bumps python from 3.13.3-alpine to 3.13.4-alpine. --- updated-dependencies: - dependency-name: python dependency-version: 3.13.4-alpine dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps python from 3.13.3-alpine to 3.13.4-alpine. --- updated-dependencies: - dependency-name: python dependency-version: 3.13.4-alpine dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Update CONTRIBUTING.md * Update CONTRIBUTING.md --------- Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com>
* handle empty auth credentials * update test cases * upgrade code * update code * remove check route * fix test case * fixes and update usermenu --------- Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com>
* fix route * format fix * introduce flag for auth * update env * changed default value * fix test cases * fix e2 test cases * Add dynamic variable for isAuthEnabled * Clean up * Clean up and fix tests * Update code * Fix code quality issues --------- Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com> Co-authored-by: Kate <kate@kgthreads.com> Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org>
* Add project_health_metrics node and query * Add health field to the ProjectNode that represents sll ProjectHealthMetrics objects of the project * Add tests * Update filtering and add fields to models * Update filtering * Update tests * Save new boolean values * Add boolean mapping * Add query tests * Merge migrations * Update filtering, add migrations, and update scripts * Update tests and queries * Add test with filters * Update filtering * Update tests * Merge migrations * Revert unnecessary work and apply suggestions * Remove has_no_recent_commits from project * Add missing fields for FE query * Remove project name from the test * Clean migrations * Update code --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org>
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis change introduces GitHub authentication to the application using NextAuth on the frontend and a new custom User model in the backend's Nest app. It includes backend GraphQL mutations, admin registration, and migration for the User model, as well as frontend integration with authentication UI, session management, and middleware. New tests are added for both backend and frontend authentication logic. Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
backend
frontend
backend-tests
frontend-tests
makefile
ci
labels
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
docker/docker-compose-local.yaml (1)
68-68: Remove unused Docker volume to avoid confusion.The original
db-datavolume is declared but no longer used by thedbservice. Consider removing or repurposing it to keep the Compose file clean and prevent stale data from accumulating.Also applies to: 119-119
backend/apps/github/models/common.py (1)
52-52: Field mapping reordering - verify if this serves a specific purpose.The reordering of
created_atandupdated_atfields in the mapping dictionary doesn't affect functionality. Is this change intended to match a specific API response structure or improve readability?Also applies to: 61-61
frontend/__tests__/unit/utils/middleware.test.ts (1)
18-44: Consider expanding test coverage for edge cases.The current tests cover basic happy and sad paths but miss important edge cases that could occur in production:
- Token validation errors
- Network failures during token retrieval
- Malformed requests
- Different URL scenarios
Consider adding these additional test cases:
+ test('should handle getToken rejection gracefully', async () => { + ;(getToken as jest.Mock).mockRejectedValue(new Error('Token validation failed')) + const request = createRequest() + + await expect(middleware(request)).rejects.toThrow('Token validation failed') + }) + + test('should redirect to login with different request URLs', async () => { + ;(getToken as jest.Mock).mockResolvedValue(null) + const request = createRequest('http://example.com/dashboard') + const result = await middleware(request) + + expect(NextResponse.redirect).toHaveBeenCalledWith(new URL('/auth/login', request.url)) + })frontend/src/middleware.ts (1)
5-12: Consider adding error handling for token validation.The current implementation doesn't handle potential errors from
getToken(), which could throw exceptions due to network issues or malformed tokens.Consider adding error handling:
export async function middleware(request: NextRequest) { - const token = await getToken({ req: request }) + try { + const token = await getToken({ req: request }) + } catch (error) { + // Log error and redirect to login for safety + console.error('Token validation error:', error) + return NextResponse.redirect(new URL('/auth/login', request.url)) + } + if (!token) { return NextResponse.redirect(new URL('/auth/login', request.url)) } return NextResponse.next() }backend/apps/github/models/user.py (1)
128-130: Consider validating kwargs to prevent unintended attribute setting.The current implementation allows setting any attribute via kwargs, which could potentially override critical model fields or set invalid attributes.
Consider adding validation:
+ # Define allowed extra attributes + ALLOWED_KWARGS = {'custom_field1', 'custom_field2'} # Add your allowed fields + for name, value in kwargs.items(): + if name not in ALLOWED_KWARGS: + raise ValueError(f"Setting attribute '{name}' is not allowed") setattr(user, name, value)Alternatively, if you need full flexibility, consider logging which attributes are being set for debugging purposes.
frontend/__tests__/unit/pages/Login.test.tsx (1)
40-52: Enhance assertion specificity for better test reliability.Consider improving the toast assertion to be more specific about the toast type:
expect(addToast).toHaveBeenCalledWith( expect.objectContaining({ + type: 'info', title: 'Already logged in', description: 'You are already logged in.', }) )frontend/src/utils/credentials.ts (1)
4-5: Consider adding runtime validation for GitHub OAuth credentials.While the current implementation is correct, consider adding validation to ensure the credentials are properly formatted:
+const validateGitHubCredentials = (clientId?: string, clientSecret?: string) => { + if (!clientId || !clientSecret) return false + // Basic format validation for GitHub OAuth credentials + return clientId.startsWith('Iv1.') && clientSecret.length >= 40 +} + export const GITHUB_CLIENT_ID = process.env.NEXT_SERVER_GITHUB_CLIENT_ID export const GITHUB_CLIENT_SECRET = process.env.NEXT_SERVER_GITHUB_CLIENT_SECRETfrontend/src/components/UserMenu.tsx (1)
18-26: Consider optimizing the event listener management.The
mousedownevent is correctly used for dropdown closing, which is better thanclickfor this use case. However, consider adding the dropdown state to the dependency array if the handler behavior should change based onisOpenstate.useEffect(() => { const handleClickOutside = (event: MouseEvent) => { if (dropdownRef.current && !dropdownRef.current.contains(event.target as Node)) { setIsOpen(false) } } - document.addEventListener('mousedown', handleClickOutside) - return () => document.removeEventListener('mousedown', handleClickOutside) - }, []) + if (isOpen) { + document.addEventListener('mousedown', handleClickOutside) + return () => document.removeEventListener('mousedown', handleClickOutside) + } + }, [isOpen])frontend/src/components/LoginPageContent.tsx (1)
34-40: Consider improving the disabled state messaging.The current message "Authentication is disabled" could be more helpful to users. Consider providing more context or next steps.
if (!isGitHubAuthEnabled) { return ( <div className="flex min-h-[80vh] items-center justify-center"> - <span className="text-lg text-gray-500">Authentication is disabled.</span> + <div className="text-center"> + <span className="text-lg text-gray-500">GitHub authentication is currently disabled.</span> + <p className="mt-2 text-sm text-gray-400">Please contact your administrator for access.</p> + </div> </div> ) }backend/tests/apps/nest/graphql/mutations/user_test.py (1)
43-96: Consider adding more edge case tests.While the current tests cover the main success and one failure scenario, consider adding tests for additional edge cases to improve robustness.
Additional test cases to consider:
- GitHub API failure (network error, invalid token)
- Email not found or not verified
- Database constraint violations during user creation
- Existing user authentication (not just new user creation)
Would you like me to generate these additional test cases?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
backend/poetry.lockis excluded by!**/*.lockdocs/poetry.lockis excluded by!**/*.lockfrontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (38)
.github/workflows/run-ci-cd.yaml(4 hunks)backend/apps/github/models/__init__.py(1 hunks)backend/apps/github/models/common.py(1 hunks)backend/apps/github/models/user.py(3 hunks)backend/apps/nest/admin.py(1 hunks)backend/apps/nest/apps.py(1 hunks)backend/apps/nest/graphql/mutations/__init__.py(1 hunks)backend/apps/nest/graphql/mutations/user.py(1 hunks)backend/apps/nest/graphql/nodes/user.py(1 hunks)backend/apps/nest/migrations/0001_initial.py(1 hunks)backend/apps/nest/models/__init__.py(1 hunks)backend/apps/nest/models/user.py(1 hunks)backend/pyproject.toml(1 hunks)backend/settings/base.py(2 hunks)backend/settings/graphql.py(1 hunks)backend/tests/apps/nest/graphql/mutations/user_test.py(1 hunks)backend/tests/apps/nest/graphql/nodes/user_test.py(1 hunks)backend/tests/apps/nest/models/user_test.py(1 hunks)docker/docker-compose-local.yaml(2 hunks)frontend/.env.example(1 hunks)frontend/Makefile(2 hunks)frontend/__tests__/e2e/pages/Login.spec.ts(1 hunks)frontend/__tests__/unit/pages/Login.test.tsx(1 hunks)frontend/__tests__/unit/utils/middleware.test.ts(1 hunks)frontend/jest.config.ts(1 hunks)frontend/package.json(4 hunks)frontend/playwright.config.ts(1 hunks)frontend/src/app/api/auth/[...nextauth]/route.ts(1 hunks)frontend/src/app/auth/login/page.tsx(1 hunks)frontend/src/app/layout.tsx(2 hunks)frontend/src/components/Header.tsx(2 hunks)frontend/src/components/LoginPageContent.tsx(1 hunks)frontend/src/components/UserMenu.tsx(1 hunks)frontend/src/middleware.ts(1 hunks)frontend/src/server/apolloClient.ts(1 hunks)frontend/src/utils/constants.ts(1 hunks)frontend/src/utils/credentials.ts(1 hunks)frontend/src/wrappers/provider.tsx(1 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
backend/apps/nest/admin.py
[refactor] 8-8: Too few public methods (0/2)
(R0903)
backend/apps/nest/graphql/nodes/user.py
[refactor] 9-9: Too few public methods (0/2)
(R0903)
backend/apps/nest/apps.py
[refactor] 4-4: Too few public methods (0/2)
(R0903)
backend/settings/graphql.py
[refactor] 10-10: Too few public methods (1/2)
(R0903)
[refactor] 15-15: Too many ancestors (18/7)
(R0901)
backend/apps/nest/models/user.py
[refactor] 12-12: Too few public methods (0/2)
(R0903)
[refactor] 9-9: Too few public methods (1/2)
(R0903)
backend/tests/apps/nest/graphql/nodes/user_test.py
[error] 9-9: Class 'AuthUserNode' has no 'strawberry_definition' member
(E1101)
[error] 11-11: Class 'AuthUserNode' has no 'strawberry_definition' member
(E1101)
[refactor] 4-4: Too few public methods (1/2)
(R0903)
backend/apps/nest/migrations/0001_initial.py
[refactor] 10-10: Too few public methods (0/2)
(R0903)
backend/apps/nest/graphql/mutations/user.py
[refactor] 13-13: Too few public methods (0/2)
(R0903)
[refactor] 20-20: Too few public methods (1/2)
(R0903)
🪛 dotenv-linter (3.3.0)
frontend/.env.example
[warning] 14-14: [UnorderedKey] The NEXTAUTH_SECRET key should go before the NEXT_PUBLIC_API_URL key
[warning] 15-15: [UnorderedKey] The NEXTAUTH_URL key should go before the NEXT_PUBLIC_API_URL key
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (56)
backend/pyproject.toml (1)
151-153: Appropriate per-file ignore for migrations.Ignoring
E501in migration files is generally acceptable to accommodate auto-generated long lines. Ensure migrations remain maintainable and consider re-runningruffchecks in CI to confirm no unintended lint suppressions.frontend/package.json (3)
33-34: Approve Next.js package version bumps.Upgrading
@next/eslint-plugin-next,@next/third-parties,next, andeslint-config-nextto15.3.4aligns with Next.js15.xreleases. Ensure there are no breaking changes in the upgraded minor version.Also applies to: 54-54, 86-86
55-55: Approve addition of NextAuth runtime dependency.Adding
next-auth@4.24.11supports the new GitHub OAuth integration. Verify that environment variables and NextAuth configuration files are in place for seamless authentication.
99-99: Approve Jest environment alignment.Downgrading
jest-environment-jsdomto29.7.0ensures compatibility with the existingjest@^29.7.0setup. This prevents potential mismatches between Jest and JSDOM versions.backend/apps/nest/models/__init__.py (1)
1-1: Expose custom User model at package level.Importing
Userin__init__.pyis the correct approach to make the model accessible viaapps.nest.models.User. No issues found.backend/apps/github/models/__init__.py (1)
5-5: Expose GitHub User model alongside others.Adding
from .user import Useraligns imports for consistent access to the GitHub user model. Looks good.frontend/jest.config.ts (1)
8-8: LGTM! Coverage exclusion pattern refined appropriately.The change from
!src/api/**to!src/app/api/**is a good refinement that narrows the coverage exclusion to specifically target the Next.js app directory API routes (like NextAuth handlers) while allowing coverage of other API-related code.frontend/playwright.config.ts (1)
30-30: Build command simplified - verify consistency with other environments.The change from
pnpm run build:turbotopnpm run buildsimplifies the e2e test setup. Ensure this build command produces the same output as used in other environments (development, production) to maintain test consistency.backend/apps/nest/graphql/mutations/__init__.py (1)
1-3: LGTM! Standard Python package initialization.The module follows standard Python conventions with a clear docstring and proper relative imports. This provides a clean entry point for user-related GraphQL mutations.
backend/apps/nest/admin.py (1)
8-13: LGTM! Standard Django admin configuration.The UserAdmin implementation follows Django best practices:
- Sensible ordering by username for admin interface usability
- Search functionality on relevant fields (email, username)
- Proper model registration
The static analysis warning about "too few public methods" can be ignored as it's a false positive - Django admin classes typically rely on framework functionality rather than custom public methods.
backend/apps/nest/apps.py (1)
4-5: LGTM! Standard Django app configuration.The NestConfig follows Django conventions correctly by extending AppConfig and setting the appropriate app name. The static analysis warning about "too few public methods" is a false positive - Django app configurations are typically minimal.
backend/apps/nest/graphql/nodes/user.py (1)
8-10: LGTM! Minimal field exposure is appropriate for authentication.The GraphQL node correctly implements a minimal user representation by exposing only the username field. This follows security best practices by limiting user data exposure in authentication contexts.
Note: The Pylint warning about "too few public methods" is a false positive for GraphQL type definitions using decorators.
frontend/src/utils/constants.ts (1)
121-125: LGTM! Well-structured authentication status constants.The
userAuthStatusobject provides clear, consistent constants for managing authentication states. The naming follows TypeScript conventions and the string values are appropriate for authentication flow management.frontend/src/app/layout.tsx (1)
6-6: LGTM! Proper feature flag integration for GitHub authentication.The import of
IS_GITHUB_AUTH_ENABLEDand its propagation to the Header component correctly implements feature flag functionality. This allows for conditional rendering of GitHub authentication UI elements.Also applies to: 70-70
backend/settings/base.py (2)
21-21: LGTM! Correct Django configuration for custom user model.Setting
AUTH_USER_MODEL = "nest.User"is the proper way to configure Django to use the custom User model from the nest app. This is essential for the GitHub authentication functionality.
49-49: LGTM! Proper app registration in Django settings.Adding
"apps.nest"toLOCAL_APPScorrectly registers the nest app with Django, making its models, views, and other components available to the framework.frontend/src/components/Header.tsx (1)
18-18: LGTM! Clean integration of user authentication UI.The UserMenu component is properly imported and integrated into the header layout. The conditional prop passing enables flexible authentication configuration.
Also applies to: 123-123
frontend/src/server/apolloClient.ts (1)
33-36: LGTM! Improved test client completeness.Adding the
mutatemethod to the no-op Apollo client ensures it matches the interface of a real Apollo client, supporting tests that perform GraphQL mutations for the new authentication features.frontend/src/app/auth/login/page.tsx (1)
1-6: LGTM! Clean page component implementation.The login page follows Next.js app router conventions and properly separates configuration concerns from UI logic. The component structure is clean and maintainable.
frontend/src/wrappers/provider.tsx (1)
5-5: LGTM! Proper session provider integration.The SessionProvider is correctly positioned as the outermost wrapper, making session state available throughout the application. The provider nesting follows NextAuth best practices.
Also applies to: 13-20
frontend/Makefile (1)
23-23: Verify impact on development environment consistency.Switching from Docker-based to local pnpm execution may cause inconsistencies across different development environments. Consider whether this change could affect:
- Team collaboration with different local setups
- CI/CD pipeline consistency
- Dependency version alignment
#!/bin/bash # Description: Check if other Makefile commands still use Docker for consistency # Expected: Find other commands that might need similar updates or reasons for keeping Docker # Search for other frontend Docker commands rg "docker.*frontend" --type make # Check if there are version specifications for pnpm rg "pnpm.*version|engine.*pnpm" frontend/package.jsonAlso applies to: 26-26, 50-50
backend/settings/graphql.py (3)
6-6: LGTM: Import looks correct.The import statement properly brings in UserMutations from the nest app's GraphQL mutations module.
10-12: LGTM: Mutation class definition follows GraphQL patterns.The Mutation class correctly inherits from UserMutations and will expose the user-related mutations to the GraphQL schema.
19-19: LGTM: Schema configuration properly includes mutations.The schema update correctly adds the Mutation class alongside the existing Query class, enabling GraphQL mutations in the API.
backend/apps/nest/models/user.py (3)
9-19: LGTM: Well-designed model with proper configuration.The model properly extends AbstractUser and includes good Meta configuration:
- Custom table name for clarity
- Index on username for performance
- Proper ordering and verbose names
20-26: Consider the CASCADE delete behavior carefully.The OneToOneField uses
on_delete=models.CASCADE, which means deleting a GitHub user will also delete the corresponding Nest user. This might be too aggressive depending on your use case.Consider if you want to:
- Use
SET_NULLto preserve the Nest user when GitHub user is deleted- Use
PROTECTto prevent GitHub user deletion if Nest user exists- Keep
CASCADEif the Nest user should not exist without a GitHub userWhat's the intended behavior when a GitHub user account is deleted or deactivated?
28-35: LGTM: Clear and proper string representation.The
__str__method correctly returns the username with proper type hints and documentation.frontend/src/middleware.ts (2)
5-12: LGTM: Basic authentication middleware logic is correct.The middleware properly:
- Retrieves the authentication token using next-auth
- Redirects unauthenticated users to the login page
- Allows authenticated requests to proceed
14-17: Middleware is not protecting any routes yet.The empty
matcherarray means this middleware won't actually run on any routes. You'll need to configure the routes you want to protect.Consider adding route patterns to protect, for example:
export const config = { // required authentication for all defined routes under matcher - matcher: [], + matcher: [ + '/dashboard/:path*', + '/profile/:path*', + // Add other protected routes + ], }Are there specific routes you plan to protect with this middleware?
backend/apps/github/models/user.py (2)
79-79: LGTM: Safer attribute access with default value.Using
getattrwith a defaultNoneprevents AttributeError exceptions when GitHub user attributes are missing.
105-105: LGTM: Flexible method signature for dynamic updates.The addition of
**kwargsallows the method to accept additional attributes for dynamic user updates, which supports the GitHub authentication flow.frontend/.env.example (3)
5-5: LGTM: Google Tag Manager ID cleared appropriately.The empty value suggests GTM is intentionally disabled, which is appropriate for the authentication implementation focus.
11-12: LGTM: GitHub OAuth credentials properly configured.The server-side environment variables follow Next.js conventions and will be securely handled on the backend.
14-15: LGTM: NextAuth configuration variables added correctly.The placeholder secret reminds developers to set a secure value, and the localhost URL is appropriate for local development.
frontend/__tests__/e2e/pages/Login.spec.ts (1)
1-32: LGTM: Well-structured E2E test with proper mocking.The test setup correctly mocks GraphQL responses and CSRF tokens, ensuring isolated testing of the login page behavior. The test case appropriately verifies that the authentication disabled message is displayed.
backend/tests/apps/nest/graphql/nodes/user_test.py (1)
4-16: LGTM: Comprehensive GraphQL node configuration testing.The tests properly verify the Strawberry GraphQL node configuration, including the node name and field types. The field type checking correctly handles both direct and wrapped string types.
backend/tests/apps/nest/models/user_test.py (1)
9-43: LGTM: Comprehensive User model test coverage.Excellent test implementation that thoroughly validates the custom User model without requiring database access. The tests cover all critical aspects: field types and constraints, meta options, indexing, and string representation.
frontend/src/app/api/auth/[...nextauth]/route.ts (3)
15-22: LGTM: Conditional GitHub provider configuration implemented correctly.The conditional loading of the GitHub provider based on the configuration flag ensures the authentication flow only activates when properly configured.
57-69: LGTM: JWT and session token management implemented correctly.The token flow properly stores the GitHub access token in the JWT and exposes it on the session object for use throughout the application.
75-76: LGTM: NextAuth handler exported correctly for Next.js App Router.The handler is properly exported for both GET and POST methods as required by Next.js App Router API routes.
.github/workflows/run-ci-cd.yaml (1)
298-299: LGTM! Proper secret management for GitHub OAuth and NextAuth.The environment variable additions for GitHub OAuth and NextAuth are correctly implemented:
- All sensitive values are sourced from GitHub secrets
- Consistent between staging and production environments
- Variable names align with NextAuth conventions
Also applies to: 310-311, 493-494, 505-506
frontend/__tests__/unit/pages/Login.test.tsx (2)
8-20: Well-structured mocking setup for authentication testing.The mock configurations properly isolate the component dependencies:
- NextAuth hooks for session management
- Next.js router for navigation
- Toast notifications for user feedback
- GitHub auth enablement flag
61-68: Verify signIn callback URL matches application routing.Ensure the callback URL
/is the intended destination after successful authentication. Consider testing with different callback URLs if the application supports redirect parameters.backend/apps/nest/migrations/0001_initial.py (1)
10-131: LGTM! Well-structured Django migration for custom User model.The migration properly:
- Extends Django's AbstractUser with GitHub integration
- Sets up proper foreign key relationships with cascade deletion
- Includes appropriate indexes for performance
- Follows Django migration best practices
- Has correct dependencies on auth and github apps
frontend/src/utils/credentials.ts (2)
9-11: LGTM! Proper conditional enablement logic.The boolean flag correctly ensures GitHub authentication is only enabled when both required credentials are present, preventing partial configuration issues.
12-13: Let’s check your.env.examplefiles and any Docker compose configs for these vars:#!/bin/bash # Check frontend and backend .env.example for NEXTAUTH_URL and VITE_API_URL echo "=== .env.example in frontend ===" rg -n "NEXTAUTH_URL" -n "VITE_API_URL" frontend/.env.example || true echo -e "\n=== .env.example in backend ===" rg -n "NEXTAUTH_URL" -n "VITE_API_URL" backend/.env.example || true # Check for Docker Compose overrides echo -e "\n=== docker-compose files ===" fd -t f -HI "docker-compose*.yml" . | xargs -r -n1 -I{} sh -c \ 'echo "--- {} ---"; rg -n "NEXTAUTH_URL" -n "VITE_API_URL" {} || true'frontend/src/components/UserMenu.tsx (4)
1-11: LGTM: Clean imports and type definition.The imports are well-organized and the TypeScript interface properly defines the component props with readonly modifier for immutability.
51-88: Excellent accessibility implementation.The dropdown implementation includes proper ARIA attributes (
aria-expanded,aria-haspopup,aria-controls) and uses semantic HTML structure. The focus management and keyboard navigation support are well implemented.
60-68: ```shell
#!/bin/bashSearch for default-avatar.png file anywhere in the repository
fd default-avatar.png
--- `43-43`: [web_search] ```web Does GitHub OAuth support the value 'login' for the prompt parameter?frontend/src/components/LoginPageContent.tsx (2)
1-14: LGTM: Well-structured imports and type definition.The imports are properly organized and the props interface follows TypeScript best practices with readonly modifiers.
70-76: LGTM: Clean sign-in implementation.The GitHub sign-in button is well-implemented with proper styling, accessibility, and callback URL configuration.
backend/tests/apps/nest/graphql/mutations/user_test.py (4)
1-9: LGTM: Proper test module structure.The imports are clean and properly organized, with good separation of concerns between testing utilities and application modules.
14-23: LGTM: Comprehensive GitHubAuthResult testing.The tests properly validate both positive and negative cases for the GitHubAuthResult class, ensuring the payload structure works correctly.
43-81: Excellent test coverage for successful authentication flow.The test comprehensively covers the happy path with proper mocking of:
- GitHub API client and user data
- Email fetching from GitHub
- Database operations (get_or_create)
- GitHubUser.update_data method
The assertions verify both the method calls and return values correctly.
83-96: Good edge case coverage for update_data failure.This test properly handles the scenario where
GitHubUser.update_datareturnsNone, ensuring the authentication gracefully fails rather than causing errors.
| def github_auth(self, access_token: str) -> GitHubAuthResult: | ||
| """Authenticate via GitHub OAuth2.""" | ||
| try: | ||
| github = Github(access_token) | ||
| gh_user = github.get_user() | ||
| gh_user_email = next( | ||
| (e.email for e in gh_user.get_emails() if e.primary and e.verified), "" | ||
| ) | ||
|
|
||
| github_user = GithubUser.update_data(gh_user, email=gh_user_email) | ||
| if not github_user: | ||
| return GitHubAuthResult(auth_user=None) | ||
|
|
||
| auth_user, _ = User.objects.get_or_create( | ||
| defaults={ | ||
| "email": gh_user_email, | ||
| "github_user": github_user, | ||
| }, | ||
| username=gh_user.login, | ||
| ) | ||
|
|
||
| return GitHubAuthResult(auth_user=auth_user) | ||
|
|
||
| except requests.exceptions.RequestException: | ||
| return GitHubAuthResult(auth_user=None) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and add security validations.
The GitHub authentication mutation has several areas for improvement:
-
Broader exception handling needed: PyGithub can raise
GithubExceptionand other exceptions beyondRequestException. -
Add access token validation: Validate the token format before using it.
-
Improve email retrieval logic: The current approach may not handle edge cases properly.
@strawberry.mutation
def github_auth(self, access_token: str) -> GitHubAuthResult:
"""Authenticate via GitHub OAuth2."""
+ if not access_token or not access_token.startswith(('gho_', 'ghp_')):
+ return GitHubAuthResult(auth_user=None)
+
try:
github = Github(access_token)
gh_user = github.get_user()
- gh_user_email = next(
- (e.email for e in gh_user.get_emails() if e.primary and e.verified), ""
- )
+
+ emails = gh_user.get_emails()
+ gh_user_email = next(
+ (e.email for e in emails if e.primary and e.verified),
+ None
+ )
+ if not gh_user_email:
+ return GitHubAuthResult(auth_user=None)
github_user = GithubUser.update_data(gh_user, email=gh_user_email)
if not github_user:
return GitHubAuthResult(auth_user=None)
auth_user, _ = User.objects.get_or_create(
defaults={
"email": gh_user_email,
"github_user": github_user,
},
username=gh_user.login,
)
return GitHubAuthResult(auth_user=auth_user)
- except requests.exceptions.RequestException:
+ except (requests.exceptions.RequestException, Exception) as e:
+ # Log the authentication failure for security monitoring
+ logger.warning(f"GitHub authentication failed: {str(e)}")
return GitHubAuthResult(auth_user=None)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/apps/nest/graphql/mutations/user.py around lines 24 to 48, improve
error handling by catching broader exceptions such as GithubException in
addition to RequestException to cover all GitHub API errors. Add validation for
the access_token format before using it to ensure it meets expected criteria
(e.g., non-empty string, correct length or pattern). Refine the email retrieval
logic to handle cases where no primary and verified email exists by implementing
fallback strategies or clearer defaults to avoid empty or incorrect email
values.
There was a problem hiding this comment.
is it clear that github access token starts with 'gho_', 'ghp_' . pls provide reference
There was a problem hiding this comment.
🧩 Analysis chain
@Rajgupta36 Thank you for questioning this! Let me search for the official GitHub documentation on access token formats to provide you with accurate information and proper references.
🌐 Web query:
GitHub access token format prefixes official documentation 2024
💡 Result:
GitHub Access Token Format Prefixes (2024)
GitHub uses specific prefixes in its access tokens to clearly indicate the token type and improve both security and usability. These prefixes help distinguish between token types for authentication and enhance their detection in secret scanning features.
Official Token Format Prefixes:
| Token Type | Prefix |
|---|---|
| Personal access token (classic) | ghp_ |
| Fine-grained personal access token | github_pat_ |
| OAuth access token | gho_ |
| User access token for a GitHub App | ghu_ |
| Installation access token for a GitHub App | ghs_ |
| Refresh token for a GitHub App | ghr_ |
Each token begins with its prefix, followed by an underscore, and then a unique set of characters. These prefixes are case-sensitive and are designed to improve both identification and security[2][6][8].
Examples
- Classic PAT:
ghp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx - Fine-grained PAT:
github_pat_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx - OAuth token:
gho_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx - GitHub App user token:
ghu_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx - GitHub App installation token:
ghs_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx - Refresh token:
ghr_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
Design Rationale
- Prefix Structure: Most prefixes use “gh” (for GitHub) and a letter indicating the token type, followed by an underscore (e.g.,
ghp_for personal access tokens)[6][8]. - Readability and Security: The underscore is used because it's not a Base64 character, which makes accidental duplication with random hashes unlikely and improves copying behavior (double-click selects the whole token)[6].
- Detection: These prefixes dramatically reduce the false positive rate in secret scanning and help organizations more reliably manage credentials[6][8].
For the latest and most authoritative information, see the official [GitHub documentation on authentication and token formats][2].
References:
[2]: GitHub Docs – About authentication to GitHub
[6]: GitHub Engineering Blog – Behind GitHub's new authentication token formats
[8]: Microsoft Learn – GitHub personal access token entity definition
Citations:
- 1: https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/managing-your-personal-access-tokens
- 2: https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/about-authentication-to-github
- 3: https://graphite.dev/guides/github-personal-access-token
- 4: https://docs.github.com/actions/security-guides/automatic-token-authentication
- 5: https://docs.gitlab.com/user/profile/personal_access_tokens/
- 6: https://github.blog/engineering/platform-security/behind-githubs-new-authentication-token-formats/
- 7: https://github.com/orgs/community/discussions/74701
- 8: https://learn.microsoft.com/en-us/purview/sit-defn-github-personal-access-token
- 9: https://github.com/orgs/community/discussions/39421
- 10: https://docs.gitlab.com/user/project/settings/project_access_tokens/
🌐 Web query:
GitHub personal access token format "gho_" "ghp_" official documentation
💡 Result:
GitHub's personal access tokens (PATs) have a specific format to enhance security and facilitate detection. Each token begins with a prefix indicating its type, followed by a unique identifier. For personal access tokens, the format is:
ghp_<unique_identifier>
Here, ghp_ is the prefix for personal access tokens, and <unique_identifier> is a string of alphanumeric characters. This format helps in easily identifying and managing tokens. (docs.github.com)
For more detailed information on GitHub's authentication token formats, you can refer to their official documentation. (docs.github.com)
Citations:
- 1: https://docs.github.com/en/enterprise-cloud%40latest/authentication/keeping-your-account-and-data-secure/about-authentication-to-github?utm_source=openai
- 2: https://docs.github.com/en/enterprise-cloud%40latest/authentication/keeping-your-account-and-data-secure/about-authentication-to-github?utm_source=openai
🌐 Web query:
site:docs.github.com GitHub access token format prefixes
💡 Result:
GitHub Access Token Format Prefixes
GitHub uses identifiable prefixes in the format of its access tokens to clearly indicate the token type and improve both security and secret scanning capabilities. Here are the most commonly used token prefixes as of June 2025:
| Token Type | Prefix |
|---|---|
| Personal access token (classic) | ghp_ |
| Fine-grained personal access token | github_pat_ |
| OAuth access token | gho_ |
| User access token for a GitHub App | ghu_ |
| Installation access token for a GitHub App | ghs_ |
| Refresh token for a GitHub App | ghr_ |
Details and Context:
- These prefixes are always at the beginning of the token and are case-sensitive. For example, a classic personal access token will always start with
ghp_[9][4][1]. - After the prefix, the token is typically a random string of 36 or more alphanumeric characters. For example:
ghp_A1bC2dE3fH4iJ5kL6mN7oP8qR9sT0u[2]. - Fine-grained personal access tokens use the
github_pat_prefix for easier identification and improved security scanning[9]. - Using such prefixes with an underscore (
_) as a separator avoids confusion with random strings or SHA hashes and improves detection rates for compromised secrets[1][4].
Summary Table
| Prefix | Token Type |
|---|---|
ghp_ |
Personal access token (classic) |
github_pat_ |
Fine-grained personal access token |
gho_ |
OAuth access token |
ghu_ |
User access token for a GitHub App |
ghs_ |
Installation access token for a GitHub App |
ghr_ |
Refresh token for a GitHub App |
Notes
- Always treat access tokens as sensitive secrets.
- If you reset or generate new tokens, you will receive them in this prefixed format[4][9].
- GitHub’s token length and format may evolve, so it’s recommended that integrators allow for future changes, potentially supporting tokens up to 255 characters[4].
- The prefix system is designed to make it easy to identify and manage tokens, and to support security tools in detecting exposed tokens more reliably[1][4].
For more details, consult GitHub's official documentation on authentication and token management[9][3].
Citations:
- 1: https://github.blog/engineering/platform-security/behind-githubs-new-authentication-token-formats/
- 2: https://learn.microsoft.com/en-us/purview/sit-defn-github-personal-access-token
- 3: https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/managing-your-personal-access-tokens
- 4: https://github.blog/changelog/2021-03-31-authentication-token-format-updates-are-generally-available/
- 5: https://graphite.dev/guides/github-personal-access-token
- 6: https://www.theserverside.com/blog/Coffee-Talk-Java-News-Stories-and-Opinions/How-to-create-a-GitHub-Personal-Access-Token-example
- 7: https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions
- 8: https://docs.github.com/actions/security-guides/automatic-token-authentication
- 9: https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/about-authentication-to-github
- 10: https://docs.github.com/packages/working-with-a-github-packages-registry/working-with-the-apache-maven-registry
@Rajgupta36 You're absolutely right to ask for references! Here's the accurate information with official sources:
According to GitHub's official documentation and their engineering blog, GitHub access tokens do use specific prefixes:
Official GitHub Token Prefixes:
| Token Type | Prefix |
|---|---|
| OAuth access token | gho_ |
| Personal access token (classic) | ghp_ |
| Fine-grained personal access token | github_pat_ |
| User access token for GitHub App | ghu_ |
| Installation access token for GitHub App | ghs_ |
| Refresh token for GitHub App | ghr_ |
References:
- GitHub Official Docs: About authentication to GitHub
- GitHub Engineering Blog: Behind GitHub's new authentication token formats
- GitHub Changelog: Authentication token format updates
For your OAuth flow, the most relevant prefix would be gho_ (OAuth access token), though users might also use ghp_ (classic personal access tokens).
Here's an updated validation approach:
# More comprehensive token validation
VALID_GITHUB_TOKEN_PREFIXES = ('gho_', 'ghp_', 'github_pat_', 'ghu_', 'ghs_')
if not access_token or not access_token.startswith(VALID_GITHUB_TOKEN_PREFIXES):
return GitHubAuthResult(auth_user=None)Thank you for pushing for accuracy! 🐰
✏️ Learnings added
Learnt from: Rajgupta36
PR: OWASP/Nest#1634
File: backend/apps/nest/graphql/mutations/user.py:24-48
Timestamp: 2025-06-18T21:00:33.002Z
Learning: GitHub access tokens use specific prefixes to identify token types: OAuth tokens start with `gho_`, classic personal access tokens with `ghp_`, fine-grained personal access tokens with `github_pat_`, GitHub App user tokens with `ghu_`, GitHub App installation tokens with `ghs_`, and refresh tokens with `ghr_`. This is documented in GitHub's official authentication documentation and engineering blog.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
@Rajgupta36 could you look into the bot's suggestions?
* update code * fix test case * Update middleware.test.ts
|
6 participants
Implement OWASP Nest GitHub authentication
Add the PR description here.