feat(server): with api server, service-lize#807
Conversation
Summary of ChangesHello @luoling8192, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application by introducing a complete and robust authentication system. It involves setting up a new Hono-based API server with Drizzle ORM and Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.gh.lumenfield.work/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new authentication system using better-auth for both the server and client applications, including social logins with Google and GitHub. It sets up the necessary server-side infrastructure with Docker, a database schema, and API endpoints. The client-side is updated with a new login page and UI elements to reflect the authentication state. While the overall structure is sound, there are several critical and high-severity issues related to security, correctness, and configuration that need to be addressed. These include hardcoded secrets and URLs, lack of environment variable validation, and incorrect implementation of some authentication flows on the client side.
apps/server/docker-compose.yml
Outdated
| - AUTH_GOOGLE_CLIENT_ID=changeme | ||
| - AUTH_GOOGLE_CLIENT_SECRET=changeme | ||
| - AUTH_GITHUB_CLIENT_ID=changeme | ||
| - AUTH_GITHUB_CLIENT_SECRET=changeme |
There was a problem hiding this comment.
Hardcoding secrets, even placeholder ones like changeme, is a security risk. These values could accidentally be deployed to production. It's better to use environment variable substitution and load them from a .env file that is not committed to version control.
- AUTH_GOOGLE_CLIENT_ID=${AUTH_GOOGLE_CLIENT_ID}
- AUTH_GOOGLE_CLIENT_SECRET=${AUTH_GOOGLE_CLIENT_SECRET}
- AUTH_GITHUB_CLIENT_ID=${AUTH_GITHUB_CLIENT_ID}
- AUTH_GITHUB_CLIENT_SECRET=${AUTH_GITHUB_CLIENT_SECRET}
apps/server/src/services/auth.ts
Outdated
| baseURL: 'http://localhost:3000', | ||
| trustedOrigins: ['http://localhost:5173'], |
There was a problem hiding this comment.
The baseURL and trustedOrigins are hardcoded. This will not work in staging or production environments and will cause authentication to fail. They should be configured via environment variables. You will need to add corresponding variables (e.g., BASE_URL, TRUSTED_ORIGINS) to your environment parsing in src/services/env.ts.
| baseURL: 'http://localhost:3000', | |
| trustedOrigins: ['http://localhost:5173'], | |
| baseURL: env.BASE_URL, | |
| trustedOrigins: (env.TRUSTED_ORIGINS || '').split(','), |
apps/server/src/services/env.ts
Outdated
| export function parseEnv(env: any): Env { | ||
| return { | ||
| DATABASE_URL: env.DATABASE_URL, | ||
| AUTH_GOOGLE_CLIENT_ID: env.AUTH_GOOGLE_CLIENT_ID, | ||
| AUTH_GOOGLE_CLIENT_SECRET: env.AUTH_GOOGLE_CLIENT_SECRET, | ||
| AUTH_GITHUB_CLIENT_ID: env.AUTH_GITHUB_CLIENT_ID, | ||
| AUTH_GITHUB_CLIENT_SECRET: env.AUTH_GITHUB_CLIENT_SECRET, | ||
| } | ||
| } |
There was a problem hiding this comment.
The parseEnv function doesn't validate the environment variables. If a required variable is missing, it will be undefined, leading to runtime errors that are hard to debug. You should add validation to ensure all required variables are present on startup. Using a library like zod is highly recommended for this. You would need to add zod as a dependency.
| export function parseEnv(env: any): Env { | |
| return { | |
| DATABASE_URL: env.DATABASE_URL, | |
| AUTH_GOOGLE_CLIENT_ID: env.AUTH_GOOGLE_CLIENT_ID, | |
| AUTH_GOOGLE_CLIENT_SECRET: env.AUTH_GOOGLE_CLIENT_SECRET, | |
| AUTH_GITHUB_CLIENT_ID: env.AUTH_GITHUB_CLIENT_ID, | |
| AUTH_GITHUB_CLIENT_SECRET: env.AUTH_GITHUB_CLIENT_SECRET, | |
| } | |
| } | |
| import { z } from 'zod' | |
| const envSchema = z.object({ | |
| DATABASE_URL: z.string().url(), | |
| AUTH_GOOGLE_CLIENT_ID: z.string().min(1), | |
| AUTH_GOOGLE_CLIENT_SECRET: z.string().min(1), | |
| AUTH_GITHUB_CLIENT_ID: z.string().min(1), | |
| AUTH_GITHUB_CLIENT_SECRET: z.string().min(1), | |
| // Add other required env vars like BASE_URL, CORS_ORIGINS, etc. | |
| }) | |
| export function parseEnv(env: any): Env { | |
| return envSchema.parse(env) | |
| } |
apps/server/src/app.ts
Outdated
| app.use( | ||
| '/api/auth/*', // or replace with "*" to enable cors for all routes | ||
| cors({ | ||
| origin: ['http://localhost:5173', 'https://airi.moeru.ai'], // replace with your origin |
There was a problem hiding this comment.
The CORS origins are hardcoded. This makes it difficult to manage different environments (development, staging, production). These should be configured via an environment variable. You would also need to add the corresponding variable (e.g., CORS_ORIGINS) to your environment parsing in src/services/env.ts.
origin: (env.CORS_ORIGINS || '').split(','), // Example: load from a comma-separated string in env| updatedAt: timestamp('updated_at') | ||
| .$onUpdate(() => /* @__PURE__ */ new Date()) | ||
| .notNull(), |
There was a problem hiding this comment.
The updatedAt column in the session table is missing .defaultNow(). This is inconsistent with the user and verification tables, and means the updatedAt field will be NOT NULL but have no default value on creation. The same issue exists for the account table on lines 53-55.
| updatedAt: timestamp('updated_at') | |
| .$onUpdate(() => /* @__PURE__ */ new Date()) | |
| .notNull(), | |
| updatedAt: timestamp('updated_at') | |
| .defaultNow() | |
| .$onUpdate(() => /* @__PURE__ */ new Date()) | |
| .notNull(), |
| }, { | ||
| onSuccess: (ctx: any) => { | ||
| const authToken = ctx.response.headers.get('set-auth-token') // get the token from the response headers | ||
| if (authToken) { | ||
| useAuthStore().authToken = authToken | ||
| } | ||
| }, | ||
| onError: (ctx: any) => { | ||
| isLoading.value = false | ||
| toast.error(ctx.error.message || 'Failed to sign in') | ||
| }, | ||
| }).catch((error: any) => { | ||
| isLoading.value = false | ||
| toast.error(error instanceof Error ? error.message : 'An unknown error occurred') | ||
| }) |
There was a problem hiding this comment.
The onSuccess, onError callbacks and the catch block use any for their parameters. This sacrifices type safety. You should provide proper types for ctx and error to leverage TypeScript's benefits and prevent potential runtime errors. You can likely import the context types from better-auth or define them based on the library's API.
| <div class="mt-10 flex items-center justify-center gap-6 text-xs text-gray-500 font-medium"> | ||
| <a href="#" class="transition-colors hover:text-white">Terms</a> | ||
| <span class="h-1 w-1 rounded-full bg-gray-700" /> | ||
| <a href="#" class="transition-colors hover:text-white">Privacy</a> | ||
| <span class="h-1 w-1 rounded-full bg-gray-700" /> | ||
| <a href="#" class="transition-colors hover:text-white">Help</a> | ||
| </div> |
There was a problem hiding this comment.
The links for "Terms", "Privacy", and "Help" are placeholders pointing to "#". These should either be implemented with links to the correct pages or removed if not applicable.
railway-app
bot
temporarily deployed
to
cooperative-rejoicing / production
railway-app
bot
temporarily deployed
to
cooperative-rejoicing / production
nekomeowww
changed the title
This reverts commit ff361f2.
This reverts commit 9521f04.
Co-authored-by: Neko <neko@ayaka.moe>
Co-authored-by: Neko Ayaka <neko@ayaka.moe>
Co-authored-by: Neko Ayaka <neko@ayaka.moe> Co-authored-by: Lovehsigure_520 <1260907335@qq.com>
Co-authored-by: Neko Ayaka <neko@ayaka.moe> Co-authored-by: Lovehsigure_520 <1260907335@qq.com>
Co-authored-by: Neko Ayaka <neko@ayaka.moe> Co-authored-by: Lovehsigure_520 <1260907335@qq.com>
Co-authored-by: Neko Ayaka <neko@ayaka.moe> Co-authored-by: Lovehsigure_520 <1260907335@qq.com>
3 participants
No description provided.