Skip to content

3120 remove URL's from data when they are supposed to be disabled #3121

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ajhollid merged 19 commits into from
Jan 15, 2026
Merged

3120 remove URL's from data when they are supposed to be disabled #3121

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
fb3795b
3120 remove URL's from data when they are supposed to be disabled
web-engineer Jan 10, 2026
ab4cc80
3120 persist admin value
web-engineer Jan 10, 2026
643a743
more logging
web-engineer Jan 10, 2026
f659876
select bug
web-engineer Jan 10, 2026
a13f6a3
missing value
web-engineer Jan 10, 2026
526fa24
validation bug
web-engineer Jan 10, 2026
12b28ff
type issue
web-engineer Jan 10, 2026
418068e
loggin settings
web-engineer Jan 11, 2026
76ab269
missing setting
web-engineer Jan 11, 2026
7b619e6
Remove debug logging and add feature documentation
web-engineer Jan 11, 2026
7556086
code review updates
web-engineer Jan 11, 2026
e4d1012
bug fix - settings handlechange
web-engineer Jan 11, 2026
b6f48a9
Revert caching changes - restore working implementation
web-engineer Jan 11, 2026
a70959e
Nitpick fix - ref -
web-engineer Jan 11, 2026
1494df9
missing brace
web-engineer Jan 11, 2026
3ce40b1
Fix handleSave function syntax errors
web-engineer Jan 11, 2026
1c1ac25
fix saveSettings is currently being called even when validation fails.
web-engineer Jan 12, 2026
f0a6fc4
Merge upstream/develop into fix/3120-data-leak
web-engineer Jan 15, 2026
420164d
conditional simplification as reqeusted
web-engineer Jan 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion server/src/config/services.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ export const initializeServices = async ({ logger, envSettings, settingsService
// Create DB
const checkModule = new CheckModule({ logger, Check, Monitor, User });
const inviteModule = new InviteModule({ InviteToken, crypto, stringService });
const statusPageModule = new StatusPageModule({ StatusPage, NormalizeData, stringService });
const statusPageModule = new StatusPageModule({ StatusPage, NormalizeData, stringService, AppSettings });
const userModule = new UserModule({ User, Team, GenerateAvatarImage, ParseBoolean, stringService });
const maintenanceWindowModule = new MaintenanceWindowModule({ MaintenanceWindow });
const monitorModule = new MonitorModule({
Expand Down
19 changes: 12 additions & 7 deletions server/src/db/v1/modules/statusPageModule.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
// import StatusPage from "../../models/StatusPage.js";
// import { NormalizeData } from "../../../utils/dataUtils.js";
// import ServiceRegistry from "../../../service/system/serviceRegistry.js";
// import StringService from "../../../service/system/stringService.js";

const SERVICE_NAME = "statusPageModule";

class StatusPageModule {
constructor({ StatusPage, NormalizeData, stringService }) {
constructor({ StatusPage, NormalizeData, stringService, AppSettings }) {
this.StatusPage = StatusPage;
this.NormalizeData = NormalizeData;
this.stringService = stringService;
this.AppSettings = AppSettings;
}

createStatusPage = async ({ statusPageData, image, userId, teamId }) => {
Expand Down Expand Up @@ -217,7 +217,7 @@ class StatusPageModule {
showUptimePercentage: 1,
timezone: 1,
showAdminLoginLink: 1,
url: 1,
url: { $cond: [{ $eq: ["$statusPage.showMonitorUrl", true] }, "$monitors.url", "$$REMOVE"] },
Copy link

@llamapreview llamapreview bot Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 | Confidence: High

This change introduces a data-driven conditional field in a MongoDB aggregation pipeline using $statusPage.showMonitorUrl as the condition. However, the business logic immediately after the aggregation (changed lines below) uses a different source of truth (AppSettings.showURL). This creates a critical inconsistency: the aggregation may already filter out the url field based on a status-page-specific setting, while the application-level logic filters based on a global setting. This dual-layer filtering is architecturally confusing and could lead to unpredictable behavior (e.g., a status page's showMonitorUrl might be true, but the global showURL is false, causing the URL to be fetched from the DB only to be immediately stripped in JavaScript, wasting bandwidth and processing). The single source of truth for this feature should be clearly defined and applied in one place.

Code Suggestion:

Choose one approach:
1.  **Use Global Setting Only:** Remove the aggregation condition and rely solely on the application logic.
    ```javascript
    url: 1,
    ```
2.  **Use Status Page Setting Only:** Remove the `AppSettings` dependency and logic, and ensure `statusPage.showMonitorUrl` is the authoritative setting.
    ```javascript
    // In the constructor, remove AppSettings.
    // Remove the appSettings fetch and showURL check in getStatusPageByUrl.
    // Keep the aggregation condition as is.
    ```

},
monitors: {
_id: 1,
Expand Down Expand Up @@ -260,11 +260,16 @@ class StatusPageModule {

const { statusPage, monitors } = statusPageQuery[0];

const appSettings = await this.AppSettings.findOne({ singleton: true }).lean();
const showURL = appSettings?.showURL === true;
Comment on lines 263 to 264
Copy link

@llamapreview llamapreview bot Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 | Confidence: High

This change adds a database query (AppSettings.findOne) to a public-facing API endpoint (getStatusPageByUrl). This query will execute on every request to the public status page, significantly increasing database load and latency. The AppSettings (a likely singleton configuration) should be cached. The related_context shows services.js injects dependencies into modules; a caching mechanism (e.g., in a SettingsService or SettingsModule) should be used here instead of a direct DB call. Without caching, this introduces a performance regression and a potential scalability bottleneck.

Code Suggestion:

// In services.js, inject settingsModule (which already uses AppSettings)
const statusPageModule = new StatusPageModule({ ..., settingsModule });
// In statusPageModule.js constructor
this.settingsModule = settingsModule;
// In getStatusPageByUrl
const showURL = this.settingsModule.getAppSettings()?.showURL === true;


const normalizedMonitors = monitors.map((monitor) => {
return {
...monitor,
checks: this.NormalizeData(monitor.checks, 10, 100),
};
const normalizedChecks = this.NormalizeData(monitor.checks, 10, 100);
if (showURL !== true) {
const { url, ...rest } = monitor;
return { ...rest, checks: normalizedChecks };
}
Copy link

@llamapreview llamapreview bot Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 | Confidence: High

The logic for stripping the url field is applied in the post-processing step after the aggregation pipeline. This is fragile because it assumes the url field's presence/absence hasn't already been handled by the aggregation's $cond (see Finding 1). Furthermore, this imperative deletion duplicates the intent already expressed (confusingly) in the aggregation stage. The logic for field visibility should be consolidated into a single, declarative location (ideally within the aggregation pipeline for efficiency and clarity) to prevent future maintenance errors.

Code Suggestion:

const normalizeMonitorForPublicView = (monitor, showURL) => {
    const normalizedChecks = this.NormalizeData(monitor.checks, 10, 100);
    if (!showURL) {
        const { url, ...rest } = monitor;
        return { ...rest, checks: normalizedChecks };
    }
    return { ...monitor, checks: normalizedChecks };
};
// Then in map:
const normalizedMonitors = monitors.map(monitor => normalizeMonitorForPublicView(monitor, showURL));

return { ...monitor, checks: normalizedChecks };
});

return { statusPage, monitors: normalizedMonitors };
Expand Down