[MIG] T3173: Bring back external translation platform client#2096
[MIG] T3173: Bring back external translation platform client#2096NoeBerdoz wants to merge 1 commit into
Conversation
…2094 frontend (T2913) - Delete static/src/frontend/** (PR #2094 in-Odoo OWL portal frontend) - Delete templates/portal_templates.xml + PR-added menu icons - Drop web.assets_frontend block from manifest - controllers/main.py: restored file_open serve at /translation-platform - _compute_translation_url: reads sbc_translation.webapp_base_url ICP, falls back to <web.base.url>/translation-platform (the controller's mount point); URL path /letters/letter-edit/<id> matches the webapp's routes.ts entry - correspondence.remove_local_translate / resubmit_to_translation: thin forwarders to the action_* methods so the webapp keeps calling the v14 names while view buttons keep the action_* prefix
There was a problem hiding this comment.
Code Review
This pull request refactors the translation platform to serve an external Single Page Application (SPA) from a static directory, replacing the previous internal OWL-based implementation. Significant changes include updating the controller to handle SPA routing and asset redirection, modifying the correspondence model to support external webapp URLs, and removing legacy frontend assets. Feedback highlights a security concern regarding the change to public authentication without group checks and identifies a flaw in the asset detection logic that could prevent various file types from loading correctly.
| auth="public", | ||
| ) | ||
| def translation_platform_legacy(self, page="", **kwargs): | ||
| def translation_platform(self, page=""): |
There was a problem hiding this comment.
The route authentication has been changed from user to public, and the previous check for the sbc_translation.group_user group has been removed. This allows unauthenticated users to access the translation platform's entry point. Unless this is intended for the SPA to handle its own authentication (e.g., via a custom login page), it is recommended to restore auth="user" and the group check to maintain the security model of the module.
auth="user",
)
def translation_platform(self, page=""):
if not http.request.env.user.has_group("sbc_translation.group_user"):
return redirect("/my")| if ( | ||
| "assets" in page or page.endswith(".png") or page.endswith(".jpg") | ||
| ): | ||
| return redirect(f"/sbc_translation/static/tp/{page}") |
There was a problem hiding this comment.
The logic for detecting static assets is too restrictive. It only accounts for paths containing "assets" or files ending in .png/.jpg, which misses many common web asset types (e.g., .js, .css, .svg, .woff2, .ico, .json). Any asset not matching this condition will result in the controller serving index.html instead of the actual file, leading to execution errors in the browser (e.g., "Unexpected token '<'"). A more robust heuristic for SPA routing is to check if the requested path segment contains a dot, which typically indicates a file request rather than a client-side route.
if page and "." in page.split("/")[-1]:
return redirect(f"/sbc_translation/static/tp/{page}")
Confidence Score: 3/5Not safe to merge as-is — two P1 issues in the controller need resolution first. Two P1 findings in the most critical changed file: dropping server-side auth to public and a substring-based asset routing heuristic that can break SPA navigation and leave path-traversal unchecked. Model changes are clean and manifest cleanup is straightforward, but controller issues are on the hot path for every user request. sbc_translation/controllers/main.py requires the most attention due to auth and routing logic changes.
|
| Filename | Overview |
|---|---|
| sbc_translation/controllers/main.py | Controller rewritten to serve an external SPA via file_open; switches auth to public (removes server-side auth gate), and uses a fragile substring check for asset routing that can break SPA navigation and does not guard against path-traversal sequences. |
| sbc_translation/models/correspondence.py | Adds webapp-compatible URL format for translation_url, introduces a configurable webapp base URL ICP parameter, and adds alias methods for backward-compatible webapp RPC calls. |
| sbc_translation/manifest.py | Removes web.assets_frontend block and portal_templates.xml from the manifest; straightforward cleanup matching the removal of the in-Odoo OWL frontend. |
Reviews (1): Last reviewed commit: "[MIG] T3173: surgical revert of integrat..." | Re-trigger Greptile
| if ( | ||
| "assets" in page or page.endswith(".png") or page.endswith(".jpg") | ||
| ): | ||
| return redirect(f"/sbc_translation/static/tp/{page}") |
There was a problem hiding this comment.
Fragile asset-detection heuristic breaks SPA routing
The condition "assets" in page is a substring check, so any SPA client-side route that happens to contain the word "assets" (e.g. /translation-platform/letters/assets-list) will be incorrectly redirected to a static-file URL instead of receiving index.html. The SPA's client-side router will never see those routes. Additionally, the redirect uses the raw page value without sanitising path-traversal sequences, so a crafted URL like /translation-platform/assets/../../../other_module/static/somefile could expose static files outside static/tp/.
A safer approach is to match only well-known asset extensions (and validate no .. segments are present) instead of a substring check on the directory name.
| with file_open("sbc_translation/static/tp/index.html") as app: | ||
| return app.read() |
There was a problem hiding this comment.
No error handling when built SPA assets are absent
If sbc_translation/static/tp/index.html does not exist (e.g. in a fresh checkout before running npm run build and copying the dist/ output), file_open will raise a FileNotFoundError, producing an unhandled 500. Adding a try/except that returns a friendly 503 or maintenance page would be safer for operators who haven't yet deployed the built assets.
| class TranslationPlatformController(http.Controller): | ||
| @http.route( | ||
| ["/translation-platform", "/translation-platform/<path:page>"], | ||
| type="http", | ||
| auth="user", | ||
| website=True, | ||
| auth="public", | ||
| ) |
There was a problem hiding this comment.
Unauthenticated access to the translation platform
The previous controller required user-level authentication and an explicit translator-group check before rendering the page. The new route sets the auth parameter to "public", so unauthenticated visitors receive the SPA shell with no server-side gate. If the SPA handles login internally this is intentional, but it is a significant security posture change: any regression in the SPA client-side login logic would silently expose the platform to anonymous users. Consider keeping user-level auth so Odoo redirects unauthenticated visitors to the login page before serving the app shell.
falls back to <web.base.url>/translation-platform (the controller's
mount point); URL path /letters/letter-edit/ matches the webapp's
routes.ts entry
thin forwarders to the action_* methods so the webapp keeps calling
the v14 names while view buttons keep the action_* prefix
Dependencies:
CompassionCH/translation-platform-web#51
CompassionCH/compassion-switzerland#1756