feat: adding custom/default user agent support for http monitors#3446
feat: adding custom/default user agent support for http monitors#3446akashmannil wants to merge 1 commit intodevelopfrom
Conversation
ajhollid
left a comment
There was a problem hiding this comment.
There's two major issues here
- Excess DB access
- Header injection
that need to be addressed. There's also an architectural tweak that can keep HttpProvider less tightly coupled.
Please have a look at my comments in the code review, thanks!
| private advancedMatcher: IAdvancedMatcher, | ||
| private settingsService: ISettingsService |
There was a problem hiding this comment.
We don't need a hard dependency on SettingsService here, lets just pass userAgent to the handle method instead.
|
|
||
| const headers: Record<string, string> = {}; | ||
| if (secret) headers["Authorization"] = `Bearer ${secret}`; | ||
| if (userAgent) headers["User-Agent"] = userAgent; |
There was a problem hiding this comment.
There's no validation of the header, this is a pretty serious vulnerability. userAgent must be properly sanitized.
|
|
||
| let userAgent: string | undefined = customUserAgent; | ||
| if (!userAgent && monitor.type === "http") { | ||
| const dbSettings = await this.settingsService.getDBSettings(); |
There was a problem hiding this comment.
This makes a DB call on every execution of handle. This does not scale, if you had 500 monitors polling every 15s thats an extra 2000 db calls/min. User agent should be cached on startup, and the cache invalidated on update.
Custom and Default User agent support for monitoring.
Checkmate/X.X (uptime monitor; your-instance)- Placeholder added keeping in mind of the local checkmate instance. Please do let me know if changes neededImplements #3260
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.