code-review

ServiceNow コードレビュー

概要

このスキルでは、ServiceNow スクリプトを体系的にレビューし、以下の問題を特定して修正します。

• セキュリティの脆弱性（インジェクション、XSS、権限昇格、データ漏えい）
• パフォーマンスのアンチパターン（不要なクエリ、N+1 問題、クエリ上限の未設定）
• プラットフォームのベストプラクティス違反（API の誤用、スコープの問題、非推奨メソッド）
• 保守性に関する懸念（命名、ドキュメント、複雑さ、デッドコード）
• 並行処理とトランザクションの安全性に関する問題
• クライアント側のパフォーマンスと UX の問題

使用するタイミング： 新しいスクリプトを本番環境にデプロイする前、コードレビューの実施中、パフォーマンスの問題をトラブルシューティングする際、またはセキュリティコンプライアンスのために既存のスクリプトを監査する際に使用します。

前提条件

• ロール： admin、security_admin、またはスクリプトテーブルへの読み取りアクセス権を持つ開発者
• アクセス： sys_script、sys_script_include、sys_ui_script、sys_script_client テーブル
• 知識： ServiceNow スクリプト API、OWASP のセキュリティ原則、JavaScript のベストプラクティス
• 関連スキル： コード生成のコンテキストとして development/code-assist を完了してください

手順

ステップ 1：レビュー対象のスクリプトを取得する

特定のテーブルのビジネスルールをクエリする：

MCP アプローチ：

Tool: SN-Query-Table
Parameters:
  table_name: sys_script
  query: collection=incident^active=true
  fields: sys_id,name,collection,when,script,filter_condition,order
  limit: 50

REST の代替方法：

curl -u "$SN_USER:$SN_PASS" \
  "$SN_INSTANCE/api/now/table/sys_script?sysparm_query=collection=incident^active=true&sysparm_fields=sys_id,name,collection,when,script,filter_condition,order&sysparm_limit=50" \
  -H "Accept: application/json"

スクリプトインクルードをクエリする：

MCP アプローチ：

Tool: SN-Query-Table
Parameters:
  table_name: sys_script_include
  query: active=true^sys_updated_on>=javascript:gs.daysAgo(30)
  fields: sys_id,name,script,client_callable,access,api_name
  limit: 50

クライアントスクリプトをクエリする：

MCP アプローチ：

Tool: SN-Query-Table
Parameters:
  table_name: sys_script_client
  query: table=incident^active=true
  fields: sys_id,name,table,type,fieldname,script,ui_type
  limit: 50

ステップ 2：セキュリティレビューチェックリスト

各スクリプトを、次の重要なセキュリティパターンに照らしてレビューしてください。

2a. SQL／GlideRecord インジェクション：

VULNERABILITY: Unsanitized user input in queries
BAD:
  gr.addEncodedQuery(current.variables.user_input);
  gr.addQuery('name', request.getParameter('name'));

GOOD:
  // Validate and sanitize input
  var sanitized = GlideStringUtil.escapeHTML(input);
  // Use parameterized queries
  gr.addQuery('name', sanitizedName);
  // Validate against allowlist
  var validFields = ['name', 'number', 'state'];
  if (validFields.indexOf(fieldName) === -1) return;

2b. クロスサイトスクリプティング（XSS）：

VULNERABILITY: Unescaped output in UI scripts or HTML fields
BAD:
  element.innerHTML = current.short_description;
  gs.getMessage(userInput);

GOOD:
  element.textContent = current.short_description;
  var escaped = GlideStringUtil.escapeHTML(current.short_description);

2c. 権限昇格：

VULNERABILITY: Missing role checks or using setWorkflow(false) inappropriately
BAD:
  current.setWorkflow(false); // Bypasses business rules and ACLs
  current.autoSysFields(false); // Hides audit trail

GOOD:
  // Only bypass when absolutely necessary and documented
  if (gs.hasRole('admin')) {
    current.setWorkflow(false); // REASON: Bulk import requires bypass
  }

2d. 機密データの漏えい：

VULNERABILITY: Logging or returning sensitive data
BAD:
  gs.log('User password: ' + password);
  gs.log('SSN: ' + current.social_security_number);

GOOD:
  gs.log('Authentication attempted for user: ' + username);
  // Never log PII, credentials, or tokens

ステップ 3：パフォーマンスレビューチェックリスト

3a. 上限のない GlideRecord クエリ：

ANTI-PATTERN: Query without limit
BAD:
  var gr = new GlideRecord('incident');
  gr.addQuery('active', true);
  gr.query(); // Could return millions of records

GOOD:
  var gr = new GlideRecord('incident');
  gr.addQuery('active', true);
  gr.setLimit(1000); // Always set a reasonable limit
  gr.query();

3b. N+1 クエリパターン：

ANTI-PATTERN: Query inside a loop
BAD:
  var incidents = new GlideRecord('incident');
  incidents.query();
  while (incidents.next()) {
    var user = new GlideRecord('sys_user');
    user.get(incidents.caller_id); // Query per iteration!
    // process...
  }

GOOD:
  // Use dot-walking or join queries
  var incidents = new GlideRecord('incident');
  incidents.addQuery('active', true);
  incidents.query();
  while (incidents.next()) {
    var callerName = incidents.caller_id.getDisplayValue(); // Dot-walk
    // process...
  }

  // Or use GlideAggregate for counts
  var ga = new GlideAggregate('incident');
  ga.addQuery('active', true);
  ga.groupBy('assignment_group');
  ga.addAggregate('COUNT');
  ga.query();

3c. クライアントスクリプトでの同期 GlideRecord：

ANTI-PATTERN: Synchronous server calls from client
BAD:
  var gr = new GlideRecord('sys_user');
  gr.addQuery('sys_id', userId);
  gr.query(); // Synchronous - blocks browser!
  if (gr.next()) { ... }

GOOD:
  // Use GlideAjax for async server calls
  var ga = new GlideAjax('MyScriptInclude');
  ga.addParam('sysparm_name', 'getUserInfo');
  ga.addParam('sysparm_user_id', userId);
  ga.getXMLAnswer(function(answer) {
    // Process response asynchronously
  });

3d. 不要なフィールド取得：

ANTI-PATTERN: Selecting all fields when only a few are needed
BAD:
  var gr = new GlideRecord('incident');
  gr.query(); // Returns ALL fields

GOOD:
  var gr = new GlideRecord('incident');
  gr.addQuery('active', true);
  gr.chooseWindow(0, 100);
  gr.setFields('sys_id,number,short_description,state');
  gr.query();

ステップ 4：プラットフォームのベストプラクティスレビュー

4a. 非推奨 API の使用：

DEPRECATED: Avoid these methods
BAD:
  current.getDisplayValue(); // Without field name - ambiguous
  gs.log(); // Use gs.info/warn/error instead
  Packages.java.lang.*; // Direct Java calls - unsupported

GOOD:
  current.getDisplayValue('short_description');
  gs.info('Message: {0}', current.number);

4b. スコープ違反：

ANTI-PATTERN: Accessing global scope from scoped app
BAD:
  // In scoped app, directly calling global
  var result = new IncidentUtils(); // Global script include

GOOD:
  // Use scoped API or cross-scope access
  var si = new global.IncidentUtils(); // Explicit scope prefix
  // Or use sn_ws for REST-based cross-scope communication

4c. ビジネスルールの順序：

BEST PRACTICE: Proper ordering
- 100: Data defaults and initialization
- 200: Validation rules
- 300: Data transformation
- 400: Cross-table updates (after rules)
- 500+: Notifications and integrations (after rules)

4d. GlideRecord の getValue と直接アクセス：

ANTI-PATTERN: Direct field access for comparisons
BAD:
  if (current.state == 6) { ... } // Returns GlideElement, not value

GOOD:
  if (current.getValue('state') == '6') { ... }
  // Or use typed comparison
  if (current.state.changesTo(6)) { ... }

ステップ 5：クライアントスクリプトのレビュー

5a. DOM 操作：

ANTI-PATTERN: Direct DOM manipulation
BAD:
  document.getElementById('incident.short_description').style.color = 'red';
  $('incident.state').value = '6';

GOOD:
  g_form.setMandatory('short_description', true);
  g_form.addDecoration('short_description', 'color_red');
  g_form.setValue('state', '6');

5b. isLoading チェックの欠落：

ANTI-PATTERN: onChange without isLoading guard
BAD:
  function onChange(control, oldValue, newValue, isLoading) {
    // Fires on every form load, causing unnecessary processing
    callServer(newValue);
  }

GOOD:
  function onChange(control, oldValue, newValue, isLoading) {
    if (isLoading || newValue === '') {
      return;
    }
    callServer(newValue);
  }

ステップ 6：レビューレポートを作成する

検出事項をまとめ、構成されたレビューレポートを作成します。

Code Review Report
================================================
Script: [Script Name]
Table: [Table Name]
Type: [Business Rule / Client Script / Script Include]
Reviewer: [AI-Assisted Review]
Date: [Current Date]

CRITICAL ISSUES:
  [C1] Security - SQL Injection in line X
  [C2] Security - XSS vulnerability in line Y

HIGH ISSUES:
  [H1] Performance - N+1 query pattern in line Z
  [H2] Performance - Unbounded GlideRecord query

MEDIUM ISSUES:
  [M1] Best Practice - Deprecated API usage
  [M2] Best Practice - Missing error handling

LOW ISSUES:
  [L1] Maintainability - Missing JSDoc comments
  [L2] Maintainability - Variable naming inconsistency

RECOMMENDATIONS:
  1. [Specific fix for C1]
  2. [Specific fix for C2]
  3. [Specific fix for H1]

ステップ 7：修正を適用する

修正を含むスクリプトを更新する：

MCP アプローチ：

Tool: SN-Update-Record
Parameters:
  table_name: sys_script
  sys_id: [script_sys_id]
  data:
    script: |
      // [Updated script with fixes applied]

REST の代替方法：

curl -u "$SN_USER:$SN_PASS" \
  "$SN_INSTANCE/api/now/table/sys_script/[script_sys_id]" \
  -H "Content-Type: application/json" \
  -X PATCH \
  -d '{"script": "// Updated script content"}'

ツールの使用方法

操作	MCP ツール	REST エンドポイント
ビジネスルールをクエリする	SN-Query-Table	GET /api/now/table/sys_script
クライアントスクリプトをクエリする	SN-Query-Table	GET /api/now/table/sys_script_client
スクリプトインクルードをクエリする	SN-Query-Table	GET /api/now/table/sys_script_include
UI スクリプトをクエリする	SN-Query-Table	GET /api/now/table/sys_ui_script
単一のスクリプトを取得する	SN-Get-Record	GET /api/now/table/{table}/{sys_id}
修正済みスクリプトを更新する	SN-Update-Record	PATCH /api/now/table/{table}/{sys_id}

ベストプラクティス

• 段階的にレビューする： まずセキュリティ、次にパフォーマンス、その後ベストプラクティス、最後に保守性を確認します
• 定期的なチェックを自動化する： よくあるアンチパターンを検出するスクリプトを作成します
• すべてのスクリプトタイプをまとめて確認する： ビジネスルール、クライアントスクリプト、スクリプトインクルードは相互に影響することがよくあります
• 本番環境以外で修正をテストする： 本番環境に直接コード修正を適用しないでください
• 例外を文書化する： 既知のアンチパターンを意図的に使用している場合は、その理由を説明するコメントを追加します
• スクリプトと併せて ACL もレビューする： ACL がビジネスルールのセキュリティチェックを補完していることを確認します
• デッドコードを確認する： アクティブでないスクリプトも、アップグレード中のインスタンスのパフォーマンスに影響します
• エラー処理を検証する： すべての GlideRecord.get() で、結果が null かどうかを確認します
• 更新セットをレビューする： プロモーション前に、スクリプトが適切な更新セットに含まれていることを確認します

トラブルシューティング

スクリプトテーブルにアクセスできない

症状： sys_script のクエリでアクセスが拒否される 原因： 管理者ロールまたはスクリプト関連のロールがありません 解決策： ロールの割り当てを確認します。

Tool: SN-Query-Table
Parameters:
  table_name: sys_user_has_role
  query: user=[user_sys_id]^role.name=admin
  fields: sys_id,role,user

スクリプトが大きすぎて取得できない

症状： API レスポンスでスクリプトフィールドが切り詰められる 原因： API のフィールドサイズ制限 解決策： 対象のレコードを直接取得します。

Tool: SN-Get-Record
Parameters:
  table_name: sys_script
  sys_id: [script_sys_id]
  fields: script

最近変更されたすべてのスクリプトを見つける

症状： 最近の変更をすべて確認する必要がある 解決策：

Tool: SN-Query-Table
Parameters:
  table_name: sys_script
  query: sys_updated_on>=javascript:gs.daysAgo(7)^active=true
  fields: sys_id,name,collection,when,sys_updated_by,sys_updated_on
  limit: 100

重複または競合するルールを特定する

症状： 同じテーブルに複数のルールがあり、予期しない動作が発生する 解決策：

Tool: SN-Query-Table
Parameters:
  table_name: sys_script
  query: collection=incident^active=true^when=before
  fields: sys_id,name,order,filter_condition,action_insert,action_update
  limit: 100

例

例 1：インシデントのビジネスルールの完全なセキュリティ監査

# Step 1: Get all active business rules for incident table
Tool: SN-Query-Table
Parameters:
  table_name: sys_script
  query: collection=incident^active=true
  fields: sys_id,name,when,script,order
  limit: 100

# Step 2: For each script, check for security patterns
# Look for: addEncodedQuery with user input, setWorkflow(false),
# gs.log with sensitive data, eval() usage, Packages.* calls

# Step 3: Get client scripts for incident
Tool: SN-Query-Table
Parameters:
  table_name: sys_script_client
  query: table=incident^active=true
  fields: sys_id,name,type,script
  limit: 50

# Step 4: Check for synchronous GlideRecord in client scripts
# Look for: GlideRecord().query() without callback, direct DOM access

例 2：スクリプトインクルードのパフォーマンスレビュー

# Get all active script includes with recent updates
Tool: SN-Query-Table
Parameters:
  table_name: sys_script_include
  query: active=true^sys_updated_on>=javascript:gs.daysAgo(90)
  fields: sys_id,name,script,api_name
  limit: 100

# For each script include, check for:
# - GlideRecord queries without setLimit()
# - Nested loops with queries (N+1)
# - Missing null checks after .get()
# - Large string concatenation in loops

例 3：最近デプロイされたコードのレビュー

# Get scripts from the latest update set
Tool: SN-Query-Table
Parameters:
  table_name: sys_update_xml
  query: update_set=[update_set_sys_id]^nameLIKEsys_script
  fields: sys_id,name,action,target_name
  limit: 50

# Then retrieve each script for review
Tool: SN-Get-Record
Parameters:
  table_name: sys_script
  sys_id: [script_sys_id]
  fields: name,script,collection,when

関連スキル

• development/code-assist - AI を活用したコード生成
• security/acl-management - アクセス制御リストの管理
• admin/update-set-management - デプロイに向けた更新セットの処理
• security/audit-compliance - セキュリティ強化のガイドライン