Redirect rather than proxy to about page #8344
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant changes to the WEBKNOSSOS application's configuration and routing mechanism. The primary focus is on transitioning from a proxy-based approach to a more direct about page redirection strategy. This involves renaming configuration options, updating 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: 1
🧹 Nitpick comments (2)
app/controllers/AboutPageRedirectController.scala (1)
53-55: Clarify Complex Conditional LogicThe condition in
matchesRedirectRoutecombines multiple checks, which may reduce readability and maintainability. The expression(request.identity.isEmpty || request.uri != "/")could lead to unintended behavior, especially when the user is authenticated, and the request URI is the root ("/").Consider refactoring the condition into separate, well-named boolean variables:
private def matchesRedirectRoute(request: UserAwareRequest[WkEnv, AnyContent]): Boolean = { val isWkorg = conf.Features.isWkorgInstance val matchesRoute = conf.AboutPageRedirect.routes.exists(route => matchesRouteWithWildcard(route, request.path)) val isUnauthenticatedOrNotRoot = request.identity.isEmpty || request.uri != "/" isWkorg && matchesRoute && isUnauthenticatedOrNotRoot }This enhances readability and makes the logic easier to understand and maintain.
MIGRATIONS.unreleased.md (1)
13-13: Consider enhancing the migration guide.The migration note is clear and well-documented. Consider adding an example of the new configuration format to make it easier for users to migrate.
Add configuration example:
- config options `proxy.prefix` and `proxy.routes` were renamed to `aboutPageRedirect.prefix` and `aboutPageRedirect.routes` (as we no longer proxy, but redirect). [#8344](https://github.com/scalableminds/webknossos/pull/8344) + config options `proxy.prefix` and `proxy.routes` were renamed to `aboutPageRedirect.prefix` and `aboutPageRedirect.routes` (as we no longer proxy, but redirect). Example: + ```conf + aboutPageRedirect { + prefix = "https://about.example.com" + routes = ["/about", "/terms"] + } + ``` + [#8344](https://github.com/scalableminds/webknossos/pull/8344)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
MIGRATIONS.unreleased.md(1 hunks)app/RequestHandler.scala(3 hunks)app/controllers/AboutPageRedirectController.scala(2 hunks)app/utils/SitemapWriter.scala(1 hunks)app/utils/WkConf.scala(2 hunks)conf/application.conf(1 hunks)
🔇 Additional comments (7)
app/controllers/AboutPageRedirectController.scala (1)
19-23: Class Renaming and Dependency Injection ConfirmedThe
AboutPageRedirectControllerhas been correctly renamed fromWkorgProxyController, and the dependencies are appropriately injected. This change aligns with the new redirection functionality.app/utils/SitemapWriter.scala (1)
23-23: Verify Inclusion of Redirect Routes in SitemapThe
proxyURLsnow source fromwkConf.AboutPageRedirect.routes. Ensure that these routes are intended to be included in the sitemap and that they do not contain wildcard patterns, which could generate invalid URLs.Consider adding validation to filter out any routes with wildcards and confirm that
AboutPageRedirect.routesis correctly configured.private val proxyURLs = wkConf.AboutPageRedirect.routes + .filter(route => !route.contains("*") && route.startsWith("/")) .map(SitemapURL(_))✅ Verification successful
Current Implementation is Correct
The current filtering of wildcard routes is intentional and correct. The AboutPageRedirect controller explicitly supports wildcard patterns for matching requests, but these shouldn't be included in the sitemap. The additional validation for paths starting with "/" is unnecessary as the paths are already normalized when used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find AboutPageRedirect configuration and related files rg -l "AboutPageRedirect" --type scala # Look for sitemap-related files and configurations fd -e scala -e conf sitemap # Search for route validation patterns rg -A 5 "route.*filter" --type scalaLength of output: 797
Script:
#!/bin/bash # Check WkConf configuration for AboutPageRedirect rg -A 10 "AboutPageRedirect" app/utils/WkConf.scala # Examine the redirect controller implementation cat app/controllers/AboutPageRedirectController.scalaLength of output: 3061
app/RequestHandler.scala (3)
3-3: Updated Imports ConfirmedThe import statement correctly includes
AboutPageRedirectController, replacingWkorgProxyController. This ensures that the new controller is available inRequestHandler.
21-21: Dependency Injection for AboutPageRedirectControllerThe
aboutPageRedirectControlleris properly injected intoRequestHandler. Verify that all references towkorgProxyControllerhave been removed and that the new controller functions as expected.
58-58: Review Routing Logic for Unhandled RequestsThe default handler now redirects unhandled requests using
aboutPageRedirectController.redirectToAboutPageOrSendMainView. Ensure this does not unintentionally redirect requests that should be served differently, such as static resources or valid routes not covered by earlier conditions.Evaluate the ordering of conditions in
routeRequestand consider adding specific checks for any additional routes that should bypass the redirect.conf/application.conf (1)
226-229: LGTM! Verify the configuration documentation.The configuration section is well-structured with sensible defaults. The comment accurately describes the conditional behavior.
Let's verify that the configuration is used correctly in the codebase:
✅ Verification successful
Documentation accurately reflects the implementation ✓
The configuration comment correctly describes that:
- Redirects only work when
features.isWkorgInstanceis true- The "/" route is only redirected for logged-out users
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of isWkorgInstance with aboutPageRedirect rg -A 3 "isWkorgInstance.*aboutPageRedirect|aboutPageRedirect.*isWkorgInstance"Length of output: 81
Script:
#!/bin/bash # Search for aboutPageRedirect usage rg -A 5 "aboutPageRedirect" # Search for configuration loading patterns ast-grep --pattern 'config.get("aboutPageRedirect")'Length of output: 2063
Script:
#!/bin/bash # Search for AboutPageRedirectController implementation ast-grep --pattern 'class AboutPageRedirectController $$$' # Search for isWkorgInstance usage in AboutPageRedirectController rg -A 5 "class AboutPageRedirectController"Length of output: 808
Script:
#!/bin/bash # Search for redirectToAboutPageOrSendMainView implementation ast-grep --pattern 'def redirectToAboutPageOrSendMainView $$$' # Search for the method content rg -A 10 "redirectToAboutPageOrSendMainView"Length of output: 1888
Script:
#!/bin/bash # Search for matchesRedirectRoute implementation rg -A 10 "matchesRedirectRoute" # Search for isWkorgInstance usage in the controller rg "isWkorgInstance" app/controllers/AboutPageRedirectController.scalaLength of output: 2228
app/utils/WkConf.scala (1)
139-141: LGTM! Clean implementation following established patterns.The AboutPageRedirect object correctly maps to the new configuration section and follows the codebase's configuration management patterns.
Also applies to: 253-253
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/controllers/AboutPageRedirectController.scala (1)
30-30:⚠️ Potential issueFix potential open redirect vulnerability.
Direct concatenation of request.uri with the prefix could allow malicious redirects.
Apply this fix to validate the URI:
- Fox.successful(Redirect(conf.AboutPageRedirect.prefix + request.uri, status = status)) + val safeUri = UriEncoding.encodePathSegment(request.uri, "UTF-8") + Fox.successful(Redirect(conf.AboutPageRedirect.prefix + safeUri, status = status))
🧹 Nitpick comments (3)
app/controllers/AboutPageRedirectController.scala (3)
29-30: Document status code selection.The different status codes (303 vs 301) for root vs non-root paths should be documented to explain the reasoning.
Add a comment explaining the status code selection:
+ // Use 303 for root path to prevent browser caching, 301 for other paths val status = if (request.uri == "/") SEE_OTHER else MOVED_PERMANENTLY
Line range hint
31-53: Consider extracting the main view rendering logic.The else block contains nested logic for rendering the main view. Consider extracting it to improve readability.
Extract the main view rendering to a separate private method:
+ private def renderMainView(request: UserAwareRequest[WkEnv, AnyContent]) = { + for { + multiUserOpt <- Fox.runOptional(request.identity)(user => + multiUserDAO.findOne(user._multiUser)(GlobalAccessContext)) + openGraphTags <- openGraphService.getOpenGraphTags( + request.path, + request.getQueryString("sharingToken").orElse(request.getQueryString("token"))) + } yield addCspHeader( + Ok(views.html.main( + conf, + multiUserOpt.map(_.selectedTheme).getOrElse(Theme.auto).toString, + openGraphTags.title, + openGraphTags.description, + openGraphTags.image + )) + ) + }
54-56: Document the redirect route matching logic.The complex condition combining multiple checks would benefit from documentation explaining the routing rules.
Add documentation explaining the conditions:
+ /** + * Determines if the request should be redirected based on: + * 1. If this is a wkorg instance + * 2. If the path matches configured routes + * 3. If the user is not authenticated or the path is not root + */ private def matchesRedirectRoute(request: UserAwareRequest[WkEnv, AnyContent]): Boolean =
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MIGRATIONS.unreleased.md(1 hunks)app/controllers/AboutPageRedirectController.scala(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- MIGRATIONS.unreleased.md
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (2)
app/controllers/AboutPageRedirectController.scala (2)
19-23: LGTM! Clean constructor refactoring.The class renaming and dependency changes accurately reflect the transition from proxying to redirection.
58-61: Consider validating route patterns.The wildcard matching logic assumes well-formed route patterns. Consider adding validation for the route patterns.
Run this script to check for potentially problematic route patterns:
2 participants
We decided to no longer the about page to the same domain as wk, but use a redirect to a subdomain. This PR implements the wk side of this change.
Steps to test:
isWkOrgInstance.true, setaboutPageRedirect.prefixto the target root address, andaboutPagePredirect.routesto a list of test url paths (e.g.["/test.html"]localhost:9000/test.htmlshould redirect totargetHost/test.html