Skip to content

fix(dashmate): port marks as closed if ipv6 is not disabled #2162

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
merged 5 commits into from
Sep 26, 2024
Merged

fix(dashmate): port marks as closed if ipv6 is not disabled #2162

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5c1bc51
fix(dashmate): port marks as closed if ipv6 is not disabled
shumkov Sep 22, 2024
5c99f3a
fix: 400 The plain HTTP request was sent to HTTPS port
shumkov Sep 22, 2024
3ddb35f
Update providers.js
shumkov Sep 26, 2024
1615842
refactor: use `options.timeout`
shumkov Sep 26, 2024
3f6ee40
chore: handle non 200 status code
shumkov Sep 26, 2024
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
83 changes: 68 additions & 15 deletions packages/dashmate/src/status/providers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import https from 'https';
import { PortStateEnum } from './enums/portState.js';

const MAX_REQUEST_TIMEOUT = 5000;
const MAX_RESPONSE_SIZE = 1 * 1024 * 1024; // 1 MB

const request = async (url) => {
try {
Expand Down Expand Up @@ -29,12 +31,6 @@ const requestJSON = async (url) => {
return response;
};

const requestText = async (url) => {
const response = await request(url);

return response.text();
};

const insightURLs = {
testnet: 'https://testnet-insight.dashevo.org/insight-api',
mainnet: 'https://insight.dash.org/insight-api',
Expand Down Expand Up @@ -68,15 +64,72 @@ export default {
},
mnowatch: {
checkPortStatus: async (port) => {
try {
return requestText(`https://mnowatch.org/${port}/`);
} catch (e) {
if (process.env.DEBUG) {
// eslint-disable-next-line no-console
console.warn(e);
}
return PortStateEnum.ERROR;
}
// We use http request instead fetch function to force
// using IPv4 otherwise mnwatch could try to connect to IPv6 node address
// and fail (Core listens for IPv4 only)
// https://github.com/dashpay/platform/issues/2100

const options = {
hostname: 'mnowatch.org',
port: 443,
path: `/${port}/`,
method: 'GET',
family: 4, // Force IPv4
timeout: MAX_REQUEST_TIMEOUT,
};

return new Promise((resolve) => {
const req = https.request(options, (res) => {
let data = '';

// Check if the status code is 200
if (res.statusCode !== 200) {
if (process.env.DEBUG) {
// eslint-disable-next-line no-console
console.warn(`Port check request failed with status code ${res.statusCode}`);
}
// Consume response data to free up memory
res.resume();
resolve(PortStateEnum.ERROR);
return;
}

// Optionally set the encoding to receive strings directly
res.setEncoding('utf8');

// Collect data chunks
res.on('data', (chunk) => {
data += chunk;

if (data.length > MAX_RESPONSE_SIZE) {
resolve(PortStateEnum.ERROR);

if (process.env.DEBUG) {
// eslint-disable-next-line no-console
console.warn('Port check response size exceeded');
}

req.destroy();
}
});

// Handle the end of the response
res.on('end', () => {
resolve(data);
});
Comment on lines +117 to +119
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Missing HTTP status code verification

Upon receiving the response, the code does not check the HTTP status code before resolving with the data. This could lead to handling error responses (e.g., 4xx or 5xx status codes) as if they were successful, which may cause issues downstream.

Consider verifying the res.statusCode and handle non-200 status codes appropriately. Here's a suggested change:

// Handle the end of the response
res.on('end', () => {
+   if (res.statusCode !== 200) {
+     if (process.env.DEBUG) {
+       // eslint-disable-next-line no-console
+       console.warn(`Received non-200 status code: ${res.statusCode}`);
+     }
+     resolve(PortStateEnum.ERROR);
+     return;
+   }
    resolve(data);
});

Committable suggestion was skipped due to low confidence.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! But it's better to do when we just got a response.

Copy link
Contributor

Choose a reason for hiding this comment

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

@shumkov, you're absolutely correct. Checking the status code immediately when we receive the response is a better approach. Here's how you might adjust the code:

return new Promise((resolve) => {
  const req = https.request(options, (res) => {
    let data = '';

+   // Check if the status code is 200
+   if (res.statusCode !== 200) {
+     if (process.env.DEBUG) {
+       // eslint-disable-next-line no-console
+       console.warn(`Port check request failed with status code ${res.statusCode}`);
+     }
+     // Consume response data to free up memory
+     res.resume();
+     resolve(PortStateEnum.ERROR);
+     return;
+   }

    // Optionally set the encoding to receive strings directly
    res.setEncoding('utf8');

    // Collect data chunks
    res.on('data', (chunk) => {
      data += chunk;

      if (data.length > MAX_RESPONSE_SIZE) {
        resolve(PortStateEnum.ERROR);

        if (process.env.DEBUG) {
          // eslint-disable-next-line no-console
          console.warn('Port check response size exceeded');
        }

        req.destroy();
      }
    });

    // Handle the end of the response
    res.on('end', () => {
      resolve(data);
    });
  });

  // Handle request errors
  req.on('error', (e) => {
    if (process.env.DEBUG) {
      // eslint-disable-next-line no-console
      console.warn(`Port check request failed: ${e}`);
    }

    resolve(PortStateEnum.ERROR);
  });

  req.end();
});

I've noted this for future reviews.

(•ᴥ•)ゝ

✏️ Learnings added 
Learnt from: shumkov
PR: dashpay/platform#2162
File: packages/dashmate/src/status/providers.js:117-119
Timestamp: 2024-09-26T16:21:07.033Z
Learning: In the `checkPortStatus` function, it's better to check the HTTP status code immediately upon receiving the response in the `https.request` callback, rather than in the `res.on('end')` callback.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used 
Learnt from: shumkov
PR: dashpay/platform#2162
File: packages/dashmate/src/status/providers.js:121-127
Timestamp: 2024-09-26T14:28:54.892Z
Learning: In the `mnowatch.checkPortStatus` function, failures should resolve with `PortStateEnum.ERROR` instead of rejecting the promise.

Learnt from: shumkov
PR: dashpay/platform#2162
File: packages/dashmate/src/status/providers.js:87-96
Timestamp: 2024-09-26T14:58:26.560Z
Learning: In the `checkPortStatus` function, it's better to use `options.timeout` to set the socket timeout for both request and response.

});

req.on('error', (e) => {
if (process.env.DEBUG) {
// eslint-disable-next-line no-console
console.warn(`Port check request failed: ${e}`);
}

resolve(PortStateEnum.ERROR);
});

req.end();
});
},
},
};
Loading