[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.
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