Skip to content

Commit

Permalink
fix: sanitize input for dname on signing key (#4828)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
maiconcarraro authored Oct 9, 2024
1 parent bf1ac8a commit 9257da1
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 9 deletions.
6 changes: 5 additions & 1 deletion apps/pwabuilder/src/script/components/android-form.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { AppPackageFormBase } from './app-package-form-base';
import { getManifestContext } from '../services/app-info';
import { maxSigningKeySizeInBytes } from '../utils/android-validation';
import { recordPWABuilderProcessStep, AnalyticsBehavior } from '../utils/analytics';
import { AppNameInputPattern } from '../utils/constants';
import { AppNameInputPattern, DnameInputPattern } from '../utils/constants';

@customElement('android-form')

Expand Down Expand Up @@ -786,6 +786,7 @@ export class AndroidForm extends AppPackageFormBase {
required: true,
placeholder: 'John Doe',
value: this.packageOptions.signing.fullName || '',
pattern: DnameInputPattern,
spellcheck: false,
inputHandler: (val: string) => this.packageOptions.signing.fullName = val
})}
Expand All @@ -800,6 +801,7 @@ export class AndroidForm extends AppPackageFormBase {
required: true,
placeholder: 'My Company',
value: this.packageOptions.signing.organization || '',
pattern: DnameInputPattern,
spellcheck: false,
inputHandler: (val: string) => this.packageOptions.signing.organization = val
})}
Expand All @@ -814,6 +816,7 @@ export class AndroidForm extends AppPackageFormBase {
required: true,
placeholder: 'Engineering Department',
value: this.packageOptions.signing.organizationalUnit,
pattern: DnameInputPattern,
spellcheck: false,
inputHandler: (val: string) => this.packageOptions.signing.organizationalUnit = val
})}
Expand All @@ -828,6 +831,7 @@ export class AndroidForm extends AppPackageFormBase {
required: true,
placeholder: 'US',
value: this.packageOptions.signing.countryCode,
pattern: DnameInputPattern,
spellcheck: false,
minLength: 2,
maxLength: 2,
Expand Down
4 changes: 3 additions & 1 deletion apps/pwabuilder/src/script/components/publish-pane.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1039,7 +1039,9 @@ export class PublishPane extends LitElement {
let packagingOptions = platForm!.getPackageOptions();
this.generate(this.selectedStore.toLowerCase() as Platform, packagingOptions);
} else {
form!.reportValidity();
const details = platForm.shadowRoot!.querySelector('sl-details');
const expanding = details ? details.show() : Promise.resolve();
expanding.then(() => form!.reportValidity()); // it may contain errors collapsed
}
}

Expand Down
11 changes: 6 additions & 5 deletions apps/pwabuilder/src/script/services/publish/android-publish.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import {
validateAndroidOptions,
AndroidPackageOptions,
generatePackageId
generatePackageId,
sanitizeDname
} from '../../utils/android-validation';
import { env } from '../../utils/environment';
import { findSuitableIcon, findBestAppIcon } from '../../utils/icons';
Expand All @@ -18,7 +19,7 @@ export async function generateAndroidPackage(androidOptions: AndroidPackageOptio
);
}

let headers = {...getHeaders(), 'content-type': 'application/json' };
let headers = { ...getHeaders(), 'content-type': 'application/json' };

const referrer = sessionStorage.getItem('ref');
const generateAppUrl = `${env.androidPackageGeneratorUrl}/generateAppPackage${referrer ? '?ref=' + encodeURIComponent(referrer) : ''}`;
Expand Down Expand Up @@ -56,7 +57,7 @@ export async function generateAndroidPackage(androidOptions: AndroidPackageOptio
let err = new Error(
`Error generating Android package.\nStatus code: ${response.status}\nError: ${response.statusText}\nDetails: ${responseText}`
);
Object.defineProperty(response, "stack_trace", {value: responseText});
Object.defineProperty(response, "stack_trace", { value: responseText });
//@ts-ignore
err.response = response;
throw err;
Expand Down Expand Up @@ -237,8 +238,8 @@ export function createAndroidPackageOptionsFromManifest(manifestContext: Manifes
signing: {
file: null,
alias: 'my-key-alias',
fullName: `${manifest.short_name || manifest.name || 'App'} Admin`,
organization: manifest.name || 'PWABuilder',
fullName: `${sanitizeDname(manifest.short_name) || sanitizeDname(manifest.name) || 'App'} Admin`,
organization: sanitizeDname(manifest.name) || 'PWABuilder',
organizationalUnit: 'Engineering',
countryCode: 'US',
keyPassword: '', // If empty, one will be generated by CloudAPK service
Expand Down
15 changes: 14 additions & 1 deletion apps/pwabuilder/src/script/utils/android-validation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { PackageOptions, ShareTarget, ShortcutItem } from './interfaces';
* Settings for the Android APK generation. This is the raw data passed to the CloudApk service.
* It should match the CloudApk service's AndroidPackageOptions interface: https://github.com/pwa-builder/CloudAPK/blob/master/build/androidPackageOptions.ts
*/
export interface AndroidPackageOptions extends PackageOptions {
export interface AndroidPackageOptions extends PackageOptions {
appVersion: string;
appVersionCode: number;
backgroundColor: string;
Expand Down Expand Up @@ -107,6 +107,19 @@ export function generatePackageId(host: string): string {
return parts.join('.');
}

// https://github.com/frohoff/jdk8u-jdk/blob/da0da73ab82ed714dc5be94acd2f0d00fbdfe2e9/src/share/classes/sun/security/x509/AVA.java#L95C33-L95C49
export const dnameInvalidCharacters = '\\,\\=\\+\\<\\>\\#\\;\\\\"';

// Sanitize over escape, otherwise it may conflict with bubblewrap escapes https://github.com/GoogleChromeLabs/bubblewrap/issues/373
export function sanitizeDname(input: string | undefined): string {
if (!input) {
return "";
}

const regex = new RegExp(`([${dnameInvalidCharacters}])`, 'g');
return input.replace(regex, '');
}

export function validateAndroidPackageId(packageId?: string | null): AndroidPackageValidationError[] {
const packageErrors: AndroidPackageValidationError[] = [];
if (!packageId) {
Expand Down
5 changes: 4 additions & 1 deletion apps/pwabuilder/src/script/utils/constants.ts
Original file line number Diff line number Diff line change
@@ -1 +1,4 @@
export const AppNameInputPattern = '[^\\|\\$\\@\\#\\>\\<\\)\\(\\!\\&\\%\\*]+$';
import { dnameInvalidCharacters } from './android-validation';

export const AppNameInputPattern = '[^\\|\\$\\@\\#\\>\\<\\)\\(\\!\\&\\%\\*]+$';
export const DnameInputPattern = `[^${dnameInvalidCharacters}]+$`;

0 comments on commit 9257da1

Please sign in to comment.