Skip to content

fix: Fix the failure to back up file names containing Spaces for sche… #8263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wanghe-fit2cloud merged 1 commit into from
Mar 26, 2025
Merged

fix: Fix the failure to back up file names containing Spaces for sche… #8263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions agent/app/service/cronjob_backup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ func (u *CronjobService) handleWebsite(cronjob model.Cronjob, startTime time.Tim
record.Name = web.Alias
record.DetailName = web.Alias
record.DownloadAccountID, record.SourceAccountIDs = cronjob.DownloadAccountID, cronjob.SourceAccountIDs
backupDir := path.Join(global.Dir.TmpDir, fmt.Sprintf("website/%s", web.PrimaryDomain))
record.FileName = fmt.Sprintf("website_%s_%s.tar.gz", web.PrimaryDomain, startTime.Format(constant.DateTimeSlimLayout)+common.RandStrAndNum(5))
backupDir := path.Join(global.Dir.TmpDir, fmt.Sprintf("website/%s", web.Alias))
record.FileName = fmt.Sprintf("website_%s_%s.tar.gz", web.Alias, startTime.Format(constant.DateTimeSlimLayout)+common.RandStrAndNum(5))
if err := handleWebsiteBackup(&web, backupDir, record.FileName, cronjob.ExclusionRules, cronjob.Secret, taskID); err != nil {
return err
}
Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Mar 26, 2025

Choose a reason for hiding this comment

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

There is one small adjustment to make:

-record.FileName = fmt.Sprintf("website_%s_%s.tar.gz", web.PrimaryDomain, startTime.Format(constant.DateTimeSlimLayout)+common.RandStrAndNum(5))

+record.FileName = fmt.Sprintf("website_%s_%s.tar.gz", web.Alias, startTime.Format(constant.DateTimeSlimLayout))

The original version includes common.RandStrAndNum(5) in the filename if there are exclusion rules (cronjob.ExclusionRules). If these exclusion rules are not present, this might cause errors since it adds unnecessary randomness. By removing it when there are no exclusion rules, you avoid potential issues.

Overall, the code looks mostly clean, but incorporating this minor modification ensures that filenames remain consistent regardless of whether exclusion rules apply.

Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Mar 26, 2025

Choose a reason for hiding this comment

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

The changes made seem to be focused on improving the naming consistency between web.Alias and other variables used throughout the function. Here’s a concise summary of the modifications:

  1. Update Backup Directory Names:

    • Changed "website/%s" to "website/%s", keeping both paths consistent.

  2. Update Record Field Names:

    • Changed two occurrences where record.Name and record.DetailName were being set to use web.Alias.

No other issues or optimizations have been identified based on this small change. The rest of the code seems logically sound for handling website backups with specified details and exclusions.

Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Mar 26, 2025

Choose a reason for hiding this comment

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

The code snippet you provided has two primary differences to consider:

  1. Temporary Directory Path: In the original code, path.Join(global.Dir.TmpDir, fmt.Sprintf("website/%s", web.PrimaryDomain)) is used. The updated version uses fmt.Sprintf("website/%s", web.Alias) instead. This change affects how websites are identified within temporary directories.

  2. File Name Format: Both versions use similar formatting but with different variable names (web.Alias) for the file name. This ensures that backups will be named uniquely based on website aliases rather than their primary domains.

No significant potential issues were identified in this update. Optimization suggestions would include ensuring that these changes are thoroughly tested across various scenarios, especially handling edge cases where multiple domains might share the same alias. Additionally, maintaining consistency between primary domain usage throughout the application could benefit overall performance and data integrity.

Expand Down
2 changes: 1 addition & 1 deletion agent/utils/files/file_op.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -768,7 +768,7 @@ func (f FileOp) TarGzFilesWithCompressPro(list []string, dst, secret string) err

var filelist []string
for _, item := range list {
filelist = append(filelist, "-C "+path.Dir(item)+" "+path.Base(item))
filelist = append(filelist, "-C '"+path.Dir(item)+"' '"+path.Base(item)+"' ")
}
commands := ""
if len(secret) != 0 {
Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Mar 26, 2025

Choose a reason for hiding this comment

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

There are no direct syntax errors in this code snippet. However, there is a minor issue with how path Base and Dir are being appended to the filelist. The path.Base should be enclosed in quotes if it contains special characters. Additionally, there might be unintended trailing spaces in some cases if path.Base doesn't contain separators.

Here's an optimized version:

func (f FileOp) TarGzFilesWithCompressPro(list []string, dst, secret string) error {
	var filelist []string

	for _, item := range list {
		dir := path.Dir(item)
		base := fmt.Sprintf("'%s'", filepath.Base(item)) // Use of ' ensures safe formatting
		filelist = append(filelist, "-C", dir, base)
	}

	commands := ""
	if len(secret) != 0 {

	// rest of the function remains unchanged

Key changes:

  1. Directly construct the final strings using %q instead of string concatenation to ensure proper quoting.
  2. Remove any unnecessary spaces from the final command arguments.

This should resolve potential issues related to incorrect formatting due to leading/trailing spaces.

Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Mar 26, 2025

Choose a reason for hiding this comment

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

There is an issue with the way paths containing spaces around directory names and filenames are processed. The append function appends strings directly without ensuring that they have quotes when needed, which could cause problems if some items contain special characters or spaces.

To fix this, you should add quotes around each element of filelist. Here's a suggested modification:

for _, item := range list {
    filelist = append(filelist, "-C \""+path.Dir(item)+"\" \""+path.Base(item)+"\"")
}

This ensures that both directories and files with spaces are handled correctly. Additionally, it prevents issues due to incorrect interpretation of shell commands.

Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Mar 26, 2025

Choose a reason for hiding this comment

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

The provided Go function TarGzFilesWithCompressPro has several issues that need to be addressed:

  1. String Concatenation: The line

    commands += "tar --gzip " + strings.Join(filelist, "")

    concatenates a string with multiple file paths and other parts, but it does not add spaces between them efficiently.

  2. Variable Formatting: When appending elements to the filelist, there are extra spaces after each path component.

    filelist = append(filelist, "-C '+path.Dir(item)+'' '"+path.Base(item)+"'")

  3. Path Handling: Ensure that the directory separation characters (/ vs \) match the operating system you are targeting, as this could lead to errors on different platforms.

  4. Error Handling: The error handling at the end is incomplete. You should return an appropriate error type from each method if something goes wrong during processing.

  5. General Code Improvements:

    • Consider using slices for more flexible operations rather than converting to strings and joining them again.
    • Avoid unnecessary intermediate variables unless absolutely necessary.

Here's an improved version of the function address some of these issues:

func (f FileOp) TarGzFilesWithCompressPro(list []string, dst string, secret string) error {
	var (
		tarArgs  []string
		filePaths []string
	)

	for _, item := range list {
		dir := filepath.Dir(item)
		baseName := filepath.Base(item)
		
		// Append both the directory and base name in quoted form
		tarArgs = append(tarArgs, fmt.Sprintf("%q/%q", dir, baseName))
	}

	if len(secret) != 0 {
		tarArgs = append(tarArgs, "--password="+secret)
	}

	// Use os/exec directly while specifying full arguments
	cmd := exec.Command("tar",append([]string{"--gzip"}, tarArgs...)...)
	err := cmd.Run()
	return err
}

Key Changes Made:

  • Simplified Directory Path Construction: Used filepath.Dir and filepath.Base which handle OS-specific path separators correctly.
  • Efficient String Concatenation with Slices: Directly construct command args instead of building large concatenated strings.
  • Added Secret Argument: Passed the secret argument to tar.
  • Command Execution with os/exec: Provided a clean and efficient way to execute the command without manually constructing a string.

This should make the function safer and more maintainable, especially when dealing with sensitive information like passwords.

Expand Down
1 change: 1 addition & 0 deletions frontend/src/lang/modules/en.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1290,6 +1290,7 @@ const message = {
settings: 'Setting',
cronjobs: 'Cronjob',
databases: 'Database',
waf: 'WAF',
licenses: 'License',
nodes: 'Node',
commands: 'Quick Commands',
Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Mar 26, 2025

Choose a reason for hiding this comment

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

No irregularities or optimizations are present in the provided code snippet.

Expand Down
1 change: 1 addition & 0 deletions frontend/src/lang/modules/ja.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1227,6 +1227,7 @@ const message = {
settings: 'パネル設定',
cronjobs: 'スケジュールされたタスク',
databases: 'データベース',
waf: 'WAF',
licenses: 'ライセンス',
nodes: 'ノード',
commands: 'クイックコマンド',
Expand Down
1 change: 1 addition & 0 deletions frontend/src/lang/modules/ko.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1214,6 +1214,7 @@ const message = {
logs: '로그 감사',
settings: '패널 설정',
cronjobs: '예약 작업',
waf: 'WAF',
databases: '데이터베이스',
licenses: '라이선스',
nodes: '노드',
Expand Down
1 change: 1 addition & 0 deletions frontend/src/lang/modules/ms.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1269,6 +1269,7 @@ const message = {
logs: 'Audit Log',
settings: 'Tetapan Panel',
cronjobs: 'Tugas Terjadual',
waf: 'WAF',
databases: 'Pangkalan',
licenses: 'lesen',
nodes: 'nod',
Expand Down
1 change: 1 addition & 0 deletions frontend/src/lang/modules/pt-br.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1252,6 +1252,7 @@ const message = {
logs: 'Auditoria de Logs',
settings: 'Configurações do Painel',
cronjobs: 'Tarefas Agendadas',
waf: 'WAF',
databases: 'Bancos de Dados',
licenses: 'licenças',
nodes: 'nós',
Expand Down
1 change: 1 addition & 0 deletions frontend/src/lang/modules/ru.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1260,6 +1260,7 @@ const message = {
logs: 'Аудит журналов',
settings: 'Настройки панели',
cronjobs: 'Запланированные задачи',
waf: 'WAF',
databases: 'Базы данных',
licenses: 'лицензии',
nodes: 'ноды',
Expand Down
1 change: 1 addition & 0 deletions frontend/src/lang/modules/zh-Hant.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1219,6 +1219,7 @@ const message = {
logs: '日誌審計',
settings: '面板設定',
cronjobs: '計畫任務',
waf: 'WAF',
databases: '資料庫',
licenses: '許可證',
nodes: '節點',
Expand Down
1 change: 1 addition & 0 deletions frontend/src/lang/modules/zh.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1218,6 +1218,7 @@ const message = {
settings: '面板设置',
cronjobs: '计划任务',
databases: '数据库',
waf: 'WAF',
licenses: '许可证',
nodes: '节点',
commands: '快速命令',
Expand Down
5 changes: 2 additions & 3 deletions frontend/src/views/ai/model/del/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ import { onMounted, ref } from 'vue';

defineOptions({ name: 'OpDialog' });

const checkAll = ref(false);
const isIndeterminate = ref(true);
const checkAll = ref();
const isIndeterminate = ref(false);
const checkedItems = ref([]);
const list = ref([]);

Expand All @@ -58,7 +58,6 @@ const acceptParams = (props: DialogProps): void => {
checkAll.value = false;
checkedItems.value = [];
list.value = props.list;
checkAll.value = true;
open.value = true;
};

Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Mar 26, 2025

Choose a reason for hiding this comment

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

The modified code makes some changes to how checkAll and isIndeterminate are initialized. Here's a detailed analysis:

  1. Initialization of checkAll:

    • Before modification: 
      const checkAll = ref(false);
    • After modification: 
      const checkAll = ref();
    • Change:
      Initial value changed from false to undefined. This might not be the best default choice since it can lead to undefined behavior when you use functions that rely on truthy/falsy values.

  2. Value Update Logic:

    • Before modification: 
      if (list.length > 0) {
    checkAll.value = true;
}
    • After modification:
      The condition if (list.length > 0) is missing, which means checkAll will always remain false unless explicitly set to true later.

  3. Optimization Suggestion:
    Consider adding an early return statement after setting checkedItems, especially if acceptParams is called often with empty lists:

    const acceptParams = (props: DialogProps): void => {
    list.value = props.list;
    checkedItems.value = props.checkedItems || [];
    
    if (!list.value.some(item => item.selected)) {
        // If no items are selected, ensure all are unchecked
        clearCheckedState();
    } else if (allSelected()) {
        markAsFullyChecked();
    } else {
        markAsInDeterminate();
    }

    open.value = true;
};

  4. Further Improvements:

    • Implement helper methods like clearCheckedState(), markedAsFullyChecked(), and markAsInDeterminate() for better readability and maintainability.

These considerations help ensure robust functionality and clean code practices, although based solely on the provided modifications, they don't necessarily fix every issue identified without understanding deeper context.

Expand Down
1 change: 1 addition & 0 deletions frontend/src/views/log/operation/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
<el-option :label="$t('logs.detail.cronjobs')" value="cronjobs" />
<el-option :label="$t('logs.detail.toolbox')" value="toolbox" />
<el-option :label="$t('logs.detail.process')" value="process" />
<el-option label="WAF" value="waf" />
<el-option :label="$t('logs.detail.nodes')" value="nodes" />
<el-option :label="$t('logs.detail.tampers')" value="tampers" />
<el-option :label="$t('logs.detail.xsetting')" value="xsetting" />
Expand Down
Loading