Conversation
- Add .json to static file bypass regex to prevent manifest.json from entering auth middleware - Add /manifest.json to blockedPaths to prevent redirect to manifest after login - Fixes issue polarnl#164 where Safari/WebKit users were redirected to manifest.json after login Co-authored-by: SSoggyTacoMan <80040706+SSoggyTacoMan@users.noreply.github.com>
Co-authored-by: SSoggyTacoMan <80040706+SSoggyTacoMan@users.noreply.github.com>
Addressing PR comments
SSoggyTacoMan
requested review from
Jvr2022,
andr3i1010 and
supersiem
and removed request for
andr3i1010
There was a problem hiding this comment.
Pull request overview
This PR adjusts request handling and redirect validation to better handle JSON-related paths and reduce unsafe redirect targets.
Changes:
- Extends static-file path matching in
proxyto treat*.jsonas skippable. - Expands the redirect blacklist to prevent redirects to
/manifest.json. - Adds
// AI CODE START/ENDmarkers around the modified lines.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/utils/auth/redirect.ts |
Adds /manifest.json to the redirect blacklist. |
src/proxy.ts |
Updates the static-asset extension regex to include json, causing JSON paths to bypass middleware logic. |
You can also share your feedback on Copilot code review. Take the survey.
| // AI CODE START | ||
| /\.(css|js|ts|tsx|jsx|woff2?|ttf|png|jpg|jpeg|gif|svg|webmanifest|json)$/.test(pathname) || |
There was a problem hiding this comment.
Adding json to the “static files” extension allowlist means any *.json request will bypass the bot/scanner detection and the auth gate below. If the intent is only to let /manifest.json through, it would be safer to special-case that path (or narrowly whitelist known public JSON assets) rather than exempting all JSON paths.
| // AI CODE START | |
| /\.(css|js|ts|tsx|jsx|woff2?|ttf|png|jpg|jpeg|gif|svg|webmanifest|json)$/.test(pathname) || | |
| pathname === "/manifest.json" || | |
| // AI CODE START | |
| /\.(css|js|ts|tsx|jsx|woff2?|ttf|png|jpg|jpeg|gif|svg|webmanifest)$/.test(pathname) || |
| const blockedPaths = ['/api/', '/auth/sign-out', '/manifest.json']; | ||
| // AI CODE END | ||
| if (blockedPaths.some(blocked => path.startsWith(blocked))) { |
There was a problem hiding this comment.
blockedPaths is recreated on every call to getValidRedirectPath. Since it’s a static blacklist, consider moving it to module scope (or making it const outside the function) to avoid repeated allocations and keep the function focused on validation logic.
|
@supersiem zijn de comments van copilot nog goed om wat mee te doen? ik merge trouwens pas als @Jvr2022 security check doet wsant hij is mr hacker |
This pull request makes minor adjustments to improve file handling and security checks in the codebase. The changes focus on updating file extension matching and expanding the list of blocked redirect paths.
Improvements to file handling:
src/proxy.tsto include.jsonfiles, ensuring that requests for JSON files are properly handled or filtered.Security enhancements:
/manifest.jsonto the list of blocked redirect paths insrc/utils/auth/redirect.tsto prevent potential security issues with redirects to this file.