Skip to content
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

fix: rewrite BE endpoint resolution method #1169

Merged
merged 1 commit into from
Nov 20, 2024
Merged

fix: rewrite BE endpoint resolution method #1169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 8 additions & 10 deletions Server/controllers/monitorController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import { getTokenFromHeaders } from "../utils/utils.js";
import logger from "../utils/logger.js";
import { handleError, handleValidationError } from "./controllerUtils.js";
import dns from "dns";
import axios from "axios";

const SERVICE_NAME = "monitorController";

Expand Down Expand Up @@ -288,18 +288,16 @@
}

try {
let { monitorURL } = req.query;
monitorURL = new URL(monitorURL);
await new Promise((resolve, reject) => {
dns.resolve(monitorURL.hostname, (error) => {
if (error) {
reject(error);
}
resolve();
});
const { monitorURL } = req.query;
const parsedUrl = new URL(monitorURL);
const response = await axios.get(parsedUrl, {
timeout: 5000,
validateStatus: () => true,
Comment on lines +291 to +295
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Critical: Implement SSRF protection measures

The current implementation is vulnerable to Server-Side Request Forgery (SSRF) attacks. An attacker could provide a URL pointing to internal network resources.

Apply this diff to add basic SSRF protection:

 const { monitorURL } = req.query;
 const parsedUrl = new URL(monitorURL);
+ // Block private IP ranges and localhost
+ const hostname = parsedUrl.hostname;
+ if (hostname === 'localhost' || 
+     hostname === '127.0.0.1' ||
+     hostname.startsWith('192.168.') ||
+     hostname.startsWith('10.') ||
+     hostname.startsWith('172.16.')) {
+   throw new Error('Access to internal networks is not allowed');
+ }
 const response = await axios.get(parsedUrl, {

Committable suggestion skipped: line range outside the PR's diff.

});

Check failure

Code scanning / CodeQL

Server-side request forgery Critical

The
URL
Loading
of this request depends on a
user-provided value
Loading
.
return res.status(200).json({
success: true,
code: response.status,
statusText: response.statusText,
msg: `URL resolved successfully`,
});
} catch (error) {
Expand Down
24 changes: 13 additions & 11 deletions Server/tests/controllers/monitorController.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import jwt from "jsonwebtoken";
import sinon from "sinon";
import { successMessages } from "../../utils/messages.js";
import logger from "../../utils/logger.js";
import dns from "dns";
import axios from "axios";
const SERVICE_NAME = "monitorController";

describe("Monitor Controller - getAllMonitors", () => {
Expand Down Expand Up @@ -476,38 +476,40 @@ describe("Monitor Controller - createMonitor", () => {
});
});

describe("Monitor Controllor - checkEndpointResolution", () => {
let req, res, next, dnsResolveStub;
describe("Monitor Controller - checkEndpointResolution", () => {
let req, res, next, axiosGetStub;
beforeEach(() => {
req = { query: { monitorURL: "https://example.com" } };
res = { status: sinon.stub().returnsThis(), json: sinon.stub() };
next = sinon.stub();
dnsResolveStub = sinon.stub(dns, "resolve");
axiosGetStub = sinon.stub(axios, "get");
});
afterEach(() => {
dnsResolveStub.restore();
sinon.restore();
});
it("should resolve the URL successfully", async () => {
dnsResolveStub.callsFake((hostname, callback) => callback(null));
axiosGetStub.resolves({ status: 200, statusText: "OK" });
await checkEndpointResolution(req, res, next);
expect(res.status.calledWith(200)).to.be.true;
expect(
res.json.calledWith({
success: true,
code: 200,
statusText: "OK",
msg: "URL resolved successfully",
})
).to.be.true;
expect(next.called).to.be.false;
});
it("should return an error if DNS resolution fails", async () => {
const dnsError = new Error("DNS resolution failed");
dnsError.code = "ENOTFOUND";
dnsResolveStub.callsFake((hostname, callback) => callback(dnsError));
it("should return an error if endpoint resolution fails", async () => {
const axiosError = new Error("resolution failed");
axiosError.code = "ENOTFOUND";
axiosGetStub.rejects(axiosError);
await checkEndpointResolution(req, res, next);
expect(next.calledOnce).to.be.true;
const errorPassedToNext = next.getCall(0).args[0];
expect(errorPassedToNext).to.be.an.instanceOf(Error);
expect(errorPassedToNext.message).to.include("DNS resolution failed");
expect(errorPassedToNext.message).to.include("resolution failed");
expect(errorPassedToNext.code).to.equal("ENOTFOUND");
expect(errorPassedToNext.status).to.equal(500);
});
Expand Down