Refactor Boilerplate project template url escaping (#9869) #9874
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request updates URL handling and query string processing across client and server components. Changes include decoding URLs before assignment and logging, ensuring proper encoding for navigation links, and simplifying URL concatenation logic. Several files now use a new query string parsing class in place of HttpUtility methods. Additionally, default navigation fallbacks have been revised, and a new extension method is added for passing query parameters to controllers. Changes
Sequence Diagram(s)sequenceDiagram
participant NM as NavigationManager
participant CAC as ClientAppCoordinator
participant LDH as LoggingDelegatingHandler
participant QSC as QueryStringCollection
NM->>CAC: Navigate with raw URL
CAC->>CAC: Decode URL using HttpUtility.UrlDecode
CAC->>LDH: Log decoded URL update
NM->>LDH: HTTP request sent with URL
LDH->>QSC: Parse query string via QueryStringCollection.Parse
QSC-->>LDH: Return parsed query data
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 (
|
ysmoradi
changed the title
![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 (3)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/NotAuthorizedPage.razor.cs (1)
61-61: Consider extracting duplicated URL construction logic.While the URL encoding implementation is correct, this logic is duplicated from
NotAuthorizedPage.SignOut. Consider extracting it to a shared helper method to improve maintainability.Example refactor:
+private static class NavigationHelper +{ + public static string BuildSignInUrl(string returnUrl) + { + return $"{Urls.SignInPage}?return-url={Uri.EscapeDataString(returnUrl)}"; + } +} private async Task SignOut() { await AuthManager.SignOut(CurrentCancellationToken); var returnUrl = ReturnUrl ?? NavigationManager.GetRelativePath(); - NavigationManager.NavigateTo($"{Urls.SignInPage}?return-url={Uri.EscapeDataString(returnUrl)}"); + NavigationManager.NavigateTo(NavigationHelper.BuildSignInUrl(returnUrl)); } protected override async Task OnAfterFirstRenderAsync() { await base.OnAfterFirstRenderAsync(); await AuthManager.SignOut(CurrentCancellationToken); var returnUrl = ReturnUrl ?? NavigationManager.GetRelativePath(); - NavigationManager.NavigateTo($"{Urls.SignInPage}?return-url={Uri.EscapeDataString(returnUrl)}"); + NavigationManager.NavigateTo(NavigationHelper.BuildSignInUrl(returnUrl)); }src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/QueryStringCollection.cs (1)
14-14: Consider caching the escaped values.The current implementation performs both escape and unescape operations on every add operation, which could be optimized.
- keyValues[Uri.EscapeDataString(Uri.UnescapeDataString(key))] = Uri.EscapeDataString(Uri.UnescapeDataString(value ?? "")); + var escapedKey = Uri.EscapeDataString(Uri.UnescapeDataString(key)); + var escapedValue = Uri.EscapeDataString(Uri.UnescapeDataString(value ?? "")); + keyValues[escapedKey] = escapedValue;src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignUp/SignUpPage.razor (1)
81-82: Consider extracting the URL construction logic.The confirm link URL construction is complex and could benefit from being moved to a dedicated method or helper class.
+private string GetConfirmUrl(string? email, string? phoneNumber, string? returnUrl) => + $"{Urls.ConfirmPage}?email={Uri.EscapeDataString(email ?? "")}&phoneNumber={Uri.EscapeDataString(phoneNumber ?? "")}&return-url={Uri.EscapeDataString(returnUrl ?? Urls.HomePage)}"; -<BitLink Href="@($"{Urls.ConfirmPage}?email={Uri.EscapeDataString(signUpModel.Email ?? "")}&phoneNumber={Uri.EscapeDataString(signUpModel.PhoneNumber ?? "")}&return-url={Uri.EscapeDataString(ReturnUrlQueryString ?? Urls.HomePage)}")"> +<BitLink Href="@GetConfirmUrl(signUpModel.Email, signUpModel.PhoneNumber, ReturnUrlQueryString)">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/ClientAppCoordinator.cs(3 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/AppPageBase.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignIn/SignInPanel.razor(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignUp/SignUpPage.razor(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/NotAuthorizedPage.razor.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/HttpMessageHandlers/LoggingDelegatingHandler.cs(3 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/ODataQuery.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Web/Program.Middlewares.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Controllers/IAppController.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Extensions/UriExtensions.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/QueryStringCollection.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs
🔇 Additional comments (13)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/NotAuthorizedPage.razor.cs (1)
47-47: LGTM! Proper URL encoding implementation.The change correctly uses
Uri.EscapeDataStringto encode the return URL, preventing potential URL injection vulnerabilities.src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Controllers/IAppController.cs (1)
31-36: LGTM! Well-designed extension method.The new extension method maintains consistency with existing methods while providing a fluent interface for adding multiple query strings. The implementation correctly delegates to the
AddQueryStringsmethod and follows the same generic pattern.src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/AppPageBase.cs (1)
23-23: LGTM! Proper URL encoding.The addition of
Uri.EscapeDataString()ensures that special characters in the relative path are properly encoded, preventing potential URL injection vulnerabilities.src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/HttpMessageHandlers/LoggingDelegatingHandler.cs (1)
11-11: LGTM! Enhanced URL logging.The addition of
HttpUtility.UrlDecodeimproves the readability of logged URLs by decoding them before logging. The change is consistently applied to both request and response logging.Also applies to: 27-27
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/ODataQuery.cs (1)
31-31: LGTM! Modern query string handling.The replacement of
HttpUtility.ParseQueryStringwithQueryStringCollectionaligns with the PR's objective of modernizing URL handling while maintaining the same functionality.src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Extensions/UriExtensions.cs (2)
7-8: LGTM! The query string handling has been improved.The switch to
QueryStringCollectionprovides better control over URL encoding/decoding and maintains consistency with the new query string handling approach across the codebase.Also applies to: 12-13
27-27: LGTM! Culture parameter extraction has been simplified.The change to use
QueryStringCollection.Parsesimplifies the culture parameter extraction while maintaining the same functionality.src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/QueryStringCollection.cs (1)
71-81: Add validation for malformed query strings.The current implementation might not handle all edge cases of malformed query strings.
Consider adding validation for:
- Maximum key/value lengths
- Invalid characters
- Duplicate keys
foreach (var pair in pairs) { // Split the pair into key and value using '='. var parts = pair.Split(['='], 2); + if (parts[0].Length > 2048) // Example max length + continue; string key = parts[0]; string value = parts.Length > 1 ? parts[1] : string.Empty; + if (value.Length > 2048) // Example max length + continue; qsCollection.Add(key, value); }src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignIn/SignInPanel.razor (1)
81-81: LGTM! URL escaping and fallback have been improved.The changes properly escape the return URL and provide a sensible default to the homepage when no return URL is specified.
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignUp/SignUpPage.razor (1)
79-79: LGTM! URL escaping and fallback have been improved.The changes properly escape the return URL and provide a sensible default to the homepage when no return URL is specified.
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/ClientAppCoordinator.cs (2)
58-58: LGTM! URL decoding added for telemetry.The addition of URL decoding ensures that telemetry data contains human-readable URLs instead of encoded ones.
90-91: LGTM! Consistent URL decoding in location change handler.URL decoding is consistently applied to both the telemetry context and logging, improving readability of the logs.
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Web/Program.Middlewares.cs (1)
298-298: LGTM! Consistent query string parsing.The change to use
QueryStringCollection.Parsealigns with the broader effort to standardize query string handling across the codebase.
closes #9869
Summary by CodeRabbit
Bug Fixes
Refactor