Bugfix/#729 secure static uploads #807
Conversation
This by itself secures the static 'uploads' route, but it of course also breaks all benign access that doesn't send an authentication token within the request header.
This is a prototype that works but still needs some polishing. See the various added FIXMEs.
This now makes the getUserImageURL function of User.ts superfluous. See FIXME in User.ts for implications of this. Additionally the authenticated user-image.directive.ts now works mostly as intended, in contrast to the prototype variant prior to this commit.
Note the FIXME in token.interceptor.ts, i.e.: The Angular HttpInterceptor does not help with securing requests made directly by the browser for <img> or <video> tags, so this TokenInterceptor is probably useless and can/should be removed before creating the PR for this branch/issue #729, if I am not mistaken.
Now passport.authenticate also includes assignProperty: 'jwtData'.
This uses the 'jwtData' specified via passport.authenticate's 'assignProperty' option in the passportJwtMiddlewareFactory. The actual data with the 'tokenPayload' is created in the passportJwtStrategyFactory. CurrentUserDecorator and RoleAuthorization have been somewhat simplified as a result of this change.
Using '@UseBefore(passportJwtMiddleware)' is now functionally required due to the changes in CurrentUserDecorator and RoleAuthorization since the 'jwtData' must be set. (Although this probably should have been added to MediaController.ts regardless.)
By the way, this package is also used by npm itself, see: https://github.com/npm/npm/blob/4c65cd952bc8627811735bea76b9b110cc4fc80e/package.json#L109
@PatrickSkowronek What's your status? |
|
@torss I'm at right now 😄 |
| * {} | ||
| */ | ||
| @Delete('/cache') | ||
| @Authorized(['admin']) |
There was a problem hiding this comment.
I like this function, but is it usable in the Frontend?
There was a problem hiding this comment.
No, and I only wrote that route because I wanted to improve the DownloadController tests, which I in turn only did because of the reduced coverage. (So that was by design & doesn't really have anything to do with the PR.) But I concur, it would be nice to have that functionality be accessible from the admin front-end; will try to add that now.
|
|
||
| transform(url: string, type: string = 'media'): string { | ||
| const key = this.availableTypes[type]; | ||
| const token = localStorage[key].split(' ')[1]; |
There was a problem hiding this comment.
This can be undefined , which is on the first login. We need to catch the undefined error!
There was a problem hiding this comment.
Wow, good catch! I completely missed that.
The actual problem was not the JwtPipe - the localStorage[key] there must never be undefined when the transform gets called, so it should throw an error and extra undefined-handling code is not needed. Instead, the real problem was that JwtPipe got used before login completion in the first place! This happened because of changes in one of the merges, i.e. the ones re. PR #751 with the actualProfilePicturePath stuff I already had somewhat annoying issues with before, as mentioned in some of the comments/commits above.
(I think I should open a GitHub issue for refactoring that thing.)
Parts of that (i.e. updateProfilePicture) get called in the UserService's setUser, which got called in the login method of our AuthenticationService before the tokens were set, thus causing the undefined error. More precisely, the error occurs in the anonymous AppComponent function for userService.data.subscribe, and so it still occurred even without setUser, since that function gets called on initialization anyway, but didn't sufficiently check for empty (string) input.
And the reason for why I didn't notice this problem at all is because of another issue (this time one for which I, and not the merged stuff, is to blame ^^): I simply forgot to add code that clears the mediaToken on logout to the AuthenticationService! Ha.
The code to handle mediaTokens analogous to the regular token was missing in the AuthenticationService constructor and unsetAuthData method, the latter resulting in the persistence / leak of the mediaToken after logging out!
Erroneously returned profile.path instead of profile.picture.path.
Calling userService.setUser before the mediaToken is set caused an issue due to setUser currently calling updateProfilePicture, thus causing a "chain reaction" that ultimately leads to the AppComponent calling the jwtPipe.transform method, which fails without mediaToken. This also removes a superfluous separate call to updateProfilePicture, which was presumably added originally because of the previously broken checkNewProfilePicturePath UserService method used in setUser.
Adding CHANGELOG lines for such small fixes is probably borderline nonsensical, but there already is another typo entry, so I guess I will add this for now. Maybe all the very small changes should be grouped in the future?
| <picture class="responsive-image"> | ||
| <source *ngFor="let breakpoint of this.responsiveImageData.breakpoints" media="(min-width: {{breakpoint.screenSize}}px)" | ||
| srcset="api/{{breakpoint.pathToImage}}{{breakpoint.pathToRetinaImage ? ' 1x, ' + breakpoint.pathToRetinaImage + ' 2x' : ''}}"> | ||
| srcset="api/{{breakpoint.pathToImage | jwt}}{{breakpoint.pathToRetinaImage ? ' 1x, api/' + (breakpoint.pathToRetinaImage | jwt) + ' 2x' : ''}}"> |
There was a problem hiding this comment.
This route is not working properly! The old route does still work. I added a new picture and it does not work anymore.
There was a problem hiding this comment.
Thanks, I think this must have been broken by this.
Will fix it ASAP - After Sleeping Again :P
There was a problem hiding this comment.
Never mind, the commit I previously referred to wasn't the issue - it did in fact provide the solution! The actual problem discovered here is that the ResponsiveImageService didn't account for absolute directory paths. (Which also is what was fixed in said commit, but for user profile pictures.)
Should be fixed now!
Plus minor refactoring to use path.join.
Description:
Closes #729
This secures the
'uploads'route, which was previously completely unprotected / public.Due to browser API limitations it proved to be apparently impossible to modify the request headers for
<img>s or<video>s, meaning that we can't simply attach our JWTlocalStorage['token']as we do for various other API requests. And while it is possible to separately download image files viaBackendServicesgetDownloador similar, this is work-around is not feasible for serving large videos! Thus a different approach was required:A new, specialized
'mediaToken'is generated in addition to the regular'token'. This'mediaToken'is only granted access to routes using the accompanying newpassportJwtMiddlewareMedia(that means the underlying newpassportJwtStrategyMedia). So if the'mediaToken'is accidentally or willfully passed on by a user, the recipient can't use it to fully impersonate the user / can't use it to gain access to the actualgelipage functionality that requires the regular'token'. This is important because this new'mediaToken'is transmitted in form of a URL query parameter.This solution means that
<img>s and more importantly<video>s are now secured without compromising functionality (e.g. by always separately pre-loading entire files). There is also a beneficial side-effect to attaching the'mediaToken'as URL query parameter: Users can still copy the file-URLs with their attached'mediaToken'and send them to anyone else. Recipients of such URLs can still view the file, but only as long as the'mediaToken's last. Since we apparently already employ randomized (unguessable) filenames, there is no major risk of abusing'mediaToken's to gain access to files that have not already been published in some way. Nevertheless, this is not perfect and sharing is currently not really intended as a result, seeKnown Issuesfor more.Improvements
'uploads'route by means of the approach described above.JwtPipeto easily append'mediaToken's to file URLs.FileComponentto flexibly display a single file e.g. in a new tab. It is currently used for theEdit course→Media→Open in new Tabfunctionality. (This was originally intended as an unfortunate necessity to separately download files viaBackendServicesgetDownload- which, again, would have been bad for large videos - but it now has become a simply more flexible, Angular-controllable form to display a single file versus just opening the file-URL in a new tab.)'mediaToken'to various file URLs viaJwtPipeand adapted front-end code in general.'upload'strings in the API withconfig.uploadFolder.@UseBeforemiddleware inMediaController.DownloadController→getArchivedFile→idinput usage.picture.pathbackslash issue / regression. This doesn't really have anything to do with issue 🐛 BUG: Fix security leak: static upload path #729, the problem just came up after the merge. See this commit for details.DownloadControllercoverage via new tests with newDeleteCacheadmin-only API.DeleteCache, housed in a new'Misc.'admin sub-component.@Controller-based replacement for the static'uploads'route:UploadsController. SeeKnown Issuesfor more.)Known Issues:
'mediaToken'. This however should be no significant issue, since they are mostly invisible to the user (e.g. unless a user chooses toCopy image addressor similar). Opening a new tab via Angular can route to the newFileComponent, which tries to gracefully embed the real requested file.UploadsControllerin place of theexpress.staticsystem, but while that controller seemingly worked, it caused problems for video requests that I could not solve so far without modifying therouting-controllerslibrary. Perhaps a viable future alternative would be to useexpressdirectly to achieve this, although this wouldn't be too elegant in comparison to everything else using therouting-controllers.mpse-geli/devGitter room about storing the tokens inlocalStoragedue to potential XSS attacks. This is not directly related to this issue 🐛 BUG: Fix security leak: static upload path #729, but if we should switch to using cookies for authentication (which must then prevent CSRF attacks and would require pervasive changes), then we would no longer have to transmit the'mediaToken'as URL query parameter as well.Possible superior solution for the future:
A better, more robust general solution for the future (independent of the mentioned potential switch to cookies above) might be to just remove the entire
'mediaToken'system again, and instead generate random proxy-filenames for each individual file-listing request. One drawback of this would be temporary server-side storage to associate the proxy-filenames with the real paths. But perhaps this too could be mitigated by generating JWTs per file, using them directly as file ID and relying on the JWT verification for security. Then the only drawback might be computational inefficiency due to the high number of JWTs that need to be generated.