Skip to content

πŸ›‘οΈ Sentinel: Fix sensitive data leakage in error responses#20

Open
AGI-Corporation wants to merge 3 commits intomainfrom
fix/sensitive-data-leakage-in-errors-7009615417473966508
Open

πŸ›‘οΈ Sentinel: Fix sensitive data leakage in error responses#20
AGI-Corporation wants to merge 3 commits intomainfrom
fix/sensitive-data-leakage-in-errors-7009615417473966508

Conversation

@AGI-Corporation
Copy link
Copy Markdown
Owner

@AGI-Corporation AGI-Corporation commented Mar 19, 2026

🚨 Severity: HIGH

πŸ’‘ Vulnerability: Sensitive Data Leakage in Error Responses

🎯 Impact: JWT tokens and License Keys were being included in the params of ActivepiecesError, which the global backend error handler serializes directly into API responses. This could lead to credential exposure in logs or to unauthorized parties.

πŸ”§ Fix:

  • Updated InvalidJwtTokenErrorParams and InvalidLicenseKeyParams in packages/shared/src/lib/common/activepieces-error.ts to exclude sensitive fields.
  • Updated calls to ActivepiecesError in connection-key.service.ts and license-keys-controller.ts to remove the sensitive values from parameters.

βœ… Verification:

  • Verified that npx eslint passes for the modified files.
  • Confirmed with read_file that the changes were correctly applied.
  • Ensured no unwanted pnpm-lock.yaml file is included in the PR.

PR created automatically by Jules for task 7009615417473966508 started by @AGI-Corporation

Summary by CodeRabbit

  • Bug Fixes

    • Error responses no longer include sensitive authentication tokens or license keys in error details.
    • Redirect flow now targets a specific origin (instead of a wildcard) and fixes a success message typo.
  • Documentation

    • Added guidance documenting the sensitive-data-in-errors issue and prevention recommendations.

Remove JWT tokens and License Keys from `ActivepiecesError` parameters to prevent them from being serialized and sent to the client via the global error handler.

Modified Files:
- `packages/shared/src/lib/common/activepieces-error.ts`: Update error parameter types.
- `packages/server/api/src/app/ee/connection-keys/connection-key.service.ts`: Update error usage.
- `packages/server/api/src/app/ee/license-keys/license-keys-controller.ts`: Update error usage.

Co-authored-by: AGI-Corporation <186229839+AGI-Corporation@users.noreply.github.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 19, 2026

πŸ“ Walkthrough

Walkthrough

This PR removes sensitive JWT tokens and license keys from error payloads by changing error param types to empty records and updating places that populated errors to emit empty params objects; also adds documentation and tweaks redirect origin resolution.

Changes

Cohort / File(s) Summary
Documentation
.jules/sentinel.md
Added sentinel entry documenting a sensitive-data leakage in error params and guidance to avoid sensitive fields in error parameter types.
Shared error types
packages/shared/src/lib/common/activepieces-error.ts
Changed InvalidJwtTokenErrorParams and InvalidLicenseKeyParams to Record<string, never> (removed { token } and { key } fields).
Server: error emission
packages/server/api/src/app/ee/connection-keys/connection-key.service.ts, packages/server/api/src/app/ee/license-keys/license-keys-controller.ts
Replaced error params that previously included JWT token or licenseKey values with empty params: {} when throwing ActivepiecesError.
Server: redirect origin
packages/server/api/src/app/app.ts
Added platformUtils usage to derive platformId and compute targetOrigin via domainHelper.getPublicUrl({ platformId }); fixed a typo in the success message.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I sniffed the logs and hopped with care,

no token crumbs left in the air.
Empty pockets, secrets kept,
A tidy trail where leeks once leapt.
Hooray β€” no keys for foxes to share! πŸ₯•βœ¨

πŸš₯ Pre-merge checks | βœ… 3
βœ… Passed checks (3 passed)
Check name Status Explanation
Title check βœ… Passed The title clearly and specifically describes the main change: fixing sensitive data leakage in error responses, which matches the primary objective of removing JWT tokens and license keys from error parameters.
Description check βœ… Passed The description clearly explains what the PR does, the vulnerability being fixed, the implementation details, and verification steps, but does not follow the provided template structure with 'What does this PR do?' and 'Explain How the Feature Works' sections.
Docstring Coverage βœ… Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
πŸ“ Generate docstrings
  • Create stacked PR
  • Commit on current branch
πŸ§ͺ Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/sensitive-data-leakage-in-errors-7009615417473966508
πŸ“ Coding Plan
  • Generate coding plan for human review comments

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.

❀️ Share

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

Remove JWT tokens and License Keys from `ActivepiecesError` parameters to prevent them from being serialized and sent to the client via the global error handler.

Modified Files:
- `packages/shared/src/lib/common/activepieces-error.ts`: Update error parameter types.
- `packages/server/api/src/app/ee/connection-keys/connection-key.service.ts`: Update error usage.
- `packages/server/api/src/app/ee/license-keys/license-keys-controller.ts`: Update error usage.

Co-authored-by: AGI-Corporation <186229839+AGI-Corporation@users.noreply.github.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

πŸ€– Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.jules/sentinel.md:
- Line 1: Update the incident note header that currently reads "## 2025-03-19 -
Sensitive Data Leakage in Error Parameters" to the correct PR date (replace
2025-03-19 with 2026-03-19) so the sentinel entry's timestamp matches the change
set; ensure the header text "Sensitive Data Leakage in Error Parameters" remains
unchanged.

ℹ️ Review info
βš™οΈ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 593c2d8e-4450-4f67-8624-ebf0f357e817

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between f61e720 and ddf0875.

πŸ“’ Files selected for processing (4)
  • .jules/sentinel.md
  • packages/server/api/src/app/ee/connection-keys/connection-key.service.ts
  • packages/server/api/src/app/ee/license-keys/license-keys-controller.ts
  • packages/shared/src/lib/common/activepieces-error.ts

@@ -0,0 +1,4 @@
## 2025-03-19 - Sensitive Data Leakage in Error Parameters
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟑 Minor

Fix the incident note date to match this PR timeline.

Line 1 says 2025-03-19, but this change set is from March 19, 2026. Keeping the exact date accurate matters for security audit trails.

πŸ“ Suggested patch
-## 2025-03-19 - Sensitive Data Leakage in Error Parameters
+## 2026-03-19 - Sensitive Data Leakage in Error Parameters
πŸ“ Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
## 2025-03-19 - Sensitive Data Leakage in Error Parameters
## 2026-03-19 - Sensitive Data Leakage in Error Parameters
πŸ€– Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.jules/sentinel.md at line 1, Update the incident note header that currently
reads "## 2025-03-19 - Sensitive Data Leakage in Error Parameters" to the
correct PR date (replace 2025-03-19 with 2026-03-19) so the sentinel entry's
timestamp matches the change set; ensure the header text "Sensitive Data Leakage
in Error Parameters" remains unchanged.

- Remove JWT tokens and License Keys from `ActivepiecesError` parameters to prevent serialization in API responses.
- Restrict `postMessage` target origin in `/redirect` endpoint from `*` to the platform-specific origin for secure OAuth code transmission.
- Fix import order in `app.ts` to satisfy linting rules.

Modified Files:
- `packages/shared/src/lib/common/activepieces-error.ts`
- `packages/server/api/src/app/ee/connection-keys/connection-key.service.ts`
- `packages/server/api/src/app/ee/license-keys/license-keys-controller.ts`
- `packages/server/api/src/app/app.ts`
- `.jules/sentinel.md` (new)

Co-authored-by: AGI-Corporation <186229839+AGI-Corporation@users.noreply.github.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
packages/server/api/src/app/app.ts (1)

254-255: Guard origin resolution to avoid 500s in redirect flow.
Line 254 and Line 255 can throw (platform lookup / URL parsing). In that case, /redirect fails hard instead of returning a controlled response.

Proposed hardening
-                const platformId = await platformUtils.getPlatformIdForRequest(request)
-                const targetOrigin = new URL(await domainHelper.getPublicUrl({ platformId })).origin
-                return reply
-                    .type('text/html')
-                    .send(
-                        `<script>if(window.opener){window.opener.postMessage({ 'code': '${encodeURIComponent(
-                            params.code,
-                        )}' }, '${targetOrigin}')}</script> <html>Redirect successfully, this window should close now</html>`,
-                    )
+                try {
+                    const platformId = await platformUtils.getPlatformIdForRequest(request)
+                    const publicUrl = await domainHelper.getPublicUrl({ platformId })
+                    const targetOrigin = new URL(publicUrl).origin
+                    return reply
+                        .type('text/html')
+                        .send(
+                            `<script>if(window.opener){window.opener.postMessage({ 'code': '${encodeURIComponent(
+                                params.code,
+                            )}' }, '${targetOrigin}')}</script> <html>Redirect successfully, this window should close now</html>`,
+                        )
+                }
+                catch (error) {
+                    request.log.warn({ error }, 'Failed to resolve redirect target origin')
+                    return reply.code(400).send('Unable to resolve redirect origin')
+                }

Also applies to: 261-261

πŸ€– Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/server/api/src/app/app.ts` around lines 254 - 255, The platform
lookup and URL parsing calls (platformUtils.getPlatformIdForRequest and
domainHelper.getPublicUrl, used to compute targetOrigin) can throw and currently
cause a 500 in the /redirect flow; wrap the platformId retrieval and URL parsing
in a try/catch, log the error, and return a controlled response from the
redirect handler (e.g., a safe redirect or a 4xx/302 fallback) instead of
allowing the exception to propagate; apply the same guard around the later usage
at the block referencing the same domainHelper.getPublicUrl call (the code
around line 261) so targetOrigin resolution failures are consistently handled.
πŸ€– Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/server/api/src/app/app.ts`:
- Around line 254-255: The platform lookup and URL parsing calls
(platformUtils.getPlatformIdForRequest and domainHelper.getPublicUrl, used to
compute targetOrigin) can throw and currently cause a 500 in the /redirect flow;
wrap the platformId retrieval and URL parsing in a try/catch, log the error, and
return a controlled response from the redirect handler (e.g., a safe redirect or
a 4xx/302 fallback) instead of allowing the exception to propagate; apply the
same guard around the later usage at the block referencing the same
domainHelper.getPublicUrl call (the code around line 261) so targetOrigin
resolution failures are consistently handled.

ℹ️ Review info
βš™οΈ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: bdc16a0b-046f-4a05-b2d7-56d145db4948

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between ddf0875 and b058d36.

πŸ“’ Files selected for processing (1)
  • packages/server/api/src/app/app.ts

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.

1 participant