Add PDF streaming viewer and token caching#174
Conversation
RsbhThakur
left a comment
RsbhThakur
left a comment
There was a problem hiding this comment.
Many critical issues with this PR. please fix all before merging.
akashroy8210
left a comment
akashroy8210
left a comment
There was a problem hiding this comment.
The update removes unnecessary debug logs, adds safer OneDrive access-token refresh handling, preserves the streamed file content type, and fixes the Authorization header format.
RsbhThakur
left a comment
RsbhThakur
left a comment
There was a problem hiding this comment.
Kindly fix the issues mentioned in the comment, and review it once yourself, also create a changelog with complete details and comment it here so that reviewing would be easy next time. Not mergeable until the issues get fix.
| router.post("/updated", catchAsync(ContributionController.GetContributionsUpdatedSince)); | ||
| router.post("/br",isAuthenticated, ContributionController.GetBrContribution) | ||
|
|
||
| router.get('/view/:id',catchAsync(ContributionController.viewFile)) |
There was a problem hiding this comment.
Security Vulnerability: This route is currently unprotected. There is no isAuthenticated middleware applied to it. Since the browser automatically sends cookie credentials for window.open tabs, this route should be protected. Otherwise, any unauthenticated user or external script can access and download files directly from the server.
| } | ||
| window.open(preview_url, "_blank"); | ||
| window.open( | ||
| `${API_BASE_URL}/api/contribution/view/${file._id}`, |
There was a problem hiding this comment.
Functional Regression (Office Documents): Opening the backend streaming URL works great for PDFs and images (which browsers can render inline natively). However, browsers cannot render Office files (like .docx, .xlsx, .pptx) natively.
Previously, clicking "View" on a .docx file opened Microsoft's web viewer (file.webUrl) so the user could read it online. Now, this change forces a raw binary download of the Office file. We should only use the backend stream proxy if the file type is strictly a PDF, falling back to the original webUrl for other formats.
| } | ||
| } | ||
|
|
||
| async function viewFile(req, res, next) { |
There was a problem hiding this comment.
Missing Authorization Check & Resource Leaks:
There are three issues with this controller:
- Access Bypass: There is no verification check. A regular user (non-BR) can access and download unverified files (
file.isVerified === false) if they know the MongoDB ID, bypassing the BR curation block. Please check the file status and verify the user's role before streaming. - Connection Leaks: If a user closes their tab or cancels the download mid-stream, the response connection terminates but the incoming Graph API stream remains open. We need to handle the request close event to destroy the incoming Axios stream and free up sockets.
- Missing Size Metadata: Only the
Content-Typeheader is set on the response. Without theContent-Lengthheader, the browser's PDF viewer won't display a loading progress bar or estimate total pages.
| let cachedAccessToken = null; | ||
| let tokenExpiry = 0; | ||
| let refreshPromise = null; | ||
| export async function getAccessToken() { |
There was a problem hiding this comment.
Robustness Issue (No Cache Eviction on 401):
Caching the token in memory to avoid duplicate Graph API authorization round-trips is a massive performance win.
However, if the cached token is invalidated or revoked on Microsoft's end before the local expiration (tokenExpiry) is reached, Microsoft will return a 401 Unauthorized. Since the cache never evicts the token on authorization failure, the server will continue to reuse the dead token and return errors to all users until the full hour expires. We need a way to invalidate the cache immediately upon hitting a 401 error.
3 participants
Changes
Performance
Observed timings after caching:
User Impact
Users can now open PDFs directly in a new browser tab without being redirected to OneDrive or repeatedly authenticating.