feat: add OAuth 1.0 authentication support#7482
Open
lohit-bruno wants to merge 4 commits intousebruno:mainfrom
Open
feat: add OAuth 1.0 authentication support#7482lohit-bruno wants to merge 4 commits intousebruno:mainfrom
OAuth 1.0 authentication support#7482lohit-bruno wants to merge 4 commits intousebruno:mainfrom
Conversation
Add full OAuth 1.0 (RFC 5849) authentication with support for HMAC-SHA1/256/512, RSA-SHA1/256/512, and PLAINTEXT signature methods. Includes UI components, bru/yml serialization, Postman import, code generation, CLI support, and comprehensive playwright and unit tests.
lohit-bruno
requested review from
bijin-bruno,
helloanoop and
naman-bruno
as code owners
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds end-to-end OAuth 1.0a (RFC 5849) support across Bruno: new signing library, UI components, schema/types, CLI/Electron signing and interpolation, converters/filestore support, extensive tests and fixtures, Playwright suites, and re-exports for request signing helpers. Changes
Sequence DiagramsequenceDiagram
participant User as Client/User
participant Prep as Prepare Request
participant Interp as Interpolator
participant Sign as OAuth1 Signer
participant Net as Network Layer
participant Server as API Server
User->>Prep: create/send request (may include oauth1config)
Prep->>Interp: interpolate variables (consumerKey, secrets, privateKey, ...)
Interp->>Prep: return interpolated oauth1config
Prep->>Sign: applyOAuth1ToRequest(axiosRequest, collectionPath?)
Sign->>Sign: collect params (query/body), build base string, compute signature
Sign->>Prep: attach signature (header/query/body)
Prep->>Net: send signed request
Net->>Server: HTTP request with OAuth1 params
Server->>Server: verify signature
alt signature valid
Server->>Net: 200 OK
else invalid
Server->>Net: 401 Unauthorized
end
Net->>User: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lohit-bruno
changed the title
OAuth 1.0 authentication support
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (15)
tests/auth/oauth1/fixtures/collections/bru/OAuth1 RSA-SHA1 Body 200.bru (1)
20-49: Consider centralizing the PEM test key to reduce duplication.If this same RSA key appears in other OAuth1 RSA fixtures, pulling it from a shared fixture source would lower copy/paste drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/auth/oauth1/fixtures/collections/bru/OAuth1` RSA-SHA1 Body 200.bru around lines 20 - 49, The RSA PEM private_key block in the OAuth1 RSA-SHA1 fixture is duplicated; extract the PEM into a single shared fixture (e.g., a shared "rsa_private_key" fixture) and replace the inline private_key value in OAuth1 RSA-SHA1 Body 200.bru with a reference to that shared fixture; update any other OAuth1 RSA fixtures to reference the same shared symbol so all tests consume the centralized key and eliminate copy/paste drift.tests/auth/oauth1/fixtures/collections/yml/OAuth1 HMAC-SHA1 Body JSON 200.yml (1)
11-14: Use clearly fake credential literals to reduce secret-scanner noise.These are test fixtures, but current values can still trip generic secret detectors and create noisy security reports.
Suggested placeholder cleanup
- consumerKey: consumer_key_1 - consumerSecret: consumer_secret_1 - accessToken: access_token_1 - tokenSecret: token_secret_1 + consumerKey: oauth1-test-id + consumerSecret: oauth1-test-secret + accessToken: oauth1-test-token + tokenSecret: oauth1-test-token-secret🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/auth/oauth1/fixtures/collections/yml/OAuth1` HMAC-SHA1 Body JSON 200.yml around lines 11 - 14, Replace the realistic-looking credential values in the fixture by using clearly fake placeholder literals for the keys consumerKey, consumerSecret, accessToken, and tokenSecret (for example values like "fake_consumer_key", "fake_consumer_secret", "fake_access_token", "fake_token_secret") so the test remains meaningful but won't trigger secret scanners; update the corresponding entries in the OAuth1 HMAC-SHA1 Body JSON fixture to use those fake placeholders.tests/auth/oauth1/fixtures/collections/bru/OAuth1 PLAINTEXT Query Params 200.bru (1)
29-32: Consider adding email assertion for parity with other 200 fixtures.The HMAC-SHA1 200.bru fixture includes an assertion for
res.body.resource.email. Adding it here would maintain consistency:assert { res.status: eq 200 res.body.resource.name: eq oauth1-test-resource res.body.resource.email: eq oauth1@example.com }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/auth/oauth1/fixtures/collections/bru/OAuth1` PLAINTEXT Query Params 200.bru around lines 29 - 32, Add an email assertion to the existing response assertions: inside the assert block that currently checks res.status and res.body.resource.name (the assertions referencing res.status and res.body.resource.name: eq oauth1-test-resource), add a line asserting res.body.resource.email: eq oauth1@example.com so this PLAINTEXT fixture matches the parity of the HMAC-SHA1 200.bru fixture.tests/auth/oauth1/fixtures/collections/yml/OAuth1 HMAC-SHA256 Body 200.yml (1)
22-30: Missing email assertion for consistency.Other 200 fixtures (e.g., OAuth1 HMAC-SHA1 200.bru, OAuth1 RSA-SHA256 200.yml) include an assertion for
res.body.resource.email. Consider adding it here for consistency:- expression: res.body.resource.email operator: eq value: oauth1@example.com🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/auth/oauth1/fixtures/collections/yml/OAuth1` HMAC-SHA256 Body 200.yml around lines 22 - 30, The 200 fixture is missing the email assertion for consistency with other OAuth1 200 fixtures; add an assertion entry under runtime.assertions that checks res.body.resource.email equals oauth1@example.com by inserting an assertion with expression: res.body.resource.email, operator: eq, and value: oauth1@example.com (matching the existing assertion style used for res.body.resource.name).tests/auth/oauth1/fixtures/collections/yml/OAuth1 RSA-SHA1 Body 200.yml (1)
53-61: Consider adding email assertion for consistency.Other OAuth1 200 fixtures (e.g., HMAC-SHA1, RSA-SHA1) include an assertion for
res.body.resource.email. This fixture only checksresource.name. Adding the email check would improve consistency across your test suite.💡 Suggested addition
runtime: assertions: - expression: res.status operator: eq value: '200' - expression: res.body.resource.name operator: eq value: oauth1-test-resource + - expression: res.body.resource.email + operator: eq + value: oauth1@example.com🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/auth/oauth1/fixtures/collections/yml/OAuth1` RSA-SHA1 Body 200.yml around lines 53 - 61, This fixture's runtime assertions only check res.body.resource.name but should also assert res.body.resource.email for consistency with other OAuth1 200 fixtures; update the assertions array in the YAML (the block containing the expressions "res.body.resource.name" and "res.status") to add a new assertion whose expression is "res.body.resource.email", operator "eq", and value matching the expected test email used elsewhere in OAuth1 fixtures (use the same email literal used by HMAC-SHA1/RSA-SHA1 fixtures) so the test verifies both name and email.tests/auth/oauth1/fixtures/collections/yml/OAuth1 RSA-SHA1 200.yml (1)
9-17: MissingconsumerSecretfield for schema consistency.The
OAuth1 RSA-SHA1 Body 200.ymlfixture explicitly includesconsumerSecret: ''while this file omits it entirely. For consistency across fixtures (and to ensure the YAML schema is exercised uniformly), consider adding the field.💡 Suggested addition
auth: type: oauth1 consumerKey: consumer_key_1 + consumerSecret: '' accessToken: access_token_1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/auth/oauth1/fixtures/collections/yml/OAuth1` RSA-SHA1 200.yml around lines 9 - 17, Add the missing consumerSecret field to the auth block so the fixture matches the schema used by other OAuth1 RSA-SHA1 fixtures; update the auth map by adding consumerSecret: '' alongside consumerKey, accessToken, tokenSecret, signatureMethod, version, addParamsTo, and includeBodyHash to ensure consistent schema coverage.tests/auth/oauth1/fixtures/collections/yml/OAuth1 PLAINTEXT Query Params 200.yml (1)
20-28: Consider adding email assertion for consistency.Most other OAuth1 200 fixtures include assertions for both
resource.nameandresource.email. This fixture only checksresource.name. Adding the email assertion would maintain test consistency.💡 Suggested addition
runtime: assertions: - expression: res.status operator: eq value: '200' - expression: res.body.resource.name operator: eq value: oauth1-test-resource + - expression: res.body.resource.email + operator: eq + value: oauth1@example.com🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/auth/oauth1/fixtures/collections/yml/OAuth1` PLAINTEXT Query Params 200.yml around lines 20 - 28, The fixture is missing an assertion for resource.email; update the runtime.assertions array in the OAuth1 PLAINTEXT Query Params 200.yml fixture (alongside the existing assertions that check res.status and res.body.resource.name) by adding an assertion that checks expression "res.body.resource.email" with operator "eq" and the expected email value used across other OAuth1 200 fixtures (e.g., "oauth1-test@example.com"), placing it after the resource.name assertion for consistency.tests/auth/oauth1/fixtures/collections/yml/OAuth1 RSA-SHA512 200.yml (1)
20-48: PEM keys in test fixtures are properly scoped, but add secret-scan allowlist if CI scanning is implemented.The inline PEM keys across the OAuth1 test fixtures are correctly limited to test code only and are intentional for testing the
privateKey.type: textpath. No current secret-scan configuration exists in the repository, so there's no immediate risk. However, when secret scanning (e.g., gitleaks, checkov) is added to CI, create a narrowly scoped allowlist fortests/auth/oauth1/fixtures/collections/to prevent false positives on these test PEM blocks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/auth/oauth1/fixtures/collections/yml/OAuth1` RSA-SHA512 200.yml around lines 20 - 48, This fixture contains an inline PEM private key (the multi-line "-----BEGIN PRIVATE KEY-----" value in the OAuth1 RSA-SHA512 200.yml fixture) used only for tests; add a narrowly scoped secret-scan allowlist entry in your CI secret-scanning config (e.g., gitleaks/checkov rules) that whitelists this fixture directory or filename pattern and/or the specific YAML "value" field pattern to suppress false positives, ensuring the rule is limited to tests/auth/oauth1 fixtures and does not broadly exempt other files.packages/bruno-cli/src/runner/prepare-request.js (1)
152-170: Extract OAuth1 config mapping into one helper to avoid drift.Both branches map the same OAuth1 fields. A shared builder keeps this maintainable and prevents subtle divergence later.
♻️ Suggested refactor
+const buildOAuth1Config = (source, basePath) => { + return { + consumerKey: get(source, `${basePath}.consumerKey`), + consumerSecret: get(source, `${basePath}.consumerSecret`), + accessToken: get(source, `${basePath}.accessToken`), + tokenSecret: get(source, `${basePath}.tokenSecret`), + callbackUrl: get(source, `${basePath}.callbackUrl`), + verifier: get(source, `${basePath}.verifier`), + signatureMethod: get(source, `${basePath}.signatureMethod`), + privateKey: get(source, `${basePath}.privateKey`), + privateKeyType: get(source, `${basePath}.privateKeyType`), + timestamp: get(source, `${basePath}.timestamp`), + nonce: get(source, `${basePath}.nonce`), + version: get(source, `${basePath}.version`), + realm: get(source, `${basePath}.realm`), + addParamsTo: get(source, `${basePath}.addParamsTo`), + includeBodyHash: get(source, `${basePath}.includeBodyHash`) + }; +}; @@ - if (collectionAuth.mode === 'oauth1') { - axiosRequest.oauth1config = { - consumerKey: get(collectionAuth, 'oauth1.consumerKey'), - consumerSecret: get(collectionAuth, 'oauth1.consumerSecret'), - accessToken: get(collectionAuth, 'oauth1.accessToken'), - tokenSecret: get(collectionAuth, 'oauth1.tokenSecret'), - callbackUrl: get(collectionAuth, 'oauth1.callbackUrl'), - verifier: get(collectionAuth, 'oauth1.verifier'), - signatureMethod: get(collectionAuth, 'oauth1.signatureMethod'), - privateKey: get(collectionAuth, 'oauth1.privateKey'), - privateKeyType: get(collectionAuth, 'oauth1.privateKeyType'), - timestamp: get(collectionAuth, 'oauth1.timestamp'), - nonce: get(collectionAuth, 'oauth1.nonce'), - version: get(collectionAuth, 'oauth1.version'), - realm: get(collectionAuth, 'oauth1.realm'), - addParamsTo: get(collectionAuth, 'oauth1.addParamsTo'), - includeBodyHash: get(collectionAuth, 'oauth1.includeBodyHash') - }; - } + if (collectionAuth.mode === 'oauth1') { + axiosRequest.oauth1config = buildOAuth1Config(collectionAuth, 'oauth1'); + } @@ - if (request.auth.mode === 'oauth1') { - axiosRequest.oauth1config = { - consumerKey: get(request, 'auth.oauth1.consumerKey'), - consumerSecret: get(request, 'auth.oauth1.consumerSecret'), - accessToken: get(request, 'auth.oauth1.accessToken'), - tokenSecret: get(request, 'auth.oauth1.tokenSecret'), - callbackUrl: get(request, 'auth.oauth1.callbackUrl'), - verifier: get(request, 'auth.oauth1.verifier'), - signatureMethod: get(request, 'auth.oauth1.signatureMethod'), - privateKey: get(request, 'auth.oauth1.privateKey'), - privateKeyType: get(request, 'auth.oauth1.privateKeyType'), - timestamp: get(request, 'auth.oauth1.timestamp'), - nonce: get(request, 'auth.oauth1.nonce'), - version: get(request, 'auth.oauth1.version'), - realm: get(request, 'auth.oauth1.realm'), - addParamsTo: get(request, 'auth.oauth1.addParamsTo'), - includeBodyHash: get(request, 'auth.oauth1.includeBodyHash') - }; - } + if (request.auth.mode === 'oauth1') { + axiosRequest.oauth1config = buildOAuth1Config(request, 'auth.oauth1'); + }Also applies to: 218-236
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-cli/src/runner/prepare-request.js` around lines 152 - 170, The OAuth1 mapping is duplicated; extract a helper (e.g., buildOAuth1Config(collectionAuth)) that reads oauth1.* keys using get(...) and returns the config object, then replace both occurrences (the branch where collectionAuth.mode === 'oauth1' that assigns axiosRequest.oauth1config and the similar block around lines 218-236) with a call to that helper so both branches use the same builder and avoid drift.packages/bruno-tests/src/auth/oauth1/index.js (2)
244-250: RedundantalgoMapmapping.The map keys and values are identical. You can simplify by using
signatureMethoddirectly.♻️ Simplify signature verification
case 'RSA-SHA1': case 'RSA-SHA256': case 'RSA-SHA512': { - const algoMap = { 'RSA-SHA1': 'RSA-SHA1', 'RSA-SHA256': 'RSA-SHA256', 'RSA-SHA512': 'RSA-SHA512' }; - const verifier = crypto.createVerify(algoMap[signatureMethod]); + const verifier = crypto.createVerify(signatureMethod); verifier.update(baseString); return verifier.verify(TEST_RSA_PUBLIC_KEY, signature, 'base64'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-tests/src/auth/oauth1/index.js` around lines 244 - 250, The RSA branch creates an unnecessary algoMap with identical keys/values; simplify the code by calling crypto.createVerify(signatureMethod) directly, keep the existing verifier.update(baseString) and return verifier.verify(TEST_RSA_PUBLIC_KEY, signature, 'base64') so the logic around signatureMethod, baseString, TEST_RSA_PUBLIC_KEY, and verifier.verify remains unchanged but without the redundant mapping.
557-559: Prefer named exports or consistent module.exports pattern.The current pattern assigns to
module.exportsthen adds a property. Consider using a single object export for clarity.♻️ Consolidate exports
-module.exports = router; -module.exports.TEST_RSA_PRIVATE_KEY = TEST_RSA_PRIVATE_KEY; +module.exports = router; +module.exports.TEST_RSA_PRIVATE_KEY = TEST_RSA_PRIVATE_KEY;Alternatively, use a cleaner pattern:
router.TEST_RSA_PRIVATE_KEY = TEST_RSA_PRIVATE_KEY; module.exports = router;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-tests/src/auth/oauth1/index.js` around lines 557 - 559, The file currently assigns module.exports = router and then sets module.exports.TEST_RSA_PRIVATE_KEY separately; fix this by exporting consistently with a single object or attaching the key to the router before exporting. Update the code so either router.TEST_RSA_PRIVATE_KEY = TEST_RSA_PRIVATE_KEY and then module.exports = router, or replace both lines with module.exports = { router, TEST_RSA_PRIVATE_KEY } (choosing the pattern used elsewhere), ensuring references to router and TEST_RSA_PRIVATE_KEY remain valid.tests/auth/oauth1/oauth1-runner.spec.ts (1)
13-21: Potential race condition when writing PEM file at module load.If multiple test workers load this module simultaneously, concurrent
existsSync/writeFileSynccalls could race. Consider usingwriteFileSyncwith a flag like{ flag: 'wx' }to fail if file exists, or move setup to a fixture/hook.♻️ Safer file write
for (const subdir of ['bru', 'yml']) { const pemPath = path.join(fixtureBase, subdir, 'test-private-key.pem'); - if (!fs.existsSync(pemPath)) { - fs.writeFileSync(pemPath, TEST_RSA_PRIVATE_KEY); - } + try { + fs.writeFileSync(pemPath, TEST_RSA_PRIVATE_KEY, { flag: 'wx' }); + } catch (err) { + // File already exists, which is fine + if (err.code !== 'EEXIST') throw err; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/auth/oauth1/oauth1-runner.spec.ts` around lines 13 - 21, The module-level setup that writes TEST_RSA_PRIVATE_KEY to pemPath can race across parallel test workers; update the logic around TEST_RSA_PRIVATE_KEY/pemPath to perform an atomic create-or-ignore instead of existsSync then writeFileSync: attempt to write using writeFileSync with a failing-when-exists flag (e.g., { flag: 'wx' }) wrapped in a try/catch that ignores EEXIST, or move the write into a test fixture/hook that runs single-threaded before tests; ensure you reference and update the code touching TEST_RSA_PRIVATE_KEY and pemPath in oauth1-runner.spec.ts so concurrent loads don't clobber each other.packages/bruno-app/src/components/RequestPane/Auth/OAuth1/index.js (2)
103-105: Usepath.basenamefor cross-platform filename extraction.Manual splitting with
/and\\works but the codebase already imports apathutility. Usingpath.basenameis more robust and consistent with OS-agnostic guidelines.♻️ Suggested fix
- const fileName = isFileRef ? privateKeyValue.split('/').pop().split('\\').pop() : ''; + const fileName = isFileRef ? path.basename(privateKeyValue) : '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/RequestPane/Auth/OAuth1/index.js` around lines 103 - 105, Replace the manual split logic that derives fileName from privateKeyValue with a cross-platform basename call: when computing fileName (currently using privateKeyValue.split('/').pop().split('\\').pop()), import/use the existing path utility and call path.basename(privateKeyValue) when isFileRef is true (keep privateKeyValue and isFileRef checks as-is) so filename extraction is robust across OSes.
287-304: Potential double-toggle on checkbox interaction.The
<input>hasonChangeand the<label>hasonClickwithe.preventDefault()— but clicking the checkbox directly still triggersonChange. The label'sonClickalso fires when clicking the checkbox (due to label association), butpreventDefault()on the label doesn't prevent the checkbox's native change. This should work correctly, but the pattern is unusual.A cleaner approach: wrap both in a standard
<label>and rely solely ononChange.♻️ Simplified pattern
- <div className="flex items-center gap-2"> - <input - type="checkbox" - checked={oauth1.includeBodyHash || false} - onChange={(e) => handleChange('includeBodyHash', e.target.checked)} - /> - <label - className="block cursor-pointer" - onClick={(e) => { - e.preventDefault(); handleChange('includeBodyHash', !oauth1.includeBodyHash); - }} - > - Include Body Hash - </label> - </div> + <label className="flex items-center gap-2 cursor-pointer"> + <input + type="checkbox" + checked={oauth1.includeBodyHash || false} + onChange={(e) => handleChange('includeBodyHash', e.target.checked)} + /> + <span>Include Body Hash</span> + </label>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/RequestPane/Auth/OAuth1/index.js` around lines 287 - 304, The checkbox currently handles toggling in two places (input onChange and label onClick) which can double-trigger; simplify by using a single control: remove the label's onClick and e.preventDefault(), wrap the <input> and text inside one <label> (or associate via htmlFor/id) and rely only on the input's onChange to call handleChange('includeBodyHash', e.target.checked); reference oauth1.includeBodyHash and the handleChange call to locate where to change this.tests/auth/oauth1/oauth1.spec.ts (1)
127-134: Consider a more specific checkbox selector.
page.locator('input[type="checkbox"]')could match multiple checkboxes if more are added to the OAuth1 form. Consider scoping it to the "Include Body Hash" row.♻️ Suggested selector refinement
- const checkbox = page.locator('input[type="checkbox"]'); - const bodyHashLabel = page.locator('label').filter({ hasText: 'Include Body Hash' }); + const bodyHashRow = fieldRow(page, 'Include Body Hash'); + const checkbox = bodyHashRow.locator('input[type="checkbox"]');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/auth/oauth1/oauth1.spec.ts` around lines 127 - 134, The selector for the checkbox is too broad and may match multiple inputs; scope the checkbox lookup to the "Include Body Hash" row by finding the input relative to the bodyHashLabel instead of a global input[type="checkbox"]. Replace the standalone checkbox locator with something like using bodyHashLabel.locator('input[type="checkbox"]') or bodyHashLabel.locator('input') (or use a selector that targets the row/container for the "Include Body Hash" label) so the test uses the checkbox tied to the bodyHashLabel variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-app/src/components/RequestPane/WSRequestPane/WSAuth/index.js`:
- Around line 82-92: The oauth1 UI branch in the WSAuth component is never
reachable because supportedAuthModes (checked in the useEffect that resets mode)
does not include 'oauth1' and thus the effect immediately sets mode to 'none';
fix by either adding 'oauth1' to the supportedAuthModes array or changing the
useEffect logic in WSAuth (the effect that compares current mode against
supportedAuthModes and calls setMode('none')) to preserve 'oauth1' when the
component renders the oauth1 case (or only reset when mode is truly
unsupported), so the oauth1 case in the switch can actually persist and display.
In `@packages/bruno-app/src/utils/codegenerator/auth.js`:
- Around line 58-69: The current function returns an Authorization header string
containing literal placeholders ("<signature>", "<timestamp>", "<nonce>") which
produces invalid OAuth1 auth; update the logic in the auth generation path
(where oauth1Config is read and consumerKey, token, signatureMethod,
oauthVersion are defined) to either (A) compute a proper OAuth1 signature and
runtime oauth_timestamp and oauth_nonce for the cloned request and inject those
concrete values into the Authorization value, or (B) mark OAuth1 unsupported by
returning enabled: false (or an explicit "Unsupported: OAuth1 requires runtime
signing" value) instead of emitting a misleading template; modify the code that
builds the header object so Authorization never contains raw placeholder tokens.
In `@packages/bruno-cli/src/runner/interpolate-vars.js`:
- Around line 280-293: The oauth1config.privateKey field may be an object (RSA
config) whose nested string properties need interpolation; update the
interpolation step for request.oauth1config.privateKey to detect if it's an
object and recursively walk its properties, applying _interpolate to every
string leaf (preserving non-string values), otherwise fall back to
_interpolate(... ) || ''. Modify the block handling
request.oauth1config.privateKey to call a small helper that uses the existing
_interpolate function to transform nested string fields in-place (or return the
interpolated value) so nested {{...}} templates are resolved before signing.
In `@packages/bruno-electron/src/ipc/network/interpolate-vars.js`:
- Around line 364-378: The OAuth1 interpolation block misses interpolating
addParamsTo and includeBodyHash which can change signing behavior; inside the
request.oauth1config handling (where _interpolate is used for consumerKey,
consumerSecret, accessToken, tokenSecret, callbackUrl, verifier,
signatureMethod, privateKey, timestamp, nonce, version, realm) add lines to set
request.oauth1config.addParamsTo =
_interpolate(request.oauth1config.addParamsTo) ||
request.oauth1config.addParamsTo || '' and request.oauth1config.includeBodyHash
= _interpolate(request.oauth1config.includeBodyHash) ||
request.oauth1config.includeBodyHash || '' so both fields respect
variable-driven values and fall back to existing config.
In `@packages/bruno-lang/v2/tests/oauth1.spec.js`:
- Around line 742-775: The test "should survive round-trip with multiline PEM
private key" uses a PEM-like literal in the pem variable that trips scanners;
replace that hardcoded-looking key with a clearly fake marker or generate it at
runtime instead. Update the pem assignment (the pem constant) used in the json
object passed to jsonToBru/bruToJson so it contains a non-sensitive placeholder
(e.g., include "FAKE" or "TEST-KEY" inside the BEGIN/END block) or call a small
helper that constructs a dummy multiline PEM for the test; keep all assertions
(expect(parsed.auth.oauth1.privateKey) and privateKeyType) unchanged. Ensure the
new value still contains line breaks so the round-trip behavior for multiline
strings is exercised.
In `@packages/bruno-requests/src/auth/oauth1-request-authorization.ts`:
- Around line 45-54: The code currently forces oauth_version to '1.0' via a
truthy fallback (e.g. using version || '1.0'), which prevents preserving an
explicitly empty oauth_version and changes the signed parameter set; update the
logic that builds OAuth parameters (the places that reference oauth_version and
any helper like the OAuth1 parameter construction) to only default to '1.0' when
oauth_version is undefined or null (not when it's an empty string), and apply
the same change to the other occurrences that use the truthy fallback so empty
values are preserved in the signed parameters while undefined still falls back
to '1.0'.
- Around line 428-453: The 'body' branch currently overwrites non-form payloads
by initializing URLSearchParams(''), which can drop JSON/raw bodies or add
bodies to GET/HEAD; update the 'body' case in the switch (the code using
isFormUrlEncoded, request.data, URLSearchParams, ctKey and addParamsTo) to first
validate that the request is form-urlencoded and not a GET/HEAD: if
isFormUrlEncoded is false or request.method is GET/HEAD, throw or return an
error indicating OAuth body placement is invalid for non-form requests; only
when isFormUrlEncoded is true should you build URLSearchParams, set request.data
and (if needed) set Content-Type via ctKey or 'Content-Type'.
In `@tests/auth/oauth1/fixtures/collections/yml/OAuth1` HMAC-SHA1 Body JSON
200.yml:
- Around line 17-21: The fixture currently uses addParamsTo: body together with
body.type: json (and includeBodyHash: false), which can cause OAuth params to be
ignored; update the fixture so that either set addParamsTo to header to sign
JSON payloads via the Authorization header, or change body.type from json to
form (form-encoded) when you intend to exercise body-parameter signing; adjust
the addParamsTo and/or body.type entries accordingly to ensure consistent
body-auth behavior.
In `@tests/auth/oauth1/fixtures/collections/yml/OAuth1` RSA-SHA1 Query Params
200.yml:
- Around line 18-48: The YAML fixture contains an embedded RSA private key under
the privateKey field which triggers secret scanners; remove the inline PEM and
either (a) replace it with a reference to a test-only key loader (e.g.,
loadPrivateKeyFromEnv or generateTestKeyAtRuntime) so the key is created/loaded
outside source control, or (b) if you must include it in the PR add the
repository secret-scanner allowlist entry for this fixture in the same PR;
update any tests that consume privateKey to read from the new loader/generator
or env variable instead of the inline PEM.
---
Nitpick comments:
In `@packages/bruno-app/src/components/RequestPane/Auth/OAuth1/index.js`:
- Around line 103-105: Replace the manual split logic that derives fileName from
privateKeyValue with a cross-platform basename call: when computing fileName
(currently using privateKeyValue.split('/').pop().split('\\').pop()), import/use
the existing path utility and call path.basename(privateKeyValue) when isFileRef
is true (keep privateKeyValue and isFileRef checks as-is) so filename extraction
is robust across OSes.
- Around line 287-304: The checkbox currently handles toggling in two places
(input onChange and label onClick) which can double-trigger; simplify by using a
single control: remove the label's onClick and e.preventDefault(), wrap the
<input> and text inside one <label> (or associate via htmlFor/id) and rely only
on the input's onChange to call handleChange('includeBodyHash',
e.target.checked); reference oauth1.includeBodyHash and the handleChange call to
locate where to change this.
In `@packages/bruno-cli/src/runner/prepare-request.js`:
- Around line 152-170: The OAuth1 mapping is duplicated; extract a helper (e.g.,
buildOAuth1Config(collectionAuth)) that reads oauth1.* keys using get(...) and
returns the config object, then replace both occurrences (the branch where
collectionAuth.mode === 'oauth1' that assigns axiosRequest.oauth1config and the
similar block around lines 218-236) with a call to that helper so both branches
use the same builder and avoid drift.
In `@packages/bruno-tests/src/auth/oauth1/index.js`:
- Around line 244-250: The RSA branch creates an unnecessary algoMap with
identical keys/values; simplify the code by calling
crypto.createVerify(signatureMethod) directly, keep the existing
verifier.update(baseString) and return verifier.verify(TEST_RSA_PUBLIC_KEY,
signature, 'base64') so the logic around signatureMethod, baseString,
TEST_RSA_PUBLIC_KEY, and verifier.verify remains unchanged but without the
redundant mapping.
- Around line 557-559: The file currently assigns module.exports = router and
then sets module.exports.TEST_RSA_PRIVATE_KEY separately; fix this by exporting
consistently with a single object or attaching the key to the router before
exporting. Update the code so either router.TEST_RSA_PRIVATE_KEY =
TEST_RSA_PRIVATE_KEY and then module.exports = router, or replace both lines
with module.exports = { router, TEST_RSA_PRIVATE_KEY } (choosing the pattern
used elsewhere), ensuring references to router and TEST_RSA_PRIVATE_KEY remain
valid.
In `@tests/auth/oauth1/fixtures/collections/bru/OAuth1` PLAINTEXT Query Params
200.bru:
- Around line 29-32: Add an email assertion to the existing response assertions:
inside the assert block that currently checks res.status and
res.body.resource.name (the assertions referencing res.status and
res.body.resource.name: eq oauth1-test-resource), add a line asserting
res.body.resource.email: eq oauth1@example.com so this PLAINTEXT fixture matches
the parity of the HMAC-SHA1 200.bru fixture.
In `@tests/auth/oauth1/fixtures/collections/bru/OAuth1` RSA-SHA1 Body 200.bru:
- Around line 20-49: The RSA PEM private_key block in the OAuth1 RSA-SHA1
fixture is duplicated; extract the PEM into a single shared fixture (e.g., a
shared "rsa_private_key" fixture) and replace the inline private_key value in
OAuth1 RSA-SHA1 Body 200.bru with a reference to that shared fixture; update any
other OAuth1 RSA fixtures to reference the same shared symbol so all tests
consume the centralized key and eliminate copy/paste drift.
In `@tests/auth/oauth1/fixtures/collections/yml/OAuth1` HMAC-SHA1 Body JSON
200.yml:
- Around line 11-14: Replace the realistic-looking credential values in the
fixture by using clearly fake placeholder literals for the keys consumerKey,
consumerSecret, accessToken, and tokenSecret (for example values like
"fake_consumer_key", "fake_consumer_secret", "fake_access_token",
"fake_token_secret") so the test remains meaningful but won't trigger secret
scanners; update the corresponding entries in the OAuth1 HMAC-SHA1 Body JSON
fixture to use those fake placeholders.
In `@tests/auth/oauth1/fixtures/collections/yml/OAuth1` HMAC-SHA256 Body 200.yml:
- Around line 22-30: The 200 fixture is missing the email assertion for
consistency with other OAuth1 200 fixtures; add an assertion entry under
runtime.assertions that checks res.body.resource.email equals oauth1@example.com
by inserting an assertion with expression: res.body.resource.email, operator:
eq, and value: oauth1@example.com (matching the existing assertion style used
for res.body.resource.name).
In `@tests/auth/oauth1/fixtures/collections/yml/OAuth1` PLAINTEXT Query Params
200.yml:
- Around line 20-28: The fixture is missing an assertion for resource.email;
update the runtime.assertions array in the OAuth1 PLAINTEXT Query Params 200.yml
fixture (alongside the existing assertions that check res.status and
res.body.resource.name) by adding an assertion that checks expression
"res.body.resource.email" with operator "eq" and the expected email value used
across other OAuth1 200 fixtures (e.g., "oauth1-test@example.com"), placing it
after the resource.name assertion for consistency.
In `@tests/auth/oauth1/fixtures/collections/yml/OAuth1` RSA-SHA1 200.yml:
- Around line 9-17: Add the missing consumerSecret field to the auth block so
the fixture matches the schema used by other OAuth1 RSA-SHA1 fixtures; update
the auth map by adding consumerSecret: '' alongside consumerKey, accessToken,
tokenSecret, signatureMethod, version, addParamsTo, and includeBodyHash to
ensure consistent schema coverage.
In `@tests/auth/oauth1/fixtures/collections/yml/OAuth1` RSA-SHA1 Body 200.yml:
- Around line 53-61: This fixture's runtime assertions only check
res.body.resource.name but should also assert res.body.resource.email for
consistency with other OAuth1 200 fixtures; update the assertions array in the
YAML (the block containing the expressions "res.body.resource.name" and
"res.status") to add a new assertion whose expression is
"res.body.resource.email", operator "eq", and value matching the expected test
email used elsewhere in OAuth1 fixtures (use the same email literal used by
HMAC-SHA1/RSA-SHA1 fixtures) so the test verifies both name and email.
In `@tests/auth/oauth1/fixtures/collections/yml/OAuth1` RSA-SHA512 200.yml:
- Around line 20-48: This fixture contains an inline PEM private key (the
multi-line "-----BEGIN PRIVATE KEY-----" value in the OAuth1 RSA-SHA512 200.yml
fixture) used only for tests; add a narrowly scoped secret-scan allowlist entry
in your CI secret-scanning config (e.g., gitleaks/checkov rules) that whitelists
this fixture directory or filename pattern and/or the specific YAML "value"
field pattern to suppress false positives, ensuring the rule is limited to
tests/auth/oauth1 fixtures and does not broadly exempt other files.
In `@tests/auth/oauth1/oauth1-runner.spec.ts`:
- Around line 13-21: The module-level setup that writes TEST_RSA_PRIVATE_KEY to
pemPath can race across parallel test workers; update the logic around
TEST_RSA_PRIVATE_KEY/pemPath to perform an atomic create-or-ignore instead of
existsSync then writeFileSync: attempt to write using writeFileSync with a
failing-when-exists flag (e.g., { flag: 'wx' }) wrapped in a try/catch that
ignores EEXIST, or move the write into a test fixture/hook that runs
single-threaded before tests; ensure you reference and update the code touching
TEST_RSA_PRIVATE_KEY and pemPath in oauth1-runner.spec.ts so concurrent loads
don't clobber each other.
In `@tests/auth/oauth1/oauth1.spec.ts`:
- Around line 127-134: The selector for the checkbox is too broad and may match
multiple inputs; scope the checkbox lookup to the "Include Body Hash" row by
finding the input relative to the bodyHashLabel instead of a global
input[type="checkbox"]. Replace the standalone checkbox locator with something
like using bodyHashLabel.locator('input[type="checkbox"]') or
bodyHashLabel.locator('input') (or use a selector that targets the row/container
for the "Include Body Hash" label) so the test uses the checkbox tied to the
bodyHashLabel variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7f137db4-7acc-478d-8726-4950f375cce5
📒 Files selected for processing (93)
package.jsonpackages/bruno-app/src/components/CollectionSettings/Auth/AuthMode/index.jspackages/bruno-app/src/components/CollectionSettings/Auth/Oauth1/index.jspackages/bruno-app/src/components/CollectionSettings/Auth/index.jspackages/bruno-app/src/components/FolderSettings/Auth/index.jspackages/bruno-app/src/components/FolderSettings/AuthMode/index.jspackages/bruno-app/src/components/RequestPane/Auth/AuthMode/index.jspackages/bruno-app/src/components/RequestPane/Auth/OAuth1/StyledWrapper.jspackages/bruno-app/src/components/RequestPane/Auth/OAuth1/index.jspackages/bruno-app/src/components/RequestPane/Auth/index.jspackages/bruno-app/src/components/RequestPane/WSRequestPane/WSAuth/index.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/index.jspackages/bruno-app/src/utils/codegenerator/auth.jspackages/bruno-app/src/utils/collections/index.jspackages/bruno-cli/src/runner/interpolate-vars.jspackages/bruno-cli/src/runner/prepare-request.jspackages/bruno-cli/src/runner/run-single-request.jspackages/bruno-converters/src/opencollection/common/auth.tspackages/bruno-converters/src/opencollection/types.tspackages/bruno-converters/src/postman/postman-to-bruno.jspackages/bruno-electron/src/ipc/network/index.jspackages/bruno-electron/src/ipc/network/interpolate-vars.jspackages/bruno-electron/src/ipc/network/prepare-grpc-request.jspackages/bruno-electron/src/ipc/network/prepare-request.jspackages/bruno-filestore/src/formats/yml/common/auth.tspackages/bruno-js/src/bruno-request.jspackages/bruno-lang/v2/src/bruToJson.jspackages/bruno-lang/v2/src/collectionBruToJson.jspackages/bruno-lang/v2/src/jsonToBru.jspackages/bruno-lang/v2/src/jsonToCollectionBru.jspackages/bruno-lang/v2/tests/oauth1.spec.jspackages/bruno-requests/package.jsonpackages/bruno-requests/src/auth/index.tspackages/bruno-requests/src/auth/oauth1-request-authorization.spec.tspackages/bruno-requests/src/auth/oauth1-request-authorization.tspackages/bruno-requests/src/index.tspackages/bruno-schema-types/src/common/auth.tspackages/bruno-schema/src/collections/index.jspackages/bruno-tests/src/auth/index.jspackages/bruno-tests/src/auth/oauth1/index.jsplaywright.config.tstests/auth/oauth1/fixtures/collections/bru/OAuth1 HMAC-SHA1 200.brutests/auth/oauth1/fixtures/collections/bru/OAuth1 HMAC-SHA1 401.brutests/auth/oauth1/fixtures/collections/bru/OAuth1 HMAC-SHA1 Body 200.brutests/auth/oauth1/fixtures/collections/bru/OAuth1 HMAC-SHA1 Body JSON 200.brutests/auth/oauth1/fixtures/collections/bru/OAuth1 HMAC-SHA1 POST 200.brutests/auth/oauth1/fixtures/collections/bru/OAuth1 HMAC-SHA1 Query Params 200.brutests/auth/oauth1/fixtures/collections/bru/OAuth1 HMAC-SHA256 200.brutests/auth/oauth1/fixtures/collections/bru/OAuth1 HMAC-SHA256 401.brutests/auth/oauth1/fixtures/collections/bru/OAuth1 HMAC-SHA256 Body 200.brutests/auth/oauth1/fixtures/collections/bru/OAuth1 HMAC-SHA512 200.brutests/auth/oauth1/fixtures/collections/bru/OAuth1 HMAC-SHA512 401.brutests/auth/oauth1/fixtures/collections/bru/OAuth1 PLAINTEXT 200.brutests/auth/oauth1/fixtures/collections/bru/OAuth1 PLAINTEXT 401.brutests/auth/oauth1/fixtures/collections/bru/OAuth1 PLAINTEXT Body 200.brutests/auth/oauth1/fixtures/collections/bru/OAuth1 PLAINTEXT Query Params 200.brutests/auth/oauth1/fixtures/collections/bru/OAuth1 RSA-SHA1 200.brutests/auth/oauth1/fixtures/collections/bru/OAuth1 RSA-SHA1 Body 200.brutests/auth/oauth1/fixtures/collections/bru/OAuth1 RSA-SHA1 File Key 200.brutests/auth/oauth1/fixtures/collections/bru/OAuth1 RSA-SHA1 Query Params 200.brutests/auth/oauth1/fixtures/collections/bru/OAuth1 RSA-SHA1 Variable Key 200.brutests/auth/oauth1/fixtures/collections/bru/OAuth1 RSA-SHA256 200.brutests/auth/oauth1/fixtures/collections/bru/OAuth1 RSA-SHA512 200.brutests/auth/oauth1/fixtures/collections/bru/bruno.jsontests/auth/oauth1/fixtures/collections/bru/environments/Local.brutests/auth/oauth1/fixtures/collections/yml/OAuth1 HMAC-SHA1 200.ymltests/auth/oauth1/fixtures/collections/yml/OAuth1 HMAC-SHA1 401.ymltests/auth/oauth1/fixtures/collections/yml/OAuth1 HMAC-SHA1 Body 200.ymltests/auth/oauth1/fixtures/collections/yml/OAuth1 HMAC-SHA1 Body JSON 200.ymltests/auth/oauth1/fixtures/collections/yml/OAuth1 HMAC-SHA1 POST 200.ymltests/auth/oauth1/fixtures/collections/yml/OAuth1 HMAC-SHA1 Query Params 200.ymltests/auth/oauth1/fixtures/collections/yml/OAuth1 HMAC-SHA256 200.ymltests/auth/oauth1/fixtures/collections/yml/OAuth1 HMAC-SHA256 401.ymltests/auth/oauth1/fixtures/collections/yml/OAuth1 HMAC-SHA256 Body 200.ymltests/auth/oauth1/fixtures/collections/yml/OAuth1 HMAC-SHA512 200.ymltests/auth/oauth1/fixtures/collections/yml/OAuth1 HMAC-SHA512 401.ymltests/auth/oauth1/fixtures/collections/yml/OAuth1 PLAINTEXT 200.ymltests/auth/oauth1/fixtures/collections/yml/OAuth1 PLAINTEXT 401.ymltests/auth/oauth1/fixtures/collections/yml/OAuth1 PLAINTEXT Body 200.ymltests/auth/oauth1/fixtures/collections/yml/OAuth1 PLAINTEXT Query Params 200.ymltests/auth/oauth1/fixtures/collections/yml/OAuth1 RSA-SHA1 200.ymltests/auth/oauth1/fixtures/collections/yml/OAuth1 RSA-SHA1 Body 200.ymltests/auth/oauth1/fixtures/collections/yml/OAuth1 RSA-SHA1 File Key 200.ymltests/auth/oauth1/fixtures/collections/yml/OAuth1 RSA-SHA1 Query Params 200.ymltests/auth/oauth1/fixtures/collections/yml/OAuth1 RSA-SHA1 Variable Key 200.ymltests/auth/oauth1/fixtures/collections/yml/OAuth1 RSA-SHA256 200.ymltests/auth/oauth1/fixtures/collections/yml/OAuth1 RSA-SHA512 200.ymltests/auth/oauth1/fixtures/collections/yml/environments/Local.ymltests/auth/oauth1/fixtures/collections/yml/opencollection.ymltests/auth/oauth1/init-user-data/preferences.jsontests/auth/oauth1/oauth1-runner.spec.tstests/auth/oauth1/oauth1.spec.tstests/utils/page/actions.ts
packages/bruno-app/src/components/RequestPane/WSRequestPane/WSAuth/index.js
Outdated
Show resolved
Hide resolved
Comment on lines
+428
to
+453
| switch (addParamsTo || 'header') { | ||
| case 'header': | ||
| request.headers['Authorization'] = authorizer.toHeader(oauthData).Authorization; | ||
| break; | ||
| case 'queryparams': { | ||
| const url = new URL(request.url); | ||
| Object.entries(oauthData).forEach(([key, value]) => { | ||
| if (value) url.searchParams.set(key, value); | ||
| }); | ||
| request.url = url.toString(); | ||
| break; | ||
| } | ||
| case 'body': { | ||
| const params = new URLSearchParams(isFormUrlEncoded ? request.data : ''); | ||
| Object.entries(oauthData).forEach(([key, value]) => { | ||
| if (value !== undefined) params.set(key, value); | ||
| }); | ||
| request.data = params.toString(); | ||
|
|
||
| if (!isFormUrlEncoded) { | ||
| if (ctKey) { | ||
| request.headers[ctKey] = 'application/x-www-form-urlencoded'; | ||
| } else { | ||
| request.headers['Content-Type'] = 'application/x-www-form-urlencoded'; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Reject body placement for non-form requests instead of rewriting the payload.
Line 441 initializes URLSearchParams from '' whenever the request is not form-urlencoded, so JSON/raw payloads are discarded and even GET/HEAD requests can gain a synthetic form body. This should fail fast instead of silently mutating the request into something else.
🛠 Suggested fix
case 'body': {
- const params = new URLSearchParams(isFormUrlEncoded ? request.data : '');
+ if (!hasBody || !isFormUrlEncoded) {
+ throw new Error('OAuth1 body placement requires a non-GET/HEAD application/x-www-form-urlencoded request');
+ }
+
+ const params = new URLSearchParams(typeof request.data === 'string' ? request.data : '');
Object.entries(oauthData).forEach(([key, value]) => {
if (value !== undefined) params.set(key, value);
});
request.data = params.toString();
-
- if (!isFormUrlEncoded) {
- if (ctKey) {
- request.headers[ctKey] = 'application/x-www-form-urlencoded';
- } else {
- request.headers['Content-Type'] = 'application/x-www-form-urlencoded';
- }
- }
break;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-requests/src/auth/oauth1-request-authorization.ts` around
lines 428 - 453, The 'body' branch currently overwrites non-form payloads by
initializing URLSearchParams(''), which can drop JSON/raw bodies or add bodies
to GET/HEAD; update the 'body' case in the switch (the code using
isFormUrlEncoded, request.data, URLSearchParams, ctKey and addParamsTo) to first
validate that the request is form-urlencoded and not a GET/HEAD: if
isFormUrlEncoded is false or request.method is GET/HEAD, throw or return an
error indicating OAuth body placement is invalid for non-form requests; only
when isFormUrlEncoded is true should you build URLSearchParams, set request.data
and (if needed) set Content-Type via ctKey or 'Content-Type'.
Comment on lines
+17
to
+21
| addParamsTo: body | ||
| includeBodyHash: false | ||
| body: | ||
| type: json | ||
| json: '{"test":"data"}' |
Contributor
There was a problem hiding this comment.
addParamsTo: body with type: json is a risky auth-placement combination.
This can lead to OAuth params being ignored or injected in a way that doesn’t reliably validate body-auth behavior. Prefer signing via header for JSON payload fixtures, or switch body type to form-encoded for body-param tests.
Suggested fixture adjustment (JSON body case)
- addParamsTo: body
+ addParamsTo: header
includeBodyHash: false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/auth/oauth1/fixtures/collections/yml/OAuth1` HMAC-SHA1 Body JSON
200.yml around lines 17 - 21, The fixture currently uses addParamsTo: body
together with body.type: json (and includeBodyHash: false), which can cause
OAuth params to be ignored; update the fixture so that either set addParamsTo to
header to sign JSON payloads via the Authorization header, or change body.type
from json to form (form-encoded) when you intend to exercise body-parameter
signing; adjust the addParamsTo and/or body.type entries accordingly to ensure
consistent body-auth behavior.
tests/auth/oauth1/fixtures/collections/yml/OAuth1 RSA-SHA1 Query Params 200.yml
Show resolved
Hide resolved
…ests Avoid tripping secret scanners by using obviously fake BEGIN/END markers and non-sensitive base64 content in serialization and round-trip tests.
OAuth1 requires runtime-computed nonce, timestamp, and signature that cannot be pre-computed for a static code snippet. Return an empty array instead of emitting an Authorization header with literal <signature>, <timestamp>, <nonce> placeholders.
The oauth1 switch branch was dead code since it was not in supportedAuthModes and the useEffect would reset it to 'none' before it could render.
sanish-bruno
suggested changes
Mar 17, 2026
| bearer: null, | ||
| digest: null, | ||
| ntlm: null, | ||
| oauth2: null, |
Collaborator
There was a problem hiding this comment.
@lohit-bruno don't we have to set oauth1 to null here
| bearer: null, | ||
| digest: null, | ||
| ntlm: null, | ||
| oauth2: null, |
Collaborator
There was a problem hiding this comment.
@lohit-bruno don't er have to default oauth1 to null, like we are doing for oauth2?
| }); | ||
|
|
||
| test('Verify Add Params To placement via timeline', async ({ pageWithUserData: page }) => { | ||
| test.setTimeout(3 * 60 * 1000); |
Collaborator
There was a problem hiding this comment.
@lohit-bruno we can case the value to a constant, and reuse it everywhere
| }; | ||
|
|
||
| const selectRequestPaneTab = async (page: Page, tabName: string) => { | ||
| await selectPaneTab(page, '.request-pane > .px-4', tabName); |
Collaborator
There was a problem hiding this comment.
@lohit-bruno is it possible to add a better selector for request and response panes? may be we can use testIds
| const result = collectionBruToJson(input); | ||
|
|
||
| expect(result.auth.oauth1.privateKeyType).toBe('text'); | ||
| expect(result.auth.oauth1.privateKey).toContain('-----BEGIN FAKE RSA TEST KEY-----'); |
Collaborator
There was a problem hiding this comment.
@lohit-bruno should we also assert with the entire key itself? just to make sure than the initial spaces are not included in the final key.
Collaborator
sanish-bruno
left a comment
sanish-bruno
left a comment
There was a problem hiding this comment.
PR Review
Found 5 issue(s): 0 major, 2 minor, 1 suggestion, 2 nits.
Overall this is a very well-structured PR. The OAuth 1.0 implementation follows RFC 5849 carefully, has comprehensive unit and E2E tests covering all signature methods and parameter placements, and follows the existing patterns in the codebase for adding new auth types. The Bru format serialization/deserialization and round-trip tests are thorough.
Summary
- Test fixture
OAuth1 RSA-SHA1 200.brucontains what appears to be accidental debug data invars:pre-request - Debug
console.logleft inOAuth1 HMAC-SHA1 Body 200.brufixture - Module-level
privateKeyCachehas no eviction strategy - Silent input rejection in
handlePrivateKeyChangefor@file(values lacks user feedback - Good incidental fix of missing
breakin NTLM case in prepare-request.js
| include_body_hash: false | ||
| } | ||
|
|
||
| vars:pre-request { |
Collaborator
There was a problem hiding this comment.
Nit: This vars:pre-request block has an empty variable name and a ~3KB repeating string of token_secret_1. This looks like accidentally pasted debug data. The variable is never used (no name) and adds unnecessary noise to the fixture.
Suggested fix: Remove the entire vars:pre-request block.
|
|
||
| script:post-response { | ||
| console.log(req.getBody()); | ||
| } |
Collaborator
There was a problem hiding this comment.
Nit: This console.log(req.getBody()) looks like leftover debug output.
Suggested fix: Remove the script:post-response block, or replace with a meaningful assertion.
| import crypto from 'node:crypto'; | ||
| import fs from 'node:fs'; | ||
| import nodePath from 'node:path'; | ||
|
|
Collaborator
There was a problem hiding this comment.
Suggestion: privateKeyCache is a module-level Map that never evicts entries. In a long-running Electron process, if users switch between collections with different private key files, stale entries accumulate indefinitely. The mtime-based invalidation handles content changes correctly, but entries for deleted or no-longer-used files are never removed.
Consider adding a simple max-size cap (e.g., evict oldest entry when the map exceeds 50 entries), or clearing the cache when a collection is closed.
| }; | ||
|
|
||
| const handlePrivateKeyChange = (val) => { | ||
| if (val && /^@file\(/.test(val.trim())) return; |
Collaborator
There was a problem hiding this comment.
Bug (Minor): This silently swallows any value starting with @file(, giving the user no feedback that their input was rejected. This is necessary to prevent @file() markers from being entered via the text editor (since they're a serialization-level construct), but the silent return is confusing UX.
Suggested fix: Consider showing a brief toast or visual indicator explaining that file references should use the "Upload File" button instead.
| password: get(request, 'auth.ntlm.password'), | ||
| domain: get(request, 'auth.ntlm.domain') | ||
| }; | ||
| break; |
Collaborator
There was a problem hiding this comment.
Bug (Minor): Good catch — this break was missing from the pre-existing ntlm case, causing it to fall through to the next case. Worth calling out in the PR description as an incidental fix, since it affects existing NTLM auth behavior (the fall-through would have caused the oauth1 case to also execute after ntlm).
sanish-bruno
suggested changes
Mar 17, 2026
| password: authValues.password || '' | ||
| }; | ||
| break; | ||
| case AUTH_TYPES.OAUTH1: |
Collaborator
There was a problem hiding this comment.
@lohit-bruno can we have few test cases that verify the oauth1 import from postman is working as expected
sreelakshmi-bruno
requested changes
Mar 17, 2026
Collaborator
There was a problem hiding this comment.
When using OAuth 1.0 authentication with addParamsTo: body and a GET request with a form-urlencoded body, the body parameters are not included in the signature calculation, but they ARE sent in the request body. This causes signature verification to fail on the server.
Steps to Reproduce
-
Create a request with:
- Method:
GET - Body: Form URL Encoded with
foo=bar - Auth: OAuth 1.0
- Add Params To:
Body
- Method:
-
Send the request to an OAuth 1.0 protected endpoint
-
Observe 401 Unauthorized response
Expected Behavior
Body parameters (foo=bar) should be included in the OAuth signature base string when addParamsTo: body is selected, regardless of HTTP method. Since Bruno sends the body even for GET requests, the signature should include those parameters.
sreelakshmi-bruno
requested changes
Mar 17, 2026
Collaborator
sreelakshmi-bruno
left a comment
sreelakshmi-bruno
left a comment
There was a problem hiding this comment.
Can you also add this in Bruno to Postman conversion?
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
3 participants
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.
jira
opencollection pr: opencollection-dev/opencollection#25
Description
Add OAuth 1.0 (RFC 5849) authentication with support for HMAC-SHA1/256/512, RSA-SHA1/256/512, and PLAINTEXT signature methods.
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
New Features
Tests
Chores