feat(connect): trust localhost subdomains #16721
Conversation
martykan
requested review from
mroz22,
szymonlesisz and
marekrjpolak
as code owners
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
error Error: http://10.0.0.28:4873/@trezor%2feslint: no such package available WalkthroughThe pull request introduces modifications to the Assessment against linked issues
The changes directly address the requirements outlined in issue #16671 by modifying the ✨ Finishing Touches
🪧 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
🧹 Nitpick comments (2)
packages/connect/src/utils/urlUtils.ts (1)
19-20: Consider using a more robust localhost check.While the current implementation works for basic cases, it might be vulnerable to edge cases. Consider using a more comprehensive check that accounts for various localhost formats.
Here's a suggested improvement:
- if (parts[parts.length - 1] === 'localhost') return 'localhost'; + // Check if any part of the domain is localhost + if (parts.some(part => part.toLowerCase() === 'localhost')) return 'localhost';This change would:
- Handle case sensitivity (e.g., 'LocalHost')
- Work even if 'localhost' appears in different positions
packages/connect/src/utils/__tests__/urlUtils.test.ts (1)
19-19: Enhance test coverage for localhost subdomain handling.While the basic case is covered, consider adding more test cases to ensure robust handling of localhost subdomains.
Add these test cases:
expect(getHost('http://subdomain.localhost:8088/')).toEqual('localhost'); + // Multiple subdomains + expect(getHost('http://sub1.sub2.localhost')).toEqual('localhost'); + // Case sensitivity + expect(getHost('http://sub.LOCALHOST')).toEqual('localhost'); + // Different protocols + expect(getHost('https://sub.localhost')).toEqual('localhost'); + expect(getHost('ws://sub.localhost')).toEqual('localhost');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/connect/src/utils/__tests__/urlUtils.test.ts(1 hunks)packages/connect/src/utils/urlUtils.ts(1 hunks)
🔇 Additional comments (1)
packages/connect/src/utils/urlUtils.ts (1)
19-21: Verify security implications of trusting localhost subdomains.The current implementation treats all subdomains of localhost as trusted. While this aligns with the PR objectives, it's important to ensure this doesn't introduce security vulnerabilities.
Let's check for any security considerations in the codebase:
✅ Verification successful
Localhost subdomain handling follows secure practices
The implementation aligns with standard web security practices where localhost is treated as a secure origin. The code is primarily used in development/testing contexts, and the security implications of localhost access are well understood and appropriately handled in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for security-related comments or validations around localhost rg -i "localhost.*security|security.*localhost" # Check how localhost is used in other parts of the codebase rg -i "localhost" --type tsLength of output: 16830
Description
Treat subdomains of localhost (eg. https://subdomain.localhost/) the same way we treat localhost itself.
Related Issue
Resolve #16671