Skip to content

Conversation

@mmsqe
Copy link

@mmsqe mmsqe commented Mar 17, 2025

Closes: #XXX

Description


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@mmsqe mmsqe requested a review from yihuang March 17, 2025 04:17
@codecov
Copy link

codecov bot commented Mar 17, 2025

Codecov Report

Attention: Patch coverage is 6.66667% with 28 lines in your changes missing coverage. Please review.

Project coverage is 46.13%. Comparing base (76bf49e) to head (6f1cd20).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
rpc/namespaces/ethereum/debug/trace.go 0.00% 27 Missing ⚠️
server/config/config.go 50.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #576      +/-   ##
===========================================
- Coverage    46.19%   46.13%   -0.07%     
===========================================
  Files          181      181              
  Lines        18158    18186      +28     
===========================================
+ Hits          8388     8390       +2     
- Misses        9050     9076      +26     
  Partials       720      720              
Files with missing lines Coverage Δ
server/flags/flags.go 50.00% <ø> (ø)
server/start.go 19.05% <100.00%> (+0.16%) ⬆️
server/config/config.go 30.09% <50.00%> (+0.19%) ⬆️
rpc/namespaces/ethereum/debug/trace.go 0.00% <0.00%> (ø)
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

@yihuang yihuang left a comment

Choose a reason for hiding this comment

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

are you trying to fix security issue with the file parameter when serving debug namespace public?

@mmsqe
Copy link
Author

mmsqe commented Mar 17, 2025

are you trying to fix security issue with the file parameter when serving debug namespace public?

We don't have public debug api right, but need set a specific dir for internal user to download the trace file?

@yihuang
Copy link

yihuang commented Mar 17, 2025

are you trying to fix security issue with the file parameter when serving debug namespace public?

We don't have public debug api right, but need set a specific dir for internal user to download the trace file?

the path is passed by the user, so the user know exactly where the file will be written?

@mmsqe
Copy link
Author

mmsqe commented Mar 17, 2025

the path is passed by the user, so the user know exactly where the file will be written?

Yes but we cant allow user specify any path due to security concern right?

@yihuang
Copy link

yihuang commented Mar 19, 2025

the path is passed by the user, so the user know exactly where the file will be written?

Yes but we cant allow user specify any path due to security concern right?

there's only security issue when serve debug namespace publicly.

@mmsqe
Copy link
Author

mmsqe commented Mar 19, 2025

there's only security issue when serve debug namespace publicly.

if user can call this api, mean debug already allow in api namespaces right?

@yihuang
Copy link

yihuang commented Mar 19, 2025

there's only security issue when serve debug namespace publicly.

if user can call this api, mean debug already allow in api namespaces right?

By public, I mean it can be called by un-trusted client. But user might still want to run a node for himself or trusted user to use.

Copy link

@yihuang yihuang left a comment

Choose a reason for hiding this comment

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

I made a cleanup, what do you think about the changes.

@mmsqe
Copy link
Author

mmsqe commented Mar 19, 2025

I made a cleanup, what do you think about the changes.

seems cleaner, we also allow pass in start cmd right?

@yihuang yihuang changed the title Problem: no specific dir for the log from debug_goTrace Problem: path parameter in debug_goTrace is not safe when serve public Mar 19, 2025
@yihuang yihuang changed the title Problem: path parameter in debug_goTrace is not safe when serve public Problem: path parameter in debug_goTrace is not safe when serve public rpc Mar 19, 2025
@yihuang yihuang merged commit 03a1f12 into crypto-org-chain:develop Mar 19, 2025
37 of 39 checks passed
mmsqe added a commit that referenced this pull request Apr 8, 2025
* Problem: dependency contains personal fork (#571)

* Problem: incorrect spendable balance when debug trace tx (#574)

* Problem: incorrect spendable balance when debug trace tx

* fix

* Update tests/integration_tests/test_tracers.py

Co-authored-by: mmsqe <tqd0800210105@gmail.com>
Signed-off-by: yihuang <huang@crypto.com>

---------

Signed-off-by: yihuang <huang@crypto.com>
Co-authored-by: yihuang <huang@crypto.com>

* Problem: path parameter in debug_goTrace is not safe when serve public rpc (#576)

* Problem: no specific dir for the log from debug_goTrace

* lint

* allow any

* revert

* cleanup

* fix format

* fix config

* Apply suggestions from code review

---------

Co-authored-by: HuangYi <huang@crypto.com>

---------

Signed-off-by: yihuang <huang@crypto.com>
Co-authored-by: yihuang <huang@crypto.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants