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

fix: sanitize input for dname on signing key #4828

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

maiconcarraro
Copy link
Contributor

fixes #4791
fixes https://discord.com/channels/930156205542883409/930871438666235964/1292435840554897502

PR Type

  • Bugfix

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.

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. (+ 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

  • Test: run npm run test and ensure that all tests pass
  • Target main branch (or an appropriate release branch if appropriate for a bug fix)
  • Ensure that your contribution follows standard accessibility guidelines. 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

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

Home._.PWABuilder.-.Google.Chrome.2024-10-08.11-43-32.mp4

Copy link
Contributor

Thanks maiconcarraro for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@jgw96 jgw96 added this pull request to the merge queue Oct 9, 2024
Merged via the queue into pwa-builder:main with commit 9257da1 Oct 9, 2024
3 of 5 checks passed
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.

[BUG] + in the name of an app causes a build issue
2 participants