Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

fix: don't expect ipfs to preserve a leading slash #440

Merged
merged 1 commit into from
Mar 13, 2019

Conversation

Stebalien
Copy link
Contributor

Go-ipfs now normalizes paths before adding files. This will:

  • remove any leading slashes.
  • remove any /./ components.
  • normalize any /../ components.

That breaks this test which assumes that paths are returned exactly as specified.

@alanshaw
Copy link
Contributor

I'm glad these are now normalized, we'll have to submit a PR to JS IPFS to implement this. It needs some tests to ensure the normalisation happens.

Separately, I think this change breaks the path given to resolve later in the test: /ipfs/${rootHash}${path}, I think we'd end up with /ipfs/QmHashpath/to/testfile.txt.

@alanshaw
Copy link
Contributor

@Stebalien would you mind pointing me in the direction of the PR(s) that implemented this? I just want to make sure we do the same thing...For instance, I'd like to know what happens with paths that try to traverse above root e.g. '../foo' => 'foo' or '../foo' => error? Also windows style paths i.e. backslashes and C:\\.

@alanshaw
Copy link
Contributor

See ipfs/go-ipfs-files#6 and https://golang.org/pkg/path/filepath/#Clean

(Note: we might want to consider rejecting .. instead of normalizing)

@Stebalien
Copy link
Contributor Author

Stebalien commented Feb 19, 2019

Separately, I think this change breaks the path given to resolve later in the test: /ipfs/${rootHash}${path}, I think we'd end up with /ipfs/QmHashpath/to/testfile.txt.

(got to get better about actually running tests)


This changed (incidentally) in ipfs/go-ipfs-files#6. Normalized with https://golang.org/pkg/path/#Clean.

Actually, I think we're going to need to think through this a bit. Clean normalizes .. while we should probably reject it. Thoughts?

(edited: I found the wrong Clean function)

Go-ipfs now normalizes paths before adding files. This will:

* remove any leading slashes.
* remove any `/./` components.
* normalize any `/../` components.

That breaks this test which assumes that paths are returned _exactly_ as
specified.
@Stebalien
Copy link
Contributor Author

(got to get better about actually running tests)

Ah, that's why I didn't. Didn't want to learn how to replace npm deps.

@alanshaw
Copy link
Contributor

alanshaw commented Feb 22, 2019

Actually, I think we're going to need to think through this a bit. Clean normalizes .. while we should probably reject it. Thoughts?

Do you mean .. when attempting to navigate up from root or .. anywhere in the path?

For the former, I don't have strong feelings on it. It's pretty common to not error when attempting to navigate up from root, go-lang Clean being the case in point. But also hyperlinks on the internet, path.resolve in Node.js and cd / && cd ../ && pwd works. That said, a traversal up from root is either user error or malicious so I'd be ok to error in this situation. On balance I think I'm slightly more inclined to resolve /.. to / without error.

For the latter, I guess it's just convenient to allow this when adding and it's easy to do the resolve. People will just expect it to work so why get in the way?

I noticed that Clean doesn't specify anything about \...do you think we should replace \ with / and trim /[a-zA-Z]:\\/ from the start?

@Stebalien
Copy link
Contributor Author

Stebalien commented Feb 22, 2019

Do you mean .. when attempting to navigate up from root or .. anywhere in the path?

Both? The request body feels like a declarative filesystem layout. For example, I I'd expect tar to reject tar files with .. in paths.

But I don't really fell all that strongly about this.

@alanshaw alanshaw merged commit d3ad40b into master Mar 13, 2019
@alanshaw alanshaw deleted the fix/normalized-path branch March 13, 2019 14:53
@ghost ghost removed the in progress label Mar 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants