-
Notifications
You must be signed in to change notification settings - Fork 26
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(builder): Add packageref validation for name and variant size #1185
Changes from 32 commits
d51dc32
8bd0566
882c98a
14bbb22
e75a9b1
515f683
d965ab0
30842ef
08a98fa
e89ce24
cb7d493
7c983a3
3bdf661
268ceff
39fb98e
f559009
848e54e
3db7fd3
a9b37fd
c287256
b944b26
099b564
76c73d0
df0884f
c2bf838
f1f5ecf
7b48a06
4fb44c9
652d215
65685b9
1018bca
b39da33
57dcaf4
dd6b584
fb486ac
4968ccd
da4f6a7
978931a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -37,7 +37,7 @@ export interface PackagePublishCall { | |||||||||||||||||||||||||
export class PackageReference { | ||||||||||||||||||||||||||
static DEFAULT_TAG = 'latest'; | ||||||||||||||||||||||||||
static DEFAULT_PRESET = 'main'; | ||||||||||||||||||||||||||
static PACKAGE_REGEX = /^(?<name>@?[a-z0-9][A-Za-z0-9-]{1,30}[a-z0-9])(?::(?<version>[^@]+))?(@(?<preset>[^\s]+))?$/; | ||||||||||||||||||||||||||
static PACKAGE_REGEX = /^(?<name>@?[a-z0-9][A-Za-z0-9-]{1,}[a-z0-9])(?::(?<version>[^@]+))?(@(?<preset>[^\s]+))?$/; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||
* Anything before the colon or an @ (if no version is present) is the package name. | ||||||||||||||||||||||||||
|
@@ -76,19 +76,35 @@ export class PackageReference { | |||||||||||||||||||||||||
|
||||||||||||||||||||||||||
if (!match || !match.groups?.name) { | ||||||||||||||||||||||||||
throw new Error( | ||||||||||||||||||||||||||
`Invalid package name "${ref}". Should be of the format <package-name>:<version> or <package-name>:<version>@<preset>` | ||||||||||||||||||||||||||
`Invalid package reference "${ref}". Should be of the format <package-name>:<version> or <package-name>:<version>@<preset>` | ||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const res: PartialRefValues = { name: match.groups.name }; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const nameSize = res.name.length; | ||||||||||||||||||||||||||
if (!(nameSize <= 32)) { | ||||||||||||||||||||||||||
throw new Error(`Package reference "${ref}" is too long. Package name exceeds 32 bytes`); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
if (match.groups.version) res.version = match.groups.version; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const versionSize = res.name.length; | ||||||||||||||||||||||||||
if (!(versionSize <= 32)) { | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think |
||||||||||||||||||||||||||
throw new Error(`Package reference "${ref}" is too long. Package version exceeds 32 bytes`); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
if (match.groups.preset) res.preset = match.groups.preset; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
return res; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
static isValid(ref: string) { | ||||||||||||||||||||||||||
try { | ||||||||||||||||||||||||||
PackageReference.parse(ref); | ||||||||||||||||||||||||||
} catch (err) { | ||||||||||||||||||||||||||
return false; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
return !!PackageReference.PACKAGE_REGEX.test(ref); | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The regex is already being used inside the
Suggested change
|
||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,7 @@ const argtype: z.ZodLazy<any> = z.lazy(() => | |
// <%= string interpolation %>, step.names or property.names, packages:versions | ||
const interpolatedRegex = RegExp(/^<%=\s\w+.+[\w()[\]-]+\s%>$/, 'i'); | ||
const stepRegex = RegExp(/^[\w-]+\.[.\w-]+$/, 'i'); | ||
const packageRegex = RegExp(/^(?<name>@?[a-z0-9][a-z0-9-]{1,29}[a-z0-9])(?::(?<version>[^@]+))?(@(?<preset>[^\s]+))?$/, 'i'); | ||
const packageRegex = RegExp(/^(?<name>@?[a-z0-9][a-z0-9-]{1,}[a-z0-9])(?::(?<version>[^@]+))?(@(?<preset>[^\s]+))?$/, 'i'); | ||
const jsonAbiPathRegex = RegExp(/^(?!.*\.d?$).*\.json?$/, 'i'); | ||
|
||
// This regex matches artifact names which are just capitalized words like solidity contract names | ||
|
@@ -209,6 +209,34 @@ export const pullSchema = z | |
message: `Source value: ${val} must match package formats: "package:version" or "package:version@preset" or operation name "import.Contract" or be an interpolated value`, | ||
}) | ||
) | ||
.refine( | ||
(val) => { | ||
const match = val.match(packageRegex); | ||
|
||
if (match) { | ||
const nameSize = match!.groups!.name.length; | ||
|
||
return nameSize <= 32; | ||
} else { | ||
return true; | ||
} | ||
}, | ||
(val) => ({ message: `Package reference "${val}" is too long. Package name exceeds 32 bytes` }) | ||
) | ||
.refine( | ||
(val) => { | ||
const match = val.match(packageRegex); | ||
|
||
if (match && match!.groups!.version) { | ||
const versionSize = match!.groups!.version.length; | ||
|
||
return versionSize <= 32; | ||
} else { | ||
return true; | ||
} | ||
}, | ||
(val) => ({ message: `Package reference "${val}" is too long. Package version exceeds 32 bytes` }) | ||
) | ||
.describe('Source of the cannonfile package to import from. Can be a cannonfile operation name or package name'), | ||
}) | ||
.merge( | ||
|
@@ -502,6 +530,33 @@ export const cloneSchema = z | |
message: `Source value: ${val} must match package formats: "package:version" or "package:version@preset" or be an interpolated value`, | ||
}) | ||
) | ||
.refine( | ||
(val) => { | ||
const match = val.match(packageRegex); | ||
if (match) { | ||
const nameSize = match!.groups!.name.length; | ||
|
||
return nameSize <= 32; | ||
} else { | ||
return true; | ||
} | ||
}, | ||
(val) => ({ message: `Package reference "${val}" is too long. Package name exceeds 32 bytes` }) | ||
) | ||
.refine( | ||
(val) => { | ||
const match = val.match(packageRegex); | ||
|
||
if (match && match!.groups!.version) { | ||
const versionSize = match!.groups!.version.length; | ||
|
||
return versionSize <= 32; | ||
} else { | ||
return true; | ||
} | ||
}, | ||
(val) => ({ message: `Package reference "${val}" is too long. Package version exceeds 32 bytes` }) | ||
) | ||
.describe('Name of the package to provision'), | ||
}) | ||
.merge( | ||
|
@@ -536,7 +591,34 @@ export const cloneSchema = z | |
message: `Target value: ${val} must match package formats: "package:version" or "package:version@preset" or be an interpolated value`, | ||
}) | ||
) | ||
.describe('Name of the package to provision'), | ||
.refine( | ||
(val) => { | ||
const match = val.match(packageRegex); | ||
if (match) { | ||
const nameSize = match!.groups!.name.length; | ||
|
||
return nameSize <= 32; | ||
} else { | ||
return true; | ||
} | ||
}, | ||
(val) => ({ message: `Package reference "${val}" is too long. Package name exceeds 32 bytes` }) | ||
) | ||
.refine( | ||
(val) => { | ||
const match = val.match(packageRegex); | ||
|
||
if (match && match!.groups!.version) { | ||
const versionSize = match!.groups!.version.length; | ||
|
||
return versionSize <= 32; | ||
} else { | ||
return true; | ||
} | ||
}, | ||
(val) => ({ message: `Package reference "${val}" is too long. Package version exceeds 32 bytes` }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These functions should not be repeated, maybe they can be refactored as a single new fn in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then, that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's an issue when importing from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. However inside the schema file I could refactor them into a single function that takes a value and checks the length to be less or equal to 32 bytes, so that we avoid repetition. |
||
) | ||
.describe('Name of the package to clone'), | ||
/** | ||
* (DEPRECATED) use `target` instead. Set the new preset to use for this package. | ||
* Default - "main" | ||
|
@@ -654,7 +736,12 @@ export const chainDefinitionSchema = z | |
name: z | ||
.string() | ||
.min(3) | ||
.max(31) | ||
.refine( | ||
(val) => { | ||
return new Blob([val]).size <= 32; | ||
}, | ||
(val) => ({ message: `Package name "${val}" is too long. Package name exceeds 32 bytes` }) | ||
) | ||
.refine((val) => !!val.match(RegExp(/[a-zA-Z0-9-]+/, 'gm')), { | ||
message: 'Name cannot contain any special characters', | ||
}) | ||
|
@@ -664,7 +751,12 @@ export const chainDefinitionSchema = z | |
*/ | ||
version: z | ||
.string() | ||
.max(31) | ||
.refine( | ||
(val) => { | ||
return new Blob([val]).size <= 32; | ||
}, | ||
(val) => ({ message: `Package version "${val}" is too long. Package version exceeds 32 bytes` }) | ||
) | ||
.refine((val) => !!val.match(RegExp(/[\w.]+/, 'gm')), { | ||
message: 'Version cannot contain any special characters', | ||
}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
PackageReference.isValid
fn now only uses the regex to validate, not the constructor, so it would not returnfalse
when given a too long package name.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah but we do want it to be more specific as to why the
parse
function fails, and.match
fails when the length regex is set due to the name being too long, so I removed the length limiter on the regex and on parse I added the same errors so that it specifies when the package name is too long if somehow a package makes it through the schema validation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree on the
parse
functionality with prettier errors, but thePackageReference.isValid
should returnfalse
when it is too long, and with this change it doesn't. Maybe instead of validating with the regex inside theisValid
you can use.parse
with a try catch.