feat(authserver): initialize auth server with database and authentication logic #2784
Conversation
|
Here's the code health analysis summary for commits Analysis Summary
|
|
@sentry review |
This comment has been minimized.
This comment has been minimized.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #2784 +/- ##
==========================================
- Coverage 46.39% 44.65% -1.75%
==========================================
Files 205 213 +8
Lines 11452 11914 +462
Branches 722 724 +2
==========================================
+ Hits 5313 5320 +7
- Misses 6138 6593 +455
Partials 1 1 ☔ View full report in Codecov by Sentry. |
|
@sentry review |
|
@sentry generate-test |
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
| return { | ||
| customerId: user.customerId, | ||
| profileId: user.profileId, | ||
| contextId: user.contextId, | ||
| }; |
There was a problem hiding this comment.
Potential bug: The findUser function accesses properties (profileId, contextId) from a database query result that doesn't contain them, causing it to return undefined for these fields.
-
Description: The
findUserfunction queries theusertable, which only containsusername,password, andcustomerIdcolumns. However, the function's return statement atapp/authserver/db.ts:184~188attempts to accessuser.profileIdanduser.contextIdfrom the query result. These properties do not exist on the result object, so their values will beundefined. This causes the function to return aUserRecordMiniobject whereprofileIdandcontextIdareundefined, violating the type contract and leading to runtime errors or logical failures in any downstream code that uses these values. -
Suggested fix: The
findUserfunction should be modified to only return data available from theusertable. Alternatively, ifprofileIdandcontextIdare required, the SQL query must be updated to join with the appropriate table to retrieve these values before returning them.
severity: 0.82, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
app/authserver/AuthServer.ts
Outdated
| if (!authDB.isDatabaseConnected) { | ||
| coreLogger.fatal("Database connection failed. Exiting."); |
There was a problem hiding this comment.
Potential bug: The database connection check accesses isDatabaseConnected as a property instead of calling it as a function, causing the critical startup check to be completely bypassed.
-
Description: The startup check for database connectivity is written as
if (!authDB.isDatabaseConnected). However,isDatabaseConnectedis a function, not a boolean property. In JavaScript, a function object is "truthy", so!authDB.isDatabaseConnectedwill always evaluate tofalse. Consequently, the check is completely bypassed, and the failure logic, which logs a fatal error and exits, will never be executed. This allows the server to start even with a failed database connection, leading to unexpected crashes later when a database operation is attempted. -
Suggested fix: The check should be changed to call the function:
if (!authDB.isDatabaseConnected()). This will correctly execute the database connection check and ensure the server fails fast if the database is unavailable.
severity: 0.85, confidence: 0.98
Did we get this right? 👍 / 👎 to inform future reviews.
| ): UserRecordMini | null { | ||
| const logger = getServerLogger("database"); | ||
| const query = database.prepare(SQL.FIND_USER); | ||
| const hashedPassword = this.generatePasswordHash(password); | ||
| const user = query.get(username) as UserRecordMini | null; | ||
| if (user == null) { | ||
| logger.warn(`User ${username} not found`); | ||
| return null | ||
| } | ||
| if (compareSync(password, (user as any).password) === false) { | ||
| logger.warn(`Invalid password for user ${username}`); |
There was a problem hiding this comment.
Critical security vulnerability: The findUser method is hashing the provided password and comparing it with the stored hash, but this is incorrect. The stored value should already be a hash, and compareSync should be used to compare the plain password with the stored hash. Currently, this will never authenticate users correctly.
| ): UserRecordMini | null { | |
| const logger = getServerLogger("database"); | |
| const query = database.prepare(SQL.FIND_USER); | |
| const hashedPassword = this.generatePasswordHash(password); | |
| const user = query.get(username) as UserRecordMini | null; | |
| if (user == null) { | |
| logger.warn(`User ${username} not found`); | |
| return null | |
| } | |
| if (compareSync(password, (user as any).password) === false) { | |
| logger.warn(`Invalid password for user ${username}`); | |
| findUser( | |
| database: DatabaseSync, | |
| username: string, | |
| password: string, | |
| ): UserRecordMini | null { | |
| const logger = getServerLogger("database"); | |
| const query = database.prepare(SQL.FIND_USER); | |
| const user = query.get(username) as any; | |
| if (user == null) { | |
| logger.warn(`User ${username} not found`); | |
| return null; | |
| } | |
| if (!compareSync(password, user.password)) { | |
| logger.warn(`Invalid password for user ${username}`); | |
| return null; | |
| } | |
| return { | |
| customerId: user.customerId, | |
| profileId: user.profileId || 0, | |
| contextId: user.contextId || '', | |
| }; | |
| }, |
Did we get this right? 👍 / 👎 to inform future reviews.
| function generateTicket(customerId: string): string { | ||
| console.log(`Generating ticket for customerId: ${customerId}`); | ||
| const ticket = randomUUID(); | ||
| console.log(`Generated ticket: ${ticket}`); | ||
| return ticket; | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * Retrieves a user account based on the provided username and password. | ||
| * | ||
| * @param username - The username of the account to retrieve. | ||
| * @param password - The password of the account to retrieve. | ||
| * @returns An object containing the username, ticket, and customerId if the account is found, or null if not. | ||
| */ | ||
| function retrieveUserAccount( | ||
| username: string, | ||
| password: string, | ||
| ): { username: string; ticket: string; customerId: string } | null { | ||
| const customer = authDB.findUser(username, password); | ||
| if (customer == null) { | ||
| console.log(`No user found for username: ${username}`); | ||
| return null; | ||
| } | ||
| console.log(`User found: ${username} with customerId: ${customer.customerId}`); | ||
| return { | ||
| username, | ||
| ticket: generateTicket(customer.customerId.toString()), | ||
| customerId: customer.customerId.toString(), | ||
| }; |
There was a problem hiding this comment.
Security issue: Passwords are being logged in debug statements and passed around in function calls. This could expose sensitive information in logs. Remove password logging and ensure passwords are not stored in variables longer than necessary.
| function generateTicket(customerId: string): string { | |
| console.log(`Generating ticket for customerId: ${customerId}`); | |
| const ticket = randomUUID(); | |
| console.log(`Generated ticket: ${ticket}`); | |
| return ticket; | |
| } | |
| /** | |
| * Retrieves a user account based on the provided username and password. | |
| * | |
| * @param username - The username of the account to retrieve. | |
| * @param password - The password of the account to retrieve. | |
| * @returns An object containing the username, ticket, and customerId if the account is found, or null if not. | |
| */ | |
| function retrieveUserAccount( | |
| username: string, | |
| password: string, | |
| ): { username: string; ticket: string; customerId: string } | null { | |
| const customer = authDB.findUser(username, password); | |
| if (customer == null) { | |
| console.log(`No user found for username: ${username}`); | |
| return null; | |
| } | |
| console.log(`User found: ${username} with customerId: ${customer.customerId}`); | |
| return { | |
| username, | |
| ticket: generateTicket(customer.customerId.toString()), | |
| customerId: customer.customerId.toString(), | |
| }; | |
| function generateTicket(customerId: string): string { | |
| console.log(`Generating ticket for customerId: ${customerId}`); | |
| const ticket = randomUUID(); | |
| console.log(`Generated ticket: ${ticket}`); | |
| return ticket; | |
| } | |
| function retrieveUserAccount( | |
| username: string, | |
| password: string, | |
| ): { username: string; ticket: string; customerId: string } | null { | |
| const customer = authDB.findUser(username, password); | |
| if (customer == null) { | |
| console.log(`Authentication failed for username: ${username}`); | |
| return null; | |
| } | |
| console.log(`User authenticated: ${username} with customerId: ${customer.customerId}`); | |
| return { | |
| username, | |
| ticket: generateTicket(customer.customerId.toString()), | |
| customerId: customer.customerId.toString(), | |
| }; | |
| } |
Did we get this right? 👍 / 👎 to inform future reviews.
| export interface AuthServerConfig { | ||
| dbConnectionUrl: string; | ||
| logLevel: string; | ||
| } | ||
|
|
||
| export function getConfig() : AuthServerConfig { | ||
| return { | ||
| dbConnectionUrl: process.env.AUTH_DB_CONNECTION_URL || "sqlite://:memory:", | ||
| logLevel: process.env.LOG_LEVEL || "debug", | ||
| }; |
There was a problem hiding this comment.
Missing port configuration: The AuthServer tries to read a 'port' property from the config but it's not defined in the AuthServerConfig interface or getConfig function. This will cause the server to always use the default port 3000.
| export interface AuthServerConfig { | |
| dbConnectionUrl: string; | |
| logLevel: string; | |
| } | |
| export function getConfig() : AuthServerConfig { | |
| return { | |
| dbConnectionUrl: process.env.AUTH_DB_CONNECTION_URL || "sqlite://:memory:", | |
| logLevel: process.env.LOG_LEVEL || "debug", | |
| }; | |
| export interface AuthServerConfig { | |
| dbConnectionUrl: string; | |
| logLevel: string; | |
| port: string; | |
| } | |
| export function getConfig() : AuthServerConfig { | |
| return { | |
| dbConnectionUrl: process.env.AUTH_DB_CONNECTION_URL || "sqlite://:memory:", | |
| logLevel: process.env.LOG_LEVEL || "debug", | |
| port: process.env.PORT || "3000", | |
| }; | |
| } |
Did we get this right? 👍 / 👎 to inform future reviews.
| // Database Instance Management | ||
| let databaseInstance: DatabaseSync | null = null; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Security concern: The database connection string from environment variables could allow arbitrary file system access with SQLite. Consider validating the connection string format and restricting file paths to a specific directory.
| // Database Instance Management | |
| let databaseInstance: DatabaseSync | null = null; | |
| /** | |
| function initializeDatabaseService(): AuthDatabaseService { | |
| if (databaseInstance === null) { | |
| const config = getConfig(); | |
| // Validate SQLite connection string to prevent arbitrary file access | |
| if (config.dbConnectionUrl.startsWith('sqlite://') && | |
| !config.dbConnectionUrl.includes('..') && | |
| (config.dbConnectionUrl === 'sqlite://:memory:' || | |
| config.dbConnectionUrl.startsWith('sqlite://./') || | |
| config.dbConnectionUrl.startsWith('sqlite:///tmp/'))) { | |
| databaseInstance = new DatabaseSync(config.dbConnectionUrl); | |
| } else { | |
| throw new Error('Invalid database connection URL'); | |
| } | |
| // ... rest of initialization | |
| } | |
| return DatabaseImpl.createDatabaseService(databaseInstance); | |
| } |
Did we get this right? 👍 / 👎 to inform future reviews.
|
|
||
| export function handleAuthLogin(request: IncomingMessage, response: ServerResponse) { | ||
| this.log.info("Handling AuthLogin request"); | ||
| // Implement authentication logic here | ||
| const url = new URL( | ||
| `http://${process.env["HOST"] ?? "localhost"}${request.url}`, | ||
| ); | ||
| const username = url.searchParams.get("username") ?? ""; | ||
| const password = url.searchParams.get("password") ?? ""; | ||
|
|
||
|
|
||
| response.setHeader("Content-Type", "text/plain"); | ||
| let authResponse = "Invalid Request"; | ||
| const user = retrieveUserAccount(username, password); | ||
|
|
||
| if (user !== null) { | ||
| const ticket = generateTicket(user.customerId); | ||
| if (ticket !== "") { | ||
| authResponse = AuthLoginResponse.createValid(ticket); | ||
| } | ||
| } else { | ||
|
|
||
| authResponse = AuthLoginResponse.createInvalid( | ||
| "INV-100", | ||
| "Opps!", | ||
| "https://winehq.com", | ||
| ); | ||
| } | ||
| response.end(authResponse); |
There was a problem hiding this comment.
Input validation missing: The username and password parameters from the URL are not validated for length, content, or presence. This could lead to injection attacks or denial of service.
| export function handleAuthLogin(request: IncomingMessage, response: ServerResponse) { | |
| this.log.info("Handling AuthLogin request"); | |
| // Implement authentication logic here | |
| const url = new URL( | |
| `http://${process.env["HOST"] ?? "localhost"}${request.url}`, | |
| ); | |
| const username = url.searchParams.get("username") ?? ""; | |
| const password = url.searchParams.get("password") ?? ""; | |
| response.setHeader("Content-Type", "text/plain"); | |
| let authResponse = "Invalid Request"; | |
| const user = retrieveUserAccount(username, password); | |
| if (user !== null) { | |
| const ticket = generateTicket(user.customerId); | |
| if (ticket !== "") { | |
| authResponse = AuthLoginResponse.createValid(ticket); | |
| } | |
| } else { | |
| authResponse = AuthLoginResponse.createInvalid( | |
| "INV-100", | |
| "Opps!", | |
| "https://winehq.com", | |
| ); | |
| } | |
| response.end(authResponse); | |
| export function handleAuthLogin(request: IncomingMessage, response: ServerResponse) { | |
| this.log.info("Handling AuthLogin request"); | |
| const url = new URL( | |
| `http://${process.env["HOST"] ?? "localhost"}${request.url}`, | |
| ); | |
| const username = url.searchParams.get("username") ?? ""; | |
| const password = url.searchParams.get("password") ?? ""; | |
| // Input validation | |
| if (!username || !password) { | |
| response.setHeader("Content-Type", "text/plain"); | |
| response.end(AuthLoginResponse.createInvalid("INV-101", "Missing credentials", "https://winehq.com")); | |
| return; | |
| } | |
| if (username.length > 50 || password.length > 100) { | |
| response.setHeader("Content-Type", "text/plain"); | |
| response.end(AuthLoginResponse.createInvalid("INV-102", "Invalid input length", "https://winehq.com")); | |
| return; | |
| } | |
| response.setHeader("Content-Type", "text/plain"); | |
| let authResponse = "Invalid Request"; | |
| const user = retrieveUserAccount(username, password); | |
| if (user !== null) { | |
| const ticket = generateTicket(user.customerId); | |
| if (ticket !== "") { | |
| authResponse = AuthLoginResponse.createValid(ticket); | |
| } | |
| } else { | |
| authResponse = AuthLoginResponse.createInvalid( | |
| "INV-100", | |
| "Authentication failed", | |
| "https://winehq.com", | |
| ); | |
| } | |
| response.end(authResponse); | |
| } |
Did we get this right? 👍 / 👎 to inform future reviews.
| } | ||
|
|
||
| // Database Service Interface | ||
| export interface AuthDatabaseService { | ||
| isDatabaseConnected: () => boolean; | ||
| registerUser: ( | ||
| username: string, | ||
| password: string, | ||
| customerId: number, | ||
| ) => void; | ||
| findUser: (username: string, password: string) => UserRecordMini; | ||
| updateSession: ( | ||
| customerId: number, | ||
| contextId: string, | ||
| userId: number, | ||
| ) => void; |
There was a problem hiding this comment.
Performance issue: Database operations are synchronous and could block the event loop. Consider using asynchronous database operations for better performance, especially under load.
| } | |
| // Database Service Interface | |
| export interface AuthDatabaseService { | |
| isDatabaseConnected: () => boolean; | |
| registerUser: ( | |
| username: string, | |
| password: string, | |
| customerId: number, | |
| ) => void; | |
| findUser: (username: string, password: string) => UserRecordMini; | |
| updateSession: ( | |
| customerId: number, | |
| contextId: string, | |
| userId: number, | |
| ) => void; | |
| // Consider implementing async versions of database operations: | |
| // async findUser(...): Promise<UserRecordMini | null> | |
| // async registerUser(...): Promise<void> | |
| // async updateSession(...): Promise<void> |
Did we get this right? 👍 / 👎 to inform future reviews.
app/authserver/AuthServer.ts
Outdated
| coreLogger.info("Starting Auth Server..."); | ||
| try { | ||
| if (!authDB.isDatabaseConnected) { | ||
| coreLogger.fatal("Database connection failed. Exiting."); | ||
| process.exit(1); | ||
| } | ||
| } catch (err) { | ||
| coreLogger.fatal(`Error in core server: ${String(err)}`); | ||
| process.exitCode = 1; | ||
| return; |
There was a problem hiding this comment.
Missing error handling: The database connection check uses a property 'isDatabaseConnected' that returns a boolean, but the actual database initialization in db.ts could throw exceptions that aren't caught here.
| coreLogger.info("Starting Auth Server..."); | |
| try { | |
| if (!authDB.isDatabaseConnected) { | |
| coreLogger.fatal("Database connection failed. Exiting."); | |
| process.exit(1); | |
| } | |
| } catch (err) { | |
| coreLogger.fatal(`Error in core server: ${String(err)}`); | |
| process.exitCode = 1; | |
| return; | |
| try { | |
| if (!authDB.isDatabaseConnected()) { | |
| coreLogger.fatal("Database connection failed. Exiting."); | |
| process.exit(1); | |
| } | |
| // Test database connection with a simple query | |
| authDB.getAllUsers(); | |
| } catch (err) { | |
| coreLogger.fatal(`Database connection error: ${String(err)}`); | |
| process.exit(1); | |
| } |
Did we get this right? 👍 / 👎 to inform future reviews.
Co-authored-by: seer-by-sentry[bot] <157164994+seer-by-sentry[bot]@users.noreply.github.com>
app/authserver/package.json
Outdated
| }, | ||
| "keywords": [], | ||
| "author": "", | ||
| "license": "ISC", |
There was a problem hiding this comment.
@sentry you should explain why you removed the license key
|
Sentry has determined that unit tests are not necessary for this PR. |
|
No description provided.