Fix computeEndpoint method #97
Conversation
WalkthroughThe changes in this pull request enhance the functionality and test coverage of the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 and nitpick comments (4)
src/client/rest/APIRequester.spec.ts (1)
58-71: Enhance test coverage for URL handlingConsider adding more test cases to verify:
- Multiple path segments in baseURL (e.g.,
/api/v1/base)- Multiple path segments in endpoint
- Trailing slashes in baseURL
- Query parameters in both baseURL and endpoint
Would you like me to help generate these additional test cases?
src/client/rest/APIRequester.ts (3)
55-63: LGTM! Consider adding error handling for invalid URLs.The implementation correctly fixes the URL formation issue using the URL API. The approach of normalizing the endpoint and ensuring a trailing slash in the base path is robust.
Consider adding error handling for invalid base URLs:
private computeEndpoint(endpoint: string) { const relativeEndpoint = endpoint.replace(/^\/+/, ''); - const baseURLObject = new URL(this.baseURL); + try { + const baseURLObject = new URL(this.baseURL); - if (!baseURLObject.pathname.endsWith('/')) { - baseURLObject.pathname += '/'; - } - baseURLObject.pathname += relativeEndpoint; + if (!baseURLObject.pathname.endsWith('/')) { + baseURLObject.pathname += '/'; + } + baseURLObject.pathname += relativeEndpoint; - return baseURLObject.toString(); + return baseURLObject.toString(); + } catch (error) { + throw new Error(`Invalid base URL: ${this.baseURL}`); + } }🧰 Tools
🪛 eslint (1.23.1)
[error] 55-55: Delete
;(prettier/prettier)
[error] 56-56: Delete
;(prettier/prettier)
[error] 59-59: Delete
;(prettier/prettier)
[error] 61-61: Delete
;(prettier/prettier)
[error] 63-63: Delete
;(prettier/prettier)
55-63: Consider adding test cases for URL edge cases.To ensure robust URL handling, consider adding test cases for:
- Base URLs with query parameters
- Base URLs with hash fragments
- Endpoints with encoded characters
- Invalid base URLs
Would you like me to help generate these test cases?
🧰 Tools
🪛 eslint (1.23.1)
[error] 55-55: Delete
;(prettier/prettier)
[error] 56-56: Delete
;(prettier/prettier)
[error] 59-59: Delete
;(prettier/prettier)
[error] 61-61: Delete
;(prettier/prettier)
[error] 63-63: Delete
;(prettier/prettier)
55-63: Address formatting inconsistencies.The static analysis tool indicates semicolon-related formatting issues. These should be automatically fixed by running the project's formatter.
Run your formatter to automatically fix these issues:
- const relativeEndpoint = endpoint.replace(/^\/+/, ''); - const baseURLObject = new URL(this.baseURL); + const relativeEndpoint = endpoint.replace(/^\/+/, '') + const baseURLObject = new URL(this.baseURL) - if (!baseURLObject.pathname.endsWith('/')) { - baseURLObject.pathname += '/'; + if (!baseURLObject.pathname.endsWith('/')) { + baseURLObject.pathname += '/' } - baseURLObject.pathname += relativeEndpoint; + baseURLObject.pathname += relativeEndpoint - return baseURLObject.toString(); + return baseURLObject.toString()🧰 Tools
🪛 eslint (1.23.1)
[error] 55-55: Delete
;(prettier/prettier)
[error] 56-56: Delete
;(prettier/prettier)
[error] 59-59: Delete
;(prettier/prettier)
[error] 61-61: Delete
;(prettier/prettier)
[error] 63-63: Delete
;(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/client/rest/APIRequester.spec.ts(1 hunks)src/client/rest/APIRequester.ts(1 hunks)
🧰 Additional context used
🪛 eslint (1.23.1)
src/client/rest/APIRequester.ts
[error] 55-55: Delete ;
(prettier/prettier)
[error] 56-56: Delete ;
(prettier/prettier)
[error] 59-59: Delete ;
(prettier/prettier)
[error] 61-61: Delete ;
(prettier/prettier)
[error] 63-63: Delete ;
(prettier/prettier)
🔇 Additional comments (1)
src/client/rest/APIRequester.ts (1)
55-63: Verify security implications of URL manipulation.
Let's verify that the path traversal protection in validateEndpoint is comprehensive.
✅ Verification successful
URL validation is comprehensive and secure
The implementation provides robust protection against path traversal attacks through multiple layers:
-
Explicit validation in
validateEndpoint:- Blocks directory traversal attempts (
../,..\\) - Blocks URL-encoded traversal attempts (
%2E%2E%2F,%2E%2E%5C) - Prevents query string injection
- Blocks directory traversal attempts (
-
Safe URL construction in
computeEndpoint:- Strips leading slashes to prevent path manipulation
- Uses URL constructor for proper URL parsing and encoding
- Ensures consistent path separator handling
The combination of explicit blocklist validation and proper URL construction using the built-in URL API provides adequate protection against path traversal attacks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential security issues in URL handling
# Test 1: Look for other URL manipulation methods that might need similar protection
rg -t typescript -t javascript "new URL\(" -A 5
# Test 2: Check for other path traversal protections in the codebase
rg -t typescript -t javascript "\.\./" -A 5
Length of output: 165
Script:
#!/bin/bash
# Let's try without file type filters since typescript is not recognized
# Check for URL manipulations and path traversal patterns
# Look for URL constructor usage
rg "new URL\(" -A 5
# Check for path traversal protections
rg "\.\./" -A 5
# Let's also check for validateEndpoint implementation
rg "validateEndpoint" -A 10
Length of output: 100261
Script:
#!/bin/bash
# Let's search for validateEndpoint implementation and its usage
ast-grep --pattern 'validateEndpoint($$$) {
$$$
}'
# Also search for any URL validation related code
rg -l "validateEndpoint|URL|endpoint" src/client/rest/APIRequester.ts
Length of output: 158
Script:
#!/bin/bash
# Let's examine the full content of APIRequester.ts to understand the URL validation
cat src/client/rest/APIRequester.ts
Length of output: 2665
🧰 Tools
🪛 eslint (1.23.1)
[error] 55-55: Delete ;
(prettier/prettier)
[error] 56-56: Delete ;
(prettier/prettier)
[error] 59-59: Delete ;
(prettier/prettier)
[error] 61-61: Delete ;
(prettier/prettier)
[error] 63-63: Delete ;
(prettier/prettier)
Summary
This pull request fixes an issue in the
computeEndpointmethod where ifbaseURLcontains a path andendpointdoes not start with a leading slash, it was forming URLs incorrectly.Changes
baseURLhas a path (e.g.,/api) andendpointlacks a leading slash (e.g.,users), the resulting URL correctly includes a slash between them.Test Results
Before Fix:
https://example.com/apiand endpointusersresulted inhttps://example.com/apiusers.After Fix:
https://example.com/api/users.Explanation
baseURLandendpointwere being combined, if neither had appropriate slashes and the baseURL had a path, the concatenation would not contain a slash, making incorrect URLs.computeEndpointmethod will add a forward slash if necessary.Tests
Summary by CodeRabbit
Bug Fixes
Tests