BLUE-276 Restrict first node selection based on the specified node ip and port #85

jairajdev · 2024-09-19T16:34:33Z

No description provided.

github-actions · 2024-09-19T16:35:13Z

PR Reviewer Guide 🔍

(Review updated until commit `84deacd`)

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review Error Handling The error handling in the new code does not differentiate between different types of errors, which might be necessary for better debugging and user feedback. Consider refining the error messages and handling to be more specific about the cause of the error. Code Duplication There is repeated code for validating types and sending error responses in the new changes. Consider creating a helper function to handle these repetitive tasks to make the code cleaner and more maintainable. Security Concern Ensure that the validation logic in `Utils.validateTypes` is robust against potential security issues such as injection attacks or improper input handling, especially since it deals with external inputs.

src/API.ts

urnotsam · 2024-09-19T17:04:09Z

src/Config.ts

+  restrictFirstNodeSelection: true,
+  firstNodeInfo: {
+    IP: '127.0.0.1',
+    PORT: 4000,


shouldnt the default port be 9001? 4000 is the port of the archiver

@urnotsam good catch on that. changed it to 9001 now.

kun6fup4nd4 · 2024-09-19T17:09:31Z

src/API.ts

@@ -93,16 +94,22 @@ export function registerRoutes(server: FastifyInstance<Server, IncomingMessage,
        reply.send({ success: false, error: 'Signature verification failed' })
        return
      }
+      const ip = signedFirstNodeInfo.nodeInfo.externalIp


what if ip in payload will be different from actual ip of the sender ?

The IP and Port are not even being checked in here are they? The title of this PR says restricted based off IP and Port, but I only see the public key being checked.
@mhanson-github @jairajdev

… node based on public key

src/API.ts

+          return
+        }
+        if (signedFirstNodeInfo.nodeInfo.publicKey !== signedFirstNodeInfo.sign.owner) {
+          Logger.mainLogger.error('nodeInfo.publicKey does not match signature owner', signedFirstNodeInfo)


src/API.ts

+      }
+      if (config.restrictFirstNodeSelectionByPublicKey) {
+        if (config.firstNodeInfo.PUBLIC_KEY !== '' && publicKey !== config.firstNodeInfo.PUBLIC_KEY) {
+          Logger.mainLogger.error('Invalid publicKey of first node info', signedFirstNodeInfo)


mhanson-github

I approve my own .patch file changes.

chrypnotoad · 2024-09-19T22:17:24Z

@CodiumAI-Agent /describe

chrypnotoad · 2024-09-19T22:17:38Z

/review

CodiumAI-Agent · 2024-09-19T22:18:01Z

Title

(Describe updated until commit `84deacd`)

BLUE-276 Restrict first node selection based on the specified node ip and port

PR Type

enhancement, bug fix

Description

Enhanced the first node selection process by adding type validation for signedFirstNodeInfo and its nested properties.
Introduced a check to ensure the publicKey matches the signature owner, improving security.
Added configuration options to restrict first node selection based on a specified public key.
Updated the debug mode patch to ensure it applies correctly with the current configuration.

Changes walkthrough 📝

Relevant files

Enhancement

API.ts `Enhance first node selection with validation and public key` `restriction` src/API.ts Added type validation for `signedFirstNodeInfo` and its properties. Implemented a check to ensure `publicKey` matches the signature owner. Introduced a configuration-based restriction for first node selection by public key.	+40/-4

Configuration changes

Config.ts `Add configuration for first node public key restriction` src/Config.ts Added new configuration options for restricting first node selection by public key. Set default values for the new configuration options.	+4/-0
debug_mode.patch `Update debug mode patch for configuration files` debug_mode.patch Updated patch to change `ARCHIVER_MODE` from `release` to `debug`. Adjusted patch indices to reflect current file state.	+4/-4

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

github-actions · 2024-09-19T22:18:23Z

Persistent review updated to latest commit 84deacd

chrypnotoad · 2024-09-19T22:23:08Z

src/Config.ts

@@ -184,6 +186,8 @@ let config: Config = {
    txCronSchedule: '*/5 * * * *',
  },
  workerProcessesDebugLog: false,
+  restrictFirstNodeSelectionByPublicKey: false,
+  firstNodePublicKey: '',


Should this be an environment variable? When and how does this get set?

Restrict first node selection based on the specified node ip and port

6d51e9a

github-actions bot added the Review effort [1-5]: 3 label Sep 19, 2024

github-advanced-security bot found potential problems Sep 19, 2024

View reviewed changes

src/API.ts Fixed Show fixed Hide fixed

urnotsam reviewed Sep 19, 2024

View reviewed changes

kun6fup4nd4 reviewed Sep 19, 2024

View reviewed changes

Change the first node port to 9001

9b3252f

urnotsam previously approved these changes Sep 19, 2024

View reviewed changes

Added restrictFirstNodeSelectionByPublicKey to use in selecting first…

22c7919

… node based on public key

jairajdev dismissed urnotsam’s stale review via 22c7919 September 19, 2024 19:25

github-advanced-security bot found potential problems Sep 19, 2024

View reviewed changes

jairajdev and others added 3 commits September 20, 2024 01:29

Set restrictFirstNodeSelection to false as default

12b6770

fix validateTypes

8ae1434

Remove first node IP and PORT check code

156c2a4

urnotsam previously approved these changes Sep 19, 2024

View reviewed changes

refactor restrictFirstNodeSelectionByPublicKey check

15026bc

jairajdev dismissed urnotsam’s stale review via 15026bc September 19, 2024 20:40

kun6fup4nd4 previously approved these changes Sep 19, 2024

View reviewed changes

urnotsam previously approved these changes Sep 19, 2024

View reviewed changes

Fix debug_mode.patch so it applies again.

84deacd

mhanson-github dismissed stale reviews from urnotsam and kun6fup4nd4 via 84deacd September 19, 2024 22:02

mhanson-github approved these changes Sep 19, 2024

View reviewed changes

chrypnotoad reviewed Sep 19, 2024

View reviewed changes

mhanson-github merged commit 09c4382 into dev Sep 19, 2024
7 checks passed

mhanson-github deleted the restrictFirstNodeSelection branch September 23, 2024 01:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BLUE-276 Restrict first node selection based on the specified node ip and port #85

BLUE-276 Restrict first node selection based on the specified node ip and port #85

jairajdev commented Sep 19, 2024

github-actions bot commented Sep 19, 2024 •

edited

Loading

urnotsam Sep 19, 2024

jairajdev Sep 19, 2024

kun6fup4nd4 Sep 19, 2024

chrypnotoad Sep 19, 2024

mhanson-github left a comment

chrypnotoad commented Sep 19, 2024

chrypnotoad commented Sep 19, 2024

CodiumAI-Agent commented Sep 19, 2024 •

edited

Loading

github-actions bot commented Sep 19, 2024

chrypnotoad Sep 19, 2024

BLUE-276 Restrict first node selection based on the specified node ip and port #85

BLUE-276 Restrict first node selection based on the specified node ip and port #85

Conversation

jairajdev commented Sep 19, 2024

github-actions bot commented Sep 19, 2024 • edited Loading

PR Reviewer Guide 🔍

(Review updated until commit 84deacd)

urnotsam Sep 19, 2024

Choose a reason for hiding this comment

jairajdev Sep 19, 2024

Choose a reason for hiding this comment

kun6fup4nd4 Sep 19, 2024

Choose a reason for hiding this comment

chrypnotoad Sep 19, 2024

Choose a reason for hiding this comment

mhanson-github left a comment

Choose a reason for hiding this comment

chrypnotoad commented Sep 19, 2024

chrypnotoad commented Sep 19, 2024

CodiumAI-Agent commented Sep 19, 2024 • edited Loading

Title

(Describe updated until commit 84deacd)

PR Type

Description

Changes walkthrough 📝

github-actions bot commented Sep 19, 2024

chrypnotoad Sep 19, 2024

Choose a reason for hiding this comment

github-actions bot commented Sep 19, 2024 •

edited

Loading

(Review updated until commit `84deacd`)

CodiumAI-Agent commented Sep 19, 2024 •

edited

Loading

(Describe updated until commit `84deacd`)