Skip to content

FIX - Clean OpenMetadataWorkflowConfig in TestConnection Workflow#26762

Merged
yan-3005 merged 3 commits intomainfrom
ram/fix-test-conection-pw-encrypt
Mar 25, 2026
Merged

FIX - Clean OpenMetadataWorkflowConfig in TestConnection Workflow#26762
yan-3005 merged 3 commits intomainfrom
ram/fix-test-conection-pw-encrypt

Conversation

@yan-3005
Copy link
Copy Markdown
Contributor

SM is already encrypting responses. This adds an extra layer of security by not returning unnecessary workflow info (openMetadataServerConnection with the ingestion-bot JWT token) in API responses.

The token is only passed Server -> Pipeline Service Client during trigger, never Server -> user.

Describe your changes:

Fixes

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

SM is already encrypting responses. This adds an extra layer of security
by not returning unnecessary workflow info (openMetadataServerConnection
with the ingestion-bot JWT token) in API responses.

The token is only passed Server -> Pipeline Service Client during trigger,
never Server -> user.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@yan-3005 yan-3005 self-assigned this Mar 25, 2026
Copilot AI review requested due to automatic review settings March 25, 2026 09:17
@yan-3005 yan-3005 added safe to test Add this label to run secure Github workflows on PRs To release Will cherry-pick this PR into the release branch backend labels Mar 25, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR prevents exposing openMetadataServerConnection (which can contain the ingestion-bot JWT) in Workflow API responses, ensuring the token is only used server-side when triggering the pipeline/automation runner.

Changes:

  • Stop populating openMetadataServerConnection in decrypted Workflow responses (set to null).
  • Update existing workflow creation IT to assert the connection is not returned.
  • Add integration tests to ensure GET/LIST/PUT/VERSION Workflow endpoints do not expose openMetadataServerConnection.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
openmetadata-service/src/main/java/org/openmetadata/service/resources/automations/WorkflowResource.java Nullifies openMetadataServerConnection in API responses after decrypting workflow secrets.
openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/WorkflowResourceIT.java Adds/updates IT assertions to validate workflows never return openMetadataServerConnection across endpoints.

pmbrull
pmbrull previously approved these changes Mar 25, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 25, 2026

OpenMetadata Service New-Code Coverage

PASS. Required changed-line coverage: 90.00% overall and per touched production file.

  • Overall executable changed lines: 1/1 covered (100.00%)
  • Missed executable changed lines: 0
  • Non-executable changed lines ignored by JaCoCo: 0
  • Changed production files: 1
File Covered Missed Executable Non-exec Coverage Uncovered lines
openmetadata-service/src/main/java/org/openmetadata/service/resources/automations/WorkflowResource.java 1 0 1 0 100.00% -

Only changed executable lines under openmetadata-service/src/main/java are counted. Test files, comments, imports, and non-executable lines are excluded.

…ponses

Covers line 617 - verifies decryptOrNullify nullifies openMetadataServerConnection
to prevent ingestion-bot JWT token leakage in API responses.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 25, 2026 09:43
@gitar-bot
Copy link
Copy Markdown

gitar-bot bot commented Mar 25, 2026

Code Review 👍 Approved with suggestions 0 resolved / 1 findings

Fixes TestConnection workflow to clean sensitive OpenMetadataWorkflowConfig data and adds null guards for list responses with comprehensive unit tests. Consider extending the async delete endpoint to also nullify openMetadataServerConnection for consistency.

💡 Security: Async delete endpoint may still return openMetadataServerConnection

📄 openmetadata-service/src/main/java/org/openmetadata/service/resources/automations/WorkflowResource.java:518-530

The PR successfully nullifies openMetadataServerConnection in the decryptOrNullify() method (line 617), which covers most endpoints. However, the deleteByIdAsync() endpoint at line 518-530 delegates directly to the parent class method without going through decryptOrNullify(), meaning a DELETE to /workflows/async/{id} could still return the workflow object with the JWT token.

This is a pre-existing gap (not introduced by this PR), but since the PR's goal is to comprehensively prevent JWT token exposure, it's worth addressing. Consider adding a test for this endpoint similar to the other security tests added, and ensuring it also goes through the nullification path.

🤖 Prompt for agents
Code Review: Fixes TestConnection workflow to clean sensitive OpenMetadataWorkflowConfig data and adds null guards for list responses with comprehensive unit tests. Consider extending the async delete endpoint to also nullify openMetadataServerConnection for consistency.

1. 💡 Security: Async delete endpoint may still return openMetadataServerConnection
   Files: openmetadata-service/src/main/java/org/openmetadata/service/resources/automations/WorkflowResource.java:518-530

   The PR successfully nullifies `openMetadataServerConnection` in the `decryptOrNullify()` method (line 617), which covers most endpoints. However, the `deleteByIdAsync()` endpoint at line 518-530 delegates directly to the parent class method without going through `decryptOrNullify()`, meaning a DELETE to `/workflows/async/{id}` could still return the workflow object with the JWT token.
   
   This is a pre-existing gap (not introduced by this PR), but since the PR's goal is to comprehensively prevent JWT token exposure, it's worth addressing. Consider adding a test for this endpoint similar to the other security tests added, and ensuring it also goes through the nullification path.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

@yan-3005
Copy link
Copy Markdown
Contributor Author

@Gitar Thanks for the review! However, deleteByIdAsync does not return the Workflow entity — it returns Response.accepted() wrapping a DeleteEntityResponse, which contains only jobId, a status message, the entity name, and the delete flags. The Workflow object (with openMetadataServerConnection) is never part of that response, so there is no JWT exposure risk on this endpoint.

@gitar-bot
Copy link
Copy Markdown

gitar-bot bot commented Mar 25, 2026

@Gitar Thanks for the review! However, deleteByIdAsync does not return the Workflow entity — it returns Response.accepted() wrapping a DeleteEntityResponse, which contains only jobId, a status message, the entity name, and the delete flags. The Workflow object (with openMetadataServerConnection) is never part of that response, so there is no JWT exposure risk on this endpoint.

You're right — after looking at the code more carefully, deleteByIdAsync in EntityResource.java returns Response.accepted() wrapping a DeleteEntityResponse that only contains jobId, a status message, the entity name, and the delete flags. The Workflow object (and its openMetadataServerConnection) is never serialized into that response. I'll dismiss that finding.

@sonarqubecloud
Copy link
Copy Markdown

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 25, 2026

🟡 Playwright Results — all passed (28 flaky)

✅ 3403 passed · ❌ 0 failed · 🟡 28 flaky · ⏭️ 209 skipped

Shard Passed Failed Flaky Skipped
🟡 Shard 1 451 0 4 2
🟡 Shard 2 600 0 5 32
🟡 Shard 3 612 0 3 28
🟡 Shard 4 594 0 9 47
🟡 Shard 5 586 0 1 67
🟡 Shard 6 560 0 6 33
🟡 28 flaky test(s) (passed on retry)
  • Features/DataAssetRulesEnabled.spec.ts › should enforce single domain selection for glossary term when entity rules are enabled (shard 1, 1 retry)
  • Flow/Metric.spec.ts › Verify Related Metrics Update (shard 1, 1 retry)
  • Flow/Tour.spec.ts › Tour should work from URL directly (shard 1, 1 retry)
  • Pages/UserCreationWithPersona.spec.ts › Create user with persona and verify on profile (shard 1, 1 retry)
  • Features/BulkEditEntity.spec.ts › Glossary (shard 2, 1 retry)
  • Features/DataQuality/TestCaseImportExportE2eFlow.spec.ts › Admin: Complete export-import-validate flow (shard 2, 1 retry)
  • Features/DataQuality/TestCaseIncidentPermissions.spec.ts › User with only VIEW cannot PATCH incidents (shard 2, 1 retry)
  • Features/DataQuality/TestCaseResultPermissions.spec.ts › User with only VIEW cannot PATCH results (shard 2, 1 retry)
  • Features/Glossary/GlossaryAdvancedOperations.spec.ts › should remove reviewer from term (shard 2, 1 retry)
  • Features/Permissions/GlossaryPermissions.spec.ts › Team-based permissions work correctly (shard 3, 1 retry)
  • Features/RestoreEntityInheritedFields.spec.ts › Validate restore with Inherited domain and data products assigned (shard 3, 1 retry)
  • Flow/ExploreDiscovery.spec.ts › Should display deleted assets when showDeleted is checked and deleted is not present in queryFilter (shard 3, 1 retry)
  • Pages/Customproperties-part2.spec.ts › Entity Reference List (shard 4, 1 retry)
  • Pages/Customproperties-part2.spec.ts › entityReferenceList shows item count, scrollable list, no expand toggle (shard 4, 1 retry)
  • Pages/DataContractsSemanticRules.spec.ts › Validate Description Rule Is_Set (shard 4, 1 retry)
  • Pages/DomainDataProductsRightPanel.spec.ts › Should display data product name link in panel in domain context (shard 4, 1 retry)
  • Pages/Domains.spec.ts › Data Product announcement create, edit & delete (shard 4, 1 retry)
  • Pages/DomainUIInteractions.spec.ts › Select domain from global dropdown filters explore (shard 4, 1 retry)
  • Pages/Entity.spec.ts › Certification Add Remove (shard 4, 1 retry)
  • Pages/Entity.spec.ts › Tag Add, Update and Remove (shard 4, 1 retry)
  • Pages/Entity.spec.ts › Glossary Term Add, Update and Remove (shard 4, 1 retry)
  • Pages/ExplorePageRightPanel.spec.ts › Should verify deleted user not visible in owner selection for pipeline (shard 5, 1 retry)
  • Pages/ODCSImportExport.spec.ts › Multi-object ODCS contract - object selector shows all schema objects (shard 6, 1 retry)
  • Pages/ServiceEntity.spec.ts › Tier Add, Update and Remove (shard 6, 1 retry)
  • Pages/ServiceEntity.spec.ts › Set & Update table-cp, hyperlink-cp, string, integer, markdown, number, duration, email, enum, sqlQuery, timestamp, entityReference, entityReferenceList, timeInterval, time-cp, date-cp, dateTime-cp Custom Property (shard 6, 1 retry)
  • Pages/ServiceEntity.spec.ts › Tier Add, Update and Remove (shard 6, 1 retry)
  • Pages/Users.spec.ts › Permissions for table details page for Data Consumer (shard 6, 1 retry)
  • VersionPages/GlossaryVersionPage.spec.ts › GlossaryTerm (shard 6, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@yan-3005 yan-3005 enabled auto-merge (squash) March 25, 2026 13:20
@yan-3005 yan-3005 merged commit 747e75e into main Mar 25, 2026
58 of 61 checks passed
@yan-3005 yan-3005 deleted the ram/fix-test-conection-pw-encrypt branch March 25, 2026 13:30
@github-actions
Copy link
Copy Markdown
Contributor

Changes have been cherry-picked to the 1.12.4 branch.

github-actions bot pushed a commit that referenced this pull request Mar 25, 2026
…6762)

* FIX - Clean OpenMetadataWorkflowConfig in TestConnection Workflow

SM is already encrypting responses. This adds an extra layer of security
by not returning unnecessary workflow info (openMetadataServerConnection
with the ingestion-bot JWT token) in API responses.

The token is only passed Server -> Pipeline Service Client during trigger,
never Server -> user.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* test: add unit test for WorkflowResource JWT token not exposed in responses

Covers line 617 - verifies decryptOrNullify nullifies openMetadataServerConnection
to prevent ingestion-bot JWT token leakage in API responses.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* test: add null guards on list response before iterating

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
(cherry picked from commit 747e75e)
yan-3005 added a commit that referenced this pull request Mar 26, 2026
…6762)

* FIX - Clean OpenMetadataWorkflowConfig in TestConnection Workflow

SM is already encrypting responses. This adds an extra layer of security
by not returning unnecessary workflow info (openMetadataServerConnection
with the ingestion-bot JWT token) in API responses.

The token is only passed Server -> Pipeline Service Client during trigger,
never Server -> user.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* test: add unit test for WorkflowResource JWT token not exposed in responses

Covers line 617 - verifies decryptOrNullify nullifies openMetadataServerConnection
to prevent ingestion-bot JWT token leakage in API responses.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* test: add null guards on list response before iterating

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
(cherry picked from commit 747e75e)
yan-3005 added a commit that referenced this pull request Mar 26, 2026
…6762)

* FIX - Clean OpenMetadataWorkflowConfig in TestConnection Workflow

SM is already encrypting responses. This adds an extra layer of security
by not returning unnecessary workflow info (openMetadataServerConnection
with the ingestion-bot JWT token) in API responses.

The token is only passed Server -> Pipeline Service Client during trigger,
never Server -> user.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* test: add unit test for WorkflowResource JWT token not exposed in responses

Covers line 617 - verifies decryptOrNullify nullifies openMetadataServerConnection
to prevent ingestion-bot JWT token leakage in API responses.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* test: add null guards on list response before iterating

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
(cherry picked from commit 747e75e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend safe to test Add this label to run secure Github workflows on PRs To release Will cherry-pick this PR into the release branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants