Bug#911356: ikiwiki: "po" plugin can insert raw ".po" file contents with [[!inline ... ]] directives
On Mon, 18 Feb 2019 at 18:17:57 +0100, intrigeri wrote: > Nine days later, I've not spotted any issue with your patch there. > It's worth noting that in the meantime we've released a new Tails, > that pretty often triggers this very bug. So please apply your patch > to ikiwiki upstream :) This is fixed in git master and will be in the next upstream release. smcv
Bug#911356: ikiwiki: "po" plugin can insert raw ".po" file contents with [[!inline ... ]] directives
Hi Simon, intrigeri: > I will now deploy a modified version of ikiwiki, that includes your > patch, on our production website, which will give it exposure to more > realistic usage. I'll report back in 7-10 days, which hopefully should > leave sufficient time for getting the fix in Buster; Nine days later, I've not spotted any issue with your patch there. It's worth noting that in the meantime we've released a new Tails, that pretty often triggers this very bug. So please apply your patch to ikiwiki upstream :) It would be sweet if this could make it into Buster but if that's too late, no worries (it took me some time to reply in the first place). Cheers, -- intrigeri
Bug#911356: ikiwiki: "po" plugin can insert raw ".po" file contents with [[!inline ... ]] directives
Control: tag -1 - moreinfo Hi Simon, Simon McVittie: > I have referenced an alternative solution (mostly code deletion) on the > upstream bug. Please try: > https://git.pseudorandom.co.uk/smcv/ikiwiki.git/commitdiff/wip/po-filter-every-time > I would particularly appreciate review and testing from intrigeri, who wrote > the code that I'm deleting and hopefully has a better picture of why it was/is > necessary. (I've tried to reply on the upstream bug but the anonpush mechanism seems to be broken and the CGI rejects my edit, on the grounds that the blogspam plugin is unhappy with it — presumably because I'm using Tor. So here we go :) First, thanks a lot for diving into this topic. Joey's commit 192ce7a2 was prompted by [[bugs/po_vs_templates]] and back then, it fixed that bug (and allowed me to remove some very ugly workarounds: d877b964, d4136aea), so surely it was _somewhat_ relevant to `po` users back then. But indeed, I doubt it was relevant for `inline`, which is a different beast, in that it still calls `filter` itself on the inlined content. I fully agree that "output of a filter hook is never passed back through filter hooks" should be an invariant. I've just spent quite some time trying to understand what these "preprocessing loops" were, and I've failed. I suspect that's related to the code the "Avoid loops of preprocessed pages preprocessing" comment in `IkiWiki.pm` is about, but I can't find how it could be. So I'm afraid I won't be able to shed any light on this: ten years have passed and I'm sorry my commit message back then was suboptimal :/ I'm not keen on keeping a workaround (the `alreadyfiltered` mechanism) for an under-specified bug that surely existed 10 years ago but might have been fixed since then, especially when this workaround itself clearly causes a well understood bug which causes major trouble for what might be the main user of this plugin nowadays (the Tails website, for which the `po` plugin was developed in the first place). So in principle, I'm very much in favor of removing the buggy workaround as you did. I've tested your proposed branch locally on the Tails website (957 Markdown and HTML files, 1758 PO files), that uses nested `inline`, `pagetemplate`, `toggle`, `sidebar` and more. I could not spot any issue, be it after a full rebuild, or when triggering an incremental refresh after modifying some pages involved in the most intense usage we have of the `po` plugin combined with these other ones. I will now deploy a modified version of ikiwiki, that includes your patch, on our production website, which will give it exposure to more realistic usage. I'll report back in 7-10 days, which hopefully should leave sufficient time for getting the fix in Buster; but if this timeframe is not adequate for you, feel free to release and upload without waiting for further test results from me. > (I would also like to have more extensive test coverage for the po plugin in > general: I don't think any of the ikiwiki maintainers use it, so automated > tests are the only way we can make sure it hasn't regressed.) I share these feelings: even though I'm using this plugin a lot, I deeply regret having written it before I learnt much about software testing. I can't realistically promise I'll increase the test coverage in general, but in the future I'll try my best to at least add tests for the areas affected by changes I'll submit (there's one more branch in the pipeline), simply because I have happily unlearned how to write code without writing tests. Cheers, -- intrigeri
Bug#911356: ikiwiki: "po" plugin can insert raw ".po" file contents with [[!inline ... ]] directives
Hi Simon et al., > I would particularly appreciate review and testing from intrigeri, who wrote > the code that I'm deleting and hopefully has a better picture of why it was/is > necessary. ACK. FYI this is being tracked in Tails at: https://redmine.tails.boum.org/code/issues/6907 Best wishes, -- ,''`. : :' : Chris Lamb `. `'` la...@debian.org 🍥 chris-lamb.co.uk `-
Bug#911356: ikiwiki: "po" plugin can insert raw ".po" file contents with [[!inline ... ]] directives
Control: tags -1 + moreinfo On Thu, 18 Oct 2018 at 20:11:32 -0400, Chris Lamb wrote: > There is an issue where an initial "inline" directive would be > translated correctly but subsequent inlines of the same page would > result in the raw contents of the ".po" file (ie. starting with the raw > copyright headers!) being inserted into the page instead. As noted on the upstream bug, I've added a failing test-case for this to ikiwiki. Unfortunately, I don't think either the patch on this bug or the patch sent upstream was correct: they introduce a cache that holds a large amount of wiki content in memory, which ikiwiki tries hard to avoid for performance, and I'm not confident that the cache is invalidated frequently enough for correctness. I have referenced an alternative solution (mostly code deletion) on the upstream bug. Please try: https://git.pseudorandom.co.uk/smcv/ikiwiki.git/commitdiff/wip/po-filter-every-time I would particularly appreciate review and testing from intrigeri, who wrote the code that I'm deleting and hopefully has a better picture of why it was/is necessary. (I would also like to have more extensive test coverage for the po plugin in general: I don't think any of the ikiwiki maintainers use it, so automated tests are the only way we can make sure it hasn't regressed.) Thanks, smcv
Bug#911356: ikiwiki: "po" plugin can insert raw ".po" file contents with [[!inline ... ]] directives
On Thu, 18 Oct 2018 at 20:18:29 -0400, Chris Lamb wrote: > However, I can't seem to encourage upstream to accept the patch and/or > even know what the next steps are, hence filing it here. Sorry, neither the Debian maintainer nor the most frequent upstream maintainer have as much time to review ikiwiki patches as they would like. (Both are me.) The po plugin is, in general, rather complicated, which tends to push it down my priority list - I can't review po patches without first working out (again) how it works and why it's the way it is, and because I don't use it myself, I'm concerned that I'll break it and not know. If you can put together a regression test for this bug that renders a translated page and inspects the HTML output (t/img.t, t/meta.t, or a combination of t/po.t and t/render.t might be a good basis) that would make me a lot more confident about accepting patches. At the moment I don't think we have any "full stack" test coverage that actually renders HTML from a translated page. Thanks, smcv
Bug#911356: ikiwiki: "po" plugin can insert raw ".po" file contents with [[!inline ... ]] directives
forwarded 911356 https://ikiwiki.info/todo/Re-use_translated_content_instead_of_skipping_if_previously_translated/ thanks This is being tracked upstream here: https://ikiwiki.info/todo/Re-use_translated_content_instead_of_skipping_if_previously_translated/ However, I can't seem to encourage upstream to accept the patch and/or even know what the next steps are, hence filing it here. Could the Debian maintainer(s) poke upstream on this and could this be applied in Debian in the meantime? Regards, -- ,''`. : :' : Chris Lamb `. `'` la...@debian.org / chris-lamb.co.uk `-
Bug#911356: ikiwiki: "po" plugin can insert raw ".po" file contents with [[!inline ... ]] directives
Source: ikiwiki
Version: 3.20180311-1
Severity: important
Tags: patch
Hi,
There is an issue where an initial "inline" directive would be
translated correctly but subsequent inlines of the same page would
result in the raw contents of the ".po" file (ie. starting with the raw
copyright headers!) being inserted into the page instead.
§
For example, given a "index.mdwn" containing:
[[!inline pages="inline" raw="yes"]]
[[!inline pages="inline" raw="yes"]]
… and an "index.de.po" of:
msgid "[[!inline pages=\"inline\" raw=\"yes\"]]\n"
msgstr "[[!inline pages=\"inline.de\" raw=\"yes\"]]\n"
… together with an "inline.mdwn" of:
This is inlined content.
… and an "inline.de.po" of:
msgid "This is inlined content."
msgstr "This is German inlined content."
§
This would result in the following translation:
This is the inlined content.
# SOME DESCRIPTIVE TITLE
# Copyright (C) YEAR Free Software Foundation, Inc.
# This file is distributed under the same license as the PACKAGE package.
# FIRST AUTHOR , YEAR.
… instead of (of course)
This is the inlined content.
This is the inlined content.
§
Patch against ikiwiki 3.20180311-1 attached.
Best wishes,
--
,''".
: :' : Chris Lamb
". "'" la...@debian.org / chris-lamb.co.uk
"-
diff --git a/IkiWiki/Plugin/po.pm b/IkiWiki/Plugin/po.pm
index 418e8e5..912672a 100644
--- a/IkiWiki/Plugin/po.pm
+++ b/IkiWiki/Plugin/po.pm
@@ -307,6 +307,13 @@ sub filter (@) {
$content = po_to_markup($page, $content);
setalreadyfiltered($page, $destpage);
}
+ if (istranslation($page)) {
+ if (!defined(alreadyfiltered($page, $destpage))) {
+ $content = po_to_markup($page, $content);
+ setalreadyfiltered($page, $destpage, $content);
+ }
+ $content = alreadyfiltered($page, $destpage);
+ }
return $content;
}
@@ -747,15 +754,15 @@ sub myisselflink ($$) {
my $page=shift;
my $destpage=shift;
- return exists $filtered{$page}{$destpage}
-&& $filtered{$page}{$destpage} eq 1;
+ return exists $filtered{$page}{$destpage};
}
- sub setalreadyfiltered($$) {
+ sub setalreadyfiltered($$$) {
my $page=shift;
my $destpage=shift;
+ my $content=shift;
- $filtered{$page}{$destpage}=1;
+ $filtered{$page}{$destpage}=$content;
}
sub unsetalreadyfiltered($$) {
