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

refactor: decrypt PK when it is used #858

Open
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

rndquu
Copy link
Member

@rndquu rndquu commented Oct 13, 2023

This PR refactors the code so that partner's wallet private key is decrypted only when the payment permit is about to be generated.

Rationale

We're about to expose the bot's logs to the public. Right now partners' wallets private keys are decrypted on github webhook event. It is pretty easy to leak those PKs via smlth like logger.info(JSON.stringify(bot.config)). So this PR makes sure that partners' PKs are encrypted in the initial bot config and decrypted only when necessary (i.e. before the permit generation).

QA issue run with the bot instance from the current PR's branch: rndquu-org/test-repo#48

@netlify
Copy link

netlify bot commented Oct 13, 2023

Deploy Preview for ubiquibot-staging ready!

Name Link
🔨 Latest commit 9c58746
🔍 Latest deploy log https://app.netlify.com/sites/ubiquibot-staging/deploys/652dbc4b88ad590008f3ffb0
😎 Deploy Preview https://deploy-preview-858--ubiquibot-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@rndquu rndquu marked this pull request as ready for review October 13, 2023 08:08
wannacfuture
wannacfuture previously approved these changes Oct 13, 2023
whilefoo
whilefoo previously approved these changes Oct 13, 2023
Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

I

src/bindings/config.ts Outdated Show resolved Hide resolved
0xcodercrane
0xcodercrane previously approved these changes Oct 16, 2023
Copy link
Collaborator

@whilefoo whilefoo left a comment

Choose a reason for hiding this comment

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

otherwise looks good

@@ -60,7 +60,7 @@ export const loadConfig = async (context: Context): Promise<BotConfig> => {
payout: {
networkId: networkId,
rpc: rpc,
privateKey: privateKey,
privateKeyEncrypted: privateKeyEncrypted || null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

privateKeyEncrypted is already string | undefined, so I think converting it to string | null is unnecessary step, but not a big deal.
It would also simplify Type.Union([Type.Null(), Type.String()]) into Type.Optional(Type.String())

Copy link
Member

Choose a reason for hiding this comment

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

null is more expressive that it is intentionally set to a void value instead of some type of mistake happening.

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.

None yet

5 participants