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

fix: throw when privateDecrypt fails for input secrets #399

Merged
merged 6 commits into from
Jul 12, 2023

Conversation

jirimoravcik
Copy link
Member

@jirimoravcik jirimoravcik commented Jul 4, 2023

Comments regarding the error text message are welcome.

@github-actions github-actions bot added this to the 67th sprint - Platform team milestone Jul 4, 2023
@github-actions github-actions bot added the t-platform Issues with this label are in the ownership of the platform team. label Jul 4, 2023
@jirimoravcik jirimoravcik changed the title Fix/throw error on decrypt failure fix: throw when privateDecrypt fails for input secrets Jul 4, 2023
@jirimoravcik jirimoravcik requested review from drobnikj and valekjo July 4, 2023 09:56
Copy link
Member

@drobnikj drobnikj left a comment

Choose a reason for hiding this comment

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

Is swallowing all errors a good idea? It can happen that changing input cannot help.
There is some known error what you can get from malformed input or decrypt key, see tests for ideas.

What about at least log the original error? Otherwise, it is very hard to debug in case of any unexpected issue.

try {
decryptedInput[key] = privateDecrypt({ privateKey, encryptedPassword, encryptedValue });
} catch {
throw new Error(`The input field "${key}" could not be decrypted. You can fix this by updating the field's value in the input editor.`);
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a test that expects the decrypt to throw if wrong privateKey is provided.

try {
decryptedInput[key] = privateDecrypt({ privateKey, encryptedPassword, encryptedValue });
} catch {
throw new Error(`The input field "${key}" could not be decrypted. You can fix this by updating the field's value in the input editor.`);
Copy link
Member

Choose a reason for hiding this comment

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

To get around the swallowing of the original error, maybe we could add the original error as cause to the new one, so it would be accessible?

And for the message I'd go with something like this - not saying it fixes it, just that it might, and giving some more details.

The input field "${key}" could not be decrypted. Try updating the field's value in the input editor. Decryption error: ${err}

@github-actions github-actions bot added the tested Temporary label used only programatically for some analytics. label Jul 10, 2023
@jirimoravcik
Copy link
Member Author

jirimoravcik commented Jul 10, 2023

I changed the message so now it is The input field "${key}" could not be decrypted. Try updating the field's value in the input editor. Decryption error: ${err}. I also added a test that checks that it throws with incorrect private key

@jirimoravcik jirimoravcik requested review from drobnikj and valekjo July 10, 2023 15:44
test/input_secrets.test.ts Outdated Show resolved Hide resolved
@jirimoravcik jirimoravcik requested a review from drobnikj July 11, 2023 09:09
Copy link
Member

@valekjo valekjo left a comment

Choose a reason for hiding this comment

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

👍

@jirimoravcik jirimoravcik merged commit 0f439a1 into master Jul 12, 2023
@jirimoravcik jirimoravcik deleted the fix/throw-error-on-decrypt-failure branch July 12, 2023 07:46
@fnesveda fnesveda added the validated Issues that are resolved and their solutions fulfill the acceptance criteria. label Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t-platform Issues with this label are in the ownership of the platform team. tested Temporary label used only programatically for some analytics. validated Issues that are resolved and their solutions fulfill the acceptance criteria.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants