Fix: Authentication Middleware in StatusPage#3310
Conversation
Br0wnHammer
requested review from
Owaiseimdad,
ajhollid and
karenvicent
as code owners
ajhollid
left a comment
ajhollid
left a comment
There was a problem hiding this comment.
This solution depends on route order, which isn't immediately obvious if you didn't review this PR and understand how it was implemented. It's not difficult to figure out, but we should try to avoid non-obvious solutions wherever possible, and I think we can do that here.
| if (statusPage.isPublished) { | ||
| next(); // Published — proceed to controller (no JWT) | ||
| } else { | ||
| next("route"); // Unpublished — skip to next route (which has verifyJWT) |
There was a problem hiding this comment.
This is quite brittle; it depends on the route order never changing, which is not obvious. Maintainers down the line won't know that this middleware depends on route order.
I think a more robust solution would be to have one route and just skip the JWT validation if the page is published
client/src/Utils/ApiClient.ts
Outdated
| if (window.location.pathname !== "/login") { | ||
| if ( | ||
| window.location.pathname !== "/login" && | ||
| !window.location.pathname.startsWith("/status/public") |
There was a problem hiding this comment.
This shouldn't be necessary I don't think 🤔 /status/public should never throw a 401 should it? If the page isn't published we should get a 403, if it is published we should get a 200?
2 participants
Describe your changes
This PR fixes a security vulnerability in the status page URL.
Please ensure all items are checked off before requesting a review. "Checked off" means you need to add an "x" character between brackets so they turn into checkmarks.
<div>Add</div>, use):npm run formatin server and client directories, which automatically formats your code.