-
Notifications
You must be signed in to change notification settings - Fork 90
feat: validate generic package asset labels to prevent 403 Forbidden errors #911
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
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: fgreinacher <[email protected]>
Co-authored-by: fgreinacher <[email protected]>
.map(([option, value]) => getError(`EINVALID${option.toUpperCase()}`, { [option]: value })); | ||
|
||
// Validate generic package labels | ||
if (options.assets && Array.isArray(options.assets)) { |
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.
@copilot Move this check to the existing VALIDATORS object above
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.
Moved isValidGenericPackageLabel
to the top-level scope above the VALIDATORS object in commit b7e8f14.
test/verify.test.js
Outdated
} | ||
); | ||
|
||
test.serial("Throw SemanticReleaseError if generic package asset label contains spaces", async (t) => { |
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.
@copilot Try to assert all cases in a single test. It should work with using multiple assets and
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.
Consolidated 6 individual tests into 1 test that validates all invalid label scenarios using multiple assets in a single test case. The test now checks that all 6 errors are thrown correctly in commit b7e8f14.
…lidate tests Co-authored-by: fgreinacher <[email protected]>
lib/verify.js
Outdated
// Validate generic package labels | ||
if (options.assets && Array.isArray(options.assets)) { | ||
options.assets.forEach((asset) => { | ||
if (isPlainObject(asset) && asset.target === "generic_package" && asset.label) { | ||
if (!isValidGenericPackageLabel(asset.label)) { | ||
errors.push(getError("EINVALIDGENERICPACKAGELABEL", { label: asset.label, asset })); | ||
} | ||
} | ||
}); | ||
} |
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.
@copilot Move this up to the other validation functionality
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.
Moved the generic package label validation to be immediately after the VALIDATORS-based validation, grouping all validation logic together before the required field checks in commit 8c15736.
if (isPlainObject(asset) && asset.target === "generic_package" && asset.label) { | ||
if (!isValidGenericPackageLabel(asset.label)) { | ||
errors.push(getError("EINVALIDGENERICPACKAGELABEL", { label: asset.label, asset })); |
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.
@copilot This can go in line 36
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.
I'm not sure I understand what should go in line 36. The validation check at lines 67-69 needs to produce specific EINVALIDGENERICPACKAGELABEL
error codes for each invalid asset (as expected by the tests), but integrating it into the assets
validator at line 36 would only produce a single EINVALIDASSETS
error for the entire array. Could you clarify what you'd like me to change?
Co-authored-by: fgreinacher <[email protected]>
Plan: Validate Generic Package Asset Labels
Based on the issue analysis, the 403 Forbidden error occurs when uploading assets to GitLab's generic package registry with labels containing spaces or invalid characters. GitLab's API restricts package filenames to specific characters.
Changes Made:
EINVALIDGENERICPACKAGELABEL
error definition with clear documentation of GitLab's restrictionsverify.js
withisValidGenericPackageLabel
function at the top-level scopeValidation Rules:
All 141 tests pass (consolidated from 146 tests).
Original prompt
This section details on the original issue you should resolve
<issue_title>[Help] An error occurred while making a request to the GitLab release API: HTTPError: Response code 403 (Forbidden)</issue_title>
<issue_description>
I get this error when executing the plugin in a Gitlab CI pipeline. It only happens the first time, because in the second execution it works fine.
I have a GL_TOKEN with the right permissions over the project.
Could anyone help?