use built-in Docker init, remove tini #502
Conversation
We no longer need tini with recent versions of Docker. See: * https://docs.docker.com/reference/cli/docker/container/run/#init * https://github.com/krallin/tini?#using-tini Tested and seems to work as expected when `--init` is included in `docker run` command line or `init: true` is included in the Docker Compose service stanza. I **think** it's OK to exclude the `-g` argument (pass along signals to process group) since we only run the one node process, but I'm not certain. Look for a correspoding change in the docs repo.
actual-github-bot
bot
changed the title
|
see actualbudget/docs#486 ... this PR should be merged along with that PR (or both should be closed unmerged) |
|
From a Discord chat discussion: if people upgrade Docker containers and get this change, their containers will shut down more slowly (10sec by default, IIRC) until/unless |
|
Do we still need tini with recent Docker versions if --init or init: true works, and is it safe to skip the -g argument? |
|
👋 Hi! It looks like this PR has not had any changes for a week now. Would you like someone to review this PR? If so - please remove the "[WIP]" prefix from the PR title. That will let the community know that this PR is open for a review. |
|
Based on Daniel's message I think we're OK to merge? I'm fine either way |
|
👋 Hi! It looks like this PR has not had any changes for a week now. Would you like someone to review this PR? If so - please remove the "[WIP]" prefix from the PR title. That will let the community know that this PR is open for a review. |
meonkeys
changed the title
WalkthroughThe pull request introduces several changes across multiple Dockerfiles and a Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
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: 0
🧹 Outside diff range and nitpick comments (3)
docker/stable-alpine.Dockerfile (1)
Line range hint
1-24: Add documentation comment about init requirementSince the ENTRYPOINT directive has been removed and tini is no longer used, add a comment in the Dockerfile explaining the requirement for Docker's built-in init system.
FROM alpine:3.18 as prod +# This image requires Docker's built-in init system +# Run with: docker run --init +# Or in docker-compose.yml: init: true RUN apk add --no-cache nodejsdocker-compose.yml (1)
23-24: LGTM! Consider updating documentationThe addition of
init: truecorrectly implements Docker's built-in init system. The comment clearly explains its purpose for signal forwarding.Consider adding a link to Docker's documentation about the init system in the comment:
- # forward signals to Node.js for faster container shutdown + # forward signals to Node.js for faster container shutdown + # See: https://docs.docker.com/engine/reference/run/#specify-an-init-process init: truedocker/edge-alpine.Dockerfile (1)
Line range hint
1-38: Consider adding health check for proper shutdown verificationSince we're relying on Docker's init system for signal handling, consider adding a HEALTHCHECK instruction to verify proper process management.
ENV ACTUAL_WEB_ROOT=/public EXPOSE 5006 +HEALTHCHECK --interval=30s --timeout=10s --start-period=5s --retries=3 \ + CMD wget --no-verbose --tries=1 --spider http://localhost:5006/health || exit 1 CMD ["node", "app.js"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/502.mdis excluded by!**/*.md
📒 Files selected for processing (6)
Dockerfile(0 hunks)docker-compose.yml(1 hunks)docker/edge-alpine.Dockerfile(1 hunks)docker/edge-ubuntu.Dockerfile(0 hunks)docker/stable-alpine.Dockerfile(1 hunks)docker/stable-ubuntu.Dockerfile(0 hunks)
💤 Files with no reviewable changes (3)
- Dockerfile
- docker/stable-ubuntu.Dockerfile
- docker/edge-ubuntu.Dockerfile
🔇 Additional comments (2)
docker/stable-alpine.Dockerfile (1)
11-11: Verify signal handling with Docker's built-in init system
The removal of tini is acceptable when using Docker's built-in init system. However, this change requires users to:
- Use
docker run --initflag - Set
init: truein docker-compose.yml
✅ Verification successful
Let me search for any Node.js server or process management code to ensure we have a complete picture.
Docker's built-in init is sufficient for this application
The codebase analysis shows that signal handling requirements are minimal:
- Only uses
process.on('unhandledRejection')for error logging - Creates a single HTTP/HTTPS server
- No child process spawning or complex process management
- No worker threads or clustering
The removal of tini is safe as long as Docker's built-in init is used. Users should:
- Use
docker run --initflag, or - Set
init: truein docker-compose.yml
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any direct process spawning or child processes that might need signal handling
rg -A 2 "spawn|fork|exec" src/
Length of output: 853
Script:
#!/bin/bash
# Check for server creation and process management
rg -A 3 "createServer|process\.on|cluster\." src/
# Look for main entry point
fd "index.js|server.js|app.js" --type f
Length of output: 456
docker/edge-alpine.Dockerfile (1)
19-19: Verify consistency with stable-alpine.Dockerfile
The removal of tini is consistent with the changes in stable-alpine.Dockerfile. However, ensure that the signal handling requirements are met consistently across all environments.
✅ Verification successful
Removal of tini is safe - Node.js handles signals appropriately
The codebase inspection shows that:
- The application uses Node.js's built-in process handling, with only an
unhandledRejectionhandler - No explicit signal handlers (SIGTERM/SIGINT) are implemented, meaning Node.js's default handlers are used
- Both Dockerfiles are consistent in their configuration, running the Node.js process directly
- Node.js properly handles process signals by default in container environments
Since Node.js handles process signals appropriately in containerized environments, the removal of tini is safe and consistent across both Dockerfiles.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any differences in process handling between edge and stable
rg -l "child_process|cluster" | xargs ast-grep --pattern 'require("child_process")'
Length of output: 85
Script:
#!/bin/bash
# Let's check for any process-related code or signal handling in the codebase
rg -l "process\.(on|once)\s*\(" --type js
# Also check for direct process signal handling
rg "SIGTERM|SIGINT|SIGKILL" --type js -A 2
Length of output: 99
Script:
#!/bin/bash
# Let's examine the process handling in app.js
cat src/app.js
# Also check both Dockerfiles to compare their configurations
cat docker/edge-alpine.Dockerfile
cat docker/stable-alpine.Dockerfile
Length of output: 5004
|
Hmm, can't say I agree 100% with the AI (for reasons below), but I'll make one of the suggested changes (except I'll fix the broken link it originally suggested). Reasons:
|
Bumps [cross-spawn](https://github.com/moxystudio/node-cross-spawn) from 7.0.3 to 7.0.6. - [Changelog](https://github.com/moxystudio/node-cross-spawn/blob/master/CHANGELOG.md) - [Commits](moxystudio/node-cross-spawn@v7.0.3...v7.0.6) --- updated-dependencies: - dependency-name: cross-spawn dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Can't hurt (too much) to link to relevant documentation (until the link falls out of date...) This was an AI-suggested change, for what it's worth. Except that the reasonable-looking Docker docs link provided by the AI was incorrect so... I guess I'll take the credit myself for having fact-checked it.
* fix: wrong institution_id for HYPE_HYEEIT22 in `BANKS_WITH_LIMITED_HISTORY` array * added upcoming-release-note * fixed author name in release note
meonkeys
force-pushed
the
use-builtin-docker-tini
branch
from
f1c8e63 to
52af854
Compare
|
@coderabbitai resolve |
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Outside diff range and nitpick comments (25)
migrations/1718889148000-openid.js (1)
17-21: Add Indices for Improved Query PerformanceConsider adding indices to the
pending_openid_requeststable to optimize query performance, especially if the table will be queried frequently on columns likeexpiry_time.Apply this diff to add an index on the
expiry_timecolumn:+CREATE INDEX idx_pending_openid_requests_expiry ON pending_openid_requests(expiry_time);jest.global-setup.js (1)
9-37: Error Handling in User Creation FunctionThe
createUserfunction throws errors for missing parameters and logs errors during database mutations. However, consider handling database errors more gracefully and providing meaningful error messages to aid debugging.src/accounts/password.js (1)
112-124: Consistent Password Policy EnforcementIn the
changePasswordfunction, ensure that password complexity requirements are enforced to enhance security. Consider adding validation for minimum length, character variety, and disallowed common passwords.src/app-account.js (1)
53-55: Ensure consistent response structure in API responsesThe
/login-methodsendpoint returnsmethodsdirectly at the top level of the response, whereas other endpoints wrap data inside adataobject. For consistency and to adhere to best practices, consider wrappingmethodsinside adataobject.Apply this diff to standardize the response structure:
app.get('/login-methods', (req, res) => { let methods = listLoginMethods(); - res.send({ status: 'ok', methods }); + res.send({ status: 'ok', data: { methods } }); });src/services/user-service.js (1)
194-213: Handle unique constraint violations explicitly when adding user accessIn the
addUserAccessfunction, when a unique constraint violation occurs (e.g., access already exists), an error is thrown and caught, but the specific error message is not clear. Consider handling this exception more explicitly to provide clearer feedback.Apply this diff to improve error handling:
getAccountDb().mutate( 'INSERT INTO user_access (user_id, file_id) VALUES (?, ?)', [userId, fileId], ); } catch (error) { - if (error.message.includes('UNIQUE constraint')) { + if (error.message.includes('SQLITE_CONSTRAINT_UNIQUE')) { throw new Error('Access already exists'); } throw new Error(\`Failed to add user access: \${error.message}\`); }src/accounts/openid.js (1)
285-285: Simplify null checks using optional chainingYou can simplify the null check in the
getServerHostnamefunction by using optional chaining (?.), which makes the code more concise and readable.Apply this diff to use optional chaining:
-export function getServerHostname() { - const auth = getAccountDb().first( - 'select * from auth WHERE method = ? and active = 1', - ['openid'], - ); - if (auth && auth.extra_data) { +export function getServerHostname() { + const auth = getAccountDb().first( + 'select * from auth WHERE method = ? and active = 1', + ['openid'], + ); + if (auth?.extra_data) {🧰 Tools
🪛 Biome (1.9.4)
[error] 285-285: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/app-admin.js (4)
51-58: Enhance input validation to report all missing fieldsWhen both
userNameandroleare missing from the request body, the error response only indicates one missing field. It's better user experience to inform the user of all missing required fields.Apply this diff to enhance input validation:
if (!userName || !role) { - res.status(400).send({ - status: 'error', - reason: `${!userName ? 'user-cant-be-empty' : 'role-cant-be-empty'}`, - details: `${!userName ? 'Username' : 'Role'} cannot be empty`, - }); + const missingFields = []; + if (!userName) missingFields.push('userName'); + if (!role) missingFields.push('role'); + res.status(400).send({ + status: 'error', + reason: 'missing-fields', + details: `Missing required field(s): ${missingFields.join(', ')}`, + }); return; }
103-110: Enhance input validation to report all missing fieldsSimilar to the
POST /usersendpoint, when updating a user, if bothuserNameandroleare missing, the error response only indicates one missing field. Consider modifying the validation to report all missing required fields.Apply this diff to enhance input validation:
if (!userName || !role) { - res.status(400).send({ - status: 'error', - reason: `${!userName ? 'user-cant-be-empty' : 'role-cant-be-empty'}`, - details: `${!userName ? 'Username' : 'Role'} cannot be empty`, - }); + const missingFields = []; + if (!userName) missingFields.push('userName'); + if (!role) missingFields.push('role'); + res.status(400).send({ + status: 'error', + reason: 'missing-fields', + details: `Missing required field(s): ${missingFields.join(', ')}`, + }); return; }
40-47: Refactor admin permission checks into middlewareRepeatedly checking admin permissions with
if (!isAdmin(res.locals.user_id))can be refactored into a middleware function. This reduces code duplication and improves maintainability.Example middleware:
function requireAdmin(req, res, next) { if (!isAdmin(res.locals.user_id)) { res.status(403).send({ status: 'error', reason: 'forbidden', details: 'permission-not-found', }); return; } next(); }And use it in routes:
app.post('/users', validateSessionMiddleware, requireAdmin, async (req, res) => { /*...*/ });Also applies to: 92-99, 145-151, 230-237, 284-291, 329-336, 365-372
195-195: Avoid returning values from Express route handlersReturning a value like
return false;from an Express route handler is unnecessary and may cause confusion. Simply usereturn;to end the request-response cycle.Apply this diff to remove unnecessary return values:
- return false; + return;Also applies to: 205-205, 215-215, 336-336, 371-371
src/util/validate-user.js (1)
6-7: Consider using a more descriptive constant nameThe constant
TOKEN_EXPIRATION_NEVERwith value-1could be more descriptive. Consider usingSESSION_NEVER_EXPIRESto better align with the session-based terminology used throughout the codebase.src/scripts/reset-password.js (2)
10-27: Improve error handling and user feedbackThe error handling is good, but the error messages could be more specific:
- Consider adding error codes to help with troubleshooting
- The GitHub issues URL is repeated - could be extracted to a constant
+ const GITHUB_ISSUES_URL = 'https://github.com/actualbudget/actual-server/issues'; - console.log('Error setting password:', error); + console.log('Error setting password (code: SET_PWD_01):', error); console.log( - 'Please report this as an issue: https://github.com/actualbudget/actual-server/issues', + `Please report this as an issue: ${GITHUB_ISSUES_URL}`, );
30-50: Consider adding password complexity validationThe password change functionality should validate password complexity before proceeding. This could prevent weak passwords from being set.
Would you like me to help implement password complexity validation rules?
src/util/middlewares.js (2)
34-42: Add session activity trackingConsider adding session activity tracking to help with security auditing and session management:
const validateSessionMiddleware = async (req, res, next) => { let session = await validateSession(req, res); if (!session) { return; } + // Track session activity + session.last_activity = Date.now(); + session.last_ip = req.ip; res.locals = session; next(); };
1-1: Consider renaming the file to match new terminologySince we're moving from user validation to session validation, consider renaming
validate-user.jstovalidate-session.jsfor consistency.src/app.js (2)
53-54: Consider additional security measures for admin routesThe new admin and OpenID routes might need additional security middleware beyond the global rate limiting.
Consider:
- Adding specific rate limits for these sensitive routes
- Implementing additional authentication middleware
- Adding request logging for audit purposes
Example implementation:
+const adminRateLimit = rateLimit({ + windowMs: 15 * 60 * 1000, // 15 minutes + max: 100, // limit each IP to 100 requests per windowMs + message: 'Too many admin requests from this IP' +}); -app.use('/admin', adminApp.handlers); -app.use('/openid', openidApp.handlers); +app.use('/admin', adminRateLimit, adminApp.handlers); +app.use('/openid', adminRateLimit, openidApp.handlers);
91-91: Consider using a logger instead of console.logWhile adding startup logging is good, consider using the Winston logger for consistency.
-console.log('Listening on ' + config.hostname + ':' + config.port + '...'); +logger.info(`Server listening on ${config.hostname}:${config.port}`);src/app-sync/tests/services/files-service.test.js (1)
195-224: Consider adding more specific assertionsThe test case effectively validates user access restrictions. Consider enhancing it with more specific assertions about the actual files returned, not just their count.
expect(filesService.find({ userId: 'genericUser' })).toHaveLength(1); + const userFiles = filesService.find({ userId: 'genericUser' }); + expect(userFiles[0].owner).toBe('genericUser'); expect( filesService.find({ userId: 'genericAdmin' }).length, ).toBeGreaterThan(1); + const adminFiles = filesService.find({ userId: 'genericAdmin' }); + expect(adminFiles.some(file => file.owner === 'genericUser')).toBe(true); + expect(adminFiles.some(file => file.owner === 'genericAdmin')).toBe(true);src/load-config.js (2)
110-118: Consider simplifying the multiuser configuration logicWhile the current implementation is correct, it could be simplified for better readability.
- multiuser: process.env.ACTUAL_MULTIUSER - ? (() => { - const value = process.env.ACTUAL_MULTIUSER.toLowerCase(); - if (!['true', 'false'].includes(value)) { - throw new Error('ACTUAL_MULTIUSER must be either "true" or "false"'); - } - return value === 'true'; - })() - : config.multiuser, + multiuser: process.env.ACTUAL_MULTIUSER + ? (value => { + if (!['true', 'false'].includes(value)) { + throw new Error('ACTUAL_MULTIUSER must be either "true" or "false"'); + } + return value === 'true'; + })(process.env.ACTUAL_MULTIUSER.toLowerCase()) + : config.multiuser,
153-198: Consider extracting OpenID configuration logicThe OpenID configuration logic is complex and could benefit from being extracted into a separate function for better maintainability and testability.
+function buildOpenIdConfig(config, env) { + if (!env.ACTUAL_OPENID_DISCOVERY_URL && !env.ACTUAL_OPENID_AUTHORIZATION_ENDPOINT) { + return config.openId; + } + + if (env.ACTUAL_OPENID_DISCOVERY_URL) { + return { + issuer: env.ACTUAL_OPENID_DISCOVERY_URL, + client_id: env.ACTUAL_OPENID_CLIENT_ID ?? config.openId?.client_id, + client_secret: env.ACTUAL_OPENID_CLIENT_SECRET ?? config.openId?.client_secret, + server_hostname: env.ACTUAL_OPENID_SERVER_HOSTNAME ?? config.openId?.server_hostname, + }; + } + + const required = { + authorization_endpoint: env.ACTUAL_OPENID_AUTHORIZATION_ENDPOINT, + token_endpoint: env.ACTUAL_OPENID_TOKEN_ENDPOINT, + userinfo_endpoint: env.ACTUAL_OPENID_USERINFO_ENDPOINT, + }; + + const missing = Object.entries(required) + .filter(([_, value]) => !value) + .map(([key]) => key); + + if (missing.length > 0) { + throw new Error(`Missing required OpenID configuration: ${missing.join(', ')}`); + } + + return { + issuer: { + name: env.ACTUAL_OPENID_PROVIDER_NAME, + ...required, + }, + client_id: env.ACTUAL_OPENID_CLIENT_ID ?? config.openId?.client_id, + client_secret: env.ACTUAL_OPENID_CLIENT_SECRET ?? config.openId?.client_secret, + server_hostname: env.ACTUAL_OPENID_SERVER_HOSTNAME ?? config.openId?.server_hostname, + }; +} + const finalConfig = { // ... other config - openId: (() => { - // ... current implementation - })(), + openId: buildOpenIdConfig(config, process.env),src/app-sync.js (1)
251-255: Enhance error message for missing user ID.The error handling for missing user ID is good, but the error message could be more descriptive to help with debugging.
- (() => { - throw new Error('User ID is required for file creation'); - })(), + (() => { + throw new Error('User ID is required for file creation. Ensure validateSessionMiddleware is properly configured.'); + })(),src/app-admin.test.js (2)
17-21: Consider deleting user sessions when deleting usersThe
deleteUserfunction removes the user from theusersanduser_accesstables but does not delete associated sessions from thesessionstable. This may leave orphaned sessions, leading to potential test interference or unexpected behavior.Consider updating the
deleteUserfunction to also remove sessions:const deleteUser = (userId) => { getAccountDb().mutate('DELETE FROM user_access WHERE user_id = ?', [userId]); + getAccountDb().mutate('DELETE FROM sessions WHERE user_id = ?', [userId]); getAccountDb().mutate('DELETE FROM users WHERE id = ?', [userId]); };
52-65: Reduce duplication in setup and teardown processesThe
beforeEachandafterEachblocks in multiple test suites have similar code for setting up users, sessions, and cleaning up. Consolidating this code can improve maintainability and reduce redundancy.Consider creating reusable helper functions or moving common setup and teardown code to higher-level
beforeEachandafterEachblocks.Also applies to: 83-100, 153-166, 210-224, 262-280, 322-345
src/config-types.ts (2)
23-35: ExtractopenIdconfiguration into a separate interfaceDefining a separate interface for the
openIdconfiguration enhances readability and makes theConfiginterface cleaner.Refactor the code as follows:
interface OpenIdIssuer { name: string; authorization_endpoint: string; token_endpoint: string; userinfo_endpoint: string; } interface OpenIdConfig { issuer: string | OpenIdIssuer; client_id: string; client_secret: string; server_hostname: string; } export interface Config { // ... existing properties ... openId?: OpenIdConfig; // ... other properties ... }
36-36: Ensuremultiuserproperty has a default valueThe
multiuserproperty is required but may not be present in existing configurations, potentially causing runtime errors. Consider making it optional with a default value or ensuring all configurations include it.Modify the property to be optional:
- multiuser: boolean; + multiuser?: boolean;And provide a default value when loading configurations:
config.multiuser = config.multiuser ?? false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (5)
upcoming-release-notes/498.mdis excluded by!**/*.mdupcoming-release-notes/504.mdis excluded by!**/*.mdupcoming-release-notes/507.mdis excluded by!**/*.mdupcoming-release-notes/511.mdis excluded by!**/*.mdyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (28)
docker-compose.yml(1 hunks)jest.global-setup.js(1 hunks)migrations/1718889148000-openid.js(1 hunks)migrations/1719409568000-multiuser.js(1 hunks)package.json(1 hunks)src/account-db.js(3 hunks)src/accounts/openid.js(1 hunks)src/accounts/password.js(1 hunks)src/app-account.js(4 hunks)src/app-admin.js(1 hunks)src/app-admin.test.js(1 hunks)src/app-gocardless/app-gocardless.js(2 hunks)src/app-gocardless/bank-factory.js(1 hunks)src/app-openid.js(1 hunks)src/app-secrets.js(1 hunks)src/app-simplefin/app-simplefin.js(1 hunks)src/app-sync.js(7 hunks)src/app-sync.test.js(6 hunks)src/app-sync/services/files-service.js(10 hunks)src/app-sync/tests/services/files-service.test.js(4 hunks)src/app.js(3 hunks)src/config-types.ts(2 hunks)src/load-config.js(3 hunks)src/scripts/reset-password.js(1 hunks)src/services/user-service.js(1 hunks)src/util/middlewares.js(3 hunks)src/util/validate-user.js(2 hunks)tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docker-compose.yml
🧰 Additional context used
🪛 Biome (1.9.4)
src/accounts/openid.js
[error] 285-285: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (22)
migrations/1718889148000-openid.js (1)
3-24: Ensure Data Integrity During Migration
The migration script modifies the auth table by creating a new table auth_new, transferring data, dropping the old table, and renaming auth_new to auth. This process can be risky if not handled carefully. To ensure no data is lost and all necessary information is retained, please verify that all records and columns are accurately migrated.
Run the following script to compare the schema and data before and after the migration:
✅ Verification successful
Migration Schema Changes Look Safe and Reversible
The migration appears to be well-designed and maintains data integrity:
- The original table only had a single
passwordTEXT column - The new schema adds proper structure with
method,display_name,extra_data, andactivecolumns - The migration correctly preserves the password data by moving it to
extra_data - The down migration properly restores the original schema and data
- The transaction wrapper ensures atomicity of the changes
The schema evolution aligns with the new OpenID functionality requirements as seen in openid.js, and both up and down migrations are properly implemented to handle the data transformation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that data in the 'auth' table is correctly migrated to the new structure.
# Check the schema of the old 'auth' table
echo ".schema auth" | sqlite3 account.db
# Check the schema of the new 'auth' table
echo ".schema auth_new" | sqlite3 account.db
# Count records in the old 'auth' table
echo "SELECT COUNT(*) FROM auth;" | sqlite3 account.db
# Count records in the new 'auth_new' table
echo "SELECT COUNT(*) FROM auth_new;" | sqlite3 account.db
Length of output: 601
Script:
#!/bin/bash
# Let's analyze the migration script and related files to understand the data integrity aspects
# Find all files that might contain schema definitions or tests for the auth table
fd -e js -e sql . | xargs rg -l "auth.*table|CREATE TABLE.*auth"
# Look for any test files related to this migration
fd -e test.js -e spec.js . | xargs rg -l "openid|auth.*table|migration"
# Check if there are any other migration files that modify the auth table
fd -g "*migration*.js" . | xargs rg -l "auth.*table|DROP TABLE.*auth"
# Look for the schema definition of the auth table
rg -A 5 "CREATE TABLE.*auth[^_]"
Length of output: 351
Script:
#!/bin/bash
# Let's examine the content of the relevant files to understand the schema evolution
# Check the initial auth table creation in the first migration
echo "Initial auth table creation:"
rg -A 10 "CREATE TABLE.*auth" migrations/1694360479680-create-account-db.js
# Check the openid.js file for any schema requirements
echo -e "\nAuth table usage in openid.js:"
rg "auth.*table|SELECT.*FROM.*auth|INSERT.*INTO.*auth" src/accounts/openid.js
# Look for any down migrations that might help understand the schema
echo -e "\nDown migration for the openid migration:"
rg -A 10 "export const down" migrations/1718889148000-openid.js
Length of output: 1470
src/app-openid.js (1)
20-37: Ensure Proper Authorization Checks
The endpoints /enable and /disable check if a user is an admin by calling isAdmin(res.locals.user_id). Ensure that res.locals.user_id is correctly populated by the validateSessionMiddleware and that isAdmin accurately determines the user's admin status to prevent unauthorized access.
Run the following script to verify that isAdmin correctly identifies admin users:
Replace <token> with a valid session token.
✅ Verification successful
Based on the gathered information, I can now provide the final response:
Authorization Checks are Properly Implemented
The authorization flow is correctly implemented with proper security checks:
validateSessionMiddlewarepopulatesres.localswith session data fromvalidateSessionisAdminfunction inaccount-db.jsuseshasPermissionto check if user has 'ADMIN' role- User roles are properly stored in the database with 'ADMIN' as a valid role value
- Multiple endpoints consistently use this authorization pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the user with user_id in res.locals is an admin.
# Check the role of the user in the database
USER_ID=$(echo "SELECT user_id FROM sessions WHERE token = '<token>';" | sqlite3 account.db)
echo "SELECT role FROM users WHERE id = '$USER_ID';" | sqlite3 account.db
Length of output: 372
Script:
#!/bin/bash
# Let's analyze the implementation of validateSessionMiddleware and isAdmin functions
# Find validateSessionMiddleware implementation
ast-grep --pattern 'const validateSessionMiddleware = $_'
# Find isAdmin function implementation
ast-grep --pattern 'function isAdmin($_) {
$$$
}'
# Also search for any other isAdmin implementations
ast-grep --pattern 'const isAdmin = $_'
# Search for any database configuration or user role definitions
rg -l "role|admin|permission" --type js
Length of output: 1109
Script:
#!/bin/bash
# Let's check the implementation of validateSession and hasPermission functions
# Also check how user permissions are stored/verified
# Find validateSession implementation
ast-grep --pattern 'function validateSession($_) {
$$$
}'
ast-grep --pattern 'const validateSession = $_'
# Find hasPermission implementation since isAdmin uses it
ast-grep --pattern 'function hasPermission($_) {
$$$
}'
ast-grep --pattern 'const hasPermission = $_'
# Look for permission definitions and checks
rg "hasPermission|ADMIN" -A 5 --type js
# Check how session data is structured
rg "res\.locals" -A 3 --type js
Length of output: 10002
migrations/1719409568000-multiuser.js (1)
40-44: Consider Using Consistent UUID Generation
In line 40, const userId = uuid.v4(); generates a new UUID for the default admin user. Ensure that this UUID is consistent across setups if needed. If consistency is required, consider using a fixed UUID or a mechanism to retrieve the admin user's ID reliably.
jest.global-setup.js (2)
39-61: Validate Session Existence Before Update
In the setSessionUser function, after checking that the session exists, ensure that the user ID being set is valid and exists in the users table to prevent potential foreign key constraint violations.
72-94: Wrap Database Operations in Transaction
While a transaction is started with BEGIN TRANSACTION, ensure that all database mutations are within a try-catch block to handle any exceptions and that a COMMIT or ROLLBACK is called appropriately. Currently, if an error occurs, a ROLLBACK is called, but the COMMIT is only executed if there are no errors. This is good practice, but double-check that errors are caught correctly to prevent the database from being left in an inconsistent state.
src/accounts/password.js (1)
15-32: Store Hashed Passwords Securely
The bootstrapPassword function hashes the password using bcrypt with a salt round of 12, which is considered secure. Ensure that this standard is maintained throughout the application and that no plain-text passwords are stored.
src/app-sync/services/files-service.js (1)
124-146: Ensure proper handling of user permissions in file retrieval
The find method in FilesService retrieves files based on user permissions. Ensure that the SQL query correctly handles user IDs and prevents unauthorized access. Additionally, consider parameterizing the SQL statements to prevent SQL injection attacks.
Run the following script to verify that files are correctly retrieved according to user permissions:
✅ Verification successful
SQL query is properly parameterized and access control is well-implemented
The verification reveals that:
- The SQL query uses parameterized queries with
?placeholders, preventing SQL injection - The
user_accesstable has proper foreign key constraints to bothusersandfilestables - Access control is properly implemented:
- Admin check is done via
isAdmin()which uses a role-based permission system - Non-admin users can only access files they own or have explicit access through
user_accesstable - The UNION query correctly combines owned and shared files
- Admin check is done via
- The
validatemethod is consistently used for file objects before returning them
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that non-admin users can only access their own files or files they have access to.
# Test: Run the find method for a non-admin user and check the results.
# Expected: The result should only include files owned by the user or shared with them.
# Replace <userId> with a test user ID.
node -e "
const { FilesService } = require('./src/app-sync/services/files-service');
const filesService = new FilesService(require('./src/account-db').default());
const files = filesService.find({ userId: '<userId>', limit: 1000 });
console.log(files);
"
Length of output: 772
Script:
#!/bin/bash
# Let's analyze the SQL query implementation and user permission handling
# Check for any other instances of similar SQL queries in the codebase
rg -U "SELECT.*FROM files.*JOIN user_access" -g '*.js' -A 5
# Look for the implementation of isAdmin function
rg "function isAdmin" -g '*.js' -A 5
# Check for any other file access control implementations
rg "files\.owner.*=.*\?" -g '*.js'
# Look for user_access table schema or related queries
rg "CREATE TABLE.*user_access" -g '*.{js,sql}'
rg "INSERT INTO user_access" -g '*.js' -A 3
Length of output: 1674
Script:
#!/bin/bash
# Let's check the migration file for user_access table schema
cat migrations/1719409568000-multiuser.js
# Check the implementation of getUserPermission function
rg "function getUserPermission" -g '*.js' -A 5
# Look for SQL parameter validation or sanitization
rg "sanitize|validate" -g '*.js' -A 3
# Check for any SQL injection prevention mechanisms
ast-grep --pattern 'this.validate($$$)'
Length of output: 13522
src/util/validate-user.js (1)
32-42: Verify session expiration timestamp handling
The session expiration check multiplies expires_at by MS_PER_SECOND, assuming it's stored in seconds. This assumption should be verified to prevent timing issues.
Also, consider adding a small buffer time (e.g., 5 seconds) to account for network latency and clock skew:
- session.expires_at * MS_PER_SECOND <= Date.now()
+ session.expires_at * MS_PER_SECOND <= Date.now() + 5000✅ Verification successful
Let me gather more context about how the expiration timestamp is calculated and used.
Timestamp handling is correctly implemented
The verification confirms that the timestamp handling is consistent throughout the codebase:
- Expiration timestamps are stored in seconds (Unix timestamp):
- Password auth:
Math.floor(Date.now() / 1000) + finalConfig.token_expiration * 60 - OpenID auth: Either from provider's
expires_at(in seconds) or calculated similarly
- Password auth:
- The validation correctly multiplies by
MS_PER_SECOND(1000) to compare withDate.now()(in milliseconds)
Regarding the buffer suggestion: While adding a buffer could help with clock skew, the current implementation is correct and follows standard practices. The token expiration already includes minutes-level granularity, which provides sufficient margin for typical network latency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all places where expires_at is set to verify timestamp format
rg -A 3 "expires_at.*="
Length of output: 825
Script:
#!/bin/bash
# Search for TOKEN_EXPIRATION_NEVER and MS_PER_SECOND constants
rg "TOKEN_EXPIRATION_NEVER|MS_PER_SECOND" -B 2 -A 2
# Look for expiration calculation
rg "expiration.*=" -B 2 -A 2
# Search for session creation/update patterns to understand timestamp handling
ast-grep --pattern 'UPDATE sessions SET $$$expires_at$$$'
Length of output: 3454
src/util/middlewares.js (1)
Line range hint 1-58: Verify consistent usage of new session validation
Let's verify that all instances of the old user validation have been updated to use session validation.
✅ Verification successful
Let me verify the implementation of validate-user.js to ensure it properly handles session validation.
Session validation implementation is consistent across the codebase
The verification confirms that:
- The session validation is properly implemented in
validate-user.jswith token-based authentication - All user endpoints in
app-admin.jsconsistently usevalidateSessionMiddleware res.localsusage is consistent with the session-based approach, storing user information- No legacy user validation code remains in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of validateUser
rg "validateUser"
# Search for any remaining instances of user validation terminology
rg -i "user.*validat|validat.*user"
# Search for places where res.locals is accessed to ensure compatibility
rg "res\.locals\." -A 2
Length of output: 3185
Script:
#!/bin/bash
# Check the implementation of validate-user.js
cat src/util/validate-user.js
# Check for any session-related imports or usage
rg -l "session" --type js
Length of output: 2054
package.json (1)
39-39: Verify openid-client version for security updates
The addition of openid-client looks good, but let's verify the version for any security advisories.
src/app.js (1)
14-15: LGTM: New module imports
The imports for admin and OpenID modules are properly structured.
src/app-gocardless/bank-factory.js (1)
83-83: Verify impact of bank identifier update
The change from HYPE_HYEEIT2 to HYPE_HYEEIT22 might affect existing integrations.
✅ Verification successful
Bank identifier update appears to be an isolated change
The search results show that:
- The new identifier
HYPE_HYEEIT22only appears in bank-factory.js - There are no occurrences of the old identifier
HYPE_HYEEIT2anywhere in the codebase - The bank appears to have two variants:
HYPE_BUSINESS_HYEEIT22andHYPE_HYEEIT22 - No test files or documentation mention these identifiers
Since there are no references to the old identifier in the codebase and the change is isolated to the bank identifiers list, this appears to be a safe update.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for uses of the old bank identifier
rg "HYPE_HYEEIT2[^2]" # Search for old identifier without the extra 2
# Look for related test files
fd -t f "test|spec" | rg -l "HYPE"
Length of output: 58
Script:
#!/bin/bash
# Let's try a broader search for any HYPE related bank identifiers
rg "HYPE" -A 2 -B 2
# Also search for any test files or documentation mentioning HYPE banks
fd -t f ".(js|md|json)$" -X rg "HYPE"
# Look for the exact bank identifier in any form
rg -w "HYPE_HYEEIT2"
rg -w "HYPE_HYEEIT22"
Length of output: 629
src/app-gocardless/app-gocardless.js (1)
17-17: Verify middleware change impact on all routes
The change from validateUserMiddleware to validateSessionMiddleware looks good and aligns with the new session-based validation approach.
Let's verify that this middleware change is consistent across all routes:
Also applies to: 29-29
✅ Verification successful
Middleware change is consistently implemented across the codebase
The verification shows that:
validateSessionMiddlewareis properly defined insrc/util/middlewares.jsand exported- It's consistently used across all relevant app modules (app-gocardless, app-admin, app-secrets, app-sync, app-openid)
- No instances of the old
validateUserMiddlewareremain in the codebase - The middleware is applied both at the app level (using
app.use()) and on specific routes where needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of validateUserMiddleware
rg "validateUserMiddleware" -l
# Search for validateSessionMiddleware usage in other route files
rg "validateSessionMiddleware" -l
Length of output: 195
Script:
#!/bin/bash
# Let's check the actual usage context of both middlewares
echo "=== Checking validateSessionMiddleware implementations and usage ==="
rg "validateSessionMiddleware" -C 2
echo -e "\n=== Checking if validateUserMiddleware still exists in implementations ==="
rg "validateUserMiddleware" -C 2
Length of output: 4857
src/app-sync/tests/services/files-service.test.js (2)
Line range hint 127-143: LGTM: Updated test case properly validates user access
The test case has been correctly updated to include the userId parameter, ensuring proper validation of user-specific file access.
156-163: LGTM: Test properly combines pagination with user access
The test case successfully validates both the limit parameter and user access restrictions.
src/load-config.js (1)
80-81: LGTM: Sensible defaults for new configuration options
The new configuration options have appropriate default values that maintain backward compatibility.
src/app-sync.js (1)
8-8: Verify the impact of switching to session-based authentication.
The change from validateUserMiddleware to validateSessionMiddleware is a significant authentication change that could affect existing clients.
Also applies to: 28-28
src/app-simplefin/app-simplefin.js (1)
213-219: LGTM! Improved date handling logic.
The refactoring improves readability by using a dedicated variable for the transaction date.
src/app-sync.test.js (3)
9-16: LGTM! Well-structured helper function for user creation.
The createUser helper function improves test maintainability and readability.
535-535: LGTM! Comprehensive test coverage for file ownership.
The test cases properly verify file ownership functionality with admin role.
Also applies to: 543-544, 547-548
849-850: LGTM! Updated mock file creation with owner field.
The helper function correctly includes the owner field in mock data.
tsconfig.json (1)
20-20: Including JavaScript files in TypeScript compilation
The "include" array now includes "src/**/*.js", which ensures that JavaScript files in the src directory are type-checked by the compiler. This is appropriate given that allowJs and checkJs are enabled.
|
@coderabbitai resolve (sorry if I'm breaking something here, I didn't carefully research how to control code rabbit ... looks like it is reviewing merged-in commits though) |
|
@meonkeys: It appears that CodeRabbit is reviewing merged commits even though your configuration has In your configuration, automatic reviews are enabled and are triggered by the label auto_review:
enabled: true
auto_incremental_review: true
labels:
- ':mag: Ready for review'If PRs retain the
If you've verified that the labels and workflow are set correctly and CodeRabbit is still reviewing merged commits, there might be an issue we need to address. Please let me know if you need further assistance, and we can look into it together. 🔗 Analysis chain@meonkeys: I've resolved all my review comments. If you're experiencing issues with CodeRabbit reviewing merged-in commits, we can check the configuration settings. I'll fetch the current configuration for you to review. 🏁 Scripts executedThe following scripts were executed for the analysis:
✅ Actions performedComments resolved. |
We no longer need tini with recent versions of Docker.
See:
Tested and seems to work as expected when
--initis included indocker runcommand line orinit: trueis included in the Docker Compose service stanza.I think it's OK to exclude the
-gargument (pass along signals to process group) since we only run the one node process, but I'm not certain.Look for a correspoding change in the docs repo.