Skip to content

Conversation

devin-ai-integration[bot]
Copy link
Contributor

What does this PR do?

This PR addresses a security issue where sensitive delegation credential fields were not being redacted from logs, potentially exposing private keys and other sensitive authentication data.

Changes:

  • Adds 5 missing sensitive fields to the SENSITIVE_FIELDS array in redactSensitiveData.ts:
    • private_key - RSA/EC private keys from service account credentials
    • encrypted_credentials - Encrypted credential data
    • tenant_id - Tenant identifiers that could be sensitive
    • client_email - Service account email addresses
    • serviceAccountKey - The entire service account key object
  • Adds comprehensive test cases covering both flat and nested delegation credential scenarios
  • Fixes TypeScript lint issue (replaced any with Record<string, unknown>)

Security Impact: Prevents logging of sensitive delegation credential data including private keys that could be used to impersonate service accounts.

Requested by: morgan@cal.com
Link to Devin run: https://app.devin.ai/sessions/384cd29a3e054233b6904c47b56251df

How should this be tested?

The changes include comprehensive unit tests that verify:

  1. Individual delegation credential fields are redacted when present in objects
  2. Nested delegation credential structures are properly redacted
  3. Normal fields remain untouched while sensitive fields are replaced with [REDACTED]

Additional testing recommendations:

  • Verify these fields are redacted in real delegation credential workflows
  • Check application logs to confirm no sensitive data is being logged after this change
  • Test edge cases where these fields might appear in different object structures

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox. N/A - Internal security improvement, no user-facing changes
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

Key Review Points

⚠️ Critical for reviewer:

  1. Field completeness: Verify all delegation credential sensitive fields are included - check if any were missed by searching the codebase for other sensitive patterns
  2. Security validation: Confirm each added field actually contains sensitive data that should be redacted
  3. Test coverage: Validate test cases cover the real-world scenarios where these fields appear in logs
  4. Integration impact: Consider if this change affects any existing logging/debugging workflows that might depend on seeing these fields

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have checked if my changes generate no new warnings
  • Tests pass locally with TZ=UTC yarn test packages/lib/redactSensitiveData.test.ts
  • Type checking passes with yarn type-check:ci --force

- Add private_key, encrypted_credentials, tenant_id, client_email, serviceAccountKey to SENSITIVE_FIELDS array
- Add comprehensive test cases for delegation credential sensitive fields
- Prevents logging of sensitive delegation credential data including private keys

Fixes security issue where delegation credential sensitive fields were not being redacted from logs

Co-Authored-By: morgan@cal.com <morgan@cal.com>
Copy link
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR that start with 'DevinAI'.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Copy link
Contributor

coderabbitai bot commented Sep 26, 2025

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


Comment @coderabbitai help to get the list of available commands and usage tips.

@ThyMinimalDev ThyMinimalDev marked this pull request as ready for review September 26, 2025 22:15
@graphite-app graphite-app bot requested a review from a team September 26, 2025 22:15
@dosubot dosubot bot added the automated-tests area: unit tests, e2e tests, playwright label Sep 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automated-tests area: unit tests, e2e tests, playwright ready-for-e2e size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant