Repository navigation
Conversation
…or Leakage ### Description This pull request addresses transport security and information leakage findings identified in the workspace security audit for `builder-signing-sdk`[cite: 37]. Previously, remote signer configurations permitted plain `http://` endpoints and credential-embedded URLs while potentially exposing bearer tokens and authorization headers via raw Axios error logging[cite: 37]. This PR strictly enforces HTTPS URL validation, strips credential-bearing endpoints, suppresses raw remote error output, updates SDK documentation, and integrates focused regression tests[cite: 37]. ### Key Changes & Remediations #### 1. Remote Signer Transport Security (`src/config.ts`) * **Strict HTTPS Validation:** Replaced permissive endpoint parsing with a strict validator (`validateRemoteUrl`) requiring `protocol === 'https:'`[cite: 37]. Non-HTTPS protocols or unparseable URLs immediately throw `"invalid remote url!"`[cite: 37]. * **Credential Stripping:** Disallowed embedded userinfo (`username` or `password`) in remote signer URLs to prevent accidental credential leakage and confusion during bearer token transmission[cite: 37]. #### 2. Information Leakage Suppression (`src/config.ts`) * **Sanitized Error Handling:** Suppressed raw remote error logging in `generateBuilderHeaders`[cite: 37]. Transport and network errors now resolve cleanly to `undefined` without dumping raw Axios error objects or leaking `Authorization: Bearer <token>` headers to `console.error`[cite: 37]. #### 3. Documentation & Regression Testing (`README.md`, `test/builderConfig.test.ts`) * **Regression Tests:** Added test coverage asserting rejection of `http://` endpoints, credential-bearing URLs (`https://user:password@...`), and invalid strings, while verifying that `console.error` is not invoked upon remote signer failure[cite: 37]. * **Documentation Alignment:** Updated `README.md` code snippets and configuration examples to demonstrate secure HTTPS usage, current constructor conventions, and bearer token handling[cite: 37]. ### How to Review 1. **URL Validation:** Inspect `validateRemoteUrl` in `src/config.ts` to confirm both protocol and userinfo checks fail fast with `"invalid remote url!"`[cite: 37]. 2. **Error Logging:** Verify in `src/config.ts` that `catch` blocks around the remote `post` request do not log error objects[cite: 37]. 3. **Test Suite:** Review `test/builderConfig.test.ts` to verify unit tests mock and assert the new security constraints[cite: 37].
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit c372ae2. Configure here.
Refactor payload creation to return a Promise and adjust headers structure for HTTP request.
Author
|
Resolved in the latest commit. Reverted the helper call signature back to the expected |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Description
This pull request addresses transport security and information leakage findings identified in the workspace security audit for
builder-signing-sdk[cite: 37]. Previously, remote signer configurations permitted plainhttp://endpoints and credential-embedded URLs while potentially exposing bearer tokens and authorization headers via raw Axios error logging[cite: 37]. This PR strictly enforces HTTPS URL validation, strips credential-bearing endpoints, suppresses raw remote error output, updates SDK documentation, and integrates focused regression tests[cite: 37].Key Changes & Remediations
1. Remote Signer Transport Security (
src/config.ts)validateRemoteUrl) requiringprotocol === 'https:'[cite: 37]. Non-HTTPS protocols or unparseable URLs immediately throw"invalid remote url!"[cite: 37].usernameorpassword) in remote signer URLs to prevent accidental credential leakage and confusion during bearer token transmission[cite: 37].2. Information Leakage Suppression (
src/config.ts)generateBuilderHeaders[cite: 37]. Transport and network errors now resolve cleanly toundefinedwithout dumping raw Axios error objects or leakingAuthorization: Bearer <token>headers toconsole.error[cite: 37].3. Documentation & Regression Testing (
README.md,test/builderConfig.test.ts)http://endpoints, credential-bearing URLs (https://user:password@...), and invalid strings, while verifying thatconsole.erroris not invoked upon remote signer failure[cite: 37].README.mdcode snippets and configuration examples to demonstrate secure HTTPS usage, current constructor conventions, and bearer token handling[cite: 37].How to Review
validateRemoteUrlinsrc/config.tsto confirm both protocol and userinfo checks fail fast with"invalid remote url!"[cite: 37].src/config.tsthatcatchblocks around the remotepostrequest do not log error objects[cite: 37].test/builderConfig.test.tsto verify unit tests mock and assert the new security constraints[cite: 37].Note
Medium Risk
Breaking change for integrators using
http://remote signers; behavior change masks remote failures without logging, which may complicate debugging.Overview
Tightens remote signer security in
BuilderConfig: remote URLs must be HTTPS with no embedded userinfo, replacing the priorhttp:///https://prefix check.On remote signing failures,
generateBuilderHeadersno longer logs raw Axios errors (which could expose bearer tokens); it returnsundefinedinstead. Local credential validation now rejects missing/blank fields explicitly.README is updated for Polymarket branding,
BuilderConfigexamples (env vars, separate local vs remote flows), and HTTPS/token guidance. Tests cover rejected URLs and silent error handling; existing remote tests usehttps://URLs.Reviewed by Cursor Bugbot for commit 1acb72f. Bugbot is set up for automated code reviews on this repo. Configure here.