webui/src/api/model/script.ts (1)

25-25: 🛠️ Refactor suggestion

Consider using semantic versioning format for version field.

Given that this interface is used for scripts that can execute arbitrary code (as mentioned in PR objectives), it's important to properly version scripts for security tracking and compatibility.

Consider enforcing semantic versioning format using a more specific type:

version: `${number}.${number}.${number}` // Type literal for semver format (e.g., "1.0.0")

This would help ensure consistent version formatting and make it easier to track script updates and potential security issues.

src/main/java/com/ghostchu/peerbanhelper/database/table/ScriptStorageEntity.java (1)

9-13: 💡 Codebase verification

Based on the code review, I can now provide a final assessment:

Security measures need enhancement for script storage

The current implementation has some security controls but lacks comprehensive protection:

1. Basic validation exists through AviatorEvaluator.getInstance().validate() but only checks syntax
2. No size limits are enforced on script content
3. Basic metadata parsing exists (name, author, version) but no content sanitization
4. Thread safety and caching flags are present but no access control mechanisms
5. No audit logging for script modifications

Recommended actions:

• Add content size limits to prevent memory exhaustion attacks
• Implement content sanitization beyond syntax validation
• Add access control mechanisms for script storage/retrieval
• Implement audit logging for script modifications
• Consider adding script execution timeouts

🔗 Analysis chain

Verify security measures for script storage.

Given that this entity stores scripts that can execute arbitrary code (as mentioned in PR objectives), consider implementing additional security measures:

1. Input validation/sanitization
2. Length limits for stored scripts
3. Access control mechanisms
4. Audit logging for script modifications

Let's check if there are any security measures implemented elsewhere:

---

Let's check the validation code found in ScriptEngine.java and look for any size limits in the database schema:

---

Let's check the actual script content and database schema since we found validation code but need to verify its implementation:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for security-related code around script validation and sanitization
echo "Searching for script validation..."
rg -l "script.*valid|valid.*script|sanitize.*script|script.*sanitize" --type java

echo "Searching for audit logging..."
rg -l "audit.*log.*script|script.*audit" --type java

echo "Checking for potential script size limits..."
ast-grep --pattern 'class ScriptStorageDao {
  $$$
  put($_, $_) {
    $$$
  }
}'

Length of output: 543

---

Script:

#!/bin/bash
# Check script validation implementation
echo "Checking script validation in ScriptEngine.java..."
rg -A 10 "script.*valid|valid.*script" src/main/java/com/ghostchu/peerbanhelper/scriptengine/ScriptEngine.java

echo "Checking database schema for column constraints..."
fd -e sql . | xargs cat

echo "Checking if any configuration files define script limits..."
fd -e yaml -e properties -e xml . | xargs cat

Length of output: 196463

---

Script:

#!/bin/bash
# Get the script validation implementation
echo "Checking script validation implementation..."
cat src/main/java/com/ghostchu/peerbanhelper/scriptengine/ScriptEngine.java

# Look for database schema files
echo "Looking for database schema files..."
find . -type f -name "*.sql" -o -name "schema.db" -o -name "*.ddl"

# Check for any size limits in configuration
echo "Checking configuration files for limits..."
rg -i "script.*size|size.*script|script.*limit|limit.*script|max.*script" --type yaml --type properties --type xml --type java

Length of output: 6130

webui/src/stores/userStore.ts (1)

10-14: ⚠️ Potential issue

Fix naming inconsistency in exported state.

The state variable is named scriptWarningConfirmed but exported as scriptWarningConfirm. This inconsistency could be confusing.

 return {
   licenseVersion,
-  scriptWarningConfirm: scriptWarningConfirmed,
+  scriptWarningConfirmed,
   confirmScriptWarning
 }

📝 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.

  return {
    licenseVersion,
    scriptWarningConfirmed,
    confirmScriptWarning
  }

src/main/java/com/ghostchu/peerbanhelper/btn/BtnRule.java (1)

25-26: ⚠️ Potential issue

Consider security implications of script rules.

The new scriptRules field will store scripts received from the BTN server that can execute arbitrary code. While this provides powerful functionality, it requires careful handling to prevent security vulnerabilities.

Consider implementing:

1. Script content validation and sanitization
2. Execution sandboxing
3. Rate limiting and timeout mechanisms
4. Script size limits
5. Whitelisting of allowed operations

Would you like assistance in implementing these security measures?

webui/src/views/settings/components/script/index.vue (1)

6-6: 🛠️ Refactor suggestion

Enhance error handling for better user feedback.

The current implementation only checks for missing data. Consider handling other error cases (e.g., network errors, invalid data) with appropriate error messages.

-    <a-result v-if="!data?.data" status="404" :subtitle="t('page.settings.tab.script.disable')" />
+    <a-result
+      v-if="!data?.data"
+      :status="data?.error ? 'error' : '404'"
+      :subtitle="data?.error ? t('common.error.network') : t('page.settings.tab.script.disable')"
+    />

📝 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.

    <a-result
      v-if="!data?.data"
      :status="data?.error ? 'error' : '404'"
      :subtitle="data?.error ? t('common.error.network') : t('page.settings.tab.script.disable')"
    />

webui/src/components/multiClick.vue (3)

14-18: 🛠️ Refactor suggestion

Fix timer type for browser environment.

The timer type should use the browser's ReturnType<typeof setTimeout> instead of NodeJS.Timeout.

-const timer = ref<NodeJS.Timeout | null>(null)
+const timer = ref<ReturnType<typeof setTimeout> | null>(null)

📝 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.

const emits = defineEmits<{
  (e: 'multiClick'): void
}>()
const clickCount = ref(0)
const timer = ref<ReturnType<typeof setTimeout> | null>(null)

---

6-12: 🛠️ Refactor suggestion

Add prop validation and documentation.

Props should include validation and documentation to improve component usability and prevent potential issues.

-const { required, timeLimit } = defineProps<{
-  required: number
-  timeLimit: number
-}>()
+const props = withDefaults(defineProps<{
+  /** Number of clicks required to trigger the multiClick event */
+  required: number
+  /** Time window in milliseconds to count clicks */
+  timeLimit: number
+}>(), {
+  required: 3,
+  timeLimit: 1000
+})
+
+// Runtime validation
+if (props.required < 1) {
+  console.warn('[MultiClick] required prop must be greater than 0')
+}
+if (props.timeLimit < 100) {
+  console.warn('[MultiClick] timeLimit prop should be at least 100ms')
+}

📝 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.

<script setup lang="ts">
import { ref } from 'vue'

const props = withDefaults(defineProps<{
  /** Number of clicks required to trigger the multiClick event */
  required: number
  /** Time window in milliseconds to count clicks */
  timeLimit: number
}>(), {
  required: 3,
  timeLimit: 1000
})

// Runtime validation
if (props.required < 1) {
  console.warn('[MultiClick] required prop must be greater than 0')
}
if (props.timeLimit < 100) {
  console.warn('[MultiClick] timeLimit prop should be at least 100ms')
}

---

19-35: ⚠️ Potential issue

Add cleanup and optimize click handling.

The component needs cleanup on unmount and could benefit from click debouncing.

+import { onUnmounted } from 'vue'
+
 const handleClick = () => {
+  // Debounce rapid clicks
+  if (Date.now() - lastClickTime.value < 50) return
+  lastClickTime.value = Date.now()
+
   clickCount.value++
   // ... rest of the code
 }
+
+// Cleanup timer on unmount
+onUnmounted(() => {
+  if (timer.value) {
+    clearTimeout(timer.value)
+    timer.value = null
+  }
+})

Committable suggestion skipped: line range outside the PR's diff.

webui/src/views/settings/index.vue (1)

2-19: ⚠️ Potential issue

Breaking change: Tab indices modified from 1-based to 0-based indexing.

The change in tab indexing from 1-based to 0-based could break existing deep links or bookmarks that use the query parameter. Consider adding a migration strategy or maintaining backward compatibility.

Additionally, the script tab has inconsistent styling with an icon while other tabs don't have icons.

Consider either:

1. Adding icons to all tabs for consistency, or
2. Removing the icon from the script tab

-    <a-tab-pane :key="3">
-      <template #title>
-        <icon-code-square />
-        {{ t('page.settings.tab.script') }}
-      </template>
+    <a-tab-pane :key="3" :title="t('page.settings.tab.script')">

Committable suggestion skipped: line range outside the PR's diff.

webui/src/views/settings/components/script/locale/zh-CN.ts (1)

27-28: 🛠️ Refactor suggestion

Enhance security warning with specific details.

While the current warning is good, it could be more specific about the types of risks and what constitutes a trusted source.

  'page.settings.tab.script.warning.description':
-    '请勿添加任何由陌生人发送给你的脚本！脚本拥有执行任意代码和程序的能力，这可能导致您的设备被黑客控制或攻击，添加任何脚本之前请三思而后行！',
+    '请勿添加任何由陌生人发送给你的脚本！脚本拥有执行任意代码和程序的能力，可能导致：\n\n' +
+    '• 您的设备被黑客控制或攻击\n' +
+    '• 敏感数据泄露\n' +
+    '• 系统文件被篡改或删除\n\n' +
+    '仅使用来自以下可信来源的脚本：\n' +
+    '• 官方发布渠道\n' +
+    '• 经过验证的社区贡献者\n' +
+    '• 您自己编写和审查的脚本\n\n' +
+    '添加任何脚本之前请三思而后行！',

📝 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.

  'page.settings.tab.script.warning.description':
    '请勿添加任何由陌生人发送给你的脚本！脚本拥有执行任意代码和程序的能力，可能导致：\n\n' +
    '• 您的设备被黑客控制或攻击\n' +
    '• 敏感数据泄露\n' +
    '• 系统文件被篡改或删除\n\n' +
    '仅使用来自以下可信来源的脚本：\n' +
    '• 官方发布渠道\n' +
    '• 经过验证的社区贡献者\n' +
    '• 您自己编写和审查的脚本\n\n' +
    '添加任何脚本之前请三思而后行！',

webui/src/views/settings/components/script/components/editor/aviatorscript/grammar/GrammarParser.ts (2)

13-34: 🛠️ Refactor suggestion

Remove unnecessary constructor and consider making ErrorListener class reusable.

The error handling implementation is robust, but there are a few improvements we can make:

1. Remove the unnecessary constructor:

class MyErrorListener extends ErrorListener<unknown> {
-  constructor() {
-    super()
-  }
  syntaxError(

2. Consider extracting MyErrorListener as a separate class since it could be reused in other parsers:

// Suggested new file: grammar/ErrorListener.ts
export class DSLErrorListener extends ErrorListener<unknown> {
  private errors: Array<{line: number; column: number; message: string}> = [];
  
  syntaxError(
    _recognizer: unknown,
    _offendingSymbol: unknown,
    line: number,
    column: number,
    message: string,
    _e: unknown
  ) {
    this.errors.push({ line, column, message });
  }
  
  getErrors() {
    return this.errors;
  }
}

🧰 Tools

🪛 Biome

[error] 15-17: This constructor is unnecessary.

Unsafe fix: Remove the unnecessary constructor.

(lint/complexity/noUselessConstructor)

---

38-58: ⚠️ Potential issue

Address type safety issues instead of suppressing them.

The current implementation uses @ts-expect-error annotations to suppress type checking, which could lead to runtime errors. Let's improve type safety:

- class Visitor {
-   visitChildren(ctx: ParserRuleContext) {
+ class Visitor implements DSLVisitor<unknown> {
+   visitChildren(ctx: ParserRuleContext): Array<string | unknown> | undefined {
    if (!ctx) {
      return
    }
    if (ctx.children) {
-     return ctx.children.map((child) => {
-       // @ts-expect-error 类型定义有问题
-       if (child.children && child.children.length != 0) {
-         // @ts-expect-error 类型定义有问题
-         return child.accept(this)
+     return ctx.children.map((child: ParseTree) => {
+       if (child instanceof ParserRuleContext && child.children?.length > 0) {
+         return child.accept(this)
        } else {
          return child.getText()
        }
      })
    }
  }
}

- // @ts-expect-error 类型定义有问题
- const result = tree.accept(new Visitor())
+ const result = tree.accept(new Visitor()) as Array<string | unknown>

Consider adding proper type definitions for your DSL parser and visitor to avoid type assertions and suppressions.

📝 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.

    class Visitor implements DSLVisitor<unknown> {
      visitChildren(ctx: ParserRuleContext): Array<string | unknown> | undefined {
        if (!ctx) {
          return
        }
        if (ctx.children) {
          return ctx.children.map((child: ParseTree) => {
            if (child instanceof ParserRuleContext && child.children?.length > 0) {
              return child.accept(this)
            } else {
              return child.getText()
            }
          })
        }
      }
    }
    const result = tree.accept(new Visitor()) as Array<string | unknown>
    return {

webui/src/views/settings/components/script/locale/en-US.ts (3)

15-17: 🛠️ Refactor suggestion

Add security warning for script editing.

The edit action should include a warning message similar to the delete confirmation, given the security implications of modifying scripts.

  'page.settings.tab.script.column.actions.edit': 'Edit',
  'page.settings.tab.script.column.actions.view': 'View',
  'page.settings.tab.script.column.actions.delete': 'Are you sure you want to delete this script?',
+ 'page.settings.tab.script.column.actions.edit.confirm': 'Are you sure you want to edit this script? Modified scripts will run with system privileges.',

📝 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.

  'page.settings.tab.script.column.actions.edit': 'Edit',
  'page.settings.tab.script.column.actions.view': 'View',
  'page.settings.tab.script.column.actions.delete': 'Are you sure you want to delete this script?',
  'page.settings.tab.script.column.actions.edit.confirm': 'Are you sure you want to edit this script? Modified scripts will run with system privileges.',

---

12-13: 🛠️ Refactor suggestion

Add explanation tooltip for Thread Safe property.

The "Thread Safe" column lacks an explanatory tooltip, unlike the "Cacheable" column. This technical term should be explained for users.

  'page.settings.tab.script.column.threadSafe': 'Thread Safe',
+ 'page.settings.tab.script.column.threadSafe.tips': 'Whether the script can safely run concurrently with other scripts without causing data corruption or unexpected behavior',

📝 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.

  'page.settings.tab.script.column.threadSafe': 'Thread Safe',
  'page.settings.tab.script.column.threadSafe.tips': 'Whether the script can safely run concurrently with other scripts without causing data corruption or unexpected behavior',
  'page.settings.tab.script.column.version': 'Version',

---

28-29: 🛠️ Refactor suggestion

Strengthen security warning message.

The security warning should be more explicit about potential risks and include specific examples.

  'page.settings.tab.script.warning.description':
-   'Scripts are powerful and can access system resources. Only add scripts from trusted sources as they run with system privileges.',
+   'Scripts run with system privileges and can access sensitive system resources including files, network, and processes. Only add scripts from trusted sources. All script executions are logged and monitored. Malicious scripts could compromise system security.',

📝 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.

  'page.settings.tab.script.warning.description':
    'Scripts run with system privileges and can access sensitive system resources including files, network, and processes. Only add scripts from trusted sources. All script executions are logged and monitored. Malicious scripts could compromise system security.',

webui/src/views/settings/components/info/components/btnAbilitiesModal.vue (1)

23-28: ⚠️ Potential issue

Enhance security for markdown rendering.

While markdown-it provides some basic security by escaping HTML by default, additional security measures are recommended when rendering remote content.

Consider these security enhancements:

-const md = new markdownit()
+const md = new markdownit({
+  html: false,        // Disable HTML tags in source
+  breaks: true,       // Convert '\n' in paragraphs into <br>
+  linkify: true,      // Autoconvert URL-like text to links
+  typographer: true,  // Enable smartquotes and other replacements
+  // Ensure links open in new tab and have security attributes
+  replaceLink: (link: string) => {
+    return link
+  }
+})
+
+// Add security headers to all links
+md.renderer.rules.link_open = (tokens, idx, options, env, self) => {
+  tokens[idx].attrPush(['rel', 'noopener noreferrer'])
+  tokens[idx].attrPush(['target', '_blank'])
+  return self.renderToken(tokens, idx, options)
+}

Committable suggestion skipped: line range outside the PR's diff.

webui/src/service/settings.ts (2)

98-105: ⚠️ Potential issue

Add security measures and error handling for heap dump functionality.

While the implementation is functional, heap dumps can contain sensitive information and should be handled with additional security measures.

Consider implementing:

1. Rate limiting to prevent abuse
2. Automatic cleanup of dump files
3. Error handling for URL construction

Apply this diff to improve the implementation:

 export async function GetHeapDumpFile() {
   const endpointStore = useEndpointStore()
   await endpointStore.serverAvailable
 
-  const url = new URL(urlJoin(endpointStore.endpoint, 'api/general/heapdump'), location.href)
-  url.searchParams.set('token', endpointStore.authToken)
-  return url
+  try {
+    const url = new URL(urlJoin(endpointStore.endpoint, 'api/general/heapdump'), location.href)
+    url.searchParams.set('token', endpointStore.authToken)
+    // Add a nonce to prevent caching of sensitive data
+    url.searchParams.set('nonce', Date.now().toString())
+    return url
+  } catch (error) {
+    throw new Error(`Failed to construct heap dump URL: ${error.message}`)
+  }
 }

Additionally, consider adding JSDoc comments to document:

• The security implications of heap dumps
• The expected cleanup process
• Usage guidelines for development/debugging only

Committable suggestion skipped: line range outside the PR's diff.

---

98-105: 💡 Codebase verification

Rate limiting is not implemented for heap dump endpoint

The heap dump endpoint is only protected by Role.USER_WRITE permission but lacks rate limiting protection. This could potentially be exploited to trigger multiple heap dumps in succession, which could impact system performance.

• src/main/java/com/ghostchu/peerbanhelper/module/impl/webapi/PBHGeneralController.java: The heap dump endpoint is only protected by Role.USER_WRITE permission
• No rate limiting middleware or configuration found for this sensitive operation
• Heap dumps are resource-intensive operations that should be rate-limited to prevent abuse

🔗 Analysis chain

Verify rate limiting implementation for heap dumps.

Let's verify if rate limiting is implemented for the heap dump endpoint.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for rate limiting implementation in the server code
# Expected: Find rate limiting middleware or configuration for the heap dump endpoint

# Search for heap dump route/controller definitions
echo "Searching for heap dump endpoint implementation..."
rg -l "heapdump" --type java --type kotlin --type ts

# Search for rate limiting configurations
echo "Searching for rate limiting configurations..."
rg -l "rate.*limit" --type yaml --type properties --type xml

Length of output: 478

---

Script:

#!/bin/bash
# Let's examine the heap dump endpoint implementation
echo "Checking PBHGeneralController.java implementation..."
rg -A 10 -B 10 "heapdump" "src/main/java/com/ghostchu/peerbanhelper/module/impl/webapi/PBHGeneralController.java"

# Search for any rate limiting annotations or configurations
echo "Searching for rate limiting related code..."
rg -l "RateLimit|@RateLimit|rateLimiter|RateLimiter"

# Check for Spring Security or similar security configurations
echo "Checking security configurations..."
fd -e yml -e yaml -e xml | xargs rg -l "security|protect|limit"

Length of output: 2699

---

Script:

#!/bin/bash
# Let's check the Role.USER_WRITE implementation and security context
echo "Checking Role implementation..."
rg -A 5 "enum Role" 

# Check web container security configuration
echo "Checking web container security setup..."
rg -A 10 "webContainer.javalin\(\)" 

# Check QBittorrentMainData for any rate limiting that might be reusable
echo "Checking existing rate limiting implementation..."
rg -A 10 "RateLimit" "src/main/java/com/ghostchu/peerbanhelper/downloader/impl/qbittorrent/impl/QBittorrentMainData.java"

Length of output: 32313

webui/src/views/settings/components/script/components/detailDrawer.vue (3)

78-83: ⚠️ Potential issue

Add error handling for the script content request.

The API call success callback should handle potential errors, and there's no error handling for failed requests.

 const { run, loading } = useRequest(GetScriptContent, {
   manual: true,
   onSuccess: (res) => {
+    if (!res?.data) {
+      Message.error(t('page.settings.tab.script.detail.error.load'))
+      return
+    }
     content.value = res.data
+  },
+  onError: (error) => {
+    Message.error(error.message || t('page.settings.tab.script.detail.error.unknown'))
   }
 })

📝 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.

const { run, loading } = useRequest(GetScriptContent, {
  manual: true,
  onSuccess: (res) => {
    if (!res?.data) {
      Message.error(t('page.settings.tab.script.detail.error.load'))
      return
    }
    content.value = res.data
  },
  onError: (error) => {
    Message.error(error.message || t('page.settings.tab.script.detail.error.unknown'))
  }
})

---

103-125: 🛠️ Refactor suggestion

Improve form handling and validation.

Several improvements can be made to the form handling:

1. Extract the .av extension as a constant
2. Add confirmation for unsaved changes
3. Enhance form validation

+const SCRIPT_EXTENSION = '.av'
+const hasUnsavedChanges = computed(() => {
+  if (isNew.value) return content.value.length > 0
+  return content.value !== originalContent.value
+})

 const handleOk = async () => {
   if (!isNew.value && viewOnly.value) {
     visible.value = false
     return true
   }
   if (isNew.value) {
     const validateError = await formRef.value?.validate()
-    if (validateError || form.name.length === 0) {
+    if (validateError || !form.name.trim()) {
+      Message.error(t('page.settings.tab.script.detail.error.name_required'))
       return false
     }
-    form.name = form.name + '.av'
+    form.name = form.name.trim() + SCRIPT_EXTENSION
   }
+  
+  try {
     const result = await UpsertScript(form.name, content.value)
     if (result.success) {
       Message.success(result.message)
       return true
     } else {
       Message.error(result.message)
       return false
     }
+  } catch (error) {
+    Message.error(t('page.settings.tab.script.detail.error.save'))
+    return false
+  }
 }

 const handleCancel = () => {
+  if (hasUnsavedChanges.value) {
+    Modal.confirm({
+      title: t('page.settings.tab.script.detail.confirm.unsaved.title'),
+      content: t('page.settings.tab.script.detail.confirm.unsaved.content'),
+      onOk: () => {
+        visible.value = false
+      }
+    })
+    return
+  }
   visible.value = false
 }

Committable suggestion skipped: line range outside the PR's diff.

---

85-99: 🛠️ Refactor suggestion

Add input validation and state cleanup.

The viewDetail method should validate the script ID format and ensure proper state cleanup.

 viewDetail: (id: string | undefined, readonly: boolean = false) => {
+    // Reset state
+    content.value = ''
+    form.name = ''
+    editId.value = ''
+
   if (!id) {
     isNew.value = true
-    form.name = ''
-    content.value = ''
     viewOnly.value = false
   } else {
+    // Validate script ID format
+    if (!/^[\w-]+$/.test(id)) {
+      Message.error(t('page.settings.tab.script.detail.error.invalid_id'))
+      return
+    }
     viewOnly.value = readonly
     isNew.value = false
     editId.value = id
     form.name = id
     run(editId.value)
   }
   visible.value = true
 }

Committable suggestion skipped: line range outside the PR's diff.

src/main/java/com/ghostchu/peerbanhelper/database/dao/impl/ScriptStorageDao.java (2)

22-42: 🛠️ Refactor suggestion

Improve error handling in put/get operations.

Consider the following improvements:

1. Return a boolean from put() to indicate success/failure
2. Differentiate between "not found" and "error" cases in get()

-    public void put(String key, String value) {
+    public boolean put(String key, String value) {
         ScriptStorageEntity entity = new ScriptStorageEntity(key, value);
         try {
             createOrUpdate(entity);
+            return true;
         } catch (SQLException e) {
             log.warn("Unable to create/update script storage entity", e);
+            return false;
         }
     }

-    public String get(String key) {
+    public Optional<String> get(String key) {
         try {
             var entity = queryForId(key);
             if (entity != null) {
-                return entity.getValue();
+                return Optional.of(entity.getValue());
             }
-            return null;
+            return Optional.empty();
         } catch (SQLException e) {
             log.warn("Unable to get script storage entity", e);
-            return null;
+            throw new RuntimeException("Database error while fetching script", e);
         }
     }

📝 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.

    public boolean put(String key, String value) {
        ScriptStorageEntity entity = new ScriptStorageEntity(key, value);
        try {
            createOrUpdate(entity);
            return true;
        } catch (SQLException e) {
            log.warn("Unable to create/update script storage entity", e);
            return false;
        }
    }

    public Optional<String> get(String key) {
        try {
            var entity = queryForId(key);
            if (entity != null) {
                return Optional.of(entity.getValue());
            }
            return Optional.empty();
        } catch (SQLException e) {
            log.warn("Unable to get script storage entity", e);
            throw new RuntimeException("Database error while fetching script", e);
        }
    }

---

62-78: ⚠️ Potential issue

Secure prefix search against SQL injection.

The current implementation might be vulnerable to SQL injection through the prefix parameter. Consider escaping special characters in the prefix or using prepared statements.

+    private String escapeLikePattern(String input) {
+        return input.replace("%", "\\%").replace("_", "\\_");
+    }

     public List<String> keysStartWith(String prefix) {
+        if (prefix == null) {
+            throw new IllegalArgumentException("Prefix cannot be null");
+        }
         try {
-            return queryBuilder().where().like("key", prefix + "%").query().stream().map(ScriptStorageEntity::getKey).toList();
+            String escapedPrefix = escapeLikePattern(prefix);
+            return queryBuilder().where().like("key", escapedPrefix + "%").query().stream().map(ScriptStorageEntity::getKey).toList();

Committable suggestion skipped: line range outside the PR's diff.

webui/src/views/charts/components/fieldPie.vue (1)

142-142: ⚠️ Potential issue

Watch for changes to the downloader prop.

The chart won't update when the downloader prop changes. Consider watching both the option and downloader prop.

-watch(option, (v) => {
-  run(v.field, v.enableThreshold, props.downloader)
-})
+watch(
+  [option, () => props.downloader],
+  ([opt, downloader]) => {
+    run(opt.field, opt.enableThreshold, downloader)
+  }
+)

📝 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.

watch(
  [option, () => props.downloader],
  ([opt, downloader]) => {
    run(opt.field, opt.enableThreshold, downloader)
  }
)

src/main/java/com/ghostchu/peerbanhelper/btn/BtnNetwork.java (1)

57-59: 💡 Codebase verification

Based on the shell script results, I can now provide a comprehensive response about the ScriptEngine security implementation:

Security measures need enhancement in AviatorScript implementation

The current implementation uses AviatorEvaluator with some basic security configurations, but several critical security measures are missing:

1. No explicit timeout controls for script execution
2. Reflection method access is broadly enabled via JavaMethodReflectionFunctionMissing
3. No resource usage limits (memory, CPU) beyond basic configuration
4. Missing sandboxing of script execution environment

Required actions:

• Add execution timeout controls using AviatorEvaluator.setOption(Options.TIMEOUT_SECONDS, ...)
• Restrict reflection access by implementing a custom FunctionMissing handler instead of using JavaMethodReflectionFunctionMissing
• Add resource limits through appropriate AviatorScript options
• Consider implementing a SecurityManager for script execution

🔗 Analysis chain

Verify ScriptEngine security measures.

While the constructor changes are technically correct, the introduction of a ScriptEngine component requires careful security consideration. Ensure that:

1. Only trusted scripts are allowed to execute
2. Proper input validation is implemented
3. Script execution is properly sandboxed
4. Resource limits are enforced

Let's check for security measures in the ScriptEngine implementation:

---

Let's analyze the actual implementation to understand how scripts are handled and what security measures are in place. The previous results showed usage of AviatorEvaluator, but we need more context about its configuration and usage patterns.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for security-related implementations in ScriptEngine
echo "Checking for security measures in ScriptEngine..."
rg -l "class ScriptEngine" | xargs rg -A 10 -B 10 "sandbox|security|validate|trust"

# Check for any security-related tests
echo "Checking for security-related tests..."
fd -e java -e groovy -e kotlin | rg "Test.*Script.*"

Length of output: 1314

---

Script:

#!/bin/bash
# Let's check the ScriptEngine implementation and usage more thoroughly
echo "Checking ScriptEngine implementation..."
rg -A 10 "class.*ScriptEngine"

echo "Checking AviatorEvaluator usage and configuration..."
rg -A 10 "AviatorEvaluator"

echo "Checking script loading and execution..."
rg -A 10 "compile.*script|execute.*script|eval.*script"

echo "Checking for any security configurations..."
fd -e yml -e yaml -e properties -e xml | xargs rg -l "script|aviator|security"

Length of output: 20666

src/main/java/com/ghostchu/peerbanhelper/web/JavalinWebContainer.java (2)

157-157: ⚠️ Potential issue

Implement robots.txt to prevent search engine indexing.

The robots.txt implementation is currently commented out. As per the PR objectives, this should be implemented to prevent search engine indexing. The current basic implementation can be enhanced with more specific rules.

Uncomment and enhance the implementation:

-//.get("/robots.txt", ctx -> ctx.result("User-agent: *\nDisallow: /"));
+.get("/robots.txt", ctx -> {
+    ctx.contentType("text/plain");
+    ctx.result("""
+        User-agent: *
+        Disallow: /
+        
+        # Explicitly disallow sensitive paths
+        Disallow: /route-overview
+        Disallow: /init
+        Disallow: /api/
+        """);
+});

📝 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.

                .get("/robots.txt", ctx -> {
                    ctx.contentType("text/plain");
                    ctx.result("""
                        User-agent: *
                        Disallow: /
                        
                        # Explicitly disallow sensitive paths
                        Disallow: /route-overview
                        Disallow: /init
                        Disallow: /api/
                        """);
                });

---

65-65: ⚠️ Potential issue

Security: Consider restricting route overview access in production.

The route overview plugin exposes all application endpoints at /route-overview, which could potentially leak sensitive information about your API structure. Consider:

1. Restricting this endpoint to development/testing environments only
2. Protecting it behind authentication
3. Disabling it in production

Apply this diff to make it configurable and secure:

-c.bundledPlugins.enableRouteOverview("/route-overview");
+if (Main.getMainConfig().getBoolean("server.debug-mode", false)) {
+    c.bundledPlugins.enableRouteOverview("/route-overview");
+}

Committable suggestion skipped: line range outside the PR's diff.

src/main/java/com/ghostchu/peerbanhelper/module/impl/webapi/PBHMetricsController.java (1)

55-79: 🛠️ Refactor suggestion

Consider performance optimizations and input validation.

While the implementation is functional, there are several areas for improvement:

1. The SQL query performs row-by-row aggregation in Java instead of using database-level aggregation.
2. Large date ranges could lead to memory issues as all records are loaded into memory.
3. The downloader parameter lacks input validation.

Consider this optimization:

-        Map<Long, AtomicInteger> bannedPeerTrends = new ConcurrentHashMap<>();
-        var queryBanned = historyDao.queryBuilder()
-                .selectColumns("id", "banAt")
-                .where()
-                .ge("banAt", timeQueryModel.startAt())
-                .and()
-                .le("banAt", timeQueryModel.endAt());
-        if (downloader != null && !downloader.isBlank()) {
-            queryBanned.and().eq("downloader", downloader);
-        }
-        try (var it = queryBanned.iterator()) {
-            while (it.hasNext()) {
-                var startOfDay = MiscUtil.getStartOfToday(it.next().getBanAt().getTime());
-                bannedPeerTrends.computeIfAbsent(startOfDay, k -> new AtomicInteger()).addAndGet(1);
-            }
-        }
+        var queryBanned = historyDao.queryBuilder()
+                .selectRaw("DATE(banAt) as ban_date, COUNT(*) as ban_count")
+                .where()
+                .ge("banAt", timeQueryModel.startAt())
+                .and()
+                .le("banAt", timeQueryModel.endAt());
+        if (downloader != null && !downloader.isBlank()) {
+            // Add input validation
+            if (!downloader.matches("^[a-zA-Z0-9._-]+$")) {
+                throw new IllegalArgumentException("Invalid downloader parameter");
+            }
+            queryBanned.and().eq("downloader", downloader);
+        }
+        queryBanned.groupBy("ban_date")
+                   .orderBy("ban_date", true);

Also consider:

1. Adding pagination support for large datasets
2. Adding date range validation to prevent excessive memory usage

Committable suggestion skipped: line range outside the PR's diff.

src/main/java/com/ghostchu/peerbanhelper/Main.java (1)

153-161: 🛠️ Refactor suggestion

Enhance logging level configuration robustness and clarity.

Several improvements can be made to the logging level configuration:

1. Add error logging in the catch block to track configuration issues
2. Validate the input logging level
3. Add documentation for the system property
4. Fix code formatting

Apply this diff to implement the suggested improvements:

-
-        try{
-            var targetLevel = System.getProperty("pbh.log.level");
-            if(targetLevel != null) {
-                var rootLogger = LoggerFactory.getLogger(Logger.ROOT_LOGGER_NAME);
-                ch.qos.logback.classic.Logger logbackLogger = (ch.qos.logback.classic.Logger) rootLogger;
-                logbackLogger.setLevel(Level.toLevel(targetLevel));
-            }
-        }catch (Throwable ignored){}
+        // Configure root logger level from system property 'pbh.log.level'
+        // Valid values: OFF, ERROR, WARN, INFO, DEBUG, TRACE
+        try {
+            String targetLevel = System.getProperty("pbh.log.level");
+            if (targetLevel != null) {
+                Level level = Level.toLevel(targetLevel, null);
+                if (level == null) {
+                    log.warn("Invalid log level specified in pbh.log.level: {}", targetLevel);
+                    return;
+                }
+                var rootLogger = LoggerFactory.getLogger(Logger.ROOT_LOGGER_NAME);
+                ch.qos.logback.classic.Logger logbackLogger = (ch.qos.logback.classic.Logger) rootLogger;
+                logbackLogger.setLevel(level);
+                log.info("Root logger level set to: {}", level);
+            }
+        } catch (Throwable e) {
+            log.error("Failed to configure logging level", e);
+        }

📝 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.

        // Configure root logger level from system property 'pbh.log.level'
        // Valid values: OFF, ERROR, WARN, INFO, DEBUG, TRACE
        try {
            String targetLevel = System.getProperty("pbh.log.level");
            if (targetLevel != null) {
                Level level = Level.toLevel(targetLevel, null);
                if (level == null) {
                    log.warn("Invalid log level specified in pbh.log.level: {}", targetLevel);
                    return;
                }
                var rootLogger = LoggerFactory.getLogger(Logger.ROOT_LOGGER_NAME);
                ch.qos.logback.classic.Logger logbackLogger = (ch.qos.logback.classic.Logger) rootLogger;
                logbackLogger.setLevel(level);
                log.info("Root logger level set to: {}", level);
            }
        } catch (Throwable e) {
            log.error("Failed to configure logging level", e);
        }

webui/src/views/settings/components/script/components/editor/aviatorscript/suggestions.ts (2)

3-3: ⚠️ Potential issue

Fix typo in parameter name

The parameter name has a typo: moncaco should be monaco.

-const getSuggestion = (moncaco: MonacoEditor) => [
+const getSuggestion = (monaco: MonacoEditor) => [

📝 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.

const getSuggestion = (monaco: MonacoEditor) => [

---

793-804: ⚠️ Potential issue

Add security warnings for module loading operations

The load and require operations can execute external code. Consider adding security warnings in the labels to inform users about potential risks.

-    label: 'load(path)',
+    label: 'load(path) ⚠️ Security: Loads and executes external code',
-    label: 'require(path)',
+    label: 'require(path) ⚠️ Security: Loads and executes external code',

📝 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.

  {
    label: 'load(path) ⚠️ Security: Loads and executes external code',
    kind: moncaco.languages.CompletionItemKind.Snippet,
    insertText: 'load($0)',
    insertTextRules: moncaco.languages.CompletionItemInsertTextRule.InsertAsSnippet
  },
  {
    label: 'require(path) ⚠️ Security: Loads and executes external code',
    kind: moncaco.languages.CompletionItemKind.Snippet,
    insertText: 'require($0)',
    insertTextRules: moncaco.languages.CompletionItemInsertTextRule.InsertAsSnippet
  }

src/main/java/com/ghostchu/peerbanhelper/util/rule/matcher/IPMatcher.java (1)

47-48: ⚠️ Potential issue

Prevent Potential NullPointerException by Ensuring ips Initialization

There is a risk that ips might be null when match0() is called, leading to a NullPointerException. Ensure that ips is properly initialized before usage or add a null check.

Apply this diff to add a null check:

 if (ip == null) return MatchResult.DEFAULT;
+ if (ips == null) {
+     log.warn("IPMatcher 'ips' is not initialized.");
+     return MatchResult.DEFAULT;
+ }
 if (ips.elementContains(ip)) {

Alternatively, ensure that setData() is always called before invoking match0().

📝 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.

        if (ip == null) return MatchResult.DEFAULT;
        if (ips == null) {
            log.warn("IPMatcher 'ips' is not initialized.");
            return MatchResult.DEFAULT;
        }
        if (ips.elementContains(ip)) {

webui/src/service/script.ts (4)

35-38: 🛠️ Refactor suggestion

Add error handling for HTTP responses to ensure robustness

The fetch calls currently do not check the HTTP response status before parsing the JSON. If the server returns an error status code (e.g., 4xx or 5xx), calling res.json() without checking res.ok may lead to unhandled exceptions or incorrect data handling.

Apply this diff to improve error handling:

In GetScriptList:

-return fetch(url, { headers: getCommonHeader() }).then((res) => {
-  return res.json()
-})
+const response = await fetch(url, { headers: getCommonHeader() });
+if (!response.ok) {
+  throw new Error(`HTTP error! status: ${response.status}`);
+}
+return response.json();

Repeat similar changes in GetScriptContent, DeleteScript, and UpsertScript functions to ensure errors are properly handled.

Also applies to: 45-48, 55-58, 71-74

---

27-30: 🛠️ Refactor suggestion

Simplify URL construction to avoid potential errors

The current use of new URL with urlJoin and location.href may lead to incorrect URL formation, especially if endpointStore.endpoint is an absolute URL. Simplifying the URL construction enhances readability and reduces the risk of errors.

Apply this diff to modify the URL construction:

-const url = new URL(
-  urlJoin(endpointStore.endpoint, 'api/expression-engine/scripts'),
-  location.href
-)
+const url = urlJoin(endpointStore.endpoint, 'api/expression-engine/scripts');

📝 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.

  const url = urlJoin(endpointStore.endpoint, 'api/expression-engine/scripts');

---

71-74: ⚠️ Potential issue

Specify 'Content-Type' header when sending request body

When sending data in the request body, it's essential to set the appropriate Content-Type header so the server can correctly interpret the payload. In this case, if content is a plain text script, you should set 'Content-Type': 'text/plain'.

Apply this diff to include the Content-Type header:

 return fetch(url, {
   headers: {
     ...getCommonHeader(),
+    'Content-Type': 'text/plain',
   },
   method: 'PUT',
   body: content,
 }).then((res) => {
   //...
 })

Committable suggestion skipped: line range outside the PR's diff.

---

11-18: ⚠️ Potential issue

Properly extend the Error class to ensure correct error handling

When extending the Error class in TypeScript, it's important to set up the constructor properly and call super() with a message parameter. This ensures that the error stack trace and message are correctly maintained. The current implementation lacks a constructor and manually assigns the name property, which may lead to unexpected behavior.

Apply this diff to fix the error class:

-export class InsecureNetworkError extends Error {
-  static name = 'InsecureNetworkError' as const
-  name = InsecureNetworkError.name
-
-  static is(err: unknown): err is InsecureNetworkError {
-    return (err as Error)?.name === InsecureNetworkError.name
-  }
-}
+export class InsecureNetworkError extends Error {
+  constructor(message?: string) {
+    super(message);
+    this.name = 'InsecureNetworkError';
+  }
+
+  static is(err: unknown): err is InsecureNetworkError {
+    return (err as Error)?.name === 'InsecureNetworkError';
+  }
+}

📝 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.

export class InsecureNetworkError extends Error {
  constructor(message?: string) {
    super(message);
    this.name = 'InsecureNetworkError';
  }

  static is(err: unknown): err is InsecureNetworkError {
    return (err as Error)?.name === 'InsecureNetworkError';
  }
}

webui/src/views/charts/index.vue (1)

20-23: ⚠️ Potential issue

Correct the spelling of plusWarpper to plusWrapper

The component name plusWarpper appears to be misspelled. It should likely be plusWrapper. This typo occurs in multiple places and can cause errors when importing or rendering the component.

Apply these diffs to fix the typos:

In the import statement at line 79:

-import plusWarpper from './components/plusWarpper.vue'
+import plusWrapper from './components/plusWrapper.vue'

In the template, replace all instances of <plusWarpper> and </plusWarpper> with <plusWrapper> and </plusWrapper>. For example:

-          <plusWarpper :title="t('page.charts.title.geoip')">
+          <plusWrapper :title="t('page.charts.title.geoip')">

-          </plusWarpper>
+          </plusWrapper>

Also applies to: 25-28, 55-58, 60-63, 65-68, 79-79

webui/src/views/settings/components/script/components/editor/index.vue (1)

86-91: 🛠️ Refactor suggestion

Avoid using non-null assertions with the ! operator

Using the non-null assertion ! operator can lead to runtime errors if the referenced value is actually null or undefined. It's safer to include null checks or default values to handle such cases.

Consider updating the code to include null checks:

- severity: monacoRef.value!.MarkerSeverity.Error,
+ severity: monacoRef.value?.MarkerSeverity.Error ?? monaco.MarkerSeverity.Error,

Apply similar changes to other instances where monacoRef.value! is used.

Also applies to: 97-102

src/main/java/com/ghostchu/peerbanhelper/btn/BtnRuleParsed.java (2)

98-100: ⚠️ Potential issue

Add exception handling for invalid IP addresses in parseIPRule method

If an invalid IP address string is encountered, IPAddressUtil.getIPAddress may throw an exception, which isn't currently caught. This could disrupt rule parsing.

Apply this diff to handle exceptions during IP address parsing:

public Map<String, IPMatcher> parseIPRule(Map<String, List<String>> raw) {
    Map<String, IPMatcher> rules = new HashMap<>();
-   raw.forEach((k, v) -> rules.put(k, new IPMatcher(version, k, v.stream().map(IPAddressUtil::getIPAddress).toList())));
+   raw.forEach((k, v) -> {
+       List<IPAddress> ipAddresses = new ArrayList<>();
+       for (String ipString : v) {
+           try {
+               IPAddress ipAddress = IPAddressUtil.getIPAddress(ipString);
+               ipAddresses.add(ipAddress);
+           } catch (Exception e) {
+               log.error("Invalid IP address '{}' in rule '{}': {}", ipString, k, e.getMessage());
+           }
+       }
+       if (!ipAddresses.isEmpty()) {
+           rules.put(k, new IPMatcher(version, k, ipAddresses));
+       } else {
+           log.warn("No valid IP addresses found for rule '{}'.", k);
+       }
+   });
    return rules;
}

📝 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.

    public Map<String, IPMatcher> parseIPRule(Map<String, List<String>> raw) {
        Map<String, IPMatcher> rules = new HashMap<>();
        raw.forEach((k, v) -> {
            List<IPAddress> ipAddresses = new ArrayList<>();
            for (String ipString : v) {
                try {
                    IPAddress ipAddress = IPAddressUtil.getIPAddress(ipString);
                    ipAddresses.add(ipAddress);
                } catch (Exception e) {
                    log.error("Invalid IP address '{}' in rule '{}': {}", ipString, k, e.getMessage());
                }
            }
            if (!ipAddresses.isEmpty()) {
                rules.put(k, new IPMatcher(version, k, ipAddresses));
            } else {
                log.warn("No valid IP addresses found for rule '{}'.", k);
            }
        });
        return rules;
    }

---

46-61: ⚠️ Potential issue

Handle possible null values for scriptRules in compileScripts method

If scriptRules is null, it could lead to a NullPointerException. Consider adding a null check to ensure robust error handling.

Apply this diff to add the null check:

private Map<String, CompiledScript> compileScripts(Map<String, String> scriptRules) {
+    if (scriptRules == null || scriptRules.isEmpty()) {
+        log.warn("No script rules to compile. 'scriptRules' is null or empty.");
+        return Collections.emptyMap();
+    }
    Map<String, CompiledScript> scripts = new HashMap<>();
    log.info(tlUI(Lang.BTN_RULES_SCRIPT_COMPILING, scriptRules.size()));
    long startAt = System.currentTimeMillis();
    scriptRules.forEach((name, content) -> {
        try {
            var script = scriptEngine.compileScript(null, name, content);
            if (script != null) {
                scripts.put(name, script);
            }
        } catch (Exception e) {
            log.error("Unable to load BTN script {}", name, e);
        }
    });
    log.info(tlUI(Lang.BTN_RULES_SCRIPT_COMPILED, scripts.size(), System.currentTimeMillis() - startAt));
    return scripts;
}

📝 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.

    private Map<String, CompiledScript> compileScripts(Map<String, String> scriptRules) {
        if (scriptRules == null || scriptRules.isEmpty()) {
            log.warn("No script rules to compile. 'scriptRules' is null or empty.");
            return Collections.emptyMap();
        }
        Map<String, CompiledScript> scripts = new HashMap<>();
        log.info(tlUI(Lang.BTN_RULES_SCRIPT_COMPILING, scriptRules.size()));
        long startAt = System.currentTimeMillis();
        scriptRules.forEach((name, content) -> {
            try {
                var script = scriptEngine.compileScript(null, name, content);
                if (script != null) {
                    scripts.put(name, script);
                }
            } catch (Exception e) {
                log.error("Unable to load BTN script {}", name, e);
            }
        });
        log.info(tlUI(Lang.BTN_RULES_SCRIPT_COMPILED, scripts.size(), System.currentTimeMillis() - startAt));
        return scripts;

src/main/java/com/ghostchu/peerbanhelper/scriptengine/ScriptEngine.java (2)

119-120: 🛠️ Refactor suggestion

Include exception details in the log message for better debugging

In the catch block of the compileScript method, the exception is caught but not included in the log message. Including the exception can provide more context and aid in troubleshooting.

Apply this diff to include the exception in the log:

} catch (Exception e) {
-    log.warn("Script Engine unable to compile the script: {}", fallbackName);
+    log.warn("Script Engine unable to compile the script: {}", fallbackName, e);
    return null;
}

📝 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.

            log.warn("Script Engine unable to compile the script: {}", fallbackName, e);
            return null;

---

92-93: ⚠️ Potential issue

Prevent IndexOutOfBoundsException when parsing script metadata

In the compileScript method, if a script line is only # or shorter, calling line.substring(2) may cause an IndexOutOfBoundsException. To ensure robustness, check the line length before performing substring(2).

Apply this diff to handle lines that are only a single #:

if (line.startsWith("#")) {
+    if (line.length() >= 2) {
         line = line.substring(2).trim();
+    } else {
+        continue;
+    }
    if (line.startsWith("@NAME")) {
        // ...

Committable suggestion skipped: line range outside the PR's diff.

src/main/java/com/ghostchu/peerbanhelper/btn/ability/BtnAbilityRules.java (2)

81-88: ⚠️ Potential issue

Possible NullPointerException in getDescription() method

The getDescription() method does not check if btnRule is null before accessing its methods. If btnRule is null, this will result in a NullPointerException.

Consider adding a null check to handle the scenario where btnRule may not be initialized:

 public TranslationComponent getDescription() {
+    if (btnRule == null) {
+        return new TranslationComponent(Lang.BTN_RULES_NOT_LOADED);
+    }
     return new TranslationComponent(Lang.BTN_ABILITY_RULES_DESCRIPTION,
             btnRule.getVersion(),
             btnRule.size(),
             btnRule.getIpRules().values().stream().mapToLong(IPMatcher::size).sum(),
             btnRule.getPeerIdRules().values().stream().mapToLong(List::size).sum(),
             btnRule.getClientNameRules().values().stream().mapToLong(List::size).sum(),
             btnRule.getPortRules().values().stream().mapToLong(List::size).sum(),
             btnRule.getScriptRules().values().size());
 }

Committable suggestion skipped: line range outside the PR's diff.

---

63-65: 🛠️ Refactor suggestion

Avoid catching Throwable; catch specific exceptions instead

Catching Throwable can mask serious errors like OutOfMemoryError and StackOverflowError, which are usually not meant to be caught. It's better to catch specific exceptions that are expected to occur.

Consider modifying the catch block to handle specific exceptions:

 try {
     BtnRule btnRule = JsonUtil.getGson().fromJson(Files.readString(btnCacheFile.toPath()), BtnRule.class);
     this.btnRule = new BtnRuleParsed(scriptEngine, btnRule);
- } catch (Throwable ignored) {
+ } catch (IOException | JsonSyntaxException e) {
+     log.error("Failed to load cache file", e);
 }

Committable suggestion skipped: line range outside the PR's diff.

webui/src/views/settings/components/script/components/scriptList.vue (2)

61-61: ⚠️ Potential issue

Typo in property name 'warp' should be 'wrap'

In the <a-space> component at line 61, the property warp is likely a typo. The correct property to enable content wrapping is wrap.

Apply this diff to correct the property name:

-            <a-space warp size="mini">
+            <a-space wrap size="mini">

📝 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.

        <a-space wrap size="mini">

---

12-13: ⚠️ Potential issue

Malformed closing tag for <a-button>

The closing tag for <a-button> is split across lines and is missing the closing angle bracket >. This may cause syntax or rendering issues.

Apply this diff to fix the closing tag:

-            {{ t('page.settings.tab.script.add') }}</a-button
-          >
+            {{ t('page.settings.tab.script.add') }}
+          </a-button>

📝 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.

          {{ t('page.settings.tab.script.add') }}
          </a-button>

src/main/java/com/ghostchu/peerbanhelper/database/dao/impl/HistoryDao.java (1)

100-107: 🛠️ Refactor suggestion

String Replacement May Introduce Logical Errors

The string replacement logic for constructing the SQL query can lead to errors if the field or downloader variables contain characters that interfere with SQL syntax. Relying on regex patterns for sanitization is not sufficient for preventing SQL injection.

Consider using parameter binding with prepared statements to ensure that all inputs are safely handled. This not only enhances security but also improves the readability and maintainability of your code.

src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/ExpressionRule.java (2)

88-92: ⚠️ Potential issue

Ensure the safety of file read and write operations.

The readScript and writeScript methods perform file read and write operations based on user input. Ensure that the scriptId parameter is properly validated and sanitized to prevent path traversal attacks or unauthorized file access.

[security_issue]
Review the implementation of the getIfAllowedScriptId method to ensure that it properly validates and sanitizes the scriptId parameter. Verify that the method correctly checks for path traversal attempts and restricts file access to the allowed script directory.

Also applies to: 111-115

---

220-220: ⚠️ Potential issue

Ensure proper error handling and logging in the shouldBanPeer method.

The shouldBanPeer method now iterates over the scripts list and executes each script using the runExpression method. Ensure that any exceptions or errors during script execution are properly caught, logged, and handled gracefully to prevent the method from abruptly terminating.

Review the implementation of the shouldBanPeer method to ensure that:

• Exceptions thrown during script execution are caught and logged appropriately.
• The method continues executing the remaining scripts even if an exception occurs in one script.
• The final CheckResult is correctly determined based on the results of all script executions.

Consider adding unit tests to validate the error handling behavior and the determination of the final CheckResult.

Also applies to: 222-222

src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/BtnNetworkOnline.java (1)

404-406: ⚠️ Potential issue

Potential NullPointerException in checkIpRule Method

The variable ipMatcher retrieved from rule.getIpRules().get(category) might be null. Calling ipMatcher.match(pa.toString()) without a null check could result in a NullPointerException. Please add a null check before using ipMatcher.

Apply this diff to fix the issue:

 var ipMatcher = rule.getIpRules().get(category);
+if (ipMatcher == null) {
+    continue;
+}
 MatchResult matchResult = ipMatcher.match(pa.toString());
 if (matchResult == MatchResult.TRUE) {
     // existing code
 }

📝 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.

            var ipMatcher = rule.getIpRules().get(category);
            if (ipMatcher == null) {
                continue;
            }
            MatchResult matchResult = ipMatcher.match(pa.toString());
            if (matchResult == MatchResult.TRUE) {

webui/src/views/settings/components/info/index.vue (5)

448-449: ⚠️ Potential issue

Ensure the multiClick component is accessible.

The multiClick component seems to require multiple clicks within a short time frame to trigger an action. Ensure that this interaction pattern is accessible to users with motor disabilities who might not be able to perform rapid clicks. Consider providing an alternative way to trigger the same action.

Some ideas to improve accessibility:

1. Provide a separate button to trigger the heap dump download directly, without the need for multiple clicks.

2. Allow users to customize the number of clicks and the time limit required to trigger the action, so they can adjust it based on their abilities.

3. Provide clear instructions and visual feedback during the interaction to help users understand what they need to do.

---

403-408: ⚠️ Potential issue

Obfuscate the App Secret.

The App Secret is sensitive information and should not be exposed in the UI directly. Obfuscate it or hide it completely.

Modify the code as follows:

-{{ btnStatus?.data.appSecret }}
+{{ btnStatus?.data.appSecret ? '••••••••••••••••••••' : '' }}

📝 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.

      <a-descriptions-item v-if="btnEnable?.data" label="App Secret">
        <a-skeleton-line v-if="btnStatusLoading.value" :rows="1" />
        <div v-else>
          {{ btnStatus?.data.appSecret ? '••••••••••••••••••••' : '' }}
        </div>
      </a-descriptions-item>

---

516-519: ⚠️ Potential issue

Handle loading state correctly.

The btnStatus request depends on the btnEnable request. Ensure that the statusLoading flag is set correctly based on both these requests to avoid any inconsistent UI states.

Modify the code as follows:

const { data: btnStatus, loading: statusLoading } = useRequest(GetBtnStatus, {
-  ready: computed(() => btnEnable.value?.data ?? false)
+  ready: computed(() => btnEnable.value?.data ?? false),
+  onSuccess: () => (btnLoading.value = false),
});

Committable suggestion skipped: line range outside the PR's diff.

---

378-387: ⚠️ Potential issue

Handle potential null values safely.

The btnStatus object might be null if the request to GetBtnStatus fails for some reason. Ensure that you handle this case safely to avoid null pointer exceptions.

Modify the code as follows:

-<a-typography-text v-else :type="btnStatus?.data.configSuccess ? 'success' : 'warning'">
+<a-typography-text v-else :type="btnStatus?.data?.configSuccess ? 'success' : 'warning'">
  {{
-   btnStatus?.data.configSuccess
+   btnStatus?.data?.configSuccess
      ? t('page.settings.tab.info.btn.status.success')
      : t('page.settings.tab.info.btn.status.fail')
  }}
</a-typography-text>

📝 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.

      <a-descriptions-item v-if="btnEnable?.data" :label="t('page.settings.tab.info.btn.status')">
        <a-skeleton-line v-if="btnStatusLoading.value" :rows="1" />
        <a-typography-text v-else :type="btnStatus?.data?.configSuccess ? 'success' : 'warning'">
          {{
            btnStatus?.data?.configSuccess
              ? t('page.settings.tab.info.btn.status.success')
              : t('page.settings.tab.info.btn.status.fail')
          }}
        </a-typography-text>
      </a-descriptions-item>

---

549-559: 🛠️ Refactor suggestion

⚠️ Potential issue

Ensure the heap dump download is non-blocking.

The downloadHeap function uses a setTimeout to delay the actual download by 1 second. This is probably done to allow the "Downloading heap dump..." message to be displayed to the user. However, this approach blocks the event loop and might cause performance issues if the heap dump file is large.

Consider using a non-blocking approach instead, such as:

1. Display the message to the user immediately.
2. Initiate the heap dump generation and wait for it to complete using a promise.
3. Once the heap dump is ready, initiate the download.

This way, the UI will remain responsive and the user will be informed about the progress.

Here's a suggested refactor:

const downloadHeap = async () => {
  Message.info(t('page.settings.tab.info.downloadHeap'))
-  setTimeout(async () => {
+  try {
+    Message.loading(t('page.settings.tab.info.generatingHeapDump'), { duration: 0 })
+    const url = await GetHeapDumpFile()
+    Message.success(t('page.settings.tab.info.heapDumpGenerated'))
     const a = document.createElement('a')
     a.href = url.toString()
     a.target = '_blank'
     a.download = 'heapdump.hprof.gz'
     a.click()
-  }, 1000)
+  } catch (err) {
+    Message.error(t('page.settings.tab.info.heapDumpError'))
+  }
}

And add the new messages to the localization files:

// en-US.ts
export default {
  page: {
    settings: {
      tab: {
        info: {
          downloadHeap: 'Downloading heap dump...',
+         generatingHeapDump: 'Generating heap dump...',
+         heapDumpGenerated: 'Heap dump generated',
+         heapDumpError: 'Error generating heap dump',
        },
      },
    },
  },
};

Committable suggestion skipped: line range outside the PR's diff.

webui/src/views/settings/components/script/components/editor/aviatorscript/grammar/DSLLexer.ts (2)

1-1: ⚠️ Potential issue

Avoid including absolute file paths in generated code comments

The generated code comment on line 1 includes an absolute file path workspace/2022-02-16-003239-023436000/DSL.g4, which may expose sensitive or unnecessary information. Consider modifying the generator settings to omit or obfuscate this path.

---

372-372: 🛠️ Refactor suggestion

Review type casting of serializedATN

The double casting serializedATN as unknown as number[] may indicate a type mismatch or an unnecessary workaround. Consider ensuring that serializedATN is of the correct type expected by ATNDeserializer().deserialize() to avoid potential runtime errors.

src/main/java/com/ghostchu/peerbanhelper/PeerBanHelperServer.java (2)

220-220: ⚠️ Potential issue

Potential Security Risk with JavaMethodReflectionFunctionMissing

Setting the function missing handler to JavaMethodReflectionFunctionMissing may allow scripts to invoke arbitrary methods via reflection, which poses a significant security risk if scripts can be influenced by untrusted sources.

Consider implementing a more restrictive FunctionMissing handler or ensuring that only trusted scripts are executed to mitigate this risk.

---

245-256: 🛠️ Refactor suggestion

Limit Exposed Functions to Necessary Methods

Registering all instance and static methods from the provided classes exposes all public methods to the scripting engine, which may unintentionally reveal sensitive operations.

Consider explicitly specifying which methods to expose or implementing a whitelist to limit the functions available to scripts. This reduces the attack surface and aligns with the principle of least privilege.