Skip to content

fix: ignore non-JSON request bodies in obfuscator - #6

Open
JasonZhouPW wants to merge 1 commit into
BitgesellOfficial:mainfrom
JasonZhouPW:fix/request-obfuscator-non-json
Open

fix: ignore non-JSON request bodies in obfuscator#6
JasonZhouPW wants to merge 1 commit into
BitgesellOfficial:mainfrom
JasonZhouPW:fix/request-obfuscator-non-json

Conversation

@JasonZhouPW

Copy link
Copy Markdown

Summary

  • makes request-body obfuscation skip non-JSON string bodies, matching existing response-body behavior
  • adds a regression test so logging does not throw when a request body is plain text

Why

obfuscateResponse already checks whether the body is JSON before parsing. obfuscateRequest parsed every string body directly, so a plain-text request body could raise a SyntaxError during logging instead of being left unchanged.

Validation

  • RED before fix: ./node_modules/.bin/mocha --timeout 20000 --require should test/logging/request-obfuscator_test.js failed on the new non-JSON request-body regression
  • GREEN after fix: same mocha command -> 23 passing
  • ./node_modules/.bin/eslint src/logging/request-obfuscator.js test/logging/request-obfuscator_test.js
  • git diff --check

Bounty context: submitted for the Bitgesell improvement / PR bounty program, related to BitgesellOfficial/bitgesell#81 and BitgesellOfficial/bitgesell#32. Payout details can be provided after maintainer approval.

@MyTH-zyxeon

Copy link
Copy Markdown

Review-assist for #6 under the Bitgesell improvement queue (#81):

  • The fix is nicely scoped: non-JSON string request bodies now follow the same "leave unchanged" path already expected for response bodies, which should prevent logging from turning plain-text request payloads into SyntaxError failures.
  • Before merge, I would explicitly confirm that valid JSON request bodies still take the existing obfuscation path. In particular, one regression for an object body with a sensitive field and one for an array body would prove the new isJSON() guard did not accidentally bypass secret redaction.
  • It may also be worth deciding what should happen for JSON primitives such as "\"plain\"" or "42". isJSON() can accept those, but the later obfuscation code may have been written with object/array bodies in mind.
  • The new test checks "does not throw" and body preservation for plain text. That is the right minimum; adding the positive redaction test above would make the safety story complete for maintainers.
  • For #81 payout hygiene, this looks like a concrete logging-safety/test cleanup. I would still wait for maintainer confirmation before any claim or payment step.

I only reviewed the public diff. No duplicate implementation PR, secret material, dependency install, live RPC call, local checkout mutation, payment action, or live-chain action from me here.

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