Make list of secure protocols customizable #2441
Conversation
...because not a package itself but a URL user trying to install it from
is actually insecure. Mention URL in the error message like
Attempting to install package ag from insecure URL user@ftp://example.com/ without `el-get-allow-insecure
for easier troubleshooting.
URL starting with 'file:///' (hostname is empty) is secure because it always points to a local file. OTOH, 'file://example.com/' (with any hostname, including 'localhost' and '127.0.0.1') is insecure as it may refer to the remote file and deciding if some hostname is actually a local in given moment in time is tricky and too error-prone.
|
Please, either rebase or merge instead of squashing into single commit ;-) |
| (string-match-p proto-rx URL))) el-get-secure-protocols)))) | ||
| (not (string-match "^[-_\.A-Za-z0-9]+@" URL))) | ||
| ;; With not empty :checksum, we can rely on `el-get-post-install' for | ||
| ;; security. |
There was a problem hiding this comment.
I originally wrote the comment claiming that we can rely on el-get-post-install, but I don't see a check directly in there. So the comment should be clarified (I guess I meant to rely on the functions that el-get-post-install calls?). And do we really need to check for a blank checksum? Wouldn't the normal check for checksum equality take care of that as well?
There was a problem hiding this comment.
And do we really need to check for a blank checksum? Wouldn't the normal check for checksum equality take care of that as well?
May be this is too defensive, but I think that empty :checksum should be considered here as if it is missing. Ideally, I would like to 'el-get-package-def to be responsible for returning semantically meaningful values, and return nil for empty :checksum. I just see no use case for empty string.
There was a problem hiding this comment.
I originally wrote the comment claiming that we can rely on el-get-post-install, but I don't see a check directly in there. So the comment should be clarified (I guess I meant to rely on the functions that el-get-post-install calls?)
I'm not quite sure I follow what you are trying to say. What should I do? ;-)
There was a problem hiding this comment.
I just see no use case for empty string.
I agree there is no use case for it, but there is no use case for a lot of other different non-checksum values. I don't see a need to specifically to look out for that one in particular. But since you are writing the code, I'll leave the final decision on this up to you.
I originally wrote the comment claiming that we can rely on el-get-post-install, but I don't see a check directly in there. So the comment should be clarified (I guess I meant to rely on the functions that el-get-post-install calls?)
I'm not quite sure I follow what you are trying to say. What should I do? ;-)
Hmm, I was bit confused while writing that. I was looking to confirm that empty strings would be rejected correctly, and for some reason failed to find the el-get-verify-checksum call in el-get-post-install. So basically, the comment should also mention el-get-verify-checksum to help silly readers like present-me, and not assume that it's obvious like past-me did ;-)
There was a problem hiding this comment.
I agree there is no use case for it, but there is no use case for a lot of other different non-checksum values. But since you are writing the code, I'll leave the final decision on this up to you.
Why call methods deeper in the chain if we know right here that is should be noop anyway? Why give the chance for (potential) errors caused by empty checksum? If there is no use for the code/option, it shouldn't be there ;-)
So, It makes sense to start cleaning things up then, step by step.
So basically, the comment should also mention
Just to double check: the comment or the docstring?
There was a problem hiding this comment.
So basically, the comment should also mention
Just to double check: the comment or the docstring?
The comment. No need to go into implementation details in the docstring.
For compatibility with Emacs versions before 24.4, fall back to 'string-match if 'string-blank-p from subr-x is not available.
For some reason, these, X-over-SSH protocols, are not considered as secure by 'el-get-insecure-check: - git+ssh - bzr+ssh - sftp
manandbytes
force-pushed
the
defcustom-secure-protocols
branch
from
2606875
to
0eab49a
Compare
| (not (string-match "^[-_\.A-Za-z0-9]+@" url)) | ||
| (not (string-match "^ssh" url))) | ||
| ;; With not empty :checksum, we can rely on `el-get-post-install' calling | ||
| ;; `el-get-verify-checksum' for security. |
manandbytes
force-pushed
the
defcustom-secure-protocols
branch
from
0eab49a
to
cb02c71
Compare
|
Any updates? |
|
Merged, thanks.
I think I'm supposed to get a notification when you push commits, but somehow I'm not seeing any (which is to say, you should add a message after pushing, otherwise I might not notice). |
Via 'el-get-secure-protocols, user can customize list of URL protocols
considered by 'el-get-insecure-check as secure.
By default, these protocols are considered as secure: