Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Placeables: Make sure text is always escaped #224

Merged
merged 1 commit into from
Mar 20, 2014
Merged

Placeables: Make sure text is always escaped #224

merged 1 commit into from
Mar 20, 2014

Conversation

khaledhosny
Copy link
Contributor

highlight_placeables() was assuming that parse_placeables() will always
detect placeables correctly and was outputting text not part of a
placeable unescaped, but this is not a safe assumption as
parse_placeables() can fail to detect some placeables and we might end
up with unesacaped HTML material.

@khaledhosny
Copy link
Contributor Author

For the record, here the string that triggered this: <a %(url1)s>operators and manufacturers</a> where <a %(url1)s> is not recognised as an XML tag placeable (probably because of the digit which is recognised as a placeable itself).

@dwaynebailey
Copy link
Member

@friedelwolff landed a change on TTK placeables and I wonder if it would relate to this.

I'm concerned that now we have a test framework we aren't testing for this to make sure it doesn't come back.

@dwaynebailey
Copy link
Member

My guess is that our XML placeables detection is quite strict, so that example would not qualify. I can't recall if we can handle placeables within placeables. We do detect them in a set order to prevent certain mishaps like this.

@unho
Copy link
Member

unho commented Mar 20, 2014

@khaledhosny Didn't check it live. Are you sure that this doesn't introduce any problem when translating? I mean, does it save the proper value on the DB when translating or altering the string? And if you go back is it shown properly?

@khaledhosny
Copy link
Contributor Author

@unho Do we use highlight_placeables filter on target strings, I thought it is used for the source only and this never goes back to the DB.

@khaledhosny
Copy link
Contributor Author

I think this fix is worthwhile independent of the TTK placeables issue. I’m not sure if it is a regression or not (the old code that Pootle used to highlight XML tags did the parsing on its own), but I think we can bisect this.

@unho
Copy link
Member

unho commented Mar 20, 2014

I think it is gtm. If any unexpected behavior arises we can always trace back to the root of the issue and tweak whatever needs tweaking (or reverting).

highlight_placeables() was assuming that parse_placeables() will always
detect placeables correctly and was outputting text not part of a
placeable unescaped, but this is not a safe assumption as
parse_placeables() can fail to detect some placeables and we might end
up with unesacaped HTML material.
@khaledhosny khaledhosny merged commit f93da47 into translate:master Mar 20, 2014
julen added a commit to julen/pootle that referenced this pull request Jun 20, 2014
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.

None yet

3 participants