fix(integrations): remove unsafe generic ServiceNow table tools - #4206
kojiwakayama wants to merge 3 commits into
Conversation
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Warning Review limit reachedNext included review available in 3 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughServiceNow endpoints now derive their host from ChangesServiceNow instance routing
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change removes unsafe generic ServiceNow table tools while preserving the HTTPS endpoint behavior, and no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ToolSource
participant LocalCredentialAuth
participant ServiceNowTransport
ToolSource->>LocalCredentialAuth: Resolve SERVICENOW_INSTANCE
LocalCredentialAuth-->>ToolSource: Return normalized instanceOrigin
ToolSource->>ServiceNowTransport: Substitute instanceOrigin into endpoint
ServiceNowTransport->>ServiceNowTransport: Enforce instanceOrigin as allowed origin
ServiceNowTransport-->>ToolSource: Return authenticated response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 4 files. (2 skipped: 1 unsupported, 1 too large.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfe26e45da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4348e6f33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
a4348e6 to
2ebecc8
Compare
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ebecc8215
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Squashed from: - fix(integrations): remove unsafe generic ServiceNow tools - fix(integrations): bind ServiceNow endpoint hosts to SERVICENOW_INSTANCE
2ebecc8 to
1e05701
Compare
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
Admission rejected the new {{env.SERVICENOW_INSTANCE}} endpoint templates
because assertHttpsCatalogUrl refuses any URL containing a template, so
every ServiceNow tool failed at source creation. Admit ServiceNow
endpoints by their trusted template prefix (mirroring the Salesforce
instance-origin binding) and substitute the host at execution with an
HTTPS origin normalized from the SERVICENOW_INSTANCE configuration.
The normalization accepts both shapes existing configurations use, a
bare host (example.service-now.com) and a full HTTPS URL, matching the
scaffolded ServiceNowClient and OAuth route, so a full-URL value no
longer composes an invalid https://https:// endpoint. SERVICENOW_INSTANCE
is now a required credential of the ServiceNow auth plan, so a missing or
malformed value surfaces as a clear credential or configuration error
before any request is sent.
Also align the public connector description with the retained curated
tools now that the generic change_request-capable table tools are gone.
Regenerated src/integrations/_data.ts via
scripts/build/generate-integrations-module.ts. Added local tool-source
tests covering admission plus execution against the env-bound host, the
instance normalization shapes, malformed instance rejection, and the
missing-credential path.
Claude-Session: https://claude.ai/code/session_012VprCnNBzAi9PRvzhjYNcb
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@codex review |
|
@codex review |
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
|
Superseded by #4224, which consolidates this security hardening and is now merged at exact head |
Motivation
instanceHostwhile the connector attached the sensitiveSERVICENOW_ACCESS_TOKENas anAuthorization: Bearerheader, allowing a caller to direct the token to an arbitrary host.Description
query_table,create_table_record, andupdate_table_recordtool definitions from the ServiceNow connector template atcli/templates/integrations/servicenow/connector.jsonso callers can no longer supplyinstanceHostfor those generic operations.src/integrations/_data.tsto exclude the unsafe generic ServiceNow tools.src/integrations/_data.test.tsto add regression assertions that the three generic ServiceNow table tools are not present and to assert that curated incident operations remain available.Testing
python3 -m json.tool cli/templates/integrations/servicenow/connector.jsonto validate the template JSON, which succeeded.rg -n 'servicenow__(query_table|create_table_record|update_table_record)' cli/templates/integrations/servicenow/connector.json src/integrations/_data.ts, which returned no matches.git diff --checkto ensure no whitespace/format issues, which passed.deno test --no-check --allow-all src/integrations/_data.test.tsbutdenois not installed in this environment, so unit tests were not executed here (the change includes focused test adjustments; please run the suite in a Deno-enabled CI or dev environment).Codex Task
Summary by CodeRabbit