security: gh security recs #3368
Conversation
hay-kot
force-pushed
the
security/gh-security-recs
branch
from
c68a95b
to
9fcb6c0
Compare
There was a problem hiding this comment.
The code changes all look good.
In testing the happy path, everything went well (i.e. I can still import recipes and images from legitimate internet sites.
In testing the negative path (trying to access things on my internal network), things came unstuck.
This first series of images is when accessing my Vikunja instance via the URL/reverse proxy (note I installed ping in the dev container to confirm what it's resolving to):
Then for a further test, I popped the IP/port in directly, avoiding the reverse proxy and any DNS resolution:
@hay-kot I'm interested in whether you can replicate this scenario.
I dug a bit deeper, and wanted to get some evidence of what was happening, which this code change and error message shows.. effectively the IP is None, rather than 192.168.x.y as I'd expect.
|
Along the same lines as the The change would be made in the group_preferences.py schema. I don't know the impacts though. |
|
@p0lycarpio do you want to raise a feature request for that (or discuss it on Discord)? That's more a privacy issue than a security issue, if you take my meaning. |
hay-kot
force-pushed
the
security/gh-security-recs
branch
from
821d68b
to
f34b5ee
Compare
|
@boc-the-git should be good now. Thanks to some folks on discord it's 99% less bad too! |
mealie/pkgs/safehttp/transport.py
Outdated
| netloc = request.url.netloc.decode() | ||
| if ":" in netloc: # Either an IP, or a hostname:port combo | ||
| netloc_parts = netloc.split(":") | ||
| if len(netloc_parts) > 1: # netloc of username:password@hostname:port not supported |
There was a problem hiding this comment.
The check here doesn't align with the comment.. should this be checking for ... > 2?
I won't stop this delaying a merge, but will circle back to it, via Discord.
There was a problem hiding this comment.
... > 1 will always be true, except in some weird scenario where : was the first or last character of netloc (and tbh I'm not certain what happens in that scenario)
mealie/pkgs/safehttp/transport.py
Outdated
| @@ -46,8 +46,6 @@ async def handle_async_request(self, request): | |||
| netloc = request.url.netloc.decode() | |||
| if ":" in netloc: # Either an IP, or a hostname:port combo | |||
| netloc_parts = netloc.split(":") | |||
| if len(netloc_parts) > 1: # netloc of username:password@hostname:port not supported | |||
There was a problem hiding this comment.
@hay-kot see this log.. showing that even when user:pass is included in the url, it was being stripped out before getting to this netloc variable. Thus, the check on parts is irrelevant here.
|
@hay-kot this has my blessing, but note I had to make a fix. Please review, and merge/release as desired. |
What type of PR is this?
(REQUIRED)
What this PR does / why we need it:
Addresses several security concerns raised by external audit/review
Also, note that these are mitigations, they are not 100% fixes.
Which issue(s) this PR fixes:
Special notes for your reviewer:
I've tested locally and all the automated testing should pass. If possible, I would like another reviewer to pull down these changes and attempt to
Testing
See Above