Bug 38217 - TT tags breaking translation script when used to build a JS string
Summary: TT tags breaking translation script when used to build a JS string
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: I18N/L10N (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on: 38147
Blocks:
  Show dependency treegraph
 
Reported: 2024-10-21 09:14 UTC by Jonathan Druart
Modified: 2024-10-23 23:22 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2024-10-21 09:14:01 UTC
Found on bug 38147.

We all know the "don't use TT tag in HTML tags" because they break the translator script

There is another side-effect.

If you do:

  str = '<a foo="' + foo + '" bar="[% bar | uri %]"'>' + _("Click me") + '</a>' ; 

Then "Click me" won't be picked by the translator script

So I have tried this:
  str = '<a foo="%s" bar="%s">%s</a>'.format(foo, "[% bar | uri %]", _("Click me"))

But it does not seems to work either.

So the solution I found is to use an intermediate JS variable:
  let bar = "[% bar | uri %]";
  str = '<a foo="%s" bar="%s">%s</a>'.format(foo, bar, _("Click me"))
Comment 1 Jonathan Druart 2024-10-21 09:15:16 UTC
Find possible suspects:

  git grep "_(" **/*.tt|grep '\[%'
Comment 2 Jonathan Druart 2024-10-21 09:15:55 UTC
This one is confirmed at least:

koha-tmpl/intranet-tmpl/prog/en/modules/admin/identity_provider_domains.tt:                            var result = '<a class="btn btn-default btn-xs" role="button" href="/cgi-bin/koha/admin/identity_providers.pl?
domain_ops=1&amp;identity_provider_id=[%- identity_provider_id | html -%]&amp;op=edit_form&amp;identity_provider_domain_id='+ encodeURIComponent(row.identity_provider_domain_id) +'"><i class="fa-solid fa-pencil" aria-hidden="true"></i> '+_("Edit")+'</a>'+"\n";
Comment 3 David Cook 2024-10-21 23:24:43 UTC
(In reply to Jonathan Druart from comment #0)
> So the solution I found is to use an intermediate JS variable:
>   let bar = "[% bar | uri %]";
>   str = '<a foo="%s" bar="%s">%s</a>'.format(foo, bar, _("Click me"))

Another option would be something like:

let link = document.createElement('a');
link.textContent = _("Click me");
link.setAttribute('foo',foo);
link.setAttribute('bar',bar);
str = link.outerHTML;

--

While it's more verbose, it's safe from XSS and should be translation friendly. 

In the quoted version, both "foo" and "bar" are potentially vulnerable to XSS if they contain unsanitized user input.
Comment 4 David Cook 2024-10-21 23:26:03 UTC
In any case, is this translation issue a new behaviour or has this problem existed previously?
Comment 5 Jonathan Druart 2024-10-22 07:46:13 UTC
(In reply to David Cook from comment #4)
> In any case, is this translation issue a new behaviour or has this problem
> existed previously?

From Mattermost:
"""
Issue with the translation script that has been around for years. We reject the structure (TT tags inside HTML tags) from the QA script, but this is a new structure that must be rejected

OR we fix the translation script, but IIRC it is not trivial.
"""
Comment 6 Jonathan Druart 2024-10-22 07:47:33 UTC
(In reply to David Cook from comment #3)
> In the quoted version, both "foo" and "bar" are potentially vulnerable to
> XSS if they contain unsanitized user input.

We should call escape_str on all JS vars, yes. But it's not the point of the bug report.
Comment 7 David Cook 2024-10-22 22:15:40 UTC
(In reply to Jonathan Druart from comment #5)
> From Mattermost:
> """
> Issue with the translation script that has been around for years. We reject
> the structure (TT tags inside HTML tags) from the QA script, but this is a
> new structure that must be rejected

Yeah I still don't understand what you're saying there. What's the "new structure"?
Comment 8 David Cook 2024-10-22 22:16:26 UTC
(In reply to Jonathan Druart from comment #6)
> (In reply to David Cook from comment #3)
> > In the quoted version, both "foo" and "bar" are potentially vulnerable to
> > XSS if they contain unsanitized user input.
> 
> We should call escape_str on all JS vars, yes. But it's not the point of the
> bug report.

I didn't say it was. Just pointing out the robustness of the alternative I suggested.
Comment 9 Jonathan Druart 2024-10-23 06:54:26 UTC
(In reply to David Cook from comment #7)
> (In reply to Jonathan Druart from comment #5)
> > From Mattermost:
> > """
> > Issue with the translation script that has been around for years. We reject
> > the structure (TT tags inside HTML tags) from the QA script, but this is a
> > new structure that must be rejected
> 
> Yeah I still don't understand what you're saying there. What's the "new
> structure"?

The one we knew that was problematic, when building HTML nodes:
  <a href="#" [% IF required %]required[% END %]>link</a>
We catch it with the QA script, and there is a test: xt/tt_valid.t

The new one, discovered here, when building nodes using JS.
Comment 10 David Cook 2024-10-23 23:21:14 UTC
(In reply to Jonathan Druart from comment #9)
> The one we knew that was problematic, when building HTML nodes:
>   <a href="#" [% IF required %]required[% END %]>link</a>
> We catch it with the QA script, and there is a test: xt/tt_valid.t
> 
> The new one, discovered here, when building nodes using JS.

Ok I think I understand better now. Thanks. 

(That is, it's an old problem in the translation process, but it's a new problem since we're only noticing it now.)
Comment 11 David Cook 2024-10-23 23:22:38 UTC
It seems to me that we should decide on a new method (or methods) and add them as coding guidelines?

If possible, catch in QA script, although I don't know how easy that would be.