Report an empty required url as a missing field - #1
Merged
Conversation
The required-fields filter skipped url whenever it was applicable, on the assumption that an unusable url had already been reported as a format error. An empty url reports nothing: both branches of the url validation are guarded on rawUrl being truthy. So a missing url was dropped from missingFields and never surfaced, and where it was the only missing field the inputs passed sanitization with url set to an empty string. Track whether a url error was actually reported, rather than whether url was in scope, mirroring the versionValidationFailed flag added alongside the equivalent version_number guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect
The required-fields filter skips
urlwhenever it is applicable, on the assumption that an unusable url has already been reported as a format error:But
urlValidatedis set from scope (config.applicableFields.includes('url')), not from outcome. An empty url reports nothing — both branches of the url validation are guarded onrawUrlbeing truthy and fall straight through. So a missing url is dropped frommissingFieldsand never reported.Where url is the only missing required field,
errorsends up empty and the inputs pass sanitization withurlset to'', leaving the caller to fail later on an empty value:error_messagetoday""""Missing required fields: author, version_number— url omittedThe fix
Track whether a url error was actually reported rather than whether url was in scope. This mirrors
versionValidationFailed, added in ffefdd1 alongside the equivalentversion_numberguard — that one got the pattern right; the url guard predates it and never did.urlValidatedis replaced byurlValidationFailed, set at the two points that push a url error. No change toTYPE_CONFIG,URL_PATTERNS, or anyrequiredFields/applicableFieldsentry.Behaviour
An empty required url is now reported. An invalid or over-length url still reports only its format message, with no duplicate
Missing required fieldsentry — the double-reporting the original guard existed to prevent.""Missing required fields: urlInvalid ... URL format. Expected: ...URL exceeds maximum length of 500 characters""urlis inrequiredFieldsfor all four types, so this applies tobcheckandbambdaas well as the two extension types: an empty url stops passing through silently and is reported instead. Their configuration is untouched, but the outcome does change, so worth a look from anyone owning those flows. A downstream step was always going to fail on the empty url — this reports it at the point it can be explained.Tests
Three jobs added. The first two fail on
main(both currently returnerror_message: ""); the third guards the fix against over-reporting and passes either way.extension-submission: missing url produces errorextension-update: missing url produces errorextension-submission: invalid url is not also reported as missingNote on the caller
PortSwigger/extension-portalno longer depends on this: itsextract_issue_fields.pynow validates the form's required fields beforesanitize-inputsruns and takes precedence on the error message. This is a correctness fix in the shared action, not a fix that portal submissions are waiting on. The pin there isv1.0.4and will need bumping once this is tagged.🤖 Generated with Claude Code