Fix/disallow filename characters #376

aaperis · 2023-04-17T20:02:49Z

This PR prevents potential filepath naming problems that may arise due to cross-platform incompatibilities.

The procedure implemented is as follows:

a filepath is first checked whether it contains both "/" and "\". If yes, the request is rejected. This prevents generating erroneous folder structure in the next step.
all file separators in the filepath are changed to the host's OS convention
the resulting filepath is checked against a regex set of characters that we wish to disallow and if a hit is found the request is rejected.
In case of a rejection, proxy returns a 406 error response to the client and prints debug logs with details on the disallowed characters found in the filepath. Testsuite is updated, too.

The fix was tested for the case when proxy runs in linux and an s3 client (s3cmd, sda-cli) runs in either linux or windows. Not tested for mac but hopefully it behaves like linux ;-).

P.S. following standup discussion, spaces in filenames are allowed (along with the majority of unicode).

Closes #370

proxy.go

helper/helper.go

helper/helper_test.go

pontus · 2023-04-18T07:56:28Z

Does the library unescape the URI before handing it to the handler functions?

aaperis · 2023-04-18T09:18:44Z

Does the library unescape the URI before handing it to the handler functions?

Which library and URI are you referring to?

pontus · 2023-04-18T09:29:55Z

Does the library unescape the URI before handing it to the handler functions?

Which library and URI are you referring to?

The golang provided net/http and the incoming URI.

Another formulation of this is essentially - if the client sends a name such as something%5csomethingelse, what will happen?

codecov-commenter · 2023-04-18T11:22:10Z

Codecov Report

Merging #376 (7146394) into master (c25f117) will increase coverage by 0.76%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #376      +/-   ##
==========================================
+ Coverage   55.49%   56.25%   +0.76%     
==========================================
  Files           8        8              
  Lines        1202     1223      +21     
==========================================
+ Hits          667      688      +21     
  Misses        482      482              
  Partials       53       53

Flag	Coverage Δ
unittests	`56.25% <100.00%> (+0.76%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
helper/helper.go	`36.96% <100.00%> (+4.46%)`	⬆️
proxy.go	`74.29% <100.00%> (+0.32%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

aaperis · 2023-04-18T12:15:45Z

Does the library unescape the URI before handing it to the handler functions?

Which library and URI are you referring to?

The golang provided net/http and the incoming URI.

Another formulation of this is essentially - if the client sends a name such as something%5csomethingelse, what will happen?

Great point. In short, a file with the name something%5csomethingelse will appear in the inbox.

As far as I see, the path from the URI is unescaped. So, if there is potential for a mixup, I do not see how this can be mitigated on the proxy side.
Anything that slips through should be caught eventually when validating the filepaths in the xmls against the files in the inbox before ingestion starts.

norling

Some small stuff you might wanna look at, but looks good in general! :)

helper/helper.go

proxy.go

proxy_test.go

jbygdell · 2023-04-20T07:15:31Z

proxy.go

+func (p *Proxy) notAcceptableResponse(w http.ResponseWriter, _ *http.Request) {
+	log.Debug("not acceptable response")
+	w.WriteHeader(406)
+}
+


Suggested change

func (p *Proxy) notAcceptableResponse(w http.ResponseWriter, _ *http.Request) {

log.Debug("not acceptable response")

w.WriteHeader(406)

}

remove this and write the header directly otherwise you will get double debug messages

This is intended to be a rather generic debug message like the other response functions do (e.g. see line 91 in proxy.go).
example of debug output:

{"level":"info","msg":"User: dummy, Request type PUT, Path: /test/dummy/csa/sq?/data_file","time":"2023-04-20T11:51:56Z"} {"level":"debug","msg":"filepath contains disallowed characters: ?","time":"2023-04-20T11:51:56Z"} {"level":"debug","msg":"not acceptable response","time":"2023-04-20T11:51:56Z"}

There are two debug messages but not identical, the first one gives the explicit problem and the one below the response status . I can combine them in to one but the way it is now the function above is reusable and in addition I think the debug messages are more informative.

I removed the function and moved the WriteHeader in the main body.

jbygdell · 2023-04-20T07:16:23Z

proxy.go

+	filepath, err := helper.FormatUploadFilePath(rawFilepath)
+	if err != nil {
+		log.Debugf(err.Error())
+		p.notAcceptableResponse(w, r)


Suggested change

p.notAcceptableResponse(w, r)

w.WriteHeader(http.StatusNotAcceptable)

- check for improper filename characters - make filepaths os compatible - upon error return status 406

jbygdell · 2023-04-21T10:48:27Z

helper/helper.go

+	// Check for mixed "\" and "/" in filepath. Stop and throw an error if true so that
+	// we do not end up with unintended folder structure when applying ReplaceAll below
+	if strings.Contains(filePath, "\\") && strings.Contains(filePath, "/") {
+		return filePath, fmt.Errorf("filepath contains mixed '\\' and '/' characters")
+	}


This might be unnecessary in the end, but it will require manual testing to validate since / actually is a valid separator on windows.

aaperis self-assigned this Apr 17, 2023

aaperis requested a review from a team April 18, 2023 07:00

jbygdell suggested changes Apr 18, 2023

View reviewed changes

proxy.go Outdated Show resolved Hide resolved

helper/helper.go Outdated Show resolved Hide resolved

helper/helper.go Outdated Show resolved Hide resolved

helper/helper_test.go Outdated Show resolved Hide resolved

helper/helper_test.go Outdated Show resolved Hide resolved

aaperis requested review from jbygdell and pontus April 18, 2023 18:30

norling approved these changes Apr 20, 2023

View reviewed changes

helper/helper.go Show resolved Hide resolved

proxy.go Outdated Show resolved Hide resolved

proxy.go Outdated Show resolved Hide resolved

proxy_test.go Show resolved Hide resolved

jbygdell reviewed Apr 20, 2023

View reviewed changes

aaperis requested a review from jbygdell April 20, 2023 12:20

aaperis force-pushed the fix/disallow-filename-characters branch from ec2d372 to 7146394 Compare April 21, 2023 05:42

aaperis added 10 commits April 21, 2023 07:45

update readme

d2fdc0d

code cleanup

5e78e11

disallow improper filepath names

9e4326b

- check for improper filename characters - make filepaths os compatible - upon error return status 406

add tests for formatUploadPath func

f883dec

add test for 406 error response

d495228

remove leftover code

51f8163

fix backslash to slash substitution

b4a35fe

remove windows tests

fbc0e59

readable http error handling

a50700b

remove excessive debug logging

7146394

jbygdell approved these changes Apr 21, 2023

View reviewed changes

dbampalikis approved these changes Apr 21, 2023

View reviewed changes

dbampalikis merged commit 23e3a06 into master Apr 21, 2023

dbampalikis deleted the fix/disallow-filename-characters branch April 21, 2023 11:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/disallow filename characters #376

Fix/disallow filename characters #376

aaperis commented Apr 17, 2023

pontus commented Apr 18, 2023

aaperis commented Apr 18, 2023

pontus commented Apr 18, 2023

codecov-commenter commented Apr 18, 2023 •

edited

Loading

aaperis commented Apr 18, 2023 •

edited

Loading

norling left a comment

jbygdell Apr 20, 2023

jbygdell Apr 20, 2023

aaperis Apr 20, 2023 •

edited

Loading

aaperis Apr 21, 2023

jbygdell Apr 20, 2023

aaperis Apr 20, 2023 •

edited

Loading

jbygdell Apr 21, 2023

	p.notAcceptableResponse(w, r)
	w.WriteHeader(http.StatusNotAcceptable)

Fix/disallow filename characters #376

Fix/disallow filename characters #376

Conversation

aaperis commented Apr 17, 2023

pontus commented Apr 18, 2023

aaperis commented Apr 18, 2023

pontus commented Apr 18, 2023

codecov-commenter commented Apr 18, 2023 • edited Loading

Codecov Report

aaperis commented Apr 18, 2023 • edited Loading

norling left a comment

Choose a reason for hiding this comment

jbygdell Apr 20, 2023

Choose a reason for hiding this comment

jbygdell Apr 20, 2023

Choose a reason for hiding this comment

aaperis Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

aaperis Apr 21, 2023

Choose a reason for hiding this comment

jbygdell Apr 20, 2023

Choose a reason for hiding this comment

aaperis Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

jbygdell Apr 21, 2023

Choose a reason for hiding this comment

codecov-commenter commented Apr 18, 2023 •

edited

Loading

aaperis commented Apr 18, 2023 •

edited

Loading

aaperis Apr 20, 2023 •

edited

Loading

aaperis Apr 20, 2023 •

edited

Loading