fix(catalogs): raise catalog error, not raw ValueError, on a malformed URL#3435
Merged
Merged
Conversation
…d URL CatalogStackBase._validate_catalog_url read parsed.hostname, which raises ValueError on a malformed authority (e.g. an unclosed ipv6 bracket "https://[::1"). every other reject path raises the class catalog error, so the raw ValueError leaked to the caller. wrap the parse + hostname access and convert ValueError to the normal error via cls._error. twin of the bundler fix in adapters._validate_remote_url.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes URL validation in the shared catalog stack so malformed authorities (e.g., invalid IPv6 bracket forms) don’t leak a raw ValueError and instead raise the expected catalog error type. This makes specify integration catalog add <url> fail cleanly and consistently with other invalid-URL paths.
Changes:
- Wrap
urlparse()+parsed.hostnameaccess inCatalogStackBase._validate_catalog_urland convertValueErrorintocls._error(...). - Reuse the parsed
hostnamevalue for localhost detection and host presence checks. - Add regression coverage for malformed bracketed hosts (unclosed IPv6 bracket, bracketed non-IP host).
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/catalogs.py |
Converts malformed-URL ValueError into the catalog stack’s normal error type and reuses hostname safely. |
tests/integrations/test_integration_catalog.py |
Adds regression tests ensuring malformed catalog URLs are rejected with IntegrationCatalogError (not a raw ValueError). |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Low
Collaborator
|
Thank you! |
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.
fixes #3434
CatalogStackBase._validate_catalog_urlreadparsed.hostname, which raisesValueErroron a malformed authority (unclosed ipv6 brackethttps://[::1, bracketed non-ip hosthttps://[not-an-ip]). the method's other reject paths all raise the class catalog error, so the raw ValueError leaked instead of failing cleanly. reachable viaspecify integration catalog add <url>.twin of the bundler validator fix (#3432 / #3433).
fix: wrap the parse + hostname access and convert
ValueErrorto the normal error viacls._error, then reuse the parsed hostname for the existing scheme/host checks.added a regression test (
test_malformed_url_rejected_cleanly) covering an unclosed ipv6 bracket and a bracketed non-ip host. i confirmed it fails on the pre-fix code by stashing the source and re-running (raw ValueError leaks); the url-validation suite passes with the fix.