Fix/migl/162 #178
Fix/migl/162 #178
Conversation
Forbid URLs with ".." see mattgodbolt#162 for further information. Signed-off-by: Michael Glembotzki <[email protected]>
Without this fix, "/" is used as the default location for the root path! Signed-off-by: Michael Glembotzki <[email protected]>
| @@ -867,6 +867,9 @@ bool Connection::processHeaders(uint8_t* first, uint8_t* last) { | |||
| if (requestUri == nullptr) { | |||
| return sendBadRequest("Malformed request line"); | |||
| } | |||
| if (std::string(requestUri).find("..") != std::string::npos) { | |||
There was a problem hiding this comment.
Is it safe to assume an URL shouldn't contain any ..? Eg. a/b/c/../d seems ok to me.
There was a problem hiding this comment.
Do you mean something like that?
std::string merge_uri(std::string & uri) {
std::istringstream is_uri {uri};
std::vector<std::string> v;
std::string parsed_uri;
for (std::string line; std::getline(is_uri, line, '/'); ) {
if(line == ".") {
continue;
} if(line == "..") {
if(!v.empty()) {
v.pop_back();
}
} else if(line != "") {
line = "/" + line;
v.push_back(line);
}
}
for (auto const & dir : v){
parsed_uri += dir;
}
return parsed_uri.empty() ? "/" : parsed_uri;
}
For comparison, nginx has your described behavior. e.g. a/b/c/../d becomes a/b/d . But one must also say, nginx has a complex state machine that not only protects against directory triversal, but can also handle all other special characters. If that's what we want, I'd at least like to take commit 5e7d92c and drop the other one.
There was a problem hiding this comment.
Indeed, the empty root path fix is fine to merge 👍
I'd say the path traversal needs some more work. Maybe std filesystem / path could get handy here?
Codecov Report
@@ Coverage Diff @@
## master #178 +/- ##
==========================================
- Coverage 34.65% 34.59% -0.06%
==========================================
Files 53 53
Lines 2323 2327 +4
Branches 354 356 +2
==========================================
Hits 805 805
- Misses 1429 1432 +3
- Partials 89 90 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Does this pr also block using |
URL decoding is not implemented: |
see #162