-
Notifications
You must be signed in to change notification settings - Fork 188
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
possible fix for #910 #911
Conversation
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.
Instead of this just strip the username and password using the uri
module.
Co-authored-by: Andreas Rumpf <[email protected]>
So which do you guys prefer? If the url is dirty, ask the user for a new one, or attempt to fix it as @dom96 suggested? As for @dom96 idea, I was thinking of deleting everything between "https://" up to the first "@" (inclusive), but then I am concerned that it may not always be correct since I do not know the exhaustive list of possible combination in the url. |
In addition, there is another possibility in which passwords and tokens may be supplied in the url, which is via http get. I don't think github allows that and it is not recommended to do that, but some other sites might. I don't think it is possible to properly scrub such information in http get urls. How do you know which argument is the password/token? I am thinking to just detect for any '@' or '?' in the url, and ask the user if any such characters is found. What do you think? |
I agree with this solution. |
In the URI RFCs the data between You don't need to do any parsing yourself, just parseUri the url, clear
This isn't something that any sane website would allow, we shouldn't be detecting for this. |
Actually, thinking about it some more. I think we should just quit if the URL contains any passwords, this is because it implies that the URL won't work without them, the user should change their git remote. So I would say:
You can add a hint to the error as well to instruct the user on how to fix their git remote. |
I have a couple of questions: what to do when the url contains password and the detection algorithm. First, if the url contains a password or an access token, based on the feedback so far, we have several options:
I personally prefer option no 2. Second, regarding detection algorithm, wouldn't it be more efficient to detect the Regarding http get, I did a little research and it turns out it is fairly common to have something like Do we need to cover for the case of http get? I am ambivalent about this. I am fine either way whether we cover http get or not. Let's decide on this and hopefully we can reach a consensus and get this over with. |
I prefer (1) because like your oauth quote supports: other applications being able to access the oauth token is a security risk. Nimble should scream at the user about it, not silently ignore it and ask for a git URL.
We're not parsing gigabytes of data here. Don't optimise prematurely. Parsing the URL will take care of edge cases that we may not predict and it's much clearer about what's being done.
If your URI is accessible in plaintext inside a commonly accessed file then there is no such assumption. But if you really want you can also make Nimble quit when the |
Possible fix for issue #910