feat: add simple auth to server requests #1449
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces a comprehensive authentication mechanism for the application. It adds WebSocket authentication, creates a login page, implements authentication middleware, and enhances route security. The changes focus on protecting server routes and WebSocket connections by introducing token-based authentication, a login interface, and methods to validate user credentials. The implementation ensures that critical routes and connections require proper authentication before granting access. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
cpvalente
force-pushed
the
cloud-password
branch
3 times, most recently
from
d4ff59c to
ec9f2a6
Compare
| if (key === 'token' && value === hashedPassword) { | ||
| return next(); | ||
| } | ||
| } |
There was a problem hiding this comment.
| } | |
| } | |
| const url = new URL(`http://localhost${req.url}`); | |
| const maybeToken = url.searchParams.get('token'); | |
| if (maybeToken === hashedPassword) { | |
| return next(); | |
| } |
allow ws clients without cookies to connect
There was a problem hiding this comment.
how would the token come up here? do we need an interface in ontime to generate the token or should the websocket client include the raw password in the connection?
What is the normal use case?
There was a problem hiding this comment.
I was thinking for when companion connects to the ws endpoint directly
Yeah some place to get the token would be needed, but that could be in the dashboard
cpvalente
force-pushed
the
cloud-password
branch
from
d23eac8 to
b85f0d1
Compare
cpvalente
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (3)
apps/server/src/middleware/authenticate.ts (1)
66-73: Refactor duplicate authentication logic to improve maintainability.The authentication logic in the
authenticatemiddleware (lines 66-73) and theauthenticateSocketfunction (lines 116-139) share similar patterns for validating tokens from cookies and query parameters. Refactoring this common logic into a shared utility function will reduce code duplication and enhance maintainability.Consider creating a utility function, such as
isAuthenticated(req: Request | IncomingMessage): boolean, that encapsulates the token validation logic for both HTTP requests and WebSocket connections.Also applies to: 116-139
apps/server/src/middleware/noop.ts (1)
3-5: Add documentation to clarify the purpose ofnoopMiddleware.Providing a brief comment or JSDoc annotation will help other developers understand why this middleware exists and when it should be used.
Example:
/** + * A no-operation middleware function that simply calls `next()`. + * Used as a placeholder when no authentication is required. */ export function noopMiddleware(_req: Request, _res: Response, next: NextFunction) { next(); }apps/server/src/app.ts (1)
98-100: Review static file authentication strategy.The current implementation authenticates all static file requests, which might:
- Impact performance by checking authentication for public assets
- Expose sensitive path information in redirects
Consider:
- Creating a whitelist of public assets that don't require authentication
- Using a separate route for authenticated static files
- Implementing proper error handling for unauthorized requests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/server/package.jsonis excluded by none and included by nonepnpm-lock.yamlis excluded by!**/pnpm-lock.yamland included by none
📒 Files selected for processing (8)
apps/server/src/adapters/WebsocketAdapter.ts(2 hunks)apps/server/src/app.ts(3 hunks)apps/server/src/externals.ts(1 hunks)apps/server/src/html/login.html(1 hunks)apps/server/src/middleware/authenticate.ts(1 hunks)apps/server/src/middleware/noop.ts(1 hunks)apps/server/src/setup/index.ts(1 hunks)apps/server/src/utils/hash.ts(1 hunks)
🔇 Additional comments (2)
apps/server/src/middleware/authenticate.ts (1)
43-47: VerifysecureandsameSitecookie options align with your deployment environment.The authentication cookie is set with
secure: true, which means it will only be sent over HTTPS connections. If the application is served over HTTP (e.g., during development or in environments without SSL), authentication may fail. Similarly,sameSite: 'strict'can prevent the cookie from being sent in some cross-origin requests.Ensure that the
secureandsameSiteoptions are appropriate for your application's deployment environment. If necessary, adjust these settings or implement environment-specific configurations.apps/server/src/setup/index.ts (1)
90-91: Validate login.html file existence during startup.The login path is added but there's no validation to ensure the file exists. This could lead to 404 errors if the file is missing.
Run this script to check file existence:
| const url = new URL(req.url || '', `http://${req.headers.host}`); | ||
| const token = url.searchParams.get('token'); | ||
| if (token === hashedPassword) { | ||
| return next(); | ||
| } |
There was a problem hiding this comment.
Validate URL construction in authenticateSocket to prevent security risks.
When constructing the URL with new URL(req.url || '', \http://${req.headers.host}`), relying on req.headers.host` can be insecure, as headers can be spoofed by clients. This might lead to incorrect URL parsing or security vulnerabilities.
Apply this diff to use a trusted base URL:
- const url = new URL(req.url || '', `http://${req.headers.host}`);
+ const url = new URL(req.url || '', 'http://localhost');Alternatively, validate req.headers.host before use or employ a default value when it is undefined or untrusted.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const url = new URL(req.url || '', `http://${req.headers.host}`); | |
| const token = url.searchParams.get('token'); | |
| if (token === hashedPassword) { | |
| return next(); | |
| } | |
| const url = new URL(req.url || '', 'http://localhost'); | |
| const token = url.searchParams.get('token'); | |
| if (token === hashedPassword) { | |
| return next(); | |
| } |
cpvalente
force-pushed
the
cloud-password
branch
from
d4112a4 to
a054d18
Compare
cpvalente
force-pushed
the
cloud-password
branch
from
a054d18 to
b2b5414
Compare
cpvalente
force-pushed
the
cloud-password
branch
from
89d721f to
1290564
Compare
This is a proposal for protecting the HTTP connections with a password
Strategy
Notes:
Whis is a viable solution for ontime-cloud, but not for Ontime