feat: custom model (Final2x-core v4.0.0)#683
Conversation
Summary of ChangesHello @Tohrusky, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the user experience for selecting and managing custom Super-Resolution models within the Final2x application by upgrading the UI components. Additionally, it includes routine updates to various project dependencies, ensuring the codebase remains current and stable. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces the ability for users to specify custom models by adding the tag prop to the model selection component, and also updates several dependencies. The change to allow custom models is a good feature, but it introduces a potential security vulnerability. My review includes a comment with high severity to address a path traversal risk, with a detailed explanation and code examples on how to fix it. The dependency updates in package.json look fine.
| <n-select | ||
| v-model:value="selectedSRModel" | ||
| :options="modelOptions" | ||
| filterable | ||
| tag | ||
| clearable | ||
| style="width: 465px" | ||
| /> |
There was a problem hiding this comment.
Adding the tag prop is a great way to allow custom models. However, it introduces a potential security vulnerability. Since users can input any string, a malicious user could provide a path traversal payload (e.g., ../../some/other/file). If the backend Final2x-core uses this input to construct a file path without proper sanitization, it could lead to reading or executing arbitrary files on the user's system.
To mitigate this, you should validate the input to ensure it doesn't contain path characters like /, \, or ...
I recommend replacing v-model:value with a combination of :value and @update:value to intercept and validate the input before updating the store.
You can modify the <n-select> component like this:
<n-select
:value="selectedSRModel"
@update:value="handleModelUpdate"
:options="modelOptions"
filterable
tag
clearable
style="width: 465px"
/>And then, add the following to your <script setup> section. You'll need to import useNotification from naive-ui and get the notification instance.
import { useNotification } from 'naive-ui'
// ...
const notification = useNotification()
function handleModelUpdate(value: string | null): void {
// Allow clearing the value
if (value === null) {
selectedSRModel.value = value
return
}
const isExistingOption = modelOptions.some(option => option.value === value)
// If it's a new custom model (not in the predefined options), validate it
if (!isExistingOption && (value.includes('/') || value.includes('\\') || value.includes('..'))) {
notification.error({
// Consider adding a dedicated translation key for this message
content: 'Invalid custom model name. It cannot contain path characters.',
duration: 3000
})
// By not updating `selectedSRModel.value`, the select input will revert to the previous valid value.
} else {
selectedSRModel.value = value
}
}App.vue already wraps the app in NNotificationProvider, so useNotification is ready to be used.
No description provided.