sync-server eslint --fix #4362
sync-server eslint --fix #4362
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
WalkthroughThis pull request encompasses several refactoring updates across the codebase. Changes include broad reordering of import statements and converting variable declarations from Suggested labels
Suggested reviewers
✨ Finishing Touches
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: 1
🔭 Outside diff range comments (6)
packages/sync-server/src/app-gocardless/banks/hype_hyeeit22.js (2)
47-68: 🛠️ Refactor suggestionAdd error handling for malformed Unicode sequences.
The Unicode parsing logic assumes well-formed sequences but lacks error handling for malformed input.
if (transaction.proprietaryBankTransactionCode == 'p2p') { let str = transaction.remittanceInformationUnstructured; let idx = str.indexOf('\\U'); let start_idx = idx; let codepoints = []; while (idx !== -1) { + try { codepoints.push(parseInt(str.slice(idx + 2, idx + 6), 16)); const next_idx = str.indexOf('\\U', idx + 6); if (next_idx == idx + 6) { idx = next_idx; continue; } str = str.slice(0, start_idx) + String.fromCodePoint(...codepoints) + str.slice(idx + 6); codepoints = []; idx = str.indexOf('\\U'); start_idx = idx; + } catch (error) { + console.error('Failed to parse Unicode sequence:', error); + break; + } } transaction.remittanceInformationUnstructured = str; }
31-38: 💡 Verification agent🧩 Analysis chain
Review payee name parsing logic for robustness.
The comment indicates a potential issue if payee names contain dashes. Consider implementing a more robust parsing strategy.
🏁 Script executed:
#!/bin/bash # Search for any instances of payee names containing dashes to assess impact rg -l "remittanceInformationUnstructured.*-.*payee"Length of output: 145
I'll now inspect the file
packages/sync-server/src/app-gocardless/banks/util/extract-payeeName-from-remittanceInfo.jsto review how it handles payee name extraction and dash parsing. This will let us compare the logic with that inhype_hyeeit22.jsand see if there's a more robust strategy implemented there.
🏁 Script executed:
#!/bin/bash # Show the first 100 lines of extract-payeeName-from-remittanceInfo.js for inspection sed -n '1,100p' packages/sync-server/src/app-gocardless/banks/util/extract-payeeName-from-remittanceInfo.jsLength of output: 1949
Payee name parsing logic in
hype_hyeeit22.jscontains an off-by-one error that could corrupt the remittance info when the delimiter isn't present.
- When
indexOf(' - ')returns -1, adding 3 yields 2 (i.e.-1 + 3 = 2), so the code erroneously slices the string starting at index 2.- This behavior risks trimming valid parts of the remittance information, especially if payee names might include dashes.
- Consider checking for a valid index (i.e. verifying that
indexOfdid not return -1 before adding 3) or leveraging a more robust extraction strategy such as the one inextractPayeeName-from-remittanceInfo.js.packages/sync-server/src/app-gocardless/banks/bnp_be_gebabebb.js (1)
30-67: 🛠️ Refactor suggestionAdd validation for additionalInformation format.
The code assumes a specific format for additionalInformation but doesn't validate it before processing.
if (transaction.additionalInformation) { + if (typeof transaction.additionalInformation !== 'string') { + console.warn('additionalInformation is not a string:', transaction.additionalInformation); + return Fallback.normalizeTransaction(transaction, _booked); + } + const additionalInformationObject = {}; const additionalInfoRegex = /(, )?([^:]+): ((\[.*?\])|([^,]*))/g; const matches =packages/sync-server/src/accounts/password.js (1)
40-44: 🛠️ Refactor suggestionConsider adding rate limiting for login attempts.
The login function could benefit from rate limiting to prevent brute force attacks.
+import rateLimit from 'express-rate-limit'; + +const loginLimiter = rateLimit({ + windowMs: 15 * 60 * 1000, // 15 minutes + max: 5 // limit each IP to 5 requests per windowMs +}); + export function loginWithPassword(password) { if (!isValidPassword(password)) { return { error: 'invalid-password' }; }packages/sync-server/src/app-account.js (2)
64-80:⚠️ Potential issueSecurity concern: Password logging.
The code logs password-related information, even if obfuscated. This could potentially expose sensitive information in logs.
- const obfuscated = - '*'.repeat(headerVal.length) || 'No password provided.'; - console.debug('HEADER VALUE: ' + obfuscated); + // Avoid logging password-related information + console.debug('Processing header authentication');
113-125: 🛠️ Refactor suggestionConsider adding password complexity requirements.
The password change functionality should validate password complexity to ensure security.
+function isPasswordComplex(password) { + const minLength = 8; + const hasUpperCase = /[A-Z]/.test(password); + const hasLowerCase = /[a-z]/.test(password); + const hasNumbers = /\d/.test(password); + const hasSpecialChar = /[!@#$%^&*]/.test(password); + return password.length >= minLength && hasUpperCase && hasLowerCase && + hasNumbers && hasSpecialChar; +} + app.post('/change-password', (req, res) => { const session = validateSession(req, res); if (!session) return; + if (!isPasswordComplex(req.body.password)) { + res.status(400).send({ + status: 'error', + reason: 'password-complexity' + }); + return; + } + const { error } = changePassword(req.body.password);
🧹 Nitpick comments (6)
packages/sync-server/src/app-gocardless/banks/revolut_revolt21.js (1)
13-56: Consider refactoring duplicate date extraction logic.The date extraction logic is duplicated in both Bizum payment conditions. Consider extracting it into a helper function.
export default { ...Fallback, institutionIds: ['REVOLUT_REVOLT21'], + _extractDate(transaction) { + const date = transaction.bookingDate || + transaction.bookingDateTime || + transaction.valueDate || + transaction.valueDateTime; + return d.format(d.parseISO(date), 'yyyy-MM-dd'); + }, + normalizeTransaction(transaction, _booked) { if ( transaction.remittanceInformationUnstructuredArray[0].startsWith( 'Bizum payment from: ', ) ) { - const date = - transaction.bookingDate || - transaction.bookingDateTime || - transaction.valueDate || - transaction.valueDateTime; - return { ...transaction, payeeName: transaction.remittanceInformationUnstructuredArray[0].replace( 'Bizum payment from: ', '', ), remittanceInformationUnstructured: transaction.remittanceInformationUnstructuredArray[1], - date: d.format(d.parseISO(date), 'yyyy-MM-dd'), + date: this._extractDate(transaction), }; } if ( transaction.remittanceInformationUnstructuredArray[0].startsWith( 'Bizum payment to: ', ) ) { - const date = - transaction.bookingDate || - transaction.bookingDateTime || - transaction.valueDate || - transaction.valueDateTime; - return { ...transaction, payeeName: formatPayeeName(transaction), remittanceInformationUnstructured: transaction.remittanceInformationUnstructuredArray[1], - date: d.format(d.parseISO(date), 'yyyy-MM-dd'), + date: this._extractDate(transaction), }; }packages/sync-server/src/app-gocardless/banks/bnp_be_gebabebb.js (1)
28-29: Convert creditorName to const.The variable can be declared as const and assigned its final value in one step.
- let creditorName = transaction.creditorName; + const creditorName = transaction.creditorName ?? + (transaction.additionalInformation ? null : transaction.creditorName);packages/sync-server/src/app-secrets.js (1)
19-31: Consider enhancing error handling details.While the error handling is present, consider providing more specific error details to aid in debugging:
} catch (error) { console.error('Failed to fetch auth method:', error); return res.status(500).send({ status: 'error', reason: 'database-error', - details: 'Failed to validate authentication method', + details: `Failed to validate authentication method: ${error.message}`, }); }packages/sync-server/src/util/validate-user.js (1)
14-18: Consider usingconstfortokenvariable.The
tokenvariable can be declared asconstafter line 17 since it's not reassigned after that point. Consider refactoring to improve immutability:- let { token } = req.body || {}; - - if (!token) { - token = req.headers['x-actual-token']; - } + const token = req.body?.token || req.headers['x-actual-token'];packages/sync-server/src/services/secrets-service.js (1)
17-21: Consider using private class field fordb.The
dbproperty could be made private using the#prefix to better encapsulate the database connection.class SecretsDb { constructor() { this.debug = createDebug('actual:secrets-db'); - this.db = null; + #db = null; }packages/sync-server/src/sync-simple.js (1)
30-31: Consider using array methods for better performance.The current implementation uses a for...of loop to process messages. Consider using array methods for better performance and readability.
- for (const msg of messages) { - const info = db.mutate( + const processMessage = (msg) => db.mutate( `INSERT OR IGNORE INTO messages_binary (timestamp, is_encrypted, content) VALUES (?, ?, ?)`, [ msg.getTimestamp(), msg.getIsencrypted() ? 1 : 0, Buffer.from(msg.getContent()), ], - ); + ); + const results = messages.map(processMessage); + results.forEach((info, idx) => { if (info.changes > 0) { - trie = merkle.insert(trie, Timestamp.parse(msg.getTimestamp())); + trie = merkle.insert(trie, Timestamp.parse(messages[idx].getTimestamp())); } - } + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (71)
packages/sync-server/migrations/1694360000000-create-folders.js(1 hunks)packages/sync-server/migrations/1719409568000-multiuser.js(1 hunks)packages/sync-server/src/account-db.js(8 hunks)packages/sync-server/src/accounts/openid.js(9 hunks)packages/sync-server/src/accounts/password.js(6 hunks)packages/sync-server/src/app-account.js(8 hunks)packages/sync-server/src/app-admin.js(2 hunks)packages/sync-server/src/app-admin.test.js(1 hunks)packages/sync-server/src/app-gocardless/app-gocardless.js(3 hunks)packages/sync-server/src/app-gocardless/banks/abanca_caglesmm.js(1 hunks)packages/sync-server/src/app-gocardless/banks/abnamro_abnanl2a.js(1 hunks)packages/sync-server/src/app-gocardless/banks/american_express_aesudef1.js(1 hunks)packages/sync-server/src/app-gocardless/banks/bancsabadell_bsabesbbb.js(1 hunks)packages/sync-server/src/app-gocardless/banks/bank.interface.ts(1 hunks)packages/sync-server/src/app-gocardless/banks/bankinter_bkbkesmm.js(1 hunks)packages/sync-server/src/app-gocardless/banks/belfius_gkccbebb.js(1 hunks)packages/sync-server/src/app-gocardless/banks/berliner_sparkasse_beladebexxx.js(2 hunks)packages/sync-server/src/app-gocardless/banks/bnp_be_gebabebb.js(2 hunks)packages/sync-server/src/app-gocardless/banks/cbc_cregbebb.js(1 hunks)packages/sync-server/src/app-gocardless/banks/commerzbank_cobadeff.js(1 hunks)packages/sync-server/src/app-gocardless/banks/danskebank_dabno22.js(1 hunks)packages/sync-server/src/app-gocardless/banks/easybank_bawaatww.js(2 hunks)packages/sync-server/src/app-gocardless/banks/entercard_swednokk.js(1 hunks)packages/sync-server/src/app-gocardless/banks/fortuneo_ftnofrp1xxx.js(1 hunks)packages/sync-server/src/app-gocardless/banks/hype_hyeeit22.js(3 hunks)packages/sync-server/src/app-gocardless/banks/ing_ingddeff.js(1 hunks)packages/sync-server/src/app-gocardless/banks/ing_pl_ingbplpw.js(1 hunks)packages/sync-server/src/app-gocardless/banks/integration-bank.js(1 hunks)packages/sync-server/src/app-gocardless/banks/kbc_kredbebb.js(1 hunks)packages/sync-server/src/app-gocardless/banks/mbank_retail_brexplpw.js(1 hunks)packages/sync-server/src/app-gocardless/banks/nbg_ethngraaxxx.js(1 hunks)packages/sync-server/src/app-gocardless/banks/norwegian_xx_norwnok1.js(1 hunks)packages/sync-server/src/app-gocardless/banks/revolut_revolt21.js(1 hunks)packages/sync-server/src/app-gocardless/banks/sandboxfinance_sfin0000.js(1 hunks)packages/sync-server/src/app-gocardless/banks/seb_kort_bank_ab.js(1 hunks)packages/sync-server/src/app-gocardless/banks/seb_privat.js(1 hunks)packages/sync-server/src/app-gocardless/banks/sparnord_spnodk22.js(1 hunks)packages/sync-server/src/app-gocardless/banks/spk_karlsruhe_karsde66.js(2 hunks)packages/sync-server/src/app-gocardless/banks/spk_marburg_biedenkopf_heladef1mar.js(1 hunks)packages/sync-server/src/app-gocardless/banks/spk_worms_alzey_ried_malade51wor.js(1 hunks)packages/sync-server/src/app-gocardless/banks/ssk_dusseldorf_dussdeddxxx.js(1 hunks)packages/sync-server/src/app-gocardless/banks/tests/abanca_caglesmm.spec.js(1 hunks)packages/sync-server/src/app-gocardless/banks/tests/belfius_gkccbebb.spec.js(1 hunks)packages/sync-server/src/app-gocardless/banks/tests/easybank_bawaatww.spec.js(1 hunks)packages/sync-server/src/app-gocardless/banks/tests/ing_pl_ingbplpw.spec.js(1 hunks)packages/sync-server/src/app-gocardless/banks/tests/integration_bank.spec.js(1 hunks)packages/sync-server/src/app-gocardless/banks/tests/nationwide_naiagb21.spec.js(1 hunks)packages/sync-server/src/app-gocardless/banks/tests/ssk_dusseldorf_dussdeddxxx.spec.js(1 hunks)packages/sync-server/src/app-gocardless/banks/tests/virgin_nrnbgb22.spec.js(1 hunks)packages/sync-server/src/app-gocardless/services/gocardless-service.js(2 hunks)packages/sync-server/src/app-gocardless/services/tests/gocardless-service.spec.js(2 hunks)packages/sync-server/src/app-gocardless/tests/bank-factory.spec.js(1 hunks)packages/sync-server/src/app-openid.js(4 hunks)packages/sync-server/src/app-secrets.js(2 hunks)packages/sync-server/src/app-simplefin/app-simplefin.js(4 hunks)packages/sync-server/src/app-sync.js(12 hunks)packages/sync-server/src/app-sync.test.js(1 hunks)packages/sync-server/src/app-sync/tests/services/files-service.test.js(3 hunks)packages/sync-server/src/app-sync/validation.js(1 hunks)packages/sync-server/src/app.js(1 hunks)packages/sync-server/src/db.js(3 hunks)packages/sync-server/src/load-config.js(2 hunks)packages/sync-server/src/migrations.js(1 hunks)packages/sync-server/src/scripts/health-check.js(1 hunks)packages/sync-server/src/secrets.test.js(1 hunks)packages/sync-server/src/services/secrets-service.js(1 hunks)packages/sync-server/src/sync-simple.js(4 hunks)packages/sync-server/src/util/middlewares.js(2 hunks)packages/sync-server/src/util/paths.js(1 hunks)packages/sync-server/src/util/prompt.js(3 hunks)packages/sync-server/src/util/validate-user.js(3 hunks)
✅ Files skipped from review due to trivial changes (46)
- packages/sync-server/src/app-gocardless/banks/entercard_swednokk.js
- packages/sync-server/src/migrations.js
- packages/sync-server/src/app-gocardless/banks/nbg_ethngraaxxx.js
- packages/sync-server/src/app-gocardless/banks/commerzbank_cobadeff.js
- packages/sync-server/src/app-gocardless/banks/tests/ing_pl_ingbplpw.spec.js
- packages/sync-server/src/app-gocardless/banks/ing_ingddeff.js
- packages/sync-server/src/app-gocardless/banks/tests/easybank_bawaatww.spec.js
- packages/sync-server/src/app-admin.test.js
- packages/sync-server/src/app-gocardless/banks/tests/abanca_caglesmm.spec.js
- packages/sync-server/src/app-gocardless/banks/norwegian_xx_norwnok1.js
- packages/sync-server/src/app-gocardless/banks/american_express_aesudef1.js
- packages/sync-server/src/app-gocardless/banks/seb_kort_bank_ab.js
- packages/sync-server/src/app-gocardless/banks/sandboxfinance_sfin0000.js
- packages/sync-server/src/app-gocardless/banks/abnamro_abnanl2a.js
- packages/sync-server/src/app-gocardless/banks/tests/nationwide_naiagb21.spec.js
- packages/sync-server/src/secrets.test.js
- packages/sync-server/src/app-sync/tests/services/files-service.test.js
- packages/sync-server/src/app-gocardless/banks/mbank_retail_brexplpw.js
- packages/sync-server/src/load-config.js
- packages/sync-server/src/app-gocardless/banks/fortuneo_ftnofrp1xxx.js
- packages/sync-server/src/app-gocardless/banks/tests/virgin_nrnbgb22.spec.js
- packages/sync-server/src/app-gocardless/banks/sparnord_spnodk22.js
- packages/sync-server/src/app-gocardless/banks/spk_worms_alzey_ried_malade51wor.js
- packages/sync-server/src/app-gocardless/banks/belfius_gkccbebb.js
- packages/sync-server/src/app-gocardless/banks/tests/integration_bank.spec.js
- packages/sync-server/src/app-gocardless/banks/seb_privat.js
- packages/sync-server/src/app-gocardless/banks/danskebank_dabno22.js
- packages/sync-server/src/app-gocardless/banks/abanca_caglesmm.js
- packages/sync-server/src/app-sync/validation.js
- packages/sync-server/src/app-gocardless/banks/bancsabadell_bsabesbbb.js
- packages/sync-server/src/scripts/health-check.js
- packages/sync-server/src/app-gocardless/banks/spk_marburg_biedenkopf_heladef1mar.js
- packages/sync-server/src/app-gocardless/banks/cbc_cregbebb.js
- packages/sync-server/src/app-gocardless/banks/integration-bank.js
- packages/sync-server/src/app-gocardless/banks/berliner_sparkasse_beladebexxx.js
- packages/sync-server/src/app-gocardless/banks/kbc_kredbebb.js
- packages/sync-server/migrations/1719409568000-multiuser.js
- packages/sync-server/src/app-gocardless/banks/ing_pl_ingbplpw.js
- packages/sync-server/src/app-gocardless/banks/bank.interface.ts
- packages/sync-server/src/app-gocardless/tests/bank-factory.spec.js
- packages/sync-server/src/app-gocardless/banks/bankinter_bkbkesmm.js
- packages/sync-server/src/app-simplefin/app-simplefin.js
- packages/sync-server/src/app-gocardless/services/tests/gocardless-service.spec.js
- packages/sync-server/src/app-gocardless/banks/tests/ssk_dusseldorf_dussdeddxxx.spec.js
- packages/sync-server/src/app.js
- packages/sync-server/src/app-sync.test.js
🧰 Additional context used
🧠 Learnings (3)
packages/sync-server/src/app-gocardless/banks/easybank_bawaatww.js (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4253
File: packages/sync-server/src/app-gocardless/banks/nationwide_naiagb21.js:41-44
Timestamp: 2025-02-11T17:25:39.615Z
Learning: In bank sync providers, core fields like transactionId and amount should be mutated directly on the transaction object as they aren't covered by the fallback normalization logic and shouldn't be exposed for user mapping. Display-related fields should use editedTrans.
packages/sync-server/src/app-gocardless/banks/ssk_dusseldorf_dussdeddxxx.js (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4253
File: packages/sync-server/src/app-gocardless/banks/integration-bank.js:68-71
Timestamp: 2025-02-11T17:28:02.196Z
Learning: In the integration-bank.js normalizeTransaction method, mutating the original transaction object by adding remittanceInformationUnstructuredArrayString and remittanceInformationStructuredArrayString properties is acceptable.
packages/sync-server/src/app-gocardless/banks/bnp_be_gebabebb.js (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4253
File: packages/sync-server/src/app-gocardless/banks/nationwide_naiagb21.js:41-44
Timestamp: 2025-02-11T17:25:39.615Z
Learning: In bank sync providers, core fields like transactionId and amount should be mutated directly on the transaction object as they aren't covered by the fallback normalization logic and shouldn't be exposed for user mapping. Display-related fields should use editedTrans.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Visual regression
- GitHub Check: Functional
- GitHub Check: web
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: Build Docker image (alpine)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Build Docker image (ubuntu)
🔇 Additional comments (35)
packages/sync-server/src/app-gocardless/banks/easybank_bawaatww.js (2)
1-7: LGTM!The import statements have been reordered according to eslint rules without affecting functionality.
39-39: LGTM!The change to use object property shorthand syntax improves code readability while maintaining the same behavior.
packages/sync-server/src/app-gocardless/banks/ssk_dusseldorf_dussdeddxxx.js (1)
28-35: LGTM! Clean and safe linting changes.The addition of curly braces improves code consistency while preserving the existing logic for handling additional transaction information.
packages/sync-server/src/app-gocardless/banks/spk_karlsruhe_karsde66.js (1)
2-4: LGTM! Clean import reordering.The reordering of imports improves code consistency while maintaining the same functionality.
packages/sync-server/src/app-gocardless/banks/tests/belfius_gkccbebb.spec.js (1)
1-21: LGTM! Well-structured test case.The test effectively verifies the transaction ID normalization logic with clear setup and assertions.
packages/sync-server/src/util/paths.js (1)
1-3: LGTM! Clean import organization.The import statements are well-organized with proper spacing, and the config module integration maintains the existing functionality.
packages/sync-server/migrations/1694360000000-create-folders.js (1)
1-3: LGTM! Clean import organization.The import statements are well-organized with proper spacing, and the config module integration maintains the existing functionality.
packages/sync-server/src/db.js (1)
13-13: LGTM! Improved variable declarations.Good conversion of
lettoconstfor variables that aren't reassigned, improving code clarity and enforcing immutability.Also applies to: 22-22, 38-39
packages/sync-server/src/app-secrets.js (2)
1-8: LGTM! Clean import organization.The import statements are well-organized and properly grouped.
35-35: LGTM! Improved variable declaration.Good conversion of
lettoconstforcanSaveSecretsas it's not reassigned.packages/sync-server/src/util/validate-user.js (1)
20-20: LGTM! Variable declarations improved.The conversion of
lettoconstforsession,peer,peerIp, andmatchedvariables improves code quality by enforcing immutability where variables are not reassigned.Also applies to: 51-52, 58-58
packages/sync-server/src/util/middlewares.js (1)
2-4: LGTM! Import ordering and variable declarations improved.The changes improve code organization through proper import ordering and enhance immutability by using
constfor thesessionvariable.Also applies to: 39-39
packages/sync-server/src/util/prompt.js (1)
4-4: LGTM! Variable declarations improved.The conversion of multiple variables to
const(rl,promise,answer,password,password2) improves code quality by enforcing immutability where variables are not reassigned.Also applies to: 9-9, 16-16, 22-22, 29-29, 44-44, 80-80
packages/sync-server/src/services/secrets-service.js (1)
2-3: LGTM! Import ordering improved.The addition and ordering of the
getAccountDbimport improves code organization.packages/sync-server/src/sync-simple.js (2)
6-8: LGTM! Import statements are well organized.The imports are logically grouped and follow a clear pattern: node built-ins first, followed by external dependencies, and then local imports.
74-75: Verify SQL injection protection.The SQL query uses a parameter placeholder which is good practice. However, let's verify that the
sinceparameter is properly sanitized before being used in the query.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for input validation or sanitization of the 'since' parameter rg -A 5 "since.*\b(sanitize|validate|check)" .Length of output: 48
Let's get more context on how the
sinceparameter is defined and used. I'll run an additional shell script to inspect the surrounding code of the SQL query (including howsinceis assigned and passed intodb.all) in the file. This will help us determine whether the parameter is properly handled via parameter binding or if additional sanitization is needed.
🏁 Script executed:
#!/bin/bash # Print the context around the SQL query and the usage of the 'since' parameter in the file rg -C 10 "db\.all\(" packages/sync-server/src/sync-simple.jsLength of output: 732
SQL Injection Protection Confirmed
The SQL query uses a parameterized placeholder (
?) with the[since]array, which ensures that the value is safely bound and escapes any malicious input. Based on the code inpackages/sync-server/src/sync-simple.js, explicit sanitization isn’t needed since parameterized queries inherently protect against SQL injection.packages/sync-server/src/app-openid.js (2)
9-13: LGTM! Middleware imports are well organized.The middleware imports are grouped together, improving code organization.
50-53: Verify the error status code change.The error status code for the
/disableendpoint was changed from 500 to 401. Let's verify this is the correct status code for the error scenarios.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for error handling patterns in OpenID-related code rg -A 5 "disableOpenID.*error" .Length of output: 34
I ran the first search but got no output, so I’ve extended the investigation. To get more insight, please run the following shell script which displays the surrounding lines in the file and locates the import/definition of
disableOpenID:Once those results are available, we can confirm whether the 401 status code is intentional for error scenarios (likely reflecting an unauthorized error) rather than a 500 (internal server error). If the overall context (including how
disableOpenIDis used or defined) corroborates that errors here are auth-related, then the status code change to 401 is correct.Please review the output of the above commands manually so we can be sure that the error handling matches the intended API error contract.
🏁 Script executed:
#!/bin/bash # Display code context around the error handling in app-openid.js echo "=== packages/sync-server/src/app-openid.js (lines 40-60) ===" sed -n '40,60p' packages/sync-server/src/app-openid.js echo "=== Searching for import statement for disableOpenID ===" rg -n "import .*disableOpenID" . echo "=== Searching for the definition (async function) for disableOpenID ===" rg -n "async disableOpenID" .Length of output: 1186
Verification of
/disableerror status code changeThe code now explicitly returns a 401 when an error occurs during the OpenID disable process—distinct from the 403 returned when a user isn’t an admin. This indicates that any error coming from the disable operation is treated as an authentication/authorization issue rather than as a generic server failure. The admin check is clear (rejecting with 403 when the user is not admin), and upon receiving an error from
disableOpenIDthe endpoint returns a 401, which is consistent with authentication error semantics.Based on the file context and how the response codes are applied, the change from 500 to 401 appears to be intentional and correct.
packages/sync-server/src/accounts/password.js (1)
21-22: Verify bcrypt salt rounds configuration.The password hashing uses a fixed salt rounds value of 12. Let's verify this is the recommended value for the current bcrypt version.
✅ Verification successful
🌐 Web query:
What is the recommended number of salt rounds for bcrypt in 2024?💡 Result:
The recommended number of salt rounds (work factor) for bcrypt in 2024 is 12 as the new baseline standard[4][5][7]. This represents an industry-wide shift from the previous default of 10 rounds due to increased computing power making lower values less secure[4][6].
Key Considerations
Security vs Performance Balance
- A work factor of 12 (~0.2-0.3 seconds/hash) provides strong security while maintaining acceptable performance for most applications[4][5]
- Higher values (13-15) are recommended for high-security systems if hardware allows:
- 13: ~1 second/hash
- 15: ~3 seconds/hash[5]
- Lower values like 10 (~0.05 seconds) are now considered insufficient for modern security requirements[4]
Implementation Guidance
// Recommended configuration const saltRounds = 12; // OWASP-compliant baseline const hashedPassword = await bcrypt.hash(plainPassword, saltRounds);Best Practices
- Benchmark your specific hardware - aim for 250-500ms per hash[4][7]
- Store work factor with each hash (automatically handled by bcrypt's output format)[6]
- Use cryptographically secure random salts (16 bytes)[1][8]
- Automatically upgrade legacy hashes when users log in[4]
The security community has shifted recommendations due to:
- Brute-force attacks becoming more feasible with modern GPUs/ASICs[5]
- OWASP's updated guidance emphasizing adaptive work factors[4]
- Default changes in major frameworks like Laravel and PHP[4]
For most applications starting new projects in 2024, 12 rounds provides the optimal balance between security and user experience[4][5][7]. Existing systems should consider migrating from lower values like 10 through gradual hash upgrades[4].
Citations:
- 1: https://schimizu.com/understanding-salt-bcrypt-argon2id-ncrypt-and-pepper-essential-concepts-for-secure-password-df160ab062bf
- 2: https://www.honeybadger.io/blog/node-password-hashing/
- 3: https://gist.github.com/608d842811b41b3122906d43164322e2
- 4: https://securinglaravel.com/security-tip-increase-your-bcrypt/
- 5: https://blog.logrocket.com/password-hashing-node-js-bcrypt/
- 6: https://en.wikipedia.org/wiki/Bcrypt
- 7: https://kinde.com/guides/authentication/passwords/bcrypt-hashing-guide/
- 8: https://auth0.com/blog/hashing-in-action-understanding-bcrypt/
Salt Rounds Configuration Verified
The web query confirms that a salt rounds value of 12 is the recommended baseline for bcrypt in 2024. The code usinghashPassword(password)with a fixed 12 rounds adheres to the current industry standard.packages/sync-server/src/app-account.js (1)
13-17: LGTM! Middleware and validation imports are well organized.The imports are logically grouped: core functionality first, followed by middleware and validation utilities.
packages/sync-server/src/account-db.js (2)
2-8: LGTM! Import statements are well organized.The imports are logically grouped and ordered, improving code readability.
22-23: LGTM! Appropriate use of const declarations.The change from
lettoconstfor these variables is correct as they are not reassigned after initialization.Also applies to: 28-29, 38-39, 101-102, 110-111, 151-151, 173-173, 200-201, 205-206, 210-211
packages/sync-server/src/app-gocardless/app-gocardless.js (2)
4-11: LGTM! Import statements are well organized with new middleware imports.The imports are logically grouped and the addition of middleware imports is appropriate.
101-101: LGTM! Appropriate use of const declarations in route handlers.The change from
lettoconstfor route parameters is correct as they are not reassigned.Also applies to: 124-124
packages/sync-server/src/accounts/openid.js (2)
2-10: LGTM! Import statements are well organized.The imports are logically grouped and the addition of
TOKEN_EXPIRATION_NEVERis appropriate.
33-33: LGTM! Appropriate use of const declarations in OpenID authentication code.The change from
lettoconstfor these variables is correct as they are not reassigned. The changes maintain the security of the authentication code.Also applies to: 52-53, 162-162, 171-171, 234-234
packages/sync-server/src/app-admin.js (3)
3-12: LGTM! Import statements are well organized.The imports are logically grouped and ordered, improving code readability.
13-13: LGTM! Express app correctly declared as const.The change from
lettoconstfor the Express app instance is appropriate as it should not be reassigned.
305-305: LGTM! Appropriate use of const declaration in delete handler.The change from
lettoconstfortotalDeletedis correct as it's assigned once from the function result.packages/sync-server/src/app-sync.js (4)
2-24: LGTM!The import statements have been properly organized and grouped.
69-73: LGTM!The conversion from
lettoconstis appropriate as these variables are not reassigned.
196-208: LGTM!The conversion from
lettoconstis appropriate as these variables are not reassigned.
247-250: LGTM!The use of object property shorthand notation improves code readability.
Also applies to: 274-275
packages/sync-server/src/app-gocardless/services/gocardless-service.js (2)
1-5: LGTM!The import statements have been properly organized and grouped.
149-149: LGTM!The conversion from
lettoconstis appropriate as this variable is not reassigned.
This is just the lint autofixes to start bringing the sync server in line with the linting rules in the main repo
Linting is still disabled while the rest of the issues are looked at.
The CodeQL errors aren't introduced here, but the lines around them have been edited.
I didn't include a release note here, there's no meaningful change but I can if you'd like.