-
Notifications
You must be signed in to change notification settings - Fork 229
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
Validate and normalize hosted url. #3030
Conversation
We normalize the URL for a _hosted pub server_ to have no slash if the server is just a bare domain like `https://example.com`, but if the URL contains a path like `https://example.com/my-folder` then we always normalize to `https://example.com/my-folder/`. The reason for normalizing the URL is to improve consistency in `pubspec.lock` and make it easier to implement authentication without risks of being tricked by incorrect prefixes. Additionally, be normalizing to no slash for empty paths, and paths always ending in a slash when path is non-empty, we gain the benefit that relative URLs can always be constructed correctly using `hostedUrl.resolve('api/packages/$package')`. This additionally forbids a few edge cases such as: * querystring in the hosted URL (`https://server.com/?query`), * fragment in the hosted URL (`https://server.com/#hash`), * user-info in the hosted URL (`https://user:[email protected]`). These may have worked with previous versions of the `pub` client, but most likely the _querystring_ or _fragment_ would cause URLs to be garbled. Any user-info would likely have been ignored, this was not tested, any usage of these options is considered unlikely. Previously, `dart pub publish` would ignore the path in the hosted URL and always upload to `/api/packages/new`. This commit fixes this issue.
@themisir I'm hoping this will make authentication easier... Let me know what you think. |
I am a bit busy for a few days. I will continue working on authentication soon 🤞 |
That's fine. I'm busy too finding bugs in the solver. As long as we get back to it, there is no rush :D |
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.
LGTM, comments are nits.
} | ||
|
||
/// Cache value for [server]. |
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.
Assuming that a publish command pretty much always needs to look at the server property, it seems easier to just have a final Uri server = (){ return ...; }();
property and get rid of the lazy loading.
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.
if only... members like argResults
can't be accessed in an initializer 😭
Fixes #3024
We normalize the URL for a hosted pub server to have no slash if the
server is just a bare domain like
https://example.com
, but if the URLcontains a path like
https://example.com/my-folder
then we alwaysnormalize to
https://example.com/my-folder/
.The reason for normalizing the URL is to improve consistency in
pubspec.lock
and make it easier to implement authentication withoutrisks of being tricked by incorrect prefixes.
Additionally, be normalizing to no slash for empty paths, and paths
always ending in a slash when path is non-empty, we gain the benefit
that relative URLs can always be constructed correctly using
hostedUrl.resolve('api/packages/$package')
.This additionally forbids a few edge cases such as:
https://server.com/?query
),https://server.com/#hash
),https://user:[email protected]
).These may have worked with previous versions of the
pub
client, butmost likely the querystring or fragment would cause URLs to be garbled.
Any user-info would likely have been ignored, this was not tested, any
usage of these options is considered unlikely.
Previously,
dart pub publish
would ignore the path in the hosted URLand always upload to
/api/packages/new
. This commit fixes this issue.