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

Android: Properly validate godot_project_name_string for Android special chars #54255

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Oct 26, 2021

Fixes #52659.

This true enabled p_escape_quotes in xml_escape:

godot/core/string/ustring.cpp

Lines 3876 to 3884 in f2cf52e

String String::xml_escape(bool p_escape_quotes) const {
String str = *this;
str = str.replace("&", "&");
str = str.replace("<", "&lt;");
str = str.replace(">", "&gt;");
if (p_escape_quotes) {
str = str.replace("'", "&apos;");
str = str.replace("\"", "&quot;");
}

And apparently that caused issues.

That's not it, see #54255 (comment).

It was added like this originally by @amanj120 in #42185 as part of implementing Android App Bundle support.

@akien-mga akien-mga added bug platform:android cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:3.3 topic:export labels Oct 26, 2021
@akien-mga akien-mga added this to the 4.0 milestone Oct 26, 2021
@akien-mga akien-mga requested a review from m4gr3d October 26, 2021 07:39
@akien-mga akien-mga requested a review from a team as a code owner October 26, 2021 07:39
@akien-mga akien-mga marked this pull request as draft October 26, 2021 07:43
@akien-mga
Copy link
Member Author

So this needs more work, there's actually a full table of characters that need to be escaped but not using standard XML escapes... :|
https://developer.android.com/guide/topics/resources/string-resource.html#FormattingAndStyling

@akien-mga akien-mga force-pushed the android-export-gradle-dont-escape-quotes branch from d58dc32 to 9204a3a Compare October 26, 2021 08:01
@akien-mga akien-mga changed the title Android: Don't escape quotes for godot_project_name_string Android: Properly validate godot_project_name_string for Android special chars Oct 26, 2021
@akien-mga akien-mga marked this pull request as ready for review October 26, 2021 08:01
@akien-mga
Copy link
Member Author

akien-mga commented Oct 26, 2021

This now does the full escaping magic, aside from handling U+XXXX unicode codes. I'm not sure that's something we support for the Android package name in the first place so I didn't bother. Could be implemented later on if there's a need for it though.

Tested with:

[application]

config/name="Do's and
\"Some\"        t@b?
 Don'ts"

Generates:

<?xml version="1.0" encoding="utf-8"?>
<!--WARNING: THIS FILE WILL BE OVERWRITTEN AT BUILD TIME-->
<resources>
        <string name="godot_project_name_string">Do\'s and\n\"Some\"\tt\@b\?\n Don\'ts</string>
</resources>

And works fine too on device:
image

Also to note, I used xml_escape(false) since we already escape the quotes for Android's purpose and they're otherwise valid in the XML. But it wouldn't be wrong either to also escape them to XML codes, Android docs mention that both are supported (so it should still detect \&apos; as an Android-escaped single quote).

@akien-mga
Copy link
Member Author

This now does the full escaping magic, aside from handling U+XXXX unicode codes. I'm not sure that's something we support for the Android package name in the first place so I didn't bother. Could be implemented later on if there's a need for it though.

I just tested to be sure, using e.g. U+2013 in a project name does not trigger an error. It's just handled as a simple string, not as a unicode codepoint.

@akien-mga akien-mga merged commit 8473062 into godotengine:master Oct 26, 2021
@akien-mga akien-mga deleted the android-export-gradle-dont-escape-quotes branch October 26, 2021 16:22
@akien-mga
Copy link
Member Author

Cherry-picked for 3.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot export to android if package name contains single quote
2 participants