Release 0.15.4 #1472
Release 0.15.4 #1472
Conversation
Reading from config file was overwriting the environment variables. Fixed it by reading the values from already updated common.appConfig.
WalkthroughThe changes in this pull request include updates to the Docker workflow configuration, modifications to various Visual Studio Code settings, and enhancements to the authentication logic across multiple files. Key changes involve renaming properties for consistency, simplifying the retrieval of application settings by removing file read operations, and updating the Dockerfile to use a newer Node.js version. Additionally, the version number in the Changes
Poem
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: 4
🧹 Outside diff range and nitpick comments (14)
Dockerfile (1)
Line range hint
23-38: Consider adding a non-root user for better securityWhile the current changes are good, consider adding a non-root user in the runner stage for better security. This is a recommended practice for production containers.
Add these lines before the WORKDIR instruction in the runner stage:
FROM --platform=${TARGETPLATFORM} ${BASE_DISTRO} AS runner RUN apk add --no-cache tini +RUN addgroup -S rtl && adduser -S rtl -G rtl WORKDIR /RTL +RUN chown -R rtl:rtl /RTL COPY --from=builder /RTL/rtl.js ./rtl.js COPY --from=builder /RTL/package.json ./package.json COPY --from=builder /RTL/frontend ./frontend COPY --from=builder /RTL/backend ./backend COPY --from=builder /RTL/node_modules/ ./node_modules +USER rtl EXPOSE 3000server/controllers/shared/authenticate.ts (2)
Line range hint
55-73: Consider enhancing SSO authentication security.While the current implementation is functional, consider these security enhancements:
- Add rate limiting for SSO authentication attempts
- Implement cookie rotation mechanism
- Add request origin validation
Line range hint
106-115: Use timing-safe comparison for password verification.The current password comparison using direct equality (
===) could be vulnerable to timing attacks. Consider using a timing-safe comparison.Apply this diff:
- if (common.appConfig.rtlPass === currPassword) { + if (crypto.timingSafeEqual( + Buffer.from(common.appConfig.rtlPass), + Buffer.from(currPassword) + )) {backend/controllers/shared/authenticate.js (2)
Line range hint
51-57: Simplify JWT authentication logic with SSO enabledThe current implementation verifies the JWT token but then returns a 406 error. This is unnecessary computation since we know we'll reject the request when SSO is enabled.
Consider simplifying the logic:
if (+common.appConfig.SSO.rtlSSO) { - if (authenticateWith === 'JWT' && jwt.verify(authenticationValue, common.secret_key)) { + if (authenticateWith === 'JWT') { logger.log({ selectedNode: req.session.selectedNode, level: 'INFO', fileName: 'Authenticate', msg: 'User Authenticated' }); res.status(406).json({ message: 'SSO Authentication Error', error: 'Login with Password is not allowed with SSO.' }); }
Security concern: Password hashing implementation uses weak algorithm
The password hashing implementation uses SHA-256 (via
crypto.createHash('sha256')) which is not suitable for password hashing. The current implementation:
- Uses a fast cryptographic hash (SHA-256) instead of a dedicated password hashing algorithm
- Lacks salt for protecting against rainbow table attacks
- Missing work factor/iterations for protection against brute-force attacks
Key locations requiring updates:
backend/utils/config.js: Usescrypto.createHash('sha256')for password hashingbackend/utils/common.js:replacePasswordWithHashfunction stores plain hashed passwordsRecommendation: Replace with a proper password hashing algorithm like bcrypt, Argon2, or PBKDF2 with appropriate salt and work factors.
🔗 Analysis chain
Line range hint
106-120: Verify password hashing implementationThe password reset logic looks correct, but let's verify the security of the password hashing implementation.
Let me gather more information about the hashing implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the password hashing implementation in common.js # Expected: Should use a secure hashing algorithm (e.g., bcrypt, argon2) with proper salt # Search for password hashing implementation rg -A 5 "replacePasswordWithHash" # Check for secure hashing library imports rg -l "bcrypt|argon2|pbkdf2|scrypt"Length of output: 3626
Script:
#!/bin/bash # Check the hash implementation and crypto usage rg -A 5 "hash\." backend/utils/ rg -A 5 "crypto" backend/utils/ # Check the imports in common.js and config.js rg "^import.*crypto" backend/utils/ rg "^const.*crypto" backend/utils/Length of output: 5067
src/app/app.component.ts (2)
Line range hint
102-112: Enhance access key validation and error handlingWhile the validation logic is functional, consider these improvements:
- Extract the magic number
32into a named constant- Consolidate the duplicate error message into a constant
- Add validation for access key character set (e.g., alphanumeric)
+ const MIN_ACCESS_KEY_LENGTH = 32; + const ACCESS_KEY_ERROR_MESSAGE = 'Access key too short. It should be at least 32 characters long.'; if (!this.sessionService.getItem('token')) { if (+action.payload.SSO.rtlSSO) { - if (!this.accessKey || this.accessKey.trim().length < 32) { + if (!this.isValidAccessKey(this.accessKey)) { - this.router.navigate(['./error'], { state: { errorCode: '406', errorMessage: 'Access key too short. It should be at least 32 characters long.' } }); + this.router.navigate(['./error'], { state: { errorCode: '406', errorMessage: ACCESS_KEY_ERROR_MESSAGE } }); } else { this.store.dispatch(login({ payload: { password: sha256(this.accessKey).toString(), defaultPassword: false } })); } } else { - this.router.navigate(['./login'], { state: { logoutReason: 'Access key too short. It should be at least 32 characters long.' } }); + this.router.navigate(['./login'], { state: { logoutReason: ACCESS_KEY_ERROR_MESSAGE } }); } } + private isValidAccessKey(key: string): boolean { + if (!key || key.trim().length < MIN_ACCESS_KEY_LENGTH) return false; + return /^[a-zA-Z0-9]+$/.test(key); // Allow only alphanumeric characters + }
Line range hint
1-200: Consider architectural improvementsThe component has several responsibilities and could benefit from:
- Using a single destruction subject instead of an array of subjects
- Splitting the authentication logic into a separate service
- Creating a dedicated idle service to handle timeout logic
Example of using a single destruction subject:
private readonly destroy$ = new Subject<void>(); ngOnInit() { // Use it in all subscriptions someObservable$.pipe(takeUntil(this.destroy$)).subscribe(); } ngOnDestroy() { this.destroy$.next(); this.destroy$.complete(); }server/utils/config.ts (1)
Line range hint
350-370: Consider implementing a proper migration framework.The current approach of handling property renames with temporary code and comments like "remove in a year" is risky:
- No tracking of which configs have been migrated
- No versioning of config schema
- Manual cleanup reminder through comments
Consider implementing:
- A config version field
- A migration framework that tracks applied changes
- Automated tests for config migrations
Example schema version tracking:
interface ConfigSchema { version: string; nodes: Node[]; // ... other fields } const CURRENT_SCHEMA_VERSION = '0.15.4';🧰 Tools
🪛 Biome
[error] 302-302: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 303-303: rtlSSO is assigned to itself.
This is where is assigned.
(lint/correctness/noSelfAssign)
backend/utils/config.js (1)
Line range hint
364-380: Improve deprecation notice and simplify JSON operations.The code can be improved in two ways:
- The comment about removal timing should specify an exact date.
- The JSON operations can be simplified by using object spread.
Apply these improvements:
- // Added in v0.15.1, remove in a year? + // Added in v0.15.1, remove after 2025-11-15 if (!config.nodes) { return; } config.nodes.map((node) => { if (node.Authentication) { - node.authentication = JSON.parse(JSON.stringify(node.Authentication)); + node.authentication = { ...node.Authentication }; delete node.Authentication; } if (node.Settings) { - node.settings = JSON.parse(JSON.stringify(node.Settings)); + node.settings = { ...node.Settings }; delete node.Settings; } return node; });🧰 Tools
🪛 Biome
[error] 323-323: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 324-324: rtlSSO is assigned to itself.
This is where is assigned.
(lint/correctness/noSelfAssign)
server/utils/common.ts (3)
14-15: Consider splitting the large initialization for better readability.While the SSO property renaming is good, the
appConfiginitialization could be more maintainable if split into smaller, logical groups.Consider restructuring like this:
- public appConfig: ApplicationConfig = { defaultNodeIndex: 0, selectedNodeIndex: 0, rtlConfFilePath: '', dbDirectoryPath: join(dirname(fileURLToPath(import.meta.url)), '..', '..'), rtlPass: '', allowPasswordUpdate: true, enable2FA: false, secret2FA: '', SSO: this.ssoInit, nodes: [] }; + public appConfig: ApplicationConfig = { + // Node configuration + defaultNodeIndex: 0, + selectedNodeIndex: 0, + nodes: [], + + // Path configuration + rtlConfFilePath: '', + dbDirectoryPath: join(dirname(fileURLToPath(import.meta.url)), '..', '..'), + + // Security configuration + rtlPass: '', + allowPasswordUpdate: true, + enable2FA: false, + secret2FA: '', + SSO: this.ssoInit + };
Line range hint
200-300: Consider refactoring error handling for better maintainability.The error handling logic in
handleErrormethod could be more maintainable by extracting the header cleanup logic into a separate method.Consider this approach:
+ private cleanupHeaders(err: any, headers: string[]) { + headers.forEach(header => { + if (err.options?.headers?.[header]) { + delete err.options.headers[header]; + } + if (err.response?.request?.headers?.[header]) { + delete err.response.request.headers[header]; + } + }); + } public handleError = (errRes, fileName, errMsg, selectedNode: SelectedNode) => { const err = JSON.parse(JSON.stringify(errRes)); if (!selectedNode) { selectedNode = this.selectedNode; } switch (selectedNode.lnImplementation) { case 'LND': - if (err.options && err.options.headers && err.options.headers['Grpc-Metadata-macaroon']) { - delete err.options.headers['Grpc-Metadata-macaroon']; - } - if (err.response && err.response.request && err.response.request.headers && err.response.request.headers['Grpc-Metadata-macaroon']) { - delete err.response.request.headers['Grpc-Metadata-macaroon']; - } + this.cleanupHeaders(err, ['Grpc-Metadata-macaroon']); break; case 'CLN': - if (err.options && err.options.headers && err.options.headers.rune) { - delete err.options.headers.rune; - } - if (err.response && err.response.request && err.response.request.headers && err.response.request.headers.rune) { - delete err.response.request.headers.rune; - } + this.cleanupHeaders(err, ['rune']); break; case 'ECL': - if (err.options && err.options.headers && err.options.headers.authorization) { - delete err.options.headers.authorization; - } - if (err.response && err.response.request && err.response.request.headers && err.response.request.headers.authorization) { - delete err.response.request.headers.authorization; - } + this.cleanupHeaders(err, ['authorization']); break;
545-545: Enhance logging message for better clarity.While the property name update is good, the logging message could be more descriptive.
Consider this improvement:
- this.logger.log({ selectedNode: selNode, level: 'INFO', fileName: 'Config Setup Variable', msg: 'SSO: ' + this.appConfig.SSO.rtlSSO }); + this.logger.log({ selectedNode: selNode, level: 'INFO', fileName: 'Config Setup Variable', msg: 'SSO Configuration Status: ' + (this.appConfig.SSO.rtlSSO ? 'Enabled' : 'Disabled') });backend/controllers/shared/RTLConf.js (2)
102-102: Simplify property access using optional chainingConsider using optional chaining to simplify the expression when accessing
req.session.selectedNode.index.Apply this diff to simplify the code:
-appConfData.selectedNodeIndex = (req.session.selectedNode && req.session.selectedNode.index ? req.session.selectedNode.index : common.selectedNode.index); +appConfData.selectedNodeIndex = req.session.selectedNode?.index || common.selectedNode.index;🧰 Tools
🪛 Biome
[error] 102-102: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
109-121: Avoid using thedeleteoperator for better performanceUsing the
deleteoperator can negatively impact JavaScript engine optimizations. Instead of deleting properties, assignundefinedornullto the properties to maintain object shape and improve performance.Apply this diff to set properties to
undefined:-delete appConfData.SSO.rtlCookiePath; -delete appConfData.SSO.cookieValue; -delete appConfData.SSO.logoutRedirectLink; -appConfData.secret2FA = ''; -appConfData.dbDirectoryPath = ''; -appConfData.nodes[selNodeIdx].authentication = new Authentication(); -delete appConfData.nodes[selNodeIdx].settings.bitcoindConfigPath; -delete appConfData.nodes[selNodeIdx].settings.lnServerUrl; -delete appConfData.nodes[selNodeIdx].settings.swapServerUrl; -delete appConfData.nodes[selNodeIdx].settings.boltzServerUrl; -delete appConfData.nodes[selNodeIdx].settings.enableOffers; -delete appConfData.nodes[selNodeIdx].settings.enablePeerswap; -delete appConfData.nodes[selNodeIdx].settings.channelBackupPath; +appConfData.SSO.rtlCookiePath = undefined; +appConfData.SSO.cookieValue = undefined; +appConfData.SSO.logoutRedirectLink = undefined; +appConfData.secret2FA = ''; +appConfData.dbDirectoryPath = ''; +appConfData.nodes[selNodeIdx].authentication = new Authentication(); +appConfData.nodes[selNodeIdx].settings.bitcoindConfigPath = undefined; +appConfData.nodes[selNodeIdx].settings.lnServerUrl = undefined; +appConfData.nodes[selNodeIdx].settings.swapServerUrl = undefined; +appConfData.nodes[selNodeIdx].settings.boltzServerUrl = undefined; +appConfData.nodes[selNodeIdx].settings.enableOffers = undefined; +appConfData.nodes[selNodeIdx].settings.enablePeerswap = undefined; +appConfData.nodes[selNodeIdx].settings.channelBackupPath = undefined;🧰 Tools
🪛 Biome
[error] 109-109: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 110-110: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 111-111: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 115-115: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 116-116: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 117-117: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 118-118: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 119-119: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 120-120: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 121-121: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
.github/workflows/docker-release.yml(2 hunks).gitignore(0 hunks).vscode/launch.json(0 hunks).vscode/settings.json(0 hunks)Dockerfile(2 hunks)backend/controllers/shared/RTLConf.js(2 hunks)backend/controllers/shared/authenticate.js(2 hunks)backend/models/config.model.js(1 hunks)backend/utils/common.js(2 hunks)backend/utils/config.js(2 hunks)frontend/index.html(1 hunks)package.json(1 hunks)server/controllers/shared/RTLConf.ts(2 hunks)server/controllers/shared/authenticate.ts(2 hunks)server/models/config.model.ts(1 hunks)server/utils/common.ts(2 hunks)server/utils/config.ts(2 hunks)src/app/app.component.ts(1 hunks)src/app/shared/services/consts-enums-functions.ts(1 hunks)
💤 Files with no reviewable changes (3)
- .gitignore
- .vscode/launch.json
- .vscode/settings.json
✅ Files skipped from review due to trivial changes (2)
- package.json
- src/app/shared/services/consts-enums-functions.ts
🧰 Additional context used
🪛 Biome
backend/controllers/shared/RTLConf.js
[error] 102-102: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 109-109: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 110-110: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 111-111: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 115-115: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 116-116: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 117-117: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 118-118: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 119-119: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 120-120: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 121-121: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
backend/utils/config.js
[error] 323-323: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 324-324: rtlSSO is assigned to itself.
This is where is assigned.
(lint/correctness/noSelfAssign)
server/controllers/shared/RTLConf.ts
[error] 105-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 112-112: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 113-113: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 114-114: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 118-118: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 119-119: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 120-120: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 121-121: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 122-122: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 123-123: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 124-124: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
server/utils/config.ts
[error] 302-302: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 303-303: rtlSSO is assigned to itself.
This is where is assigned.
(lint/correctness/noSelfAssign)
🔇 Additional comments (21)
Dockerfile (2)
3-3: LGTM! Improved build stage naming
Good improvement in the multi-stage build syntax by explicitly using the AS keyword. This makes the Dockerfile more readable and follows best practices.
Also applies to: 23-23
1-1: Verify compatibility with Node.js 20
The upgrade to node:20-alpine is a significant version change. While using an LTS version is good practice, please ensure:
- All dependencies are compatible with Node.js 20
- The application has been tested with this version
Let's check the package.json for any potential compatibility issues:
✅ Verification successful
Node.js 20 upgrade appears safe to proceed
The analysis shows favorable conditions for the Node.js 20 upgrade:
- No explicit Node.js version constraints in package.json or dependencies
- All dependencies are using recent versions compatible with Node.js 20
- Recent commit f02f0c0 already addressed Node.js v20 upgrade
- Build scripts don't show version-specific requirements
The upgrade to node:20-alpine aligns with the project's recent Node.js version update and appears to be a deliberate, planned change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check package.json for Node.js engine requirements and dependencies
# Check for engine requirements
echo "Checking Node.js engine requirements..."
if [ -f package.json ]; then
jq '.engines' package.json
fi
# List direct dependencies that might have Node.js version constraints
echo "Checking dependencies..."
if [ -f package.json ]; then
jq '.dependencies' package.json
fi
Length of output: 1026
Script:
#!/bin/bash
# Let's check the package-lock.json for any potential Node.js engine requirements in dependencies
echo "Checking package-lock.json for Node.js engine requirements..."
if [ -f package-lock.json ]; then
# Search for "engines" and "node" requirements in dependencies
rg -A 2 '"engines":\s*{[^}]*"node"' package-lock.json
fi
# Let's also check for any build or test scripts that might indicate Node.js version requirements
echo "Checking npm scripts..."
if [ -f package.json ]; then
jq '.scripts' package.json
fi
# Check for any Node.js version related comments in recent commits
echo "Checking recent commits for Node.js version mentions..."
git log --grep="node" --grep="Node" -i --since="6 months ago" --pretty=format:"%h %s"
Length of output: 1514
.github/workflows/docker-release.yml (2)
44-47: LGTM! Enhanced version resolution debugging.
The added echo statements improve observability by logging all variables involved in version determination. This will help diagnose version-related issues during workflow execution.
14-14: LGTM! Pinning to a specific Ubuntu version.
Explicitly specifying Ubuntu 22.04 instead of using ubuntu-latest provides better stability and predictability for the CI pipeline.
server/models/config.model.ts (1)
4-4: LGTM! Property renamed for consistency.
The renaming from rtlSso to rtlSSO aligns with the standardization of SSO-related property naming across the codebase.
Let's verify that this property has been consistently updated across the codebase:
backend/models/config.model.js (1)
2-3: LGTM! Property rename improves naming consistency.
The rename from rtlSso to rtlSSO aligns with JavaScript naming conventions where acronyms in camelCase should be uppercase.
Let's verify that all references to this property have been updated consistently:
server/controllers/shared/authenticate.ts (2)
55-55: Verify SSO property name consistency across the codebase.
The property name change from rtlSso to rtlSSO needs to be consistent across all files.
103-103: Verify SSO property name consistency in password reset flow.
The property name change from rtlSso to rtlSSO in the password reset logic should be consistent with other SSO-related changes.
backend/controllers/shared/authenticate.js (1)
Line range hint 58-71: LGTM! Secure implementation of password verification
The PASSWORD authentication path implements several security best practices:
- Uses timing-safe comparison to prevent timing attacks
- Ensures minimum cookie length for security
- Properly refreshes cookies and generates new tokens
src/app/app.component.ts (2)
101-101: LGTM: Action type update follows Redux patterns
The change from FETCH_APPLICATION_SETTINGS to SET_APPLICATION_SETTINGS aligns with common Redux patterns where fetch actions are followed by set actions for state updates.
Line range hint 113-133: Improve login/logout robustness
Consider these enhancements to the login/logout handling:
- The
setTimeoutfor container size update is fragile. Consider using Angular'sViewChildwithstatic: trueorngAfterViewInit. - Extract the idle timer countdown value (11) into a configuration constant.
- Add error handling for failed login attempts.
Let's verify if there are any error handlers for failed login attempts:
server/controllers/shared/RTLConf.ts (3)
11-11: LGTM: Import changes align with refactoring
The import changes correctly reflect the refactoring effort, replacing the SSO import with Authentication model.
102-106: LGTM: Improved configuration handling
The refactoring to use common.appConfig directly instead of reading from file is a good improvement that reduces I/O operations and simplifies the code.
🧰 Tools
🪛 Biome
[error] 105-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
107-128: Verify token verification impact
The token verification logic handles both authenticated and unauthenticated scenarios. However, we should verify that this change doesn't affect other parts of the application that might expect these properties to be completely removed rather than set to undefined.
🧰 Tools
🪛 Biome
[error] 112-112: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 113-113: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 114-114: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 118-118: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 119-119: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 120-120: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 121-121: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 122-122: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 123-123: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 124-124: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
server/utils/config.ts (2)
Line range hint 1-391: Verify secure handling of sensitive environment variables.
The code reads several sensitive configuration values from environment variables. Ensure:
- Environment variables are properly sanitized
- Sensitive values are not logged
- Environment variables are cleared after use
🧰 Tools
🪛 Biome
[error] 302-302: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 303-303: rtlSSO is assigned to itself.
This is where is assigned.
(lint/correctness/noSelfAssign)
Line range hint 320-326: Enhance cookie path validation for security.
The current validation only checks if the cookie path is non-empty. Consider adding additional security checks:
- Validate if the path is absolute
- Ensure the path has proper permissions
- Verify the path is within allowed directories
🧰 Tools
🪛 Biome
[error] 317-317: logoutRedirectLink is assigned to itself.
This is where is assigned.
(lint/correctness/noSelfAssign)
backend/utils/common.js (2)
12-12: LGTM! Property rename for consistency.
The property rename from rtlSso to rtlSSO maintains consistency with the codebase's naming conventions.
531-531: LGTM! Updated logging statement.
The logging statement has been correctly updated to use the renamed property rtlSSO.
frontend/index.html (1)
18-18: LGTM! Build artifact update.
The update to the JavaScript bundle files is consistent with the version update for release 0.15.4.
Let's verify if this change is part of the version update:
✅ Verification successful
Build artifacts update aligns with version 0.15.4-beta release
The JavaScript bundle changes in frontend/index.html are consistent with the version update from 0.15.3-beta to 0.15.4-beta as confirmed in package.json. These are expected build artifacts generated during the release process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the version update in package.json and related files
# Check package.json for version
echo "Checking package.json version:"
rg -A 1 '"version":\s*"0.15.4' package.json
# Check version constant in TypeScript
echo "Checking version constant:"
rg -A 1 'VERSION\s*=\s*"0.15.4' src/app/shared/services/consts-enums-functions.ts
Length of output: 311
Script:
#!/bin/bash
# Let's check the version changes in the PR
# First, check the current version in package.json
echo "Current version in package.json:"
cat package.json | grep -A1 '"version":'
# Check git history for version changes
echo -e "\nVersion changes in git history:"
git log -p -S'"version":' --pretty=format:'%h %s' -- package.json | head -n 10
# Check for any version constants in the codebase
echo -e "\nSearching for version constants:"
rg -i 'version\s*=\s*["\'][\d\.]+' --type ts
Length of output: 718
backend/controllers/shared/RTLConf.js (2)
11-11: Import statement is correctly added
The Authentication model is imported properly from config.model.js.
105-125: Ensure proper handling of JWT verification
When verifying the JWT token, if there's an error, the code sanitizes appConfData and proceeds to send the response. Consider whether it's appropriate to return limited application settings without authentication. This might expose sensitive information inadvertently.
Run the following script to check where getApplicationSettings is called and how the response is used:
✅ Verification successful
Let me gather more context about the authentication flow and how the sensitive data is handled.
Let me check how this endpoint is used in the frontend to understand the authentication flow better.
Based on the gathered information, I can now provide a final response about the JWT verification concern:
Current JWT verification behavior is appropriate for the application flow
The current implementation is secure because:
- The endpoint
getApplicationSettingsis specifically designed to handle both authenticated and unauthenticated states - When JWT verification fails, the code properly sanitizes sensitive data by:
- Removing all authentication-related paths and credentials
- Stripping configuration paths and server URLs
- Limiting node information to only the selected node
- Clearing security tokens and 2FA secrets
- Other sensitive endpoints (like
/file,/config/:nodeType,/node) are properly protected with theisAuthenticatedmiddleware
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of getApplicationSettings to assess how the response is utilized.
# Search for all invocations of getApplicationSettings
rg 'getApplicationSettings'
# Review the call sites to ensure that returning sanitized data without authentication doesn't introduce security risks.
Length of output: 807
Script:
#!/bin/bash
# Check the authentication flow and middleware
rg -A 5 "router\.get\('/', getApplicationSettings\)"
# Look for authentication middleware
rg -B 5 "router\.get\('/', getApplicationSettings\)"
# Check how the Authentication class is used
ast-grep --pattern 'class Authentication {
$$$
}'
# Check for other JWT verification patterns
rg -A 3 'jwt.verify'
Length of output: 7717
Script:
#!/bin/bash
# Check frontend usage of application settings
rg -A 5 "getApplicationSettings" src/
# Check the authentication service implementation
rg -A 10 "class AuthenticationService" src/
# Look for initial application bootstrap/loading
rg -A 5 "loadApplicationSettings" src/
Length of output: 122
🧰 Tools
🪛 Biome
[error] 109-109: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 110-110: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 111-111: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 115-115: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 116-116: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 117-117: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 118-118: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 119-119: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 120-120: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 121-121: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
| jwt.verify(token, common.secret_key, (err, user) => { | ||
| if (err) { | ||
| // Delete unnecessary data for initial response (without security token) | ||
| const selNodeIdx = appConfData.nodes.findIndex((node) => node.index === appConfData.selectedNodeIndex) || 0; |
There was a problem hiding this comment.
Fix potential null reference with optional chaining
The findIndex operation could return -1, making the || 0 fallback potentially incorrect.
Apply this fix:
- const selNodeIdx = appConfData.nodes.findIndex((node) => node.index === appConfData.selectedNodeIndex) || 0;
+ const selNodeIdx = appConfData.nodes.findIndex((node) => node?.index === appConfData.selectedNodeIndex) ?? 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const selNodeIdx = appConfData.nodes.findIndex((node) => node.index === appConfData.selectedNodeIndex) || 0; | |
| const selNodeIdx = appConfData.nodes.findIndex((node) => node?.index === appConfData.selectedNodeIndex) ?? 0; |
| delete appConfData.SSO.rtlCookiePath; | ||
| delete appConfData.SSO.cookieValue; | ||
| delete appConfData.SSO.logoutRedirectLink; | ||
| appConfData.secret2FA = ''; | ||
| appConfData.dbDirectoryPath = ''; | ||
| appConfData.nodes[selNodeIdx].authentication = new Authentication(); | ||
| delete appConfData.nodes[selNodeIdx].settings.bitcoindConfigPath; | ||
| delete appConfData.nodes[selNodeIdx].settings.lnServerUrl; | ||
| delete appConfData.nodes[selNodeIdx].settings.swapServerUrl; | ||
| delete appConfData.nodes[selNodeIdx].settings.boltzServerUrl; | ||
| delete appConfData.nodes[selNodeIdx].settings.enableOffers; | ||
| delete appConfData.nodes[selNodeIdx].settings.enablePeerswap; | ||
| delete appConfData.nodes[selNodeIdx].settings.channelBackupPath; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve performance by avoiding delete operators
Multiple delete operators can impact performance. Consider using undefined assignments instead.
Apply this refactoring:
- delete appConfData.SSO.rtlCookiePath;
- delete appConfData.SSO.cookieValue;
- delete appConfData.SSO.logoutRedirectLink;
+ appConfData.SSO.rtlCookiePath = undefined;
+ appConfData.SSO.cookieValue = undefined;
+ appConfData.SSO.logoutRedirectLink = undefined;
- delete appConfData.nodes[selNodeIdx].settings.bitcoindConfigPath;
- delete appConfData.nodes[selNodeIdx].settings.lnServerUrl;
- delete appConfData.nodes[selNodeIdx].settings.swapServerUrl;
- delete appConfData.nodes[selNodeIdx].settings.boltzServerUrl;
- delete appConfData.nodes[selNodeIdx].settings.enableOffers;
- delete appConfData.nodes[selNodeIdx].settings.enablePeerswap;
- delete appConfData.nodes[selNodeIdx].settings.channelBackupPath;
+ appConfData.nodes[selNodeIdx].settings.bitcoindConfigPath = undefined;
+ appConfData.nodes[selNodeIdx].settings.lnServerUrl = undefined;
+ appConfData.nodes[selNodeIdx].settings.swapServerUrl = undefined;
+ appConfData.nodes[selNodeIdx].settings.boltzServerUrl = undefined;
+ appConfData.nodes[selNodeIdx].settings.enableOffers = undefined;
+ appConfData.nodes[selNodeIdx].settings.enablePeerswap = undefined;
+ appConfData.nodes[selNodeIdx].settings.channelBackupPath = undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| delete appConfData.SSO.rtlCookiePath; | |
| delete appConfData.SSO.cookieValue; | |
| delete appConfData.SSO.logoutRedirectLink; | |
| appConfData.secret2FA = ''; | |
| appConfData.dbDirectoryPath = ''; | |
| appConfData.nodes[selNodeIdx].authentication = new Authentication(); | |
| delete appConfData.nodes[selNodeIdx].settings.bitcoindConfigPath; | |
| delete appConfData.nodes[selNodeIdx].settings.lnServerUrl; | |
| delete appConfData.nodes[selNodeIdx].settings.swapServerUrl; | |
| delete appConfData.nodes[selNodeIdx].settings.boltzServerUrl; | |
| delete appConfData.nodes[selNodeIdx].settings.enableOffers; | |
| delete appConfData.nodes[selNodeIdx].settings.enablePeerswap; | |
| delete appConfData.nodes[selNodeIdx].settings.channelBackupPath; | |
| appConfData.SSO.rtlCookiePath = undefined; | |
| appConfData.SSO.cookieValue = undefined; | |
| appConfData.SSO.logoutRedirectLink = undefined; | |
| appConfData.secret2FA = ''; | |
| appConfData.dbDirectoryPath = ''; | |
| appConfData.nodes[selNodeIdx].authentication = new Authentication(); | |
| appConfData.nodes[selNodeIdx].settings.bitcoindConfigPath = undefined; | |
| appConfData.nodes[selNodeIdx].settings.lnServerUrl = undefined; | |
| appConfData.nodes[selNodeIdx].settings.swapServerUrl = undefined; | |
| appConfData.nodes[selNodeIdx].settings.boltzServerUrl = undefined; | |
| appConfData.nodes[selNodeIdx].settings.enableOffers = undefined; | |
| appConfData.nodes[selNodeIdx].settings.enablePeerswap = undefined; | |
| appConfData.nodes[selNodeIdx].settings.channelBackupPath = undefined; |
🧰 Tools
🪛 Biome
[error] 112-112: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 113-113: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 114-114: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 118-118: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 119-119: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 120-120: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 121-121: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 122-122: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 123-123: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 124-124: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
| config.SSO.rtlSSO = +process?.env?.RTL_SSO; | ||
| } else if (config.SSO && config.SSO.rtlSSO) { | ||
| config.SSO.rtlSso = config.SSO.rtlSSO; | ||
| config.SSO.rtlSSO = config.SSO.rtlSSO; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the SSO configuration assignment.
The current code has redundant assignments and can be simplified.
- if (process?.env?.RTL_SSO) {
- config.SSO.rtlSSO = +process?.env?.RTL_SSO;
- } else if (config.SSO && config.SSO.rtlSSO) {
- config.SSO.rtlSSO = config.SSO.rtlSSO;
- }
+ config.SSO.rtlSSO = process?.env?.RTL_SSO ? +process?.env?.RTL_SSO : config.SSO?.rtlSSO ?? 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| config.SSO.rtlSSO = +process?.env?.RTL_SSO; | |
| } else if (config.SSO && config.SSO.rtlSSO) { | |
| config.SSO.rtlSso = config.SSO.rtlSSO; | |
| config.SSO.rtlSSO = config.SSO.rtlSSO; | |
| config.SSO.rtlSSO = process?.env?.RTL_SSO ? +process?.env?.RTL_SSO : config.SSO?.rtlSSO ?? 0; |
🧰 Tools
🪛 Biome
[error] 302-302: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 303-303: rtlSSO is assigned to itself.
This is where is assigned.
(lint/correctness/noSelfAssign)
| config.SSO.rtlSSO = +process?.env?.RTL_SSO; | ||
| } | ||
| else if (config.SSO && config.SSO.rtlSSO) { | ||
| config.SSO.rtlSso = config.SSO.rtlSSO; | ||
| config.SSO.rtlSSO = config.SSO.rtlSSO; |
There was a problem hiding this comment.
Fix self-assignment and improve optional chaining.
There are two issues in this code segment:
- The self-assignment
config.SSO.rtlSSO = config.SSO.rtlSSOis redundant and can be removed. - The environment variable access can be improved with optional chaining.
Apply this diff to fix the issues:
- config.SSO.rtlSSO = +process?.env?.RTL_SSO;
+ config.SSO.rtlSSO = +process?.env?.RTL_SSO ?? 0;
- else if (config.SSO && config.SSO.rtlSSO) {
- config.SSO.rtlSSO = config.SSO.rtlSSO;
+ else if (config.SSO?.rtlSSO) {
+ // Value already set in config.SSO.rtlSSO📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| config.SSO.rtlSSO = +process?.env?.RTL_SSO; | |
| } | |
| else if (config.SSO && config.SSO.rtlSSO) { | |
| config.SSO.rtlSso = config.SSO.rtlSSO; | |
| config.SSO.rtlSSO = config.SSO.rtlSSO; | |
| config.SSO.rtlSSO = +process?.env?.RTL_SSO ?? 0; | |
| } | |
| else if (config.SSO?.rtlSSO) { | |
| // Value already set in config.SSO.rtlSSO |
🧰 Tools
🪛 Biome
[error] 323-323: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 324-324: rtlSSO is assigned to itself.
This is where is assigned.
(lint/correctness/noSelfAssign)
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
0.15.3-betato0.15.4-betain multiple files..gitignorelist for better tracking.