Fix 2455: Check auth for pictrs when instance is private. #2477
Conversation
|
You need to run |
There was a problem hiding this comment.
This requires 1-2 DB reads for every single picture requested now, reading the site to check if its a private instance, and if it is, reading from local_user to make sure they're actually a user.
That performance hit scares me, because some pages like Home can request hundreds of pictures.
I don't think this is on you to fix tho, its on us to create a in-memory singleton of local site settings, that gets updated on every save. Then we could at least reduce this to one read validating the local user, that should only get run on a private instances.
Also the format check is failing, run cargo +nightly fmt to fix
crates/routes/src/images.rs
Outdated
| if site.private_instance { | ||
| let jwt = req | ||
| .cookie("jwt") | ||
| .expect("No auth header for picture access"); |
There was a problem hiding this comment.
No expect, do a .map_err(error::ErrorUnauthorized)
There was a problem hiding this comment.
OK, that should be done in upload() on line 92 as well i think?
| .cookie("jwt") | ||
| .expect("No auth header for picture access"); | ||
| if get_local_user_view_from_jwt(jwt.value(), context.pool(), context.secret()).await.is_err() { | ||
| return Ok(HttpResponse::Unauthorized().finish()); |
There was a problem hiding this comment.
I think its better to return the Error(error::ErrorUnauthorized)
There was a problem hiding this comment.
I think in both cases we should return a HttpResponse. I'm not sure how to do that honestly.
Is replacing expect() with .map_err(error::ErrorUnauthorized) enough above?
There was a problem hiding this comment.
I spose that's fine, althoug that actix errors are also HttpResponses.
There was a problem hiding this comment.
@dessalines I'm getting errors about types not being correct. I'm really unsure how to do that, and need help on those - sorry not yet familiar with those rust libs.
There was a problem hiding this comment.
There are other examples in this file of .map_err . If you run cargo check, it should tell you the exact error.
I agree on the performance concern. I see up to 600 requests per user easily if you have some image intense posts. Thanks for the review, i did run the formatter now. |
dessalines
force-pushed
the
check-auth-for-imgs-private-instance
branch
from
91ed275 to
f24678f
Compare
#2455