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

[BUG] + in the name of an app causes a build issue #4791

Closed
doublezze opened this issue Aug 16, 2024 · 11 comments · Fixed by #4828
Closed

[BUG] + in the name of an app causes a build issue #4791

doublezze opened this issue Aug 16, 2024 · 11 comments · Fixed by #4828
Labels

Comments

@doublezze
Copy link

Link to the report card page

https://www.pwabuilder.com/reportcard?site=https://mbyapps.com/mbyappsportal.php

What Store were you trying to package for?

Android Google Play

What is the error message you received?

Internal Server Error

Status code: 500. Error generating app package: Command failed: ./gradlew assembleRelease

Paste your stack trace below

The site I was testing is: https://mbyapps.com/mbyappsportal.php
undefined

What environment were you using?

Windows 10

Additional context

None

@maiconcarraro
Copy link
Contributor

It seems related to the "+" plus sign in your app name, I found a workaround that you can try:

Expand all settings:
image

Find the Signing key section and remove the plus sign:
image

After this change it worked for me to download your package. I kept app and short names w/ plus sign. Still a bug to fix, but sharing the workaround.
image

@jgw96 jgw96 changed the title [BUG] Packaging Error [BUG] + in the name of an app causes a build issue Aug 19, 2024
@jgw96
Copy link
Contributor

jgw96 commented Aug 19, 2024

@maiconcarraro do you think this is a bug in PWABuilder or in the android build tools? Hard to tell where this bug is coming from.

@maiconcarraro
Copy link
Contributor

@jgw96 well, its more related to UX/DX, either PWABuilder or bubblewrap could sanitise/automatically escape these special characters when generating the key, I dont think its a bug because it will happen anyway if you try keytool directly w/ same args https://stackoverflow.com/a/11809689/3257568

I'd say that would be better to PWABuilder validate/escape since it automatically prefill the inputs for a new key, and especially validate user inputs to prevent the fail, but its up to you to decide on this behavior. The main issue is the unfriendly exception, if it correctly gives feedback to the user that is an invalid input, should be good.

@maiconcarraro
Copy link
Contributor

@jgw96 actually, we can say it's a bug because the options are kinda "hidden" inside of the settings
image

so for new users they will have no idea what is the problem, so the correct approach is to escape/remove the characters when prefilling.

@jgw96
Copy link
Contributor

jgw96 commented Aug 19, 2024

I agree with this, we should update our form validation to warn about the + character.

@maiconcarraro
Copy link
Contributor

@jgw96 another person had the same issue but with "&", source: https://discord.com/channels/930156205542883409/930871438666235964/1275537693899620394 just to add more context

@doublezze
Copy link
Author

Maicon's workaround worked!! Thanks. I'll proceed to test the Android Google Play package. The Windows package though successfully downloaded has an incorrect app name mbyapps+ - mbyappsportal instead of just mbyapps+

@doublezze
Copy link
Author

Both Windows and Android packages have incorrect app names. Windows show the name as mbyapps+ - mbyappsportal while Android has name as mbyapps.com. Is this a settings issue on my part or a bug?

@jgw96
Copy link
Contributor

jgw96 commented Aug 27, 2024

@doublezze can you send a screenshot of the settings you used for packaging either app?

@doublezze
Copy link
Author

Screenshot_28-8-2024_0736_www pwabuilder com
Screenshot_28-8-2024_0639_www pwabuilder com
Screenshot_28-8-2024_0639_www pwabuilder com
Screenshot_28-8-2024_0736_www pwabuilder com

github-merge-queue bot pushed a commit that referenced this issue Oct 9, 2024
fixes #4791 
fixes
https://discord.com/channels/930156205542883409/930871438666235964/1292435840554897502
<!-- Link to relevant issue (for ex: "fixes #1234") which will
automatically close the issue once the PR is merged -->

## PR Type
<!-- Please uncomment one ore more that apply to this PR -->

- Bugfix
<!-- - Feature -->
<!-- - Code style update (formatting) -->
<!-- - Refactoring (no functional changes, no api changes) -->
<!-- - Build or CI related changes -->
<!-- - Documentation content changes -->
<!-- - Sample app changes -->
<!-- - Other... Please describe: -->


## Describe the current behavior?

Depending on the manifest app name or short name it can create an
unexpected value for signing key inputs, which causes differentes
issues:
- The invalid inputs for signing key aren't visible by default (inside
of sl-details), even after form submit.
- The developer won't know which fields are wrong because the error
message is unfriendly.
- There is no input validation for these fields.

<!-- Please describe the current behavior that is being modified or link
to a relevant issue. -->

## Describe the new behavior?

After some investigation on bubblewrap, Andre already added some
characters escapes here:
GoogleChromeLabs/bubblewrap#373 but it's not
covering all possible characters that can invalidate the input as coded
here:
https://github.com/frohoff/jdk8u-jdk/blob/da0da73ab82ed714dc5be94acd2f0d00fbdfe2e9/src/share/classes/sun/security/x509/AVA.java#L95C33-L95C49

We can still open an issue or PR to escape it on bubblewrap, but decided
to add a sanitize + validation layer for PWABuilder similar to the
existing [input validation for app
name](https://github.com/pwa-builder/PWABuilder/blob/0b9255a0eb60e38bc0793b5f761a6fe9c06cdb12/apps/pwabuilder/src/script/utils/constants.ts#L1).
(+ Friendly)

Behaviors added:
- New sanitize function to automatically generate a buildable app at
first try.
- New dname input validation for the signing key fields to prevent users
from typing wrong characters.
- On form's submit, if there is any error, it automatically show
`sl-details` content allowing to scroll into the error.

## PR Checklist

- [x] Test: run `npm run test` and ensure that all tests pass
- [x] Target main branch (or an appropriate release branch if
appropriate for a bug fix)
- [x] Ensure that your contribution follows [standard accessibility
guidelines](https://docs.microsoft.com/en-us/microsoft-edge/accessibility/design).
Use tools like https://webhint.io/ to validate your changes.


## Additional Information

I couldn't find a validate manifest to test the sanitize at first try,
the original manifest from the issue is currently down, but here a
simple demo of the function:

![image](https://github.com/user-attachments/assets/7e403b80-b85a-4de0-ba98-5efcb95674d7)

And demo for the input validation + show sl-details on errors.


https://github.com/user-attachments/assets/aba77184-3343-4086-9131-370becc0cd30
@doublezze
Copy link
Author

doublezze commented Oct 9, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants