feat: Implement request logging functionality #5812
Conversation
Jenkins BuildsClick to see older builds (159)
|
qfrank
force-pushed
the
feat/logging_rpc_request_response
branch
3 times, most recently
from
81abc0d
to
e2b6b17
Compare
qfrank
force-pushed
the
feat/logging_rpc_request_response
branch
4 times, most recently
from
e09099f
to
a73ba7b
Compare
✔️ status-go/prs/linux/PR-5812#9 🔹 ~2 min 18 sec 🔹 a73ba7b 🔹 📦 linux package |
qfrank
force-pushed
the
feat/logging_rpc_request_response
branch
from
a73ba7b
to
54be013
Compare
There was a problem hiding this comment.
@qfrank, logging the input/output of CallRPC or CallPrivateRPC looks okay in test environments, but many non-RPC functions in mobile/status.go receive passwords. I understand most receive hashed passwords, but I think it's better practice to redact/remove password arguments from the logs.
There's even a tiny chance that the client will mistakenly send the plain text password to some of these functions. I know this happened in PRs while the dev was working on them, but it's exactly in PR builds where logging in/out will be enabled, so it's better if we don't take this risk and redact all password args.
The Sha3 function is particularly important because clients call it by passing plain text passwords, so this one we should not log the input in any environment.
if you look closely, I didn't pass the password/mnemonic related param to |
Indeed 👍🏼 The call to 
|
|
Actually, I'm not sure if we should remove password like stuff from log as it only applies in test env. There's a hidden password passed like |
Please help me understand @qfrank, how will logging plain text passwords help us diagnose bugs? As I mentioned in my review comment #5812 (review), PR builds can be used for various purposes and somebody could forget and use real/good passwords while testing and they will be logged, and possibly shared somewhere. PR builds can be used by all kinds of consumers, not just well informed devs & QAs (think designers or a Product Owner, or a less informed external contributor). The other serious risk with logging the plain text password for test env is that the logic deciding if req/res are logged relies on enabling a single environment variable. One thing I've seen more than once in pipelines is that they can mess up env vars quite easily. That's why I made this other comment in Tetiana's issue status-im/status-mobile#21176 (comment) about considering a hard check to turn full req/res logging a no-op in release builds, that is, this would take precedence over any environment variable due to the risks of exposing private data in production. I'm curious to hear what other reviewers think. Maybe I'm being too paranoid 🤷🏼 |
|
I removed |
qfrank
force-pushed
the
feat/logging_rpc_request_response
branch
from
54be013
to
b8ee586
Compare
qfrank
changed the title
qfrank
force-pushed
the
feat/logging_rpc_request_response
branch
5 times, most recently
from
8affc0e
to
8fc1f8e
Compare
|
Thank you for the nice and detailed |
qfrank
force-pushed
the
feat/logging_rpc_request_response
branch
7 times, most recently
from
a7f208f
to
0b02aca
Compare
qfrank
force-pushed
the
feat/logging_rpc_request_response
branch
from
0b02aca
to
70540da
Compare
There was a problem hiding this comment.
Thank you @qfrank for all changes.
Please don't take this 12 comments as sign of something bad. I appreciate your PR, I'm very happy to see tests, you've done a great job here 👍 I only left some many comments because I know you are a good dev that will actually learn from this.
And I'm sure you will love OOP some day 👿 😄
|
all your feedback addressed, pls help force merge, thank you ❤️ @igor-sirotin |
|
Force-merging, as most lack of diff-coverage is coming from |
| if fnType.Kind() != reflect.Func { | ||
| panic("fn must be a function") | ||
| } | ||
|
|
There was a problem hiding this comment.
logAndCall expects fn to return at max 1 value. I am wondering whether we should add extra check for that:
if fnType.NumOut() > 1 {
panic("fn must return at max 1 value")
}or maybe be more flexible and return []any instead.
There was a problem hiding this comment.
logAndCall is implemented based on the situation of mobile/status.go. The front-end and back-end mainly interact through json strings, so there is at most one return type. Such a definition should be sufficient. If additional verification is to be added, what I can think of may be that the num of function return type is 1 and the return type must be a string. As for []any, I currently can't think of its application scenario ：）
|
@qfrank alarm 😄
msg=verifyDatabasePassword
params="[0x2a95c22f1b80ba16fd4fd87600cb7ddae69f8b454078ecc35f4d8db642950afe 0xBAF4C353A3DC6A9F6B539020E2EA08490F8C7CA63D3AD1277DA307F6BC25199C]"It's not caught by Lines 285 to 287 in 6696e6f |
|
In general, we replace this with a single JSON argument for better compatibility. So probably |
|
@qfrank please, let me know when when this is fixed and how I can call the method and I'll verify it, thanks you! |
I'll fix it after dinner 🙂 |
Key changes:
In
requestlog/request_log.go:In
mobile/init_logging_test.go:The main features added are:
These changes enhance the project's logging capabilities, particularly for request operations, which can be crucial for debugging and monitoring in Ethereum-based applications.
relate mobile issue
relate mobile PR
relate go-ethereum PR