fix: Included ip address to headers of the signin flow

[sujitaw]

What

• Included ip address for the api call being made through nextjs backend to the nest js backend during the authentication process

---

Important

Add client IP to sign-in headers and refactor password encryption handling.

• Behavior:
  • Add client IP address to headers in authorize() in authOptions.ts for sign-in requests.
  • Update CRYPTO_PRIVATE_KEY to NEXT_PUBLIC_CRYPTO_PRIVATE_KEY in Auth.ts and SchemaListUtils.ts.
• Refactoring:
  • Remove encryptPasswordActionForSignIn and encryptPasswordAction functions from passwordActions.ts.
  • Use passwordEncryption directly in SessionManager.tsx and user-auth-form.tsx.
• Misc:
  • Remove passwordActions.ts file.

^{This description was created by for 40d1e22. You can customize this summary. It will automatically update as commits are pushed.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to 882ae4a in 1 minute and 50 seconds. Click for details.

• Reviewed 34 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 3 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. src/utils/authOptions.ts:77

• Draft comment:
  Using 'undefined' as a fallback for the 'User-Agent' header may lead to omitted header. Confirm if this behavior is intended or if the header should be conditionally added only when defined.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The difference between undefined and no fallback is minimal here - both will result in the header being omitted in the fetch request if no user-agent is present. The comment doesn't point out any actual problem, just asks for confirmation of intended behavior. This falls under the rule about not asking authors to confirm their intentions. Maybe there's a subtle difference between undefined and no fallback in how the fetch API handles headers that I'm not considering? Maybe this is a security-sensitive area where being explicit matters? Even if there were subtle differences, the comment is still asking for confirmation rather than pointing out a concrete issue. The code will work correctly either way. Delete the comment as it violates the rule about not asking authors to confirm their intentions, and doesn't identify any concrete issue that needs fixing.

2. src/utils/authOptions.ts:78

• Draft comment:
  Extracting clientIp from the 'x-forwarded-for' header may include extra whitespace. Consider trimming the IP to ensure a clean value, e.g., by using .trim() after split.
• 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% While technically correct that IPs could have whitespace, this is a relatively minor edge case. The IP is only being used as a header value for logging/tracking purposes (passed in x-forwarded-for header to backend). Even with whitespace, it would still work for that purpose. The code already handles the more important IPv6 mapping case. The presence of whitespace could potentially cause issues with IP-based security features or logging consistency. Not trimming could lead to duplicate entries for the same IP. Looking at the code context, this IP is only used as a pass-through header value, not for security checks or precise logging. The impact of whitespace would be minimal. While technically correct, this suggestion addresses an edge case that has minimal impact on the actual functionality. The comment should be removed as it doesn't highlight a significant issue.

3. src/utils/authOptions.ts:121

• Draft comment:
  Currently 'clientIp' defaults to 'unknown' when not provided. Consider conditionally adding the 'x-forwarded-for' header only if a valid IP is available to prevent sending misleading values downstream.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The comment has some merit since sending 'unknown' as an IP could be misleading. However, the receiving server likely expects this header and may handle the 'unknown' value appropriately. The current implementation is a common pattern for handling missing IPs. Without knowing the server's requirements, we can't be certain this is an issue. We don't know if the receiving server has specific requirements for the x-forwarded-for header. The current implementation might be intentional to ensure the header is always present. While the suggestion could be an improvement, we lack strong evidence that the current approach is problematic or that conditional header inclusion would be better. The comment makes a reasonable suggestion but without clear evidence of an issue or knowledge of server requirements, it's not a strong enough case to keep.

Workflow ID: wflow_hlz06goZHzLYfGJ9

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed 779b219 in 1 minute and 19 seconds. Click for details.

• Reviewed 13 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 0 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

Workflow ID: wflow_bivfNhWvWU2D45No

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Potential runtime error: if the 'x-forwarded-for' header is missing, clientIp becomes undefined, causing clientIp.startsWith(...) to throw. Guard the call (e.g., if (clientIp && clientIp.startsWith('::ffff:'))).

Suggested change

	if (clientIp.startsWith('::ffff:')) {
	if (clientIp && clientIp.startsWith('::ffff:')) {

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 92eade8 in 2 minutes and 2 seconds. Click for details.

• Reviewed 159 lines of code in 5 files
• Skipped 0 files when reviewing.
• Skipped posting 4 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. src/app/actions/passwordActions.ts:1

• Draft comment:
  File removed. Ensure that no remaining references to these functions exist.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. src/app/api/Auth.ts:226

• Draft comment:
  Using NEXT_PUBLIC_CRYPTO_PRIVATE_KEY exposes your encryption key on the client side—confirm that this is acceptable.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The change from CRYPTO_PRIVATE_KEY to NEXT_PUBLIC_CRYPTO_PRIVATE_KEY is concerning as it explicitly makes the encryption key available to client-side code. This is generally a security anti-pattern. The function passwordEncryption() is using this key for AES encryption, which should ideally happen server-side. This seems like a legitimate security concern worth raising. I might be wrong about where this encryption is meant to happen - perhaps there's a specific requirement for client-side encryption. Also, without seeing the full architecture, I can't be certain this isn't part of a larger security strategy. Even if client-side encryption is intentional, exposing encryption keys to the client is still a security risk that should be carefully considered. The comment appropriately asks for confirmation rather than demanding a change. Keep the comment as it raises a valid security concern about the exposure of encryption keys, while allowing the team to confirm if this is their intended approach.

3. src/features/components/SessionManager.tsx:89

• Draft comment:
  The PR description mentions including the IP address in headers, but this header is not visible here. Verify that the IP is added elsewhere (e.g. in getHeaderConfigs) as intended.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment asks the PR author to verify if the IP address is added elsewhere, which is against the rules. It does not provide a specific code suggestion or ask for a specific test to be written. Therefore, it should be removed.

4. src/app/api/Auth.ts:232

• Draft comment:
  Consider whether JSON.stringify is necessary for encrypting a plain string, as it may add extra quotes.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_dDikenoR3ViOjh6u

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 40d1e22 in 29 seconds. Click for details.

• Reviewed 13 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. src/utils/authOptions.ts:80

• Draft comment:
  Good fix: Ensures clientIp is defined before calling startsWith to avoid runtime errors.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative, as it simply acknowledges a good fix without providing any actionable feedback or suggestions. It does not align with the rules, which prohibit purely informative comments.

Workflow ID: wflow_VSBhiXh9G1X1aMDK

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}