Skip to content

update#43966

Merged
dantavori merged 16 commits intomasterfrom
fix-service-now-pentest
Apr 29, 2026
Merged

update#43966
dantavori merged 16 commits intomasterfrom
fix-service-now-pentest

Conversation

@michal-dagan
Copy link
Copy Markdown
Contributor

@michal-dagan michal-dagan commented Apr 20, 2026

Contributing to Cortex XSOAR Content

Make sure to register your contribution by filling the contribution registration form

The Pull Request will be reviewed only after the contribution registration form is filled.

Status

  • In Progress
  • Ready
  • In Hold - (Reason for hold)

Related Issues

https://jira-dc.paloaltonetworks.com/browse/CIAC-16562

Description

Must have

  • Tests
  • Documentation

@michal-dagan michal-dagan marked this pull request as ready for review April 20, 2026 12:35
@michal-dagan michal-dagan requested a review from dantavori as a code owner April 20, 2026 12:35
@content-bot
Copy link
Copy Markdown
Contributor

🤖 AI-Powered Code Review Available

You can leverage AI-powered code review to assist with this PR!

Available Commands:

  • @marketplace-ai-reviewer start review - Initiate a full AI code review
  • @marketplace-ai-reviewer re-review - Incremental review for new commits

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 20, 2026

Coverage

Coverage Report
FileStmtsMissCoverMissing
Packs/ServiceNow/Integrations/ServiceNowv2
   ServiceNowv2.py157427882%214, 226, 232, 480, 482, 486, 511, 552, 559–560, 598–599, 726–729, 739, 749–759, 770, 781–782, 854–855, 862, 868–874, 885–886, 899, 903, 916, 942, 944, 946–947, 949–950, 952–955, 957, 999, 1004, 1032–1037, 1040, 1043, 1154, 1295–1301, 1303, 1306–1307, 1309–1312, 1314, 1316–1317, 1319, 1321–1322, 1324–1325, 1327, 1350–1351, 1353, 1362–1363, 1394, 1433, 1436, 1443, 1471, 1476, 1564, 1619, 1667, 1719, 1800, 1862, 1865, 1914, 1916–1918, 1920–1921, 1962, 1965–1967, 1981, 1986–1988, 2014, 2049–2050, 2058, 2109, 2153–2156, 2158, 2161, 2168, 2231–2236, 2238–2239, 2241–2243, 2245–2246, 2248–2250, 2252–2253, 2255, 2257, 2269, 2272, 2274, 2287–2292, 2294–2295, 2297–2299, 2301–2302, 2304–2306, 2308–2309, 2311, 2323–2324, 2327, 2329, 2347, 2350, 2378, 2381, 2415, 2418, 2422, 2450, 2490, 2516, 2546, 2644, 2656, 2667, 2670, 2674–2675, 2679, 2681–2682, 2754, 2760, 2771, 2819–2820, 2861–2864, 2886–2887, 2891–2892, 2927, 2931, 2937, 3029, 3032–3034, 3165–3169, 3171–3172, 3176–3177, 3232–3233, 3259, 3397, 3399, 3403–3404, 3579–3581, 3587, 3604, 3607, 3611, 3642, 3722, 3748–3749, 3754, 3770, 3791, 3812–3814, 3833–3835, 3877, 3885, 3941–3943, 3945, 3947, 3949, 3951, 3953, 3955, 3957, 3959, 3963, 3965, 3967, 3969–3970, 3975–3976, 3984
TOTAL157427882% 

Tests Skipped Failures Errors Time
180 0 💤 0 ❌ 0 🔥 27523485m 6s ⏱️

@michal-dagan michal-dagan added the ready-for-pipeline-running Whether the pr is ready for running the whole pipeline, including testing on SAAS machines label Apr 20, 2026
@michal-dagan michal-dagan added the ready-for-ai-review The PR is ready for reviewing the PR with the AI Reviewer. label Apr 20, 2026
@marketplace-ai-reviewer marketplace-ai-reviewer removed the ready-for-ai-review The PR is ready for reviewing the PR with the AI Reviewer. label Apr 20, 2026
@marketplace-ai-reviewer
Copy link
Copy Markdown
Contributor

🤖 Analysis started. Please wait for results...

@marketplace-ai-reviewer
Copy link
Copy Markdown
Contributor

🤖 AI Review Disclaimer

This review was generated by an AI-powered tool and may contain inaccuracies. Please be advised, and we extend our sincere apologies for any inconvenience this may cause.

Copy link
Copy Markdown
Contributor

@marketplace-ai-reviewer marketplace-ai-reviewer left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution to the ServiceNow integration! I've reviewed the PR and left a few notes to help polish the code.

Mainly, please guard against potential TypeErrors when accessing the name key during file operations, and ensure your release notes start with an approved prefix. There are also a couple of minor test file adjustments to make, such as moving an import to the top and adding a unit test for path sanitization.

Let me know if you have any questions!

Additionally, please address the following file-level notes:

  • Packs/ServiceNow/Integrations/ServiceNowv2/ServiceNowv2_test.py: Consider adding a unit test for the path sanitization in update_remote_system_with_entries.

@inbalapt1, @dantavori, @michal-dagan please review and approve the results generated by the AI Reviewer by responding 👍 on this comment.

Comment thread Packs/ServiceNow/Integrations/ServiceNowv2/ServiceNowv2.py Outdated
Comment thread Packs/ServiceNow/Integrations/ServiceNowv2/ServiceNowv2.py Outdated
Comment thread Packs/ServiceNow/Integrations/ServiceNowv2/ServiceNowv2_test.py Outdated
Comment thread Packs/ServiceNow/ReleaseNotes/2_9_7.md Outdated
@michal-dagan
Copy link
Copy Markdown
Contributor Author

Addressed all AI reviewer feedback:

  • Added default value guard ("") to file_data.get("name", "") and path_res.get("name", "") to prevent potential TypeError
  • Moved import os to top of test file
  • Added test_update_remote_system_with_entries_uses_basename unit test for path sanitization in update_remote_system_with_entries
  • Updated release note in 2_9_7.md to use approved prefix: 'Fixed an issue with filename handling during file upload operations.'

@marketplace-ai-reviewer re-review

@marketplace-ai-reviewer
Copy link
Copy Markdown
Contributor

🤖 Analysis started. Please wait for results...

@marketplace-ai-reviewer
Copy link
Copy Markdown
Contributor

🤖 AI Review Disclaimer

This review was generated by an AI-powered tool and may contain inaccuracies. Please be advised, and we extend our sincere apologies for any inconvenience this may cause.

Copy link
Copy Markdown
Contributor

@marketplace-ai-reviewer marketplace-ai-reviewer left a comment

Choose a reason for hiding this comment

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

🔄 Re-reviewing updates...

Important

To maintain review integrity, threads marked as Resolved without accompanying changes or explanations will be automatically unresolved for further verification.

Hi there! Thanks for your contribution to the ServiceNowv2 integration.

I've left a few notes for you to look at, primarily around safely handling None values for file names using or "" in the main code. On the testing side, please fix a TypeError related to argument counts, explicitly assert expected file names, and add coverage for missing file names.

Let me know if you have any questions!

Additionally, please address the following file-level notes:

  • Packs/ServiceNow/Integrations/ServiceNowv2/ServiceNowv2_test.py: Add test coverage for missing or None file names.

@inbalapt1, @dantavori, @marketplace-ai-reviewer, @michal-dagan please review and approve the results generated by the AI Reviewer by responding 👍 on this comment.

Comment thread Packs/ServiceNow/Integrations/ServiceNowv2/ServiceNowv2_test.py
Comment thread Packs/ServiceNow/Integrations/ServiceNowv2/ServiceNowv2_test.py Outdated
Comment thread Packs/ServiceNow/Integrations/ServiceNowv2/ServiceNowv2.py Outdated
Comment thread Packs/ServiceNow/Integrations/ServiceNowv2/ServiceNowv2.py Outdated
@marketplace-ai-reviewer
Copy link
Copy Markdown
Contributor

🤖 Analysis started. Please wait for results...

@marketplace-ai-reviewer
Copy link
Copy Markdown
Contributor

🤖 AI Review Disclaimer

This review was generated by an AI-powered tool and may contain inaccuracies. Please be advised, and we extend our sincere apologies for any inconvenience this may cause.

Copy link
Copy Markdown
Contributor

@marketplace-ai-reviewer marketplace-ai-reviewer left a comment

Choose a reason for hiding this comment

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

🔄 Re-reviewing updates...

Important

To maintain review integrity, threads marked as Resolved without accompanying changes or explanations will be automatically unresolved for further verification.

Thank you for your contribution! Re-reviewed your PR, No new issues found. Great work! 👍
Please check the previous comments to ensure all feedback has been resolved.

@dantavori, @inbalapt1, @michal-dagan, @marketplace-ai-reviewer please review and approve the results generated by the AI Reviewer by responding 👍 on this comment.

@michal-dagan michal-dagan changed the title Mythos - Pentest Evaluation Findings - ServiceNow ServiceNow Apr 20, 2026
@michal-dagan michal-dagan changed the title ServiceNow update Apr 20, 2026
@content-bot
Copy link
Copy Markdown
Contributor

This PR was automatically updated by a GitHub Action

  • ServiceNow pack version was bumped to 2.9.8.

To stop automatic version bumps, add the ignore-auto-bump-version label to the github PR.

@content-bot
Copy link
Copy Markdown
Contributor

This PR was automatically updated by a GitHub Action

  • ServiceNow pack version was bumped to 2.9.9.

To stop automatic version bumps, add the ignore-auto-bump-version label to the github PR.

Copy link
Copy Markdown
Contributor

@inbalapt1 inbalapt1 left a comment

Choose a reason for hiding this comment

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

Checked.

@inbalapt1 inbalapt1 added the ready-for-ai-review The PR is ready for reviewing the PR with the AI Reviewer. label Apr 28, 2026
@marketplace-ai-reviewer marketplace-ai-reviewer removed the ready-for-ai-review The PR is ready for reviewing the PR with the AI Reviewer. label Apr 28, 2026
@marketplace-ai-reviewer
Copy link
Copy Markdown
Contributor

🤖 Analysis started. Please wait for results...

@marketplace-ai-reviewer
Copy link
Copy Markdown
Contributor

🤖 AI Review Disclaimer

This review was generated by an AI-powered tool and may contain inaccuracies. Please be advised, and we extend our sincere apologies for any inconvenience this may cause.

Copy link
Copy Markdown
Contributor

@marketplace-ai-reviewer marketplace-ai-reviewer left a comment

Choose a reason for hiding this comment

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

Thanks for submitting your PR!
Before moving on with the detailed review, please take a moment to address the following general comments.

  • The PR title and description are too generic ('update') and do not reflect the actual changes. Please update the title and description to explicitly state that this PR fixes a path traversal issue by using os.path.basename for file names in the ServiceNowv2 integration.

@content-bot
Copy link
Copy Markdown
Contributor

Validate summary
The following errors were thrown as a part of this pr: .
If the AG100 validation in the pre-commit GitHub Action fails, the pull request cannot be force-merged.

Verdict: PR can be force merged from validate perspective? ✅

@content-bot
Copy link
Copy Markdown
Contributor

🔍 AI Triage Report Available

An automated triage report has been generated for this pipeline.

Status: failed
Report ID: d597765c93cb2e24

📋 Triage Report
💡 Resolutions are available in the full report.

⚠️ AI-generated triage. Validate before acting.

@inbalapt1
Copy link
Copy Markdown
Contributor

This PR requires Force Merge because of failing TPBs that are not related to the changes here. Attaching an empty change PR with the same failures: #43741

@inbalapt1 inbalapt1 added the ForceMerge Forcing the merge of the PR despite the build status label Apr 29, 2026
@dantavori dantavori merged commit 50425ed into master Apr 29, 2026
41 of 50 checks passed
@dantavori dantavori deleted the fix-service-now-pentest branch April 29, 2026 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs-approved ForceMerge Forcing the merge of the PR despite the build status ready-for-pipeline-running Whether the pr is ready for running the whole pipeline, including testing on SAAS machines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants