Improve return-url implementation in Boilerplate (#9854)#9856
Improve return-url implementation in Boilerplate (#9854)#9856msynk merged 1 commit intobitfoundation:developfrom yasmoradi:9854
Conversation
WalkthroughThe pull request updates multiple components across the client, server, and test projects to ensure that return URL parameters are safely encoded. The changes uniformly apply Changes
Assessment against linked issues
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: 0
🧹 Nitpick comments (2)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs (2)
415-415: Double escaping enhances URL parameter security.The double escaping of the return URL parameter is a good security practice that prevents URL injection attacks and ensures special characters are properly handled.
Consider adding URL validation to prevent open redirect vulnerabilities:
if (string.IsNullOrEmpty(returnUrl) is false) { + if (!Uri.TryCreate(returnUrl, UriKind.RelativeOrAbsolute, out var uri) || + (uri.IsAbsoluteUri && !uri.IsWellFormedOriginalString())) + { + throw new BadRequestException(Localizer[nameof(AppStrings.InvalidReturnUrl)]); + } qs += $"&return-url={Uri.EscapeDataString(Uri.EscapeDataString(returnUrl))}"; }
411-419: Enhance URL construction safety and consistency.While the return URL is properly escaped, consider improving the overall URL construction:
Apply this diff to use
UriBuilderfor safer URL construction and ensure consistent parameter escaping:-var qs = $"userName={Uri.EscapeDataString(user.UserName!)}"; - -if (string.IsNullOrEmpty(returnUrl) is false) -{ - qs += $"&return-url={Uri.EscapeDataString(Uri.EscapeDataString(returnUrl))}"; -} - -var url = $"{Urls.SignInPage}?otp={Uri.EscapeDataString(token)}&{qs}&culture={CultureInfo.CurrentUICulture.Name}"; +var builder = new UriBuilder(Urls.SignInPage); +var query = HttpUtility.ParseQueryString(string.Empty); +query["userName"] = user.UserName; +query["otp"] = token; +query["culture"] = CultureInfo.CurrentUICulture.Name; + +if (string.IsNullOrEmpty(returnUrl) is false) +{ + if (!Uri.TryCreate(returnUrl, UriKind.RelativeOrAbsolute, out var uri) || + (uri.IsAbsoluteUri && !uri.IsWellFormedOriginalString())) + { + throw new BadRequestException(Localizer[nameof(AppStrings.InvalidReturnUrl)]); + } + query["return-url"] = Uri.EscapeDataString(returnUrl); +} + +builder.Query = query.ToString(); +var url = builder.Uri.ToString();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Layout/IdentityHeader.razor(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/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.EmailConfirmation.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/PageModels/Identity/SignInPage.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build and test
🔇 Additional comments (7)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/NotAuthorizedPage.razor.cs (1)
47-47: LGTM! Proper URL encoding implementation.The changes correctly implement URL encoding for the return URL parameter using
Uri.EscapeDataString, preventing potential issues with special characters in the URL.Also applies to: 61-61
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Layout/IdentityHeader.razor (1)
7-7: LGTM! Consistent URL encoding in navigation links.The changes properly encode the return URL parameter in both sign-up and sign-in button links using
Uri.EscapeDataString, maintaining consistency with the application's URL encoding strategy.Also applies to: 10-10
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.EmailConfirmation.cs (1)
81-81: LGTM! Comprehensive URL parameter encoding.The changes properly encode all sensitive parameters (email, token, and return URL) in the URI construction using
Uri.EscapeDataString, ensuring robust handling of special characters in the confirmation link.src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignIn/SignInPanel.razor (1)
81-81: LGTM! Proper URL encoding with null handling.The change correctly implements URL encoding for the return URL parameter using
Uri.EscapeDataStringand handles null values appropriately with the null coalescing operator (?? "").src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignUp/SignUpPage.razor (2)
79-79: LGTM! URL encoding properly implemented for sign-in link.The change correctly uses
Uri.EscapeDataStringto encode the return URL and handles null values appropriately.
81-81: LGTM! URL encoding properly implemented for confirmation link.The change correctly uses
Uri.EscapeDataStringto encode all URL parameters (email, phoneNumber, return-url) and handles null values appropriately.src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/PageModels/Identity/SignInPage.cs (1)
124-124: LGTM! Test assertion updated to match URL encoding implementation.The test assertion correctly uses
Uri.EscapeDataStringto encode the return URL, ensuring it matches the implementation behavior.
msynk
changed the title
2 participants
closes #9854
Summary by CodeRabbit
Bug Fixes
Tests