-
Notifications
You must be signed in to change notification settings - Fork 150
fix(fs): prevent directory creation in root and fix create_missing_pa… #54
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: main
Are you sure you want to change the base?
fix(fs): prevent directory creation in root and fix create_missing_pa… #54
Conversation
…rents flag
This commit addresses two related issues with the mkdir operation:
1. Root directory protection (403 Forbidden):
- Added 'cannot_create_in_root' error code to APIError.js
- Added validation in hl_mkdir.js to prevent creating directories in root
- Enhanced check to prevent create_missing_parents from creating non-user
directories at root level
- Changed from 500 Internal Server Error to proper 403 Forbidden
2. create_missing_parents flag not working (422 error):
- Fixed SDK path handling - now sends full path instead of splitting into
parent+basename
- Added support for snake_case 'create_missing_parents' parameter in SDK
- Made backend conditionally create or validate parents based on flag
- Previously always called _get_existing_parent regardless of flag value
Changes:
- src/backend/src/api/APIError.js: Added 'cannot_create_in_root' error
- src/backend/src/filesystem/hl_operations/hl_mkdir.js: Added root validation
and conditional parent creation logic
- src/puter-js/src/modules/FileSystem/operations/mkdir.js: Fixed path handling
and parameter support
| const is_user_dir = await first_component_node.isUserDirectory(); | ||
|
|
||
| // Only block if it's NOT a user directory (user directories are allowed) | ||
| if ( is_user_dir !== true ) { |
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.
nit:
| if ( is_user_dir !== true ) { | |
| if(is_user_dir){ |
| actor: values.actor, | ||
| }); | ||
| parent_node = values.create_missing_parents | ||
| ? await this._create_parents({ |
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.
will values.create_missing_parents ever return a falsy value? if it will not, then logic below will never run
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.
Yes, values.create_missing_parents can be falsy, and the _get_existing_parent branch will run in those cases. Here's why:
-
FlagParam Default Behavior
InFlagParam.js(line 26), when a flag isoptional: trueand no value is provided, it defaults tofalse:
this.default = this.options.default ?? false; -
Router Handling
In themkdir.jsrouter (lines 79-82),boolify()is used which can returnfalse:
create_missing_parents: boolify(
req.body.create_missing_ancestors ??
req.body.create_missing_parents
)If neither create_missing_ancestors nor create_missing_parents is provided in the request body, or if they're explicitly set to false, the value will be false.
- When the Branch Runs
The_get_existing_parentbranch will execute when:
create_missing_parentsis not provided in the request (defaults tofalse)create_missing_parentsis explicitly set tofalse- The client doesn't pass the flag at all
The ternary at lines 317-324 is correct and both branches are reachable. The _get_existing_parent path is used when the caller expects all parent directories to already exist, which is a valid use case.
israx
left a comment
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.
good job so far
Summary
This PR fixes two critical bugs in the
mkdirfilesystem operation:403 Forbiddeninstead of500 Internal Server Error422 Unprocessable EntityProblem
Issue #1: Incorrect Error Status for Root Directory Operations
When users attempt to create directories in the root directory (e.g.,
puter.fs.mkdir('/foo')), the system returns a500 Internal Server Error. This is problematic because:500status codes indicate unexpected server failuresIssue #2: create_missing_parents Flag Not Working
The
create_missing_parentsflag was returning422 dest_does_not_existerrors instead of creating intermediate directories due to:parent+basenamebefore sending to API_get_existing_parentregardless of flag valueSolution
Changes Made
1. Backend - API Error Definition (
src/backend/src/api/APIError.js)cannot_create_in_rootwith403status2. Backend - Root Directory Validation (
src/backend/src/filesystem/hl_operations/hl_mkdir.js)create_missing_parentsfrom creating non-user directories at rootcreate_missing_parentsflag3. SDK - Path Handling Fix (
src/puter-js/src/modules/FileSystem/operations/mkdir.js)parent+basenameto sending fullpathcreate_missing_parentsparametermoveandcopyoperationsTesting
All test cases verified and passing:
mkdir('/testroot')mkdir('/testroot2', {create_missing_parents: true})mkdir('/testroot3/nested', {create_missing_parents: true})mkdir('/username/Videos/testdir')mkdir('/username/Videos/testdir/nested')mkdir('/username/Videos/testdir2/nested', {create_missing_parents: true})Technical Details
Why 403 Instead of 422?
403 Forbiddenmore accurately represents a permission/policy restrictionBackward Compatibility
parentparameter for batch operations (file uploads)Implementation Pattern
hl_write.jsfor consistencyFSNodeContext.isRootproperty for reliable root detectionChecklist
APIError.jswith status 403hl_mkdir.jscreate_missing_parentsfrom bypassing protectionRelated Files
src/backend/src/api/APIError.js- Error code definitionssrc/backend/src/filesystem/hl_operations/hl_mkdir.js- High-level mkdir operationsrc/puter-js/src/modules/FileSystem/operations/mkdir.js- SDK mkdir implementationsrc/backend/src/filesystem/hl_operations/hl_write.js- Reference implementationsrc/backend/src/filesystem/FSNodeContext.js- isRoot property definitionFixes: Root directory mkdir operations returning 500 instead of 403
Fixes: create_missing_parents flag not creating intermediate directories