-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Allow to install packed extensions from URL or local file #1456
Conversation
Signed-off-by: Roman <[email protected]>
… in add-cluster-page) Signed-off-by: Roman <[email protected]>
Signed-off-by: Roman <[email protected]>
Signed-off-by: Roman <[email protected]>
Signed-off-by: Roman <[email protected]>
Signed-off-by: Roman <[email protected]>
…ard` Signed-off-by: Roman <[email protected]>
Signed-off-by: Roman <[email protected]>
Signed-off-by: Roman <[email protected]>
Signed-off-by: Roman <[email protected]>
Signed-off-by: Roman <[email protected]>
# Conflicts: # src/common/utils/index.ts
Signed-off-by: Roman <[email protected]>
@jakolehm PTAL / test |
Signed-off-by: Roman <[email protected]>
Signed-off-by: Roman <[email protected]>
# Conflicts: # src/renderer/components/+extensions/extensions.tsx # src/renderer/utils/downloadFile.ts
Signed-off-by: Roman <[email protected]>
Signed-off-by: Roman <[email protected]>
Signed-off-by: Roman <[email protected]>
} | ||
|
||
getExtensionDestFolder(name: string) { | ||
return path.join(this.extensionsPath, sanitizeExtensionName(name)); |
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.
This sanitation is not as complete as you think. So far I don't think we put any requirements on extension names and (at least on Windows) we do not sanitize out \
or even .
s.
Plus I don't see why @
cannot be part of the dest folder path.
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.
Anyway we have to sanitize it since name might contain /
and currently loading extensions supported only from single level from ~./k8slens/extensions/<folder>
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.
True, but I am thinking more about security. If someone crafted the name to be @foo\..\..\/blarr
on windows, this would be "sanitized" to foo\..\..\-blarr
and if we were ever to say "overWrite: true" (which I think I saw in this PR somewhere) then we would be writing to some unknown place.
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.
It's not valid name for NPM-package (@foo\..\..\/blarr
) so no need to worry about it.
@jakolehm your word?
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.
@Nokel81 has a point here.. I think it's possible for an attacker to handcraft a package with invalid name. Of course if user installs a malicious package then it can do even worse things via runtime ... so maybe not super critical to fix here.
Let's create a separate issue about this.
theme="round-black" | ||
iconLeft="link" | ||
placeholder={`URL to packed extension (${this.supportedFormats.join(", ")})`} | ||
validators={InputValidators.isUrl} |
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.
This validator is wrong, the string "123.com" is reported as invalid. Also it seems that even if the validator returns an error we can still click the "Add extensions" button
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.
Dunno, this is out of scope of this PR. This validator also matching to empty string which is also invalid.
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.
The validator being wrong is yes, but not being able to click "add extensions" if it returns an error
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.
So yeah, 123.com
is invalid URL, try new URL("123.com")
:D
Just prepend with http(s) and all good, lol.
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.
BTW you might want to fix this meanwhile in separated PR! :)
Signed-off-by: Roman <[email protected]>
Yes it should and I expected it to work, so you just found one more 🐞 Congrats! 🎉 (FIXED) |
Signed-off-by: Roman <[email protected]>
Signed-off-by: Roman <[email protected]>
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. I spotted few usability issues but they should be fixed in separate PRs (I'll open issues about those).
Done:
Clipboard
for copying html-element full text-content or partial into clipboardDropFileInput
to handle drag-n-dropped files from external app, e.g. Finder / other file explorer (logic adopted originally fromadd-cluster-page
)Todo:
~/.k8slens/extensions
requires #1461 #1482 (otherwise just restart the app after installation)
close #1227
Screenshots: