Implement AES-256-GCM encryption, secure initialization, and improve SkyFi tool robustness#751
Conversation
…SkyFi tool robustness - Migrate encryption/decryption from AES-256-CBC to AES-256-GCM with integrity tag verification and strict ENCRYPTION_KEY environment variable initialization checks. - Add deterministic serialize helper for extraParams and orderId when deriving stableIdempotencyKey in skyfiTool. - Correctly apply idempotencyKey after spreading extraParams to prevent user override. - Destructure orderId and include in mcpArgs for list_orders, validate_order, and place_order. - Support AbortSignal passing in both skyfi MCP client tool calls. - Sanitize tool logging by omitting location and aoi values. - Call closeClient on invalid-AOI early-return path. - Redirect unauthenticated users to /sign-in in startSkyfiConnection. - Add unit tests under tests-unit/encryption.test.ts. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
I found two blocking compatibility/deployment issues in this head:
- P1 – AES format migration drops existing SkyFi credentials.
decrypt()now rejects every legacyiv:ciphertextvalue, butSkyfiOAuthProviderreadsaccessToken,refreshToken,clientSecret, andregistrationAccessTokenfrom the existingskyfi_oauth_tokenstable through this function. Existing rows therefore become unreadable after rollout and users are treated as disconnected. Please support a legacy CBC read path and re-encrypt on successful read (or provide a migration that runs before switching formats), while preserving the deployed key. - P1 – the required key is not wired into the checked-in deployment paths. The new import-time throw makes SkyFi modules fail whenever
ENCRYPTION_KEYis absent, but.env.local.example,docker-compose.yaml, andCLOUD_DEPLOYMENT.mddo not set or document it. Add the secret to every supported runtime/deploy path and document key generation/rotation, or add a deploy-time validation that prevents an invalid deployment.
| try { | ||
| const [ivHex, encrypted] = encryptedText.split(':'); | ||
| if (!ivHex || !encrypted) return null; | ||
| const parts = encryptedText.split(':'); |
There was a problem hiding this comment.
[P1] Preserve legacy ciphertexts during the format migration. Before this change, persisted SkyFi secrets were serialized as iv:ciphertext with AES-256-CBC; this parser now returns null unless there are exactly three parts. SkyfiOAuthProvider decrypts existing accessToken, refreshToken, clientSecret, and registrationAccessToken rows through this function, so every existing connection becomes unusable after deploy. Add a legacy CBC read/re-encrypt path or a pre-deploy migration before switching formats, and preserve the deployed key.
| const ENCRYPTION_KEY = process.env.ENCRYPTION_KEY || 'default_32_byte_secret_key_placeholder_for_local_dev'; // Must be 32 bytes | ||
| const ALGORITHM = 'aes-256-gcm'; | ||
|
|
||
| if (!process.env.ENCRYPTION_KEY) { |
There was a problem hiding this comment.
[P1] Ensure the new required secret is supplied by supported deployments. This top-level throw makes routes importing SkyfiOAuthProvider fail when ENCRYPTION_KEY is missing. The checked-in .env.local.example, docker-compose.yaml, and CLOUD_DEPLOYMENT.md omit this variable, so documented local/container/cloud setups can no longer use SkyFi. Add the variable/secret to those paths (or a deploy-time validation that prevents an invalid deployment) and document key generation/rotation.
|
The skyFI link does not open skyfi |
I've addressed this by making the redirect URI resolution for SkyFi fully dynamic and robust. Both |
- Update startSkyfiConnection() and skyfiTool to dynamically resolve redirectUri from the incoming request headers using next/headers, making it highly robust against mismatches in localhost, preview, or production domains. - Ensure type safety and compilability are maintained. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
|
|
|
The skyfi auth link still does not open skyfi |
I've identified the root cause of this: pre-existing database credentials (like client secrets and refresh tokens) were encrypted using the old |
- Enable decrypt() to seamlessly detect and decrypt legacy 2-part AES-256-CBC payloads, resolving issues where pre-existing tokens could not be decrypted. - Add unit tests for legacy CBC backwards-compatibility. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
|
The skyfi MCP is still not opening skyfi |
I've identified the root cause of this: previously registered client IDs with SkyFi were bound to a specific redirect URI (e.g., from an old dev domain, or localhost vs preview/production). When the host or domain changed, the stored |
- Support forceRegister in ensureClientRegistered(). - Force client re-registration in startSkyfiConnection() to guarantee that client ID redirect URIs always match the current domain dynamically, preventing SkyFi authorization redirect_uri mismatch errors. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
974fc40
into
jules-5330974344690781074-fa3b2eb8
2 participants
This PR addresses security, reliability, and telemetry-related improvements to the planetary copilot system:
PR created automatically by Jules for task 10643214369324289359 started by @ngoiyaeric