Skip to content
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

RSA Extension #1699

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 89 additions & 0 deletions extensions/Themadpunter/rsa.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
// Name: RSA
// ID: themadpunter-rsa
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this id should be the same as the extension ID

// Description: Encrypt text and files using RSA, the world's best asymmetric encryption algorithm.
// By: Themadpunter <https://scratch.mit.edu/users/The_Mad_Punter/>
// License: MIT

(function(Scratch) {
'use strict';

// Load the JSEncrypt library from a CDN
const script = document.createElement('script');
script.src = 'https://cdn.jsdelivr.net/npm/jsencrypt/bin/jsencrypt.min.js';
document.head.appendChild(script);
Comment on lines +10 to +13
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldnt be fetching a file url like that as:

  • this forces people to need internet for this extension to work
  • url could go down at any point

You should use a data.uri instead.
Also add /* global [library name here] */ to fix the lint errors

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok being real here, jsdelivr isnt going down any time soon, and no they should find a way to import the library without using a script tag.

Copy link
Collaborator

@CST1229 CST1229 Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the desktop app still needs to work without internet (this is why the extension library there can only update when the app does, because it's stored offline).
for importing, i think using await import() and making the extension function async is a simple option (and probably also add /+esm to the jsdelivr url so that you can access the library from import()'s return value without adding stuff to the global scope), but you should still make the extension url a data url because of the offline part

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe yeah but that does bring the question is it allowed because stuff like a script tag isnt allowed and they do similar things


class RSAExtension {
getInfo() {
return {
id: 'themadpunter_rsa',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_'s should not be used in ids

name: 'TurboRSA',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this name should be the same as the one listed on the gallery which you put as RSA

color1: '#FF0000', // Primary color (red)
color2: '#CC0000', // Secondary color (darker red)
blocks: [
{
opcode: 'generateKeys',
blockType: Scratch.BlockType.REPORTER,
text: 'Generate RSA keys',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: please make the block titles lowercase so "generate RSA keys" as currently it does not follow scratch's conventions, also blocks are not sentence's so yeah

arguments: {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary key

},
{
opcode: 'encrypt',
blockType: Scratch.BlockType.REPORTER,
text: 'Encrypt [TEXT] with public key [KEY]',
arguments: {
TEXT: {
type: Scratch.ArgumentType.STRING,
defaultValue: 'Hello, world!'
},
KEY: {
type: Scratch.ArgumentType.STRING,
defaultValue: ''
}
}
},
{
opcode: 'decrypt',
blockType: Scratch.BlockType.REPORTER,
text: 'Decrypt [TEXT] with private key [KEY]',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here as the other name comment

arguments: {
TEXT: {
type: Scratch.ArgumentType.STRING,
defaultValue: ''
},
KEY: {
type: Scratch.ArgumentType.STRING,
defaultValue: ''
}
}
}
]
};
}

generateKeys() {
const crypt = new JSEncrypt({ default_key_size: 1024 });

Check failure on line 64 in extensions/Themadpunter/rsa.js

View workflow job for this annotation

GitHub Actions / lint

'JSEncrypt' is not defined
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: this could be customizable

const publicKey = crypt.getPublicKeyB64();
const privateKey = crypt.getPrivateKeyB64();
return JSON.stringify({ publicKey, privateKey });
}

encrypt(args) {
const crypt = new JSEncrypt();

Check failure on line 71 in extensions/Themadpunter/rsa.js

View workflow job for this annotation

GitHub Actions / lint

'JSEncrypt' is not defined
crypt.setPublicKey(args.KEY);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please cast your values

const encrypted = crypt.encrypt(args.TEXT);
return encrypted ? encrypted : 'Encryption failed';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of returning text which could be the same as the actual output value you should just return an empty string

}

decrypt(args) {
const crypt = new JSEncrypt();

Check failure on line 78 in extensions/Themadpunter/rsa.js

View workflow job for this annotation

GitHub Actions / lint

'JSEncrypt' is not defined
crypt.setPrivateKey(args.KEY);
const decrypted = crypt.decrypt(args.TEXT);
return decrypted ? decrypted : 'Decryption failed';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same goes here for the returning an empty string

}
}

// Wait for the script to load before registering the extension
script.onload = () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after doing what the other reviews suggests (removing the script tag) just remove this

Scratch.extensions.register(new RSAExtension());
};
})(Scratch);
Loading