fix: added host header check (to protect against DNS rebinding attacks)

[ErikBjare]

@johan-bjareholt Could you take a look at this and finish it? The Python-implementation is here: ActivityWatch/aw-server@3e8731f

Fixes GHSA-v9fg-6g9j-h4x4

[ErikBjare]

Or @zozs could take a look at this, if you feel like writing a little bit of Rust :)

[johan-bjareholt]

@ErikBjare Fixed a fairing, will add tests soon. Was very easy to test manually with curl.
Just pushed my commit on top of yours, probably a good idea to squash them when merging.

[codecov]

Codecov Report

Merging #250 (5a0a6df) into master (f43b391) will decrease coverage by 0.23%.
The diff coverage is 64.06%.

@@            Coverage Diff             @@
##           master     #250      +/-   ##
==========================================
- Coverage   59.18%   58.95%   -0.24%     
==========================================
  Files          44       45       +1     
  Lines        5123     5216      +93     
==========================================
+ Hits         3032     3075      +43     
- Misses       2091     2141      +50     

Impacted Files	Coverage Δ
aw-server/src/endpoints/import.rs	89.09% <0.00%> (ø)
aw-server/src/logging.rs	0.00% <ø> (ø)
aw-server/src/endpoints/hostcheck.rs	65.00% <65.00%> (ø)
aw-server/src/endpoints/mod.rs	74.19% <100.00%> (+2.76%)	⬆️
aw-server/src/macros.rs	16.66% <0.00%> (-5.21%)	⬇️
aw-query/src/parser.rs	40.29% <0.00%> (-0.03%)	⬇️
aw-server/src/main.rs	0.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f43b391...5a0a6df. Read the comment docs.

[johan-bjareholt]

Tests are now added as well!

[ErikBjare]

I don't think all the client.header(Header::new("Host", "127.0.0.1:5600"))... should be needed, otherwise looks good!

[ErikBjare]

You needed to add this explicitly? The Host header should be set correctly automatically, and you shouldn't be testing against port 5600, right?

[ErikBjare]

Tried removing them, which caused the tests to break. Not sure why though, the Host header should definitely be set by the request library itself...

[johan-bjareholt]

and you shouldn't be testing against port 5600, right?

The port or address actually doesn't matter. We are never starting an actual server or starting any ports in these unit tests, they are simply injected into Rocket as if they were real requests.

The Host header should be set correctly automatically

Since the testing framework is a very basic I'll assume that they either simply have not seen this issue before (since the requests are internal to rocket, the Host header is technically pointless) or simply because the Rocket developers find it to make more sense for all headers to be explicit rather than implicit.

[ErikBjare]

Ah, I suspected something like that, but found it a bit weird and couldn't quite explain why it would work that way when the Host header is so well specified and ubiquitous.

or simply because the Rocket developers find it to make more sense for all headers to be explicit rather than implicit.

This would seem odd to me in any other context where you make requests, but perhaps it makes sense for tests like these.

[ErikBjare]

@johan-bjareholt According to codecov the tests don't cover the failing request handlers, any idea why? See here: https://app.codecov.io/gh/ActivityWatch/aw-server-rust/compare/250/tree/aw-server/src/endpoints/hostcheck.rs

Any chance there's another cause for the BadRequest that you are checking for in the tests? Perhaps check if the returned body of the request actually contains a mention of the failing host-header check?

[ErikBjare]

Some comment nits

[johan-bjareholt]

According to codecov the tests don't cover the failing request handlers, any idea why?
Any chance there's another cause for the BadRequest that you are checking for in the tests? Perhaps check if the returned body of the request actually contains a mention of the failing host-header check?

Simply adding a panic! in the fairing_error_route makes the unittest fail, so the tests work as they should.
I think there's still something broken with our coverage, I've been trying to find that out for an hour no without much success.

[ErikBjare]

Alright, ready to merge then. Thanks!