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.

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: masks ip address in request details passed as argument to MN client #3268

Merged
merged 6 commits into from
Nov 19, 2024

Conversation

konstantinabl
Copy link
Collaborator

@konstantinabl konstantinabl commented Nov 15, 2024

Description:

The issue arises from the fact requestDetails are passed as an argument to the repeatedRequest method in MN client, because they are needed for the next method call

However, further down in the method the args are logged.

Related issue(s):

Fixes #3266

@konstantinabl konstantinabl linked an issue Nov 15, 2024 that may be closed by this pull request
@konstantinabl konstantinabl self-assigned this Nov 15, 2024
@konstantinabl konstantinabl added the internal For changes that affect the project's internal workings but not its outward-facing functionality. label Nov 15, 2024
@konstantinabl konstantinabl added this to the 0.61.0 milestone Nov 15, 2024
Copy link

github-actions bot commented Nov 15, 2024

Test Results

 20 files  ± 0  269 suites  +21   33m 32s ⏱️ + 1m 41s
609 tests ± 0  603 ✅ ± 0  4 💤 ±0  2 ❌ ±0 
730 runs  +31  723 ✅ +30  5 💤 +1  2 ❌ ±0 

For more details on these failures, see this check.

Results for commit 1845b67. ± Comparison against base commit 27a10dc.

This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.
"before all" hook in "@api-batch-2 RPC Server Acceptance Tests" ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests "before all" hook in "@api-batch-2 RPC Server Acceptance Tests"
"before all" hook for "should execute "eth_getCode" for hts token" ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests eth_getCode "before all" hook for "should execute "eth_getCode" for hts token"

♻️ This comment has been updated with latest results.

@konstantinabl konstantinabl force-pushed the 3266-relay-is-logging-ip-addresses branch from d5db1d0 to e0a7de0 Compare November 15, 2024 14:30
@quiet-node quiet-node marked this pull request as draft November 15, 2024 15:04
@konstantinabl konstantinabl force-pushed the 3266-relay-is-logging-ip-addresses branch from e0a7de0 to 9302c27 Compare November 15, 2024 15:10
Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>
@konstantinabl konstantinabl force-pushed the 3266-relay-is-logging-ip-addresses branch 2 times, most recently from d4ff0f7 to 9e307bf Compare November 15, 2024 15:15
Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>
@konstantinabl konstantinabl force-pushed the 3266-relay-is-logging-ip-addresses branch from 9e307bf to 7e24f26 Compare November 15, 2024 15:17
@konstantinabl konstantinabl marked this pull request as ready for review November 15, 2024 15:33
@konstantinabl konstantinabl changed the title fix: removes request details from args so ip addresses are not logged fix: masks ip address in request details passed as argument to MN client Nov 15, 2024
quiet-node
quiet-node previously approved these changes Nov 15, 2024
Copy link
Member

@quiet-node quiet-node left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the quick great work!

Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

Looking good but I won't if there is a single location we can set this oce

packages/relay/src/lib/eth.ts Show resolved Hide resolved
natanasow
natanasow previously approved these changes Nov 18, 2024
Copy link
Collaborator

@natanasow natanasow left a comment

Choose a reason for hiding this comment

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

Approved with some nits which might be fixed in a follow-up PR due to the urgency of this one.

packages/relay/src/lib/eth.ts Outdated Show resolved Hide resolved
packages/relay/src/lib/eth.ts Show resolved Hide resolved
packages/relay/src/lib/eth.ts Show resolved Hide resolved
victor-yanev
victor-yanev previously approved these changes Nov 18, 2024
Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>
@konstantinabl konstantinabl dismissed stale reviews from victor-yanev and natanasow via 47ce73d November 19, 2024 12:09
Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>
Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>
Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>
@konstantinabl konstantinabl force-pushed the 3266-relay-is-logging-ip-addresses branch from e71c520 to 1845b67 Compare November 19, 2024 15:49
@konstantinabl konstantinabl merged commit 5584d9f into main Nov 19, 2024
52 of 62 checks passed
@konstantinabl konstantinabl deleted the 3266-relay-is-logging-ip-addresses branch November 19, 2024 16:48
quiet-node pushed a commit that referenced this pull request Nov 19, 2024
…ent (#3268)

* removes request details from passed arg to MN client

Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>

* Masks ip beforehand instead of changing object in repeated request

Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>

* Adds constant and draft unit test

Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>

* changes unit test

Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>

* Fixes unit test

Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>

* removes unused var

Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>

---------

Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>
quiet-node pushed a commit that referenced this pull request Nov 19, 2024
…ent (#3268)

* removes request details from passed arg to MN client

Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>

* Masks ip beforehand instead of changing object in repeated request

Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>

* Adds constant and draft unit test

Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>

* changes unit test

Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>

* Fixes unit test

Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>

* removes unused var

Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>

---------

Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
quiet-node added a commit that referenced this pull request Nov 19, 2024
fix: masks ip address in request details passed as argument to MN client (#3268)

* removes request details from passed arg to MN client



* Masks ip beforehand instead of changing object in repeated request



* Adds constant and draft unit test



* changes unit test



* Fixes unit test



* removes unused var



---------

Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Co-authored-by: konstantinabl <konstantina.blajukova@gmail.com>
@quiet-node quiet-node modified the milestones: 0.61.0, 0.60.0 Nov 20, 2024
@quiet-node quiet-node added bug Something isn't working internal For changes that affect the project's internal workings but not its outward-facing functionality. and removed internal For changes that affect the project's internal workings but not its outward-facing functionality. bug Something isn't working labels Nov 20, 2024
Copy link

codecov bot commented Nov 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.89%. Comparing base (b99655e) to head (1845b67).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3268      +/-   ##
==========================================
+ Coverage   76.64%   77.89%   +1.25%     
==========================================
  Files          65       66       +1     
  Lines        4449     4470      +21     
  Branches     1014     1003      -11     
==========================================
+ Hits         3410     3482      +72     
+ Misses        672      613      -59     
- Partials      367      375       +8     
Flag Coverage Δ
config-service 98.14% <ø> (ø)
relay 78.64% <100.00%> (+0.06%) ⬆️
server 83.28% <ø> (?)
ws-server 36.66% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/relay/src/lib/clients/mirrorNodeClient.ts 86.19% <100.00%> (-1.54%) ⬇️
packages/relay/src/lib/constants.ts 86.27% <100.00%> (-3.93%) ⬇️
packages/relay/src/lib/eth.ts 71.07% <100.00%> (-3.55%) ⬇️

... and 28 files with indirect coverage changes

---- 🚨 Try these New Features:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal For changes that affect the project's internal workings but not its outward-facing functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relay is logging IP addresses
6 participants