Opportunistic Token Encryption for Slack Actions#723
Opportunistic Token Encryption for Slack Actions#723jeffrey-martinez wants to merge 6 commits intomasterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the security posture of the Slack action by introducing opportunistic encryption for OAuth tokens. Previously, sensitive Slack user tokens were stored in plain text, posing a data-at-rest exposure risk. The implemented changes encrypt these tokens when the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces opportunistic encryption for Slack action tokens, which is a great security enhancement. The implementation correctly handles both encrypted and plaintext states for backward compatibility. The new encryption and decryption logic is well-contained.
I've identified a critical issue in the new tests where the state_json format is incorrect, which could mask bugs in the single-workspace scenario. Additionally, I've suggested a few refactoring opportunities to reduce code duplication and improve efficiency.
src/actions/slack/slack.ts
Outdated
| if (!request.params.state_json) { | ||
| return undefined | ||
| } | ||
| if (!request.params.state_json.startsWith("1")) { |
There was a problem hiding this comment.
what is this checking for?
There was a problem hiding this comment.
Fixed, this was an outdated way of determining if the payload was encrypted (the cid would start with "1").
| decryptedState: string | undefined, | ||
| originalState: string | undefined, | ||
| ) { | ||
| if (decryptedState && decryptedState === originalState && process.env.ENCRYPT_PAYLOAD_SLACK_APP === "true") { |
There was a problem hiding this comment.
if we decrypted the state then checked against request.params.state_json isn't that going to be encrypted?
There was a problem hiding this comment.
Yes we check if it matches the original (unencrypted case) then we know to migrate them to use encryption.
2 participants
PR 723: [feat/slack-token-encryption]:
This PR extends the Action Hub OAuth token encryption framework to the Slack action to mitigate data-at-rest exposure vulnerabilities in Looker's database.
Context and Vulnerability Addressed
While the Slack action utilizes an integration proxy (mitigating callback URL hijacking attacks described in ), its user tokens are still stored in plain-text within the state_json parameter.
To align with security best practices and ensure defense-in-depth, this PR implements opportunistic encryption for Slack tokens at rest.
Changes
🔐 Opportunistic Encryption Logic
Modifies
SlackActionandSlackClientManagerto handle tokens securely without breaking legacy states:Write Path (Encryption):
If the feature flag
ENCRYPT_PAYLOAD_SLACK_APPis enabled, Action Hub encrypts plain-text tokens using transit keys (AESTransitCrypto) before returning them to Looker as state data.Read Path (Decryption):
Action Hub attempts to decrypt incoming
state_json. If it's encrypted (verified by standard prefix matching), it is decrypted. If not, it falls back gracefully to plain text for backwards compatibility.📝 Key Files Modified
slack.ts: Introduced private helper methodsdecryptStateIfNeededandencryptStateJsontoggled by feature flags.slack_client_manager.ts: Updated client initialization to expect and utilize the decrypted payloads.test_slack.ts: Added specific unit assertions verifying that plain-state translates to encrypted state accurately, and that encrypted states feed cleanly into downstream clients.Verification & Impact
Backward Compatibility Check: Passive behaviors preserved; legacy plain-text state parameters resolve successfully.