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

[eas-json] validate EAS Submit inputs better #2198

Merged

Conversation

szdziedzic
Copy link
Member

@szdziedzic szdziedzic commented Jan 25, 2024

Why

We want to perform stricter validation of EAS Submit inputs.

How

Use Joi schema for validation

Test Plan

Automated tests
Submit test app manually

@szdziedzic
Copy link
Member Author

/changelog-entry chore Validate EAS Submit inputs better

Copy link

github-actions bot commented Jan 25, 2024

Size Change: -999 B (0%)

Total Size: 51.5 MB

Filename Size Change
./packages/eas-cli/dist/eas-linux-x64.tar.gz 51.5 MB -999 B (0%)

compressed-size-action

Copy link

codecov bot commented Jan 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b5f2f52) 54.14% compared to head (9c9320d) 54.16%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2198      +/-   ##
==========================================
+ Coverage   54.14%   54.16%   +0.03%     
==========================================
  Files         516      516              
  Lines       18798    18808      +10     
  Branches     3769     3772       +3     
==========================================
+ Hits        10176    10186      +10     
  Misses       8601     8601              
  Partials       21       21              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

packages/eas-json/src/utils.ts Outdated Show resolved Hide resolved
packages/eas-json/src/utils.ts Outdated Show resolved Hide resolved
packages/eas-json/src/utils.ts Outdated Show resolved Hide resolved
packages/eas-json/src/utils.ts Outdated Show resolved Hide resolved
@szdziedzic szdziedzic requested a review from sjchmiela January 25, 2024 14:09
Copy link
Contributor

@sjchmiela sjchmiela left a comment

Choose a reason for hiding this comment

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

Looks ok. A nice improvement could be moving from Joi to Zod and using Zod-inferred types to ensure we never pass non-resolved/non-validated credentials to the build.

Like, we could say:

const ResolvedProfile = z.object({
  ...,
  resolved: z.boolean().default(true),

Using z.infer<typeof ResolvedProfile> in a function that accepts the profile as input would ensure on type level we never forget to resolve, because the argument would need to have the resolved: true property which it could only get when resolving.

This can probably also be achieved with Joi and manual typing, I'm ok with that too.

packages/eas-json/src/submit/schema.ts Outdated Show resolved Hide resolved
@@ -111,6 +111,11 @@ export default class IosSubmitCommand {
const envAppSpecificPassword = getenv.string('EXPO_APPLE_APP_SPECIFIC_PASSWORD', '');

if (envAppSpecificPassword) {
if (!/^[a-z]{4}-[a-z]{4}-[a-z]{4}-[a-z]{4}$/.test(envAppSpecificPassword)) {
throw new Error(
'EXPO_APPLE_APP_SPECIFIC_PASSWORD must be in the format XXXX-XXXX-XXXX-XXXX, where X is a lowercase letter.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh, do we need to depend on this? Like, is this value susceptible to being used for a hack?

Copy link
Member Author

@szdziedzic szdziedzic Jan 25, 2024

Choose a reason for hiding this comment

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

I don't think so, but the format for this value is well known, so why not validate it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure

@@ -99,7 +99,7 @@ function mergeProfiles<T extends Platform>(

export function getDefaultProfile<T extends Platform>(platform: T): SubmitProfile<T> {
const Schema =
platform === Platform.ANDROID ? AndroidSubmitProfileSchema : IosSubmitProfileSchema;
platform === Platform.ANDROID ? AndroidSubmitProfileSchema : UnresolvedIosSubmitProfileSchema;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a comment why we can use Unresolved here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it matters which one we use here. I just used the same one which was used until now. I didn't think about it too much.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, because Joi will only check and throw if there is a value. If it's undefined it will do nothing. Let's use Resolved then?

@szdziedzic
Copy link
Member Author

I agree that Zod is nice, but in my opinion, if we decide to do Joi->Zod migration we should do it in a separate PR and keep the validation lib we use consistent across the whole package. I want to keep this PR as purpose-focused as possible.

ascApiKeyId: Joi.string()
.regex(/^[\dA-Z]{10}$/)
.message(
'Invalid Apple App Store Connect API Key ID ("ascApiKeyId") was specified. It should consist of 10 upper case letters or digits. Example: "AB32CDE81F". Learn more: https://expo.fyi/creating-asc-api-key.'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'Invalid Apple App Store Connect API Key ID ("ascApiKeyId") was specified. It should consist of 10 upper case letters or digits. Example: "AB32CDE81F". Learn more: https://expo.fyi/creating-asc-api-key.'
'Invalid Apple App Store Connect API Key ID ("ascApiKeyId") was specified. It should consist of 10 uppercase letters or digits. Example: "AB32CDE81F". Learn more: https://expo.fyi/creating-asc-api-key.'

appleId: Joi.string()
.email()
.message(
'Invalid Apple ID was specified. It should be a valid email address. Example: "[email protected]".'
Copy link
Member

Choose a reason for hiding this comment

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

Prefer to use domains that are guaranteed not to be used in examples. example.com and the .example TLD are valid for examples.

Suggested change
'Invalid Apple ID was specified. It should be a valid email address. Example: "name@domain.com".'
'Invalid Apple ID was specified. It should be a valid email address. Example: "name@example.com".'

appleTeamId: Joi.string()
.regex(/^[\dA-Z]{10}$/)
.message(
'Invalid Apple Team ID was specified. It should consist of 10 letters or digits. Example: "AB32CDE81F".'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'Invalid Apple Team ID was specified. It should consist of 10 letters or digits. Example: "AB32CDE81F".'
'Invalid Apple Team ID was specified. It should consist of 10 uppercase letters or digits. Example: "AB32CDE81F".'

Copy link

✅ Thank you for adding the changelog entry!

@szdziedzic szdziedzic merged commit 126c562 into main Jan 26, 2024
9 checks passed
@szdziedzic szdziedzic deleted the @szdziedzic/sanitize-values-in-eas-submit-profile-better branch January 26, 2024 09:48
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.

5 participants