Skip to content
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

lib/protocol: Apply input filtering on file names #3775

Closed
wants to merge 2 commits into from
Closed

lib/protocol: Apply input filtering on file names #3775

wants to merge 2 commits into from

Conversation

calmh
Copy link
Member

@calmh calmh commented Nov 30, 2016

Purpose

Stricter filtering of incoming filenames. Essentially, names on the wire should be in canonical shortest possible form -- no internal . or .. components, no double slashes, not be one of . or .., not be absolute (start with /), not end with /, not start with ../, and not be empty.

(.stignore and similar special files should not be served, but it is not a protocol error to talk about them, it's a Syncthing specific limitation that we will not serve them. Coming in another PR.)

On Windows, files in index messages containing backslashes are filtered out with a warning. Requests for such files get an error return and a warning logged.

Testing

The filter methods have some unit tests on them.

Documentation

Requires additions to the spec to explain the restrictions we apply here.

@Unrud for review.

@calmh
Copy link
Member Author

calmh commented Nov 30, 2016

@st-review don't merge this until we've considered it carefully

@st-review
Copy link

@calmh: Preventing merge for the time being. Push a new revision to reset!

@calmh
Copy link
Member Author

calmh commented Nov 30, 2016

Metalint is lib/protocol/protocol.go:55:1:warning: errInternalFilename is unused (deadcode), I'll fix that.

@AudriusButkevicius
Copy link
Member

Lgtm

@Unrud
Copy link
Contributor

Unrud commented Dec 1, 2016

I'm fine with this.

@calmh
Copy link
Member Author

calmh commented Dec 1, 2016

@st-review merge

@st-review
Copy link

👌 Merged as 3266aae. Thanks, @calmh!

@st-review st-review closed this Dec 1, 2016
st-review pushed a commit that referenced this pull request Dec 1, 2016
@calmh calmh deleted the inputfilter branch March 9, 2017 15:17
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Dec 2, 2017
@syncthing syncthing locked and limited conversation to collaborators Dec 2, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants