Skip to content

Conversation

@cnlangzi
Copy link
Contributor

@cnlangzi cnlangzi commented Mar 7, 2022

Changes

Fixes

  • it will be rejected if a file is uploaded inside a file

Tests

Tasks to complete before merging PR:

  • Ensure system tests are passing. If not Run them manually to check for any regressions 📋
  • Do any new system tests need added to test this change? do any existing system tests need updated? If so create a PR at 0chain/system_test
  • Merge your system tests PR to master AFTER merging this PR

Associated PRs (Link as appropriate):

@cnlangzi cnlangzi requested a review from peterlimg March 7, 2022 08:53
@cnlangzi cnlangzi linked an issue Mar 7, 2022 that may be closed by this pull request
@lpoli
Copy link
Contributor

lpoli commented Mar 7, 2022

@cnlangzi Actually this won't work. First of all, util.SplitPath does not return path's values for parent path but for file path i.e. for /a/b/c/d/e.txt it returns [a b c d e.txt] instead of [a b c d].

The other important missed point is, if there is already a file with path /a/b/c.txt and user uploaded a file with path /a/b/c.txt/d/e/f/g.txt then we cannot only check against parent path of the filepath. So we need to check for /a, /a/b, /a/b/c.txt, ....

I had considered this as well. If you make this changes then I will drop my PR.

@cnlangzi
Copy link
Contributor Author

cnlangzi commented Mar 7, 2022

@cnlangzi Actually this won't work. First of all, util.SplitPath does not return path's values for parent path but for file path i.e. for /a/b/c/d/e.txt it returns [a b c d e.txt] instead of [a b c d].

The other important missed point is, if there is already a file with path /a/b/c.txt and user uploaded a file with path /a/b/c.txt/d/e/f/g.txt then we cannot only check against parent path of the filepath. So we need to check for /a, /a/b, /a/b/c.txt, ....

I had considered this as well. If you make this changes then I will drop my PR.

fixed both of them.


// Path get current dir path
func (d *RefWalker) Path() string {
if d == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should return error when d is nil, or when index is >= length, otherwise users will have to check the code to know what happened when the returned value is "".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

}

// Parent get curerent parent path
func (d *RefWalker) Parent() string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as Path(), we'd better return errors on "" strings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

name: "nested directories",
path: "/dir1/dir2/dir3/file1",
items: []string{"dir1", "dir2", "dir3", "file1"},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding cases for empty subpath perhaps, e.g. /dir1/dir2//dir3///dir4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@lpoli lpoli removed a link to an issue Mar 8, 2022
@cnlangzi cnlangzi requested review from peterlimg March 8, 2022 09:31
@cnlangzi
Copy link
Contributor Author

cnlangzi commented Mar 8, 2022

@peterlimg let's close this PR. and @lpoli will fix this bug on his PR #571. please go to review his PR instead of it. thanks.

@cnlangzi cnlangzi closed this Mar 8, 2022
@cnlangzi cnlangzi closed this Mar 8, 2022
@cnlangzi cnlangzi deleted the fix/upload_file_inside_file_should_reject branch March 8, 2022 11:06
@cnlangzi
Copy link
Contributor Author

cnlangzi commented Mar 8, 2022

sorry. it is #578. #571 seems is outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants