-
Notifications
You must be signed in to change notification settings - Fork 39
408 - Create world #423
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.
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
base: master
Are you sure you want to change the base?
408 - Create world #423
Conversation
|
||
// Get the file contents | ||
fileContents, err := os.ReadFile(fullPath) | ||
fileContents, err = os.ReadFile(fullPath) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
Copilot Autofix
AI 6 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
if !strings.HasSuffix(datafilesPath, `default`) { | ||
// try with the last folder name replaced with "default": | ||
fullPath = util.FilePath(filepath.Dir(datafilesPath), `/`, `default`, `/`, tplInfo.path) | ||
fileContents, err = os.ReadFile(fullPath) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 days ago
To fix the issue, we need to validate and sanitize the user input (tplInfo.path
) before using it to construct the file path. Specifically:
- Ensure that the input does not contain any path traversal sequences (
..
) or path separators (/
or\
). - Use
filepath.Clean
to normalize the path and ensure it remains within a designated safe directory. - Reject any input that does not meet the validation criteria.
The changes will be made in internal/templates/templates.go
where tplInfo.path
is used to construct fullPath
.
-
Copy modified lines R233-R239
@@ -232,3 +232,9 @@ | ||
|
||
fullPath := util.FilePath(datafilesPath, `/`, tplInfo.path) | ||
// Validate and sanitize tplInfo.path to prevent path traversal | ||
cleanPath := filepath.Clean(tplInfo.path) | ||
if strings.Contains(cleanPath, "..") || strings.HasPrefix(cleanPath, "/") || strings.HasPrefix(cleanPath, "\\") { | ||
mudlog.Error("invalid template path", "module", false, "filepath", tplInfo.path) | ||
continue | ||
} | ||
fullPath := util.FilePath(datafilesPath, `/`, cleanPath) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds command line arguments to create a new world, implementing functionality to copy the "empty" world template and update configuration accordingly. The changes support creating custom worlds while providing a fallback template system for missing files.
- Adds
--create-world
and--list-worlds
command line flags for world management - Removes template files from the empty world to establish it as a clean template
- Implements template fallback to default world when templates are missing
Reviewed Changes
Copilot reviewed 208 out of 208 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
internal/worldcli/worldcli.go |
New module containing world creation and listing functionality |
internal/templates/templates.go |
Enhanced template resolution with fallback to default world |
internal/flags/flags.go |
Added command line flag handling for world operations |
_datafiles/world/empty/users/1.yaml |
Updated admin user configuration for empty world |
_datafiles/world/empty/templates/* |
Removed all template files to establish empty as clean template |
folderName := sanitizeWorldName(worldName) | ||
if folderName == "" { | ||
mudlog.Error("World Creation", "error", "Invalid world name (reserved names: 'default', 'empty')") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sanitizeWorldName
function returns an empty string for reserved names ('default', 'empty'), but the error message on line 39 suggests these are invalid. This could be confusing - consider making the error message more specific about reserved names versus truly invalid names.
folderName := sanitizeWorldName(worldName) | |
if folderName == "" { | |
mudlog.Error("World Creation", "error", "Invalid world name (reserved names: 'default', 'empty')") | |
// Check for reserved names before sanitizing | |
lowerName := strings.ToLower(worldName) | |
if lowerName == "default" || lowerName == "empty" { | |
mudlog.Error("World Creation", "error", fmt.Sprintf("'%s' is a reserved world name. Please choose a different name.", worldName)) | |
return | |
} | |
folderName := sanitizeWorldName(worldName) | |
if folderName == "" { | |
mudlog.Error("World Creation", "error", "Invalid world name. Please use only letters, numbers, underscores, and hyphens.") |
Copilot uses AI. Check for mistakes.
|
||
if err := updateConfigForNewWorld(folderName); err != nil { | ||
mudlog.Error("World Creation", "error", "Failed to update config: "+err.Error()) | ||
os.RemoveAll(worldPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using os.RemoveAll
without proper validation could be dangerous if worldPath
is manipulated. Consider adding additional validation to ensure worldPath
is within the expected directory structure before removal.
os.RemoveAll(worldPath) | |
baseWorldDir := filepath.Join("_datafiles", "world") | |
isSafe, err := isSubdirectory(baseWorldDir, worldPath) | |
if err != nil { | |
mudlog.Error("World Creation", "error", "Failed to validate world path for removal: "+err.Error()) | |
} else if isSafe { | |
os.RemoveAll(worldPath) | |
} else { | |
mudlog.Error("World Creation", "error", "Unsafe world path detected, not removing: "+worldPath) | |
} |
Copilot uses AI. Check for mistakes.
dataFilesRegex := regexp.MustCompile(`(\s*DataFiles:\s*)_datafiles/world/[^\s]+`) | ||
configText = dataFilesRegex.ReplaceAllString(configText, "${1}"+newWorldPath) | ||
|
||
langPathRegex := regexp.MustCompile(`(\s*LanguagePaths:\s*\n\s*-\s*'_datafiles/localize'\s*\n\s*-\s*)'_datafiles/world/[^/]+/localize'`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex pattern is complex and brittle. Consider using a YAML parser library instead of regex for modifying configuration files, which would be more robust and maintainable.
Copilot uses AI. Check for mistakes.
langPathRegex := regexp.MustCompile(`(\s*LanguagePaths:\s*\n\s*-\s*'_datafiles/localize'\s*\n\s*-\s*)'_datafiles/world/[^/]+/localize'`) | ||
configText = langPathRegex.ReplaceAllString(configText, "${1}'"+newWorldPath+"/localize'") | ||
|
||
if err := os.WriteFile(configPath, []byte(configText), 0644); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file permissions 0644 allow world-readable access to the configuration file. Consider using more restrictive permissions like 0600 if the config contains sensitive information.
if err := os.WriteFile(configPath, []byte(configText), 0644); err != nil { | |
if err := os.WriteFile(configPath, []byte(configText), 0600); err != nil { |
Copilot uses AI. Check for mistakes.
return true | ||
} | ||
|
||
if !strings.HasSuffix(datafilesPath, `default`) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string suffix check for 'default' could match unintended paths like 'mydefault'. Consider using filepath.Base()
to check only the directory name, or use a more precise path comparison.
if !strings.HasSuffix(datafilesPath, `default`) { | |
if filepath.Base(datafilesPath) != `default` { |
Copilot uses AI. Check for mistakes.
Description
Adds command line arguments to create a new world.
This just copies the "empty" world currently and updates the config accordingly.
Still some work to be done here.
Changes
--create-world
and--list-worlds
flagsempty
worlddefault
world's templates.Links