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

Escape gradle string in shareTarget / build.gradle #674

Merged
merged 5 commits into from
Apr 28, 2022
Merged

Conversation

andreban
Copy link
Member

Escapes the gradle string before escaping the JSON. This ensures single quotes
are escaped.

Fixes #673

Escapes the gradle string before escaping the JSON. This ensures single quotes
are escaped.

Fixes #673
@andreban andreban requested a review from PEConn April 23, 2022 07:56
@andreban andreban added the bug Something isn't working label Apr 23, 2022
Copy link
Collaborator

@PEConn PEConn left a comment

Choose a reason for hiding this comment

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

Where does this method come from? I can't find the docs for it.

I feel uneasy about escaping a string twice - how do we know this won't do something crazy?

@andreban
Copy link
Member Author

andreban commented Apr 25, 2022

It's set here: https://github.com/GoogleChromeLabs/bubblewrap/blob/main/packages/core/src/lib/TwaGenerator.ts#L404. And the implementation at

/**
* Escapes a string that will be written to the Gradle file. The characters need to be escaped
* multiple times, as they also need to be escaped inside the Gradle file.
*
* As an example, "Andre's Code" needs to be written as "Andre\\\'s Code" to the Gradle file, so
* it is properly escaped when passed to AAPT.
*/
export function escapeGradleString(input: string): string {
return input.replace(/[\\']/g, '\\\\\\$&');
}
. We own both functions - maybe a test to check if the combination gives the expected results would help?

@PEConn
Copy link
Collaborator

PEConn commented Apr 25, 2022

Yeah, that would be good, thanks.

@andreban
Copy link
Member Author

Yeah, that would be good, thanks.

Added a test. PTAL

@andreban andreban requested a review from PEConn April 26, 2022 20:24
Copy link
Collaborator

@PEConn PEConn left a comment

Choose a reason for hiding this comment

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

LGTM % nit thanks

describe('#escapeJsonString combined with #escapeGradleString returns the expected results',
() => {
it('Escapes double and single quotes', () => {
// String.raw prevents escape characters from being applied, so we can simplify howe we write
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: typo on "how"

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@andreban andreban merged commit 7ff07f2 into main Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Share Target with single quote throws an error
2 participants