-
Notifications
You must be signed in to change notification settings - Fork 147
gitpenguin -- hide dangerous blocks by default #346
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
I like this but wouldn't it be better to give a warning or something for using those blocks? Maybe a label or a button that warns the user of it's dangers (of course I'm not the one accepting these pr's, I'm just throwing ideas out there) |
|
i'm guessing half the small children won't read a warning or anything, or not understand what it means |
SharkPool-SP
left a comment
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.
Id rather you use a warning button than hide these blocks.
GitHub offers decent security with their tokens. And if kids figure out how to setup a token they can probably read the warnings
|
Plus what if people want to use those blocks in a private project |
|
supposedly the kid that i'm referencing ignored a warning already, and i also think it's not a good idea for them to be in a private project because they might assume projects can't be unpackaged |
|
Still no, cuz there's the case of private projects that aren't packaged. And people could use these blocks and be smart enough to get their token externally Plus there are multiple other extensions that have fallbacks to having exposed data in their code, but you can't rly do anything about it. Just add a warning |
changed |
|
Not a fan of this wording. Will change later |
| { | ||
| blockType: Scratch.BlockType.BUTTON, | ||
| func: "showHiddenBlocks", | ||
| text: Scratch.translate("Show dangerous blocks using a github token") |
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.
"Show potentially dangerous blocks"
|
|
||
| showHiddenBlocks() { | ||
| alert(Scratch.translate("Anyone with your github token can access your account and create repositories, delete repositories, commit changes, and more, all while pretending to be you. You should not include your token in any capacity in a project shared with other people.")) | ||
| const response = prompt(Scratch.translate("To show these blocks, type \"I will not post a project with my github token anywhere and I understand people can impersonate me if I do\" (not case sensitive).")) |
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.
Merge these 2 into a 2-line prompt:
Anyone with your GitHub token can access your account and create repositories, delete repositories, commit changes, and more, all while pretending to be you. You should not include your token in any capacity in a project shared with other people.\nTo show these blocks, type ...
| alert(Scratch.translate("Anyone with your github token can access your account and create repositories, delete repositories, commit changes, and more, all while pretending to be you. You should not include your token in any capacity in a project shared with other people.")) | ||
| const response = prompt(Scratch.translate("To show these blocks, type \"I will not post a project with my github token anywhere and I understand people can impersonate me if I do\" (not case sensitive).")) | ||
|
|
||
| if (response.toLowerCase() === "i will not post a project with my github token anywhere and i understand people can impersonate me if i do") { |
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 "i will not post a project" string should be a constant that is used both here and in the above prompt so that it's easier to edit later.
|
|
||
| if (response.toLowerCase() === "i will not post a project with my github token anywhere and i understand people can impersonate me if i do") { | ||
| dangerousBlocksHidden = false; | ||
| Scratch.vm.extensionManager.refreshBlocks() |
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.
Make the first argument here that is the extension id, so only this extension gets refreshed.
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.
Note: This is in reference to PenguinMod/PenguinMod-Vm#133, which should probably get merged soon because it's a parity change with TurboWarp.
|
|
||
| showHiddenBlocks() { | ||
| alert(Scratch.translate("Anyone with your github token can access your account and create repositories, delete repositories, commit changes, and more, all while pretending to be you. You should not include your token in any capacity in a project shared with other people.")) | ||
| const response = prompt(Scratch.translate("To show these blocks, type \"I will not post a project with my github token anywhere and I understand people can impersonate me if I do\" (not case sensitive).")) |
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 amount needed to be typed should be far shorter. This is comically long.
"I understand the risks of these blocks"
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.
that's the point, while typing it out(or copying and pasting it) they need to read what it says
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.
They'll still have to read what it says if it's shorter, it just means they don't have to type as much.
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 risks" isn't very specific and they might just ignore what it's referencing
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.
You're popping up a massive popup, they have to read it to even know what to type. I'm pretty sure they'll know what "the risks" is a reference to.
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.
would "I understand these blocks could compromise my account" work
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.
Yes.
| }, | ||
| { | ||
| blockType: Scratch.BlockType.BUTTON, | ||
| func: "showHiddenBlocks", |
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.
Maybe rename to "requestUnhidingDangerousBlocks?"
|
|
||
| showHiddenBlocks() { | ||
| alert(Scratch.translate("Anyone with your github token can access your account and create repositories, delete repositories, commit changes, and more, all while pretending to be you. You should not include your token in any capacity in a project shared with other people.")) | ||
| const response = prompt(Scratch.translate("To show these blocks, type \"I will not post a project with my github token anywhere and I understand people can impersonate me if I do\" (not case sensitive).")) |
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.
Maybe use ScratchBlocks.prompt instead? It is a bit complicated to work with, so here I did the liberty of turning it into something that can replace the regular prompt function, as long as the function it gets used in is an async function:
var response = await new Promise((res, _) => ScratchBlocks.prompt("...", "", (answer) => { res(answer) }, "Show potentially dangerous blocks", "broadcast_msg"));broadcast_msg is used here to make sure it only shows the text field, and no radio buttons. Only thing that should be replaced is the "...", which should be replaced with the prompt string.
Steve0Greatness
left a comment
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.
Alright, looks good to me. You can mark my requested changes as resolved.
small children probably don't know what a token is, why it should be kept private, how to do that, or other basic things that would be needed for this to not be a bad idea.
this unlists the blocks that take a github token as an input because some kid posted a project with their token in it