feat(skymp5-server): log guid on connection in ServerState

[nic11]

Important

Add logging for GUIDs during user connection in ServerState.cpp and login.ts.

• C++ Changes:
  • In ServerState.cpp, ServerState::Connect now logs GUID assignment and overwrites using spdlog.
• TypeScript Changes:
  • In login.ts, customPacket logs the user's GUID when connecting, using console.log.

^{This description was created by for fda9433. It will automatically update as commits are pushed.}

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to 691cda8 in 1 minute and 25 seconds

More details

• Looked at 34 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 4 drafted comments based on config settings.

1. skymp5-server/cpp/server_guest_lib/ServerState.cpp:11

• Draft comment:
  Consider verifying intent: if an existing user already exists, is overwriting it acceptable? Logging an error might indicate an unexpected condition.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 20% <= threshold 50%
  The comment is asking the PR author to verify their intent regarding overwriting an existing user, which violates the rule against asking for confirmation of intent. However, it does suggest logging an error, which could be seen as a specific code suggestion. Overall, the primary focus is on verifying intent, which is not allowed.

2. skymp5-server/cpp/server_guest_lib/ServerState.cpp:20

• Draft comment:
  Using spdlog::error for normal connection info may be misleading. Consider using info or debug level for assigning guid.
• Reason this comment was not posted:
  Marked as duplicate.

3. skymp5-server/cpp/server_guest_lib/ServerState.cpp:11

• Draft comment:
  If overwriting userInfo is expected, consider using a lower log level (e.g. warn) instead of error.
• Reason this comment was not posted:
  Marked as duplicate.

4. skymp5-server/cpp/server_guest_lib/ServerState.cpp:20

• Draft comment:
  Ensure that logging the GUID does not expose sensitive user data; consider redacting or logging at a restricted log level if necessary.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  The comment is speculative and asks for verification without clear evidence of a security issue. GUIDs are typically used as non-sensitive identifiers. The logging is at error level which is appropriate for connection issues. There's no clear indication these GUIDs contain sensitive data.
  I could be wrong about the nature of these GUIDs - in some systems they might contain or be derived from sensitive data. The logging level choice could have security implications.
  Without specific evidence that these GUIDs contain sensitive data, this is a speculative concern. The error log level suggests these are for debugging connection issues, not sensitive data.
  Delete the comment as it's speculative and asks for verification without evidence of an actual security issue.

Workflow ID: wflow_FCbqOoknxa3NdLTZ

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to fda9433 in 1 minute and 26 seconds

More details

• Looked at 48 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 6 drafted comments based on config settings.

1. skymp5-server/cpp/server_guest_lib/ServerState.cpp:11

• Draft comment:
  Consider checking that userId is within the bounds of userInfo before using operator[] to avoid potential out-of-bound access.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  The code seems to follow a pattern where bounds checking isn't done for internal methods that expect valid IDs. The Connect() method appears to be an internal method that would be called with validated IDs. If an invalid ID was passed, it would likely indicate a programming error rather than a runtime error to handle. The logging suggests this is more about tracking state changes than handling invalid input.
  I could be wrong about the internal/external nature of this method. Without seeing the header file and usage patterns, I can't be 100% certain about the validation assumptions.
  Even with that uncertainty, the consistent pattern in the codebase and the presence of explicit bounds checking in public-facing methods suggests this is working as intended.
  The comment should be deleted as it appears to be suggesting unnecessary defensive programming in what seems to be an internal method with valid ID assumptions.

2. skymp5-server/cpp/server_guest_lib/ServerState.cpp:20

• Draft comment:
  Good logging upon connection; however, consider whether spdlog::info is appropriate in production for connection events.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

3. skymp5-server/ts/systems/login.ts:95

• Draft comment:
  Logging now includes GUID info which improves traceability. Ensure that getUserGuid reliably returns a value.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

4. skymp5-server/cpp/server_guest_lib/ServerState.cpp:11

• Draft comment:
  Good use of spdlog to log an overwrite. Double-check if overwriting an existing userInfo should be an error or warning.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

5. skymp5-server/ts/systems/login.ts:97

• Draft comment:
  Logging guid along with ip improves traceability of connections. Looks good.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

6. skymp5-server/ts/systems/login.ts:137

• Draft comment:
  Consider using a distinct variable name instead of re-declaring 'guidBeforeAsyncOp' in the nested Discord auth block for clarity.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

Workflow ID: wflow_SivndkNuNt7GKrXB

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to fda9433 in 1 minute and 32 seconds

More details

• Looked at 48 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. skymp5-server/cpp/server_guest_lib/ServerState.cpp:11

• Draft comment:
  Consider adding a boundary check to ensure that userId is within the bounds of userInfo before indexing (e.g. use size() check) to avoid potential undefined behavior.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  The Connect() method is likely meant to be called when adding new users, so it may need to access indices beyond the current size. The vector would automatically grow if using operator[]. Other methods have bounds checks because they're querying existing users. The lack of bounds check here may be intentional design rather than an oversight.
  I could be wrong about the automatic vector growth - if userInfo is a fixed-size array, this could cause undefined behavior. Also, even with a vector, accessing beyond size() with operator[] isn't guaranteed safe in all implementations.
  While those are valid concerns, without seeing the ServerState.h file to confirm the type of userInfo, we don't have strong evidence that this is definitely a problem rather than intended behavior.
  Without seeing the declaration of userInfo and understanding the intended behavior, we can't be confident this is a real issue requiring change.

2. skymp5-server/cpp/server_guest_lib/ServerState.cpp:11

• Draft comment:
  Check: When an existing userInfo is found, an error is logged but the new connection still overwrites the existing data. Verify that this is the intended behavior for reconnects.
• Reason this comment was not posted:
  Comment was on unchanged code.

3. skymp5-server/ts/systems/login.ts:95

• Draft comment:
  The connection log now includes the user GUID. Ensure that logging GUIDs does not leak sensitive information in production.
• Reason this comment was not posted:
  Marked as duplicate.

Workflow ID: wflow_j2KkvmmPpxI01czK

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.