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

Validate appID before encrypting #1072

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dinosaursrarr
Copy link
Contributor

We've all done it. It's too easy to use the wrong key to encrypt with, whether you use the app name, the folder name, or get the hyphens wrong. Cropped up again recently with #1071.

If you're running pixlet encrypt from within the community repo, this will add a check that the provided key matches the ID field from some existing manifest. If not, you get an error.

Thought about @jmanske's suggestions but I don't think pixlet can assume there'll be exactly one manifest in the commit to check against. You might be updating multiple apps, in which case, which should you check? Or you might be fixing a bad encryption, and not have any local edits. App names are generally unique enough that I think this works.

We've all done it. It's too easy to use the wrong key to encrypt with, whether you use the app name, the folder name, or get the hyphens wrong. Cropped up again recently with tidbyt#1071.

If you're running pixlet encrypt from within the community repo, this will add a check that the provided key matches the ID field from some existing manifest. If not, you get an error.

Thought about @jmanske's suggestions but I don't think pixlet can assume there'll be exactly one manifest in the commit to check against. You might be updating multiple apps, in which case, which should you check? Or you might be fixing a bad encryption, and not have any local edits. App names are generally unique enough that I think this works.
@rohansingh rohansingh self-requested a review May 7, 2024 20:35
@rohansingh
Copy link
Member

Looks good. I need to test it with private apps (which can also use encryption) to make sure it works there.

@dinosaursrarr
Copy link
Contributor Author

My other idea was to change the cli to expect a path to a manifest file, rather than an appID string. Then you know exactly where to extract the appID in any environment. The only reason I didn't do that was to avoid breaking anyone by changing the interface.

@jmanske
Copy link

jmanske commented May 16, 2024

We could also have a "single arg" version of encrypt that assumes you are in your app folder and errors if there is no manifest file. Read the manifest file and pull the id. This is far and away the main use case. Also helps with usability in private apps when the ID is some UUID.

Since min args right now is set to 2 this doesn't break the interface since this would be pure enhancement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants