Skip to content

[MIG] T3173: Bring back external translation platform client#2096

Open
NoeBerdoz wants to merge 1 commit into
18.0from
T3173-migrate-auth-external
Open

[MIG] T3173: Bring back external translation platform client#2096
NoeBerdoz wants to merge 1 commit into
18.0from
T3173-migrate-auth-external

Conversation

@NoeBerdoz
Copy link
Copy Markdown
Contributor

  • Delete static/src/frontend/** (PR Integrate translation platform into Odoo 18 as a portal OWL frontend #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/ 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

Dependencies:
CompassionCH/translation-platform-web#51
CompassionCH/compassion-switzerland#1756

…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
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +23 to +25
auth="public",
)
def translation_platform_legacy(self, page="", **kwargs):
def translation_platform(self, page=""):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

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")

Comment on lines +36 to +39
if (
"assets" in page or page.endswith(".png") or page.endswith(".jpg")
):
return redirect(f"/sbc_translation/static/tp/{page}")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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}")

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 12, 2026

Confidence Score: 3/5

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

Security Review

  • Unauthenticated route (controllers/main.py): The auth parameter was changed from user-level (with translator group check) to public. Server-side access control is gone; security now depends entirely on the SPA client-side login logic.
  • Path traversal via redirect (controllers/main.py): The raw URL path parameter is forwarded unvalidated into a redirect targeting the static directory. A crafted path with traversal sequences could redirect users to static files outside the intended static/tp/ directory.

Important Files Changed

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

Comment on lines +36 to +39
if (
"assets" in page or page.endswith(".png") or page.endswith(".jpg")
):
return redirect(f"/sbc_translation/static/tp/{page}")
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +40 to +41
with file_open("sbc_translation/static/tp/index.html") as app:
return app.read()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +19 to 24
class TranslationPlatformController(http.Controller):
@http.route(
["/translation-platform", "/translation-platform/<path:page>"],
type="http",
auth="user",
website=True,
auth="public",
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 security 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant