[CFX-7608] Reject a non-http/https URL in dr auth set-url - #912
Open
chasdr wants to merge 4 commits into
Open
Conversation
Why: `dr auth set-url ftp://host` (and file://, gopher://, and the like) wrote the bad scheme straight into drconfig.yaml. The endpoint only failed later at read time, with a less obvious error. SchemeHostOnly normalizes but has to stay scheme-agnostic because export and GetBaseURL share it, so the http/https rule had no home on the write path. Changes: - add config.RequireHTTPScheme(baseURL), the http/https rule as one helper. SetURLToConfig calls it after SchemeHostOnly, so every set-url write path (arg, picker, stdin, and dr auth login <url>) rejects a bad scheme before it lands in the config file. A bare host still resolves to https. - auth.ValidateEndpoint delegates its inline scheme check to the same helper. Message is byte-identical, no duplication. - dr auth set-url <arg> now prints the reason and exits non-zero on an invalid arg instead of silently opening the interactive picker. Run becomes RunE, with SilenceErrors/SilenceUsage to match the other auth subcommands.
|
🎫 Jira: |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
… path
Why:
SetURLToConfig rejects a non-http/https scheme, but the template setup
custom-host picker writes through SaveURLToConfig, which skipped the check.
Typing ftp:// there still persisted an unusable endpoint.
Changes:
- SaveURLToConfig runs RequireHTTPScheme on a non-empty host before writing.
The empty reset case is untouched, and saveHost already surfaces the error
in the setup TUI.
- test: SaveURLToConfig("ftp://...") errors and persists nothing.
Why: The interactive picker and stdin prompt only re-ask on ErrInvalidURL. RequireHTTPScheme returned a plain error, so a custom URL with a bad scheme aborted the whole set-url flow instead of letting the user retry, unlike a malformed URL. Changes: - RequireHTTPScheme returns an error that unwraps to ErrInvalidURL, keeping its specific message for the read path and the arg path. The two picker paths (the errors.Is check in selectURLInteractively and readURLFromStdin) now re-ask on a bad scheme. - test: a rejected scheme satisfies errors.Is(err, ErrInvalidURL).
Contributor
Author
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit e8e4738. Configure here.
chasdr
added
Needs Review: CFX
00 - Ready for Review
and removed
Needs Review: CFX
labels
Code OwnershipCli Maintainers
Review requested from the teams above. Labels will be removed automatically upon approval. |
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.
PR 3 of 3: #776 was PR 1, #911 is PR 2
Summary
dr auth set-urlaccepted any URL scheme and wrote it straight intodrconfig.yaml, sodr auth set-url ftp://hoststored a dead endpoint that only failed later at read time with a vaguer error. It now rejects a non-http/https URL at write time and exits non-zero. A bare host likeapp.datarobot.comis still accepted and defaults tohttps.Notes for review
The http/https rule is one helper,
config.RequireHTTPScheme, applied at both write paths that persist the endpoint:SetURLToConfig(set-url arg, the interactive picker, stdin, anddr auth login <url>) andSaveURLToConfig(the template-setup custom-host picker).SchemeHostOnlystays scheme-agnostic becauseexportandGetBaseURLshare it, so the rule sits on top rather than inside it.Output
Technical Changes
RequireHTTPScheme.SetURLToConfigandSaveURLToConfigboth call it afterSchemeHostOnly.ValidateEndpointdelegates its inline scheme check to the same helper. message byte-identical, no duplication.RunbecomesRunEwithSilenceErrors/SilenceUsage.set-url.RequireHTTPSchemetable, both writers' scheme rows,set-urlarg exits non-zero.Breakdown
Note
Low Risk
Localized URL validation on auth/config write paths with tests; no change to token handling or API auth flow beyond earlier rejection of bad endpoints.
Overview
dr auth set-urlnow rejects endpoints whose scheme is nothttporhttpsat configuration time, with a clear error and non-zero exit, instead of saving unusable values (e.g.ftp://) intodrconfig.yamlor falling back to the interactive picker when the user passed an explicit bad URL.Validation is centralized in
config.RequireHTTPScheme, applied after normalization inSetURLToConfigandSaveURLToConfig(set-url, login, template setup).ValidateEndpointuses the same helper. Bad schemes in the interactive picker still re-prompt because the error wrapsErrInvalidURL. Bare hosts still default tohttpsviaSchemeHostOnly.Docs under
set-urldescribe the rejection behavior; tests cover the helper, both write paths, and the set-url command argument path.Reviewed by Cursor Bugbot for commit e8e4738. Configure here.