feat(oauth2): full set of token-endpoint client authentication methods (RFC 7591 / 7523 / 8705)#8051
Conversation
Replaces the binary "Add credentials to: Request Body | Basic Auth header" dropdown with the five RFC 7591 §2 / OIDC Core 1.0 §9 token endpoint auth methods — client_secret_basic, client_secret_post, client_secret_jwt, private_key_jwt, none — including JWT bearer client assertions (RFC 7521 / 7523) signed with the client secret (HMAC) or a private key (PEM or JWK / JWK Set, with optional file-path source). Legacy credentials_placement is read for backward compatibility and migrated on next save.
…ethod in yml collections UI: render the Client Secret field inside ClientAuthMethod so it only shows for methods that actually consume it (client_secret_basic / _post / _jwt) instead of as a top-level config field. YML format: bridge Bruno's tokenEndpointAuthMethod with OpenCollection's credentials.placement so yml-format collections persist the chosen method through a save/reload round-trip.
Replace the Key Source dropdown with a file-picker workflow modelled on the existing OAuth1 private-key UI: a textarea by default plus a "Select file…" button that opens the OS file dialog and atomically switches the field to file mode. Selected paths are stored relative to the collection directory and rendered as filename + change/clear icons, so the value always represents either valid PEM/JWK text or a real filesystem path.
The picker previously rewrote selected paths relative to the collection directory, but the helper had no collection root to resolve against at verify time, so loading the key threw ENOENT. Storing the absolute path matches user expectation and keeps the helper free of resolution logic.
…C 8705) mTLS authentication runs at the TLS layer via Bruno's per-domain client certificate config; the helper only adds the required client_id to the token request body. When either mTLS method is selected, the auth panel surfaces a warning if no certificate is configured for the token endpoint hostname — the host-pattern match mirrors cert-utils.js so the warning agrees with what the request runtime will actually see.
jattsson
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces legacy OAuth2 credentialsPlacement with tokenEndpointAuthMethod; adds applyTokenEndpointAuth (multiple methods including JWT/private-key/mTLS/none), readKeyFile cache, ClientAuthMethod UI, wiring into token requests, serializers/converters/schema updates, and tests/fixtures. ChangesOAuth2 Token Endpoint Client Authentication
Sequence Diagram sequenceDiagram
participant User as OAuth2 UI
participant Pane as Grant Pane
participant ClientAuth as ClientAuthMethod
participant Redux as Redux Store
participant Helper as oauth2-helper
participant TokenAuth as applyTokenEndpointAuth
participant Server as Token Endpoint
User->>Pane: open auth settings
Pane->>ClientAuth: render method & fields
User->>ClientAuth: set tokenEndpointAuthMethod + JWT/key fields
ClientAuth->>Redux: patchOAuth(...)
Pane->>Helper: request token (oauth2Config)
Helper->>TokenAuth: applyTokenEndpointAuth(oauth2Config)
TokenAuth->>TokenAuth: resolve method, load key, build/sign assertion
TokenAuth-->>Helper: { headers, bodyParams }
Helper->>Server: POST token request (merged)
Server-->>Helper: access_token response
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
packages/bruno-app/src/components/RequestPane/Auth/OAuth2/ClientAuthMethod/index.js (1)
97-305: ⚡ Quick winAdd stable
data-testids to the new auth controls.This is a brand-new Playwright surface, but the method selector, signing-alg selector, JWT inputs, and private-key browse/clear actions do not expose stable test hooks yet.
As per coding guidelines "Add
data-testidto testable elements for Playwright in React components".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-app/src/components/RequestPane/Auth/OAuth2/ClientAuthMethod/index.js` around lines 97 - 305, Add stable data-testid attributes to the new OAuth2 controls so Playwright can target them: add a data-testid to the MenuDropdown toggle for the client auth method (the div inside MenuDropdown in the tokenEndpointAuthMethod block / COMPONENT: MenuDropdown, key "input-token-endpoint-auth-method"), and likewise add a data-testid to the MenuDropdown toggle for signing algorithm (MenuDropdown in the tokenEndpointAuthSigningAlg block / key "input-signing-alg"); add data-testid props on the SingleLineEditor instances for JWT inputs (oAuth.keyId, oAuth.audience, oAuth.assertionLifetime — COMPONENT: SingleLineEditor in blocks with keys "input-key-id", "input-audience", "input-assertion-lifetime"); and add data-testid attributes to the private-key file controls/buttons (the browse and clear buttons that call handleBrowsePrivateKey and handleClearPrivateKey, and the filename span shown when isFileBacked). Ensure testids are stable, follow naming like data-testid="oauth-client-auth-method", "oauth-signing-alg", "oauth-key-id", "oauth-audience", "oauth-assertion-lifetime", "oauth-privatekey-browse", "oauth-privatekey-clear", and "oauth-privatekey-filename".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/bruno-app/src/components/RequestPane/Auth/OAuth2/ClientAuthMethod/index.js`:
- Around line 103-107: When the auth method or key-format dropdown changes,
reset dependent JWT/secret fields instead of only updating the enum; update the
handler used by METHOD_OPTIONS onClick (the code path that calls
handleChange('tokenEndpointAuthMethod', ...)) and the key-format change handler
(the dropdown at the other location referenced) to additionally clear/reset
oAuth fields that only apply to JWT or a specific key format — specifically
clear privateKey, clientSecret, tokenEndpointAuthSigningAlg, clientAssertion (or
any JWT-only fields), and any key-format-specific containers (e.g., jwk vs
pem/privateKeyFormat) so switching away from private_key_jwt/client_secret_jwt
or flipping PEM↔JWK cannot leave stale secrets serialized.
In `@packages/bruno-app/src/utils/collections/index.js`:
- Around line 421-431: The export is dropping the OAuth2 field additionalClaims;
in each place where the JWT client-auth fields are being serialized (the object
that currently includes tokenEndpointAuthMethod, tokenEndpointAuthSigningAlg,
privateKey, privateKeyType, privateKeyFormat, keyId, audience,
assertionLifetime), add serialization of additionalClaims (e.g.,
additionalClaims: get(si.request, 'auth.oauth2.additionalClaims', {} or '') as
appropriate) so the field is preserved on export; update all three branches that
mirror this block (the same pattern around
tokenEndpointAuthMethod/tokenEndpointAuthSigningAlg etc.) to include
additionalClaims.
In `@packages/bruno-electron/src/utils/oauth2.js`:
- Around line 270-272: The merged client auth headers/body (result of
applyTokenEndpointAuth) are being copied into axiosRequestConfig.headers and
data and later stored into requestDetails by getCredentialsFromTokenUrl, so
scrub sensitive fields before any persistence or debug tracing: after applying
clientAuth, remove or redact Authorization (or authorization) header and any
body params named client_secret and client_assertion from
axiosRequestConfig.headers and data (and ensure the same scrub happens in other
identical spots where Object.assign is used to merge
clientAuth.headers/bodyParams), then pass the redacted objects to
getCredentialsFromTokenUrl or to any logging/storage paths so raw credentials
are never persisted.
- Around line 270-272: The call to applyTokenEndpointAuth can throw before the
network try/catch, causing unstructured rejections and skipping the
token-refresh cleanup; wrap applyTokenEndpointAuth({ ...oAuth, accessTokenUrl:
url }) inside the same try/catch that surrounds the axios request so its errors
are handled by the existing token-error branch, then propagate its error into
the token error handling path (ensuring the cleanup branch runs). Specifically,
move or duplicate the applyTokenEndpointAuth invocation into the try block that
performs the axios request (the code that assigns into
axiosRequestConfig.headers and data/bodyParams), or add a local try/catch around
applyTokenEndpointAuth that rethrows into the existing error flow, and apply the
same change for the other occurrences that call applyTokenEndpointAuth.
In `@packages/bruno-filestore/src/formats/yml/common/auth-oauth2.ts`:
- Around line 67-89: The code is losing unsupported auth methods by collapsing
JWT/mTLS/none into "body" and then restoring "body" to client_secret_post;
update placementForMethod to only map client_secret_post -> 'body' (remove
client_secret_jwt and private_key_jwt from the mapping) so JWT and other
unsupported methods are left undefined, and keep methodForPlacement mapping
as-is (body -> client_secret_post) so only an explicit client_secret_post
round-trips; ensure buildClientCredentials continues to prefer
placementForMethod(oauth.tokenEndpointAuthMethod) ?? oauth.credentialsPlacement
so unsupported methods remain unset rather than rewritten.
In `@packages/bruno-lang/v2/src/utils.js`:
- Around line 84-86: The `@file`(...) serializer must escape characters that break
the parser and preserve Windows paths: when oauth2.privateKeyType === 'file'
replace backslashes and closing parens in oauth2.privateKey with escaped forms
(e.g., '\' -> '\\' and ')' -> '\)') before embedding in `@file(...)` (the code
that builds `value` in packages/bruno-lang/v2/src/utils.js), and update the
corresponding parsers (bruToJson.js and collectionBruToJson.js where
`.slice(6,-1)` is used) to unescape those sequences after slicing; ensure both
serialization and parsing consistently handle backslash escapes so paths like
`C:\path\to\key).pem` round-trip intact.
In `@packages/bruno-requests/src/auth/oauth2-helper.ts`:
- Around line 159-161: The code assigns sensitive client auth material from
applyTokenEndpointAuth into requestConfig.headers and data (via clientAuth,
requestConfig.headers, and data) which later gets logged; fix by avoiding
logging raw secrets: before any debug log path, create sanitized clones (e.g.,
shallow copy of requestConfig.headers and data) and replace sensitive keys like
Authorization (Basic), client_secret, client_assertion, client_assertion_type
with a redacted placeholder (e.g., "[REDACTED]") and use those clones for
logging; keep the original requestConfig and data unchanged so the real request
still sends the credentials, and apply the same change for the other similar
block around lines 244-246.
In `@packages/bruno-requests/src/auth/tokenEndpointAuth.ts`:
- Around line 92-105: The merge of opts.additionalClaims into the JWT claims in
tokenEndpointAuth.ts currently allows overriding reserved claims (iss, sub, aud,
iat, exp, jti); update the logic in the code that builds the claims object (the
claims variable and the loop over opts.additionalClaims) to ignore or reject any
additionalClaim whose name matches the reserved set
['iss','sub','aud','iat','exp','jti'] before assigning to claims (e.g., filter
out those names or throw an error), and ensure you still honor claim.enabled and
claim.value handling for non-reserved names.
- Around line 183-220: Add a guard that throws when clientId === '' for all
client-authentication methods that require an identifier: specifically inside
the client_secret_basic, client_secret_post, client_secret_jwt and
private_key_jwt branches of the tokenEndpointAuth logic (the blocks that
reference method, clientId and clientSecret), so the code throws a clear
Error(`${method} requires client_id`) before building Authorization headers,
bodyParams, or constructing the JWT (iss/sub). This prevents emitting JWTs with
undefined iss/sub or malformed requests; implement the same check used in the
tls_client_auth/self_signed_tls_client_auth branch and place it at the top of
each affected branch.
In `@packages/bruno-schema/src/collections/index.js`:
- Around line 373-416: The current Yup conditions only check grantType; update
the relevant schemas (tokenEndpointAuthSigningAlg, privateKey, privateKeyType,
privateKeyFormat, keyId, audience, assertionLifetime, additionalClaims and any
client secret field) to validate based on tokenEndpointAuthMethod as well (use
when(['grantType','tokenEndpointAuthMethod'], ...) or a when on
tokenEndpointAuthMethod) so that: private_key_jwt requires
privateKey/privateKeyType/privateKeyFormat/keyId and only allows RSA/EC algs
(RS*/ES*), client_secret_jwt requires client secret and only allows HMAC algs
(HS*), and client_secret_basic/ post allow no signing alg; tighten
tokenEndpointAuthSigningAlg oneOf checks to depend on tokenEndpointAuthMethod
(HS* vs RS*/ES*) and make other branches strip() when method isn’t applicable to
prevent saving invalid combinations.
---
Nitpick comments:
In
`@packages/bruno-app/src/components/RequestPane/Auth/OAuth2/ClientAuthMethod/index.js`:
- Around line 97-305: Add stable data-testid attributes to the new OAuth2
controls so Playwright can target them: add a data-testid to the MenuDropdown
toggle for the client auth method (the div inside MenuDropdown in the
tokenEndpointAuthMethod block / COMPONENT: MenuDropdown, key
"input-token-endpoint-auth-method"), and likewise add a data-testid to the
MenuDropdown toggle for signing algorithm (MenuDropdown in the
tokenEndpointAuthSigningAlg block / key "input-signing-alg"); add data-testid
props on the SingleLineEditor instances for JWT inputs (oAuth.keyId,
oAuth.audience, oAuth.assertionLifetime — COMPONENT: SingleLineEditor in blocks
with keys "input-key-id", "input-audience", "input-assertion-lifetime"); and add
data-testid attributes to the private-key file controls/buttons (the browse and
clear buttons that call handleBrowsePrivateKey and handleClearPrivateKey, and
the filename span shown when isFileBacked). Ensure testids are stable, follow
naming like data-testid="oauth-client-auth-method", "oauth-signing-alg",
"oauth-key-id", "oauth-audience", "oauth-assertion-lifetime",
"oauth-privatekey-browse", "oauth-privatekey-clear", and
"oauth-privatekey-filename".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a39d967e-3fa8-4a38-a290-f4b9e9c9ed82
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (33)
packages/bruno-app/src/components/Git/VisualDiffViewer/VisualDiffAuth.jspackages/bruno-app/src/components/RequestPane/Auth/OAuth2/AuthorizationCode/index.jspackages/bruno-app/src/components/RequestPane/Auth/OAuth2/ClientAuthMethod/index.jspackages/bruno-app/src/components/RequestPane/Auth/OAuth2/ClientCredentials/index.jspackages/bruno-app/src/components/RequestPane/Auth/OAuth2/GrantTypeSelector/index.jspackages/bruno-app/src/components/RequestPane/Auth/OAuth2/PasswordCredentials/index.jspackages/bruno-app/src/utils/collections/index.jspackages/bruno-converters/src/openapi/openapi-to-bruno.jspackages/bruno-converters/src/openapi/swagger2-to-bruno.jspackages/bruno-converters/src/opencollection/common/auth.tspackages/bruno-converters/src/postman/postman-to-bruno.jspackages/bruno-converters/tests/postman/postman-to-bruno/process-auth.spec.jspackages/bruno-electron/src/utils/oauth2.jspackages/bruno-filestore/src/formats/yml/common/auth-oauth2.tspackages/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/src/utils.jspackages/bruno-lang/v2/tests/examples/fixtures/bru/oauth2-examples.brupackages/bruno-lang/v2/tests/examples/fixtures/json/oauth2-examples.jsonpackages/bruno-lang/v2/tests/fixtures/request.brupackages/bruno-lang/v2/tests/fixtures/request.jsonpackages/bruno-requests/package.jsonpackages/bruno-requests/src/auth/index.tspackages/bruno-requests/src/auth/keyMaterial.tspackages/bruno-requests/src/auth/oauth1-request-authorization.tspackages/bruno-requests/src/auth/oauth2-helper.tspackages/bruno-requests/src/auth/tokenEndpointAuth.spec.tspackages/bruno-requests/src/auth/tokenEndpointAuth.tspackages/bruno-requests/src/index.tspackages/bruno-schema-types/src/common/auth.tspackages/bruno-schema/src/collections/index.js
| const clientAuth = await applyTokenEndpointAuth({ ...oAuth, accessTokenUrl: url }); | ||
| Object.assign(axiosRequestConfig.headers, clientAuth.headers); | ||
| Object.assign(data, clientAuth.bodyParams); |
There was a problem hiding this comment.
Redact client-auth credentials from debugInfo.
These merged headers/body params are later copied verbatim into requestDetails in getCredentialsFromTokenUrl(). With the new methods that means raw Authorization, client_secret, and client_assertion values can end up in the persisted/debug payload. Please scrub those fields before returning or storing the trace.
Also applies to: 466-468, 610-612, 653-655
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/bruno-electron/src/utils/oauth2.js` around lines 270 - 272, The
merged client auth headers/body (result of applyTokenEndpointAuth) are being
copied into axiosRequestConfig.headers and data and later stored into
requestDetails by getCredentialsFromTokenUrl, so scrub sensitive fields before
any persistence or debug tracing: after applying clientAuth, remove or redact
Authorization (or authorization) header and any body params named client_secret
and client_assertion from axiosRequestConfig.headers and data (and ensure the
same scrub happens in other identical spots where Object.assign is used to merge
clientAuth.headers/bodyParams), then pass the redacted objects to
getCredentialsFromTokenUrl or to any logging/storage paths so raw credentials
are never persisted.
Catch applyTokenEndpointAuth() failures in the normal token-error path.
applyTokenEndpointAuth() now does work that can throw before the surrounding network try/catch runs. A bad key path/JWK/JWT config will currently bubble out as an unstructured rejection here, and the refresh flow also skips its cleanup branch. Please wrap the helper call in the same error-handling path as the axios request.
Also applies to: 466-468, 610-612, 653-655
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/bruno-electron/src/utils/oauth2.js` around lines 270 - 272, The call
to applyTokenEndpointAuth can throw before the network try/catch, causing
unstructured rejections and skipping the token-refresh cleanup; wrap
applyTokenEndpointAuth({ ...oAuth, accessTokenUrl: url }) inside the same
try/catch that surrounds the axios request so its errors are handled by the
existing token-error branch, then propagate its error into the token error
handling path (ensuring the cleanup branch runs). Specifically, move or
duplicate the applyTokenEndpointAuth invocation into the try block that performs
the axios request (the code that assigns into axiosRequestConfig.headers and
data/bodyParams), or add a local try/catch around applyTokenEndpointAuth that
rethrows into the existing error flow, and apply the same change for the other
occurrences that call applyTokenEndpointAuth.
There was a problem hiding this comment.
The redaction gap is real but pre-existing — requestDetails already captures the unredacted body/headers for client_secret_basic (the Authorization: Basic … header) and client_secret_post (the client_secret=… body param) and has done since the OAuth2 debug pane landed in #6164 (July 2025). This PR doesn't add a new leak path; it adds new kinds of material (signed client_assertion JWTs) that flow through the same existing capture.
A proper fix belongs in getCredentialsFromTokenUrl / the request-debug builder so every client-auth method is scrubbed uniformly, not just the new ones. I'd rather not bundle that refactor with this PR — happy to open a follow-up once this lands.
There was a problem hiding this comment.
@jattsson — Fair point, and agreed: centralizing the scrub in getCredentialsFromTokenUrl is the right call so all auth methods get uniform treatment rather than patching each call-site individually.
On the second finding (wrapping applyTokenEndpointAuth in the existing error path) — that one is genuinely new code in this PR. A bad key path or malformed JWK will throw before the try/catch that handles axios errors, causing an unstructured rejection that bypasses the cleanup branch in the refresh flow. That's worth a one-liner fix here if you have the bandwidth:
// Move applyTokenEndpointAuth inside the try block, or add a local catch:
let clientAuth;
try {
clientAuth = await applyTokenEndpointAuth({ ...oAuth, accessTokenUrl: url });
} catch (err) {
return Promise.reject(err); // or return structured error object matching existing pattern
}Would you like me to open a GitHub issue to track the credential-redaction follow-up so it doesn't get lost after this lands?
🧠 Learnings used
Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.
Learnt from: sanish-bruno
Repo: usebruno/bruno PR: 7449
File: packages/bruno-electron/src/utils/collection.js:177-189
Timestamp: 2026-03-13T07:17:44.857Z
Learning: In Bruno monorepo utilities, document that segment.startLine and endLine stored in segments.js are wrapper-relative (including the IIFE opener line). Ensure reviewers understand resolveSegmentError uses scriptRelativeLine = parsed.line - wrapperOffset and maps back with blockStartLine + (scriptRelativeLine - segment.startLine) - 1, where the -1 absorbs the IIFE opener line. Do not flag off-by-one issues here because the design is deliberate and consistent; apply this understanding when evaluating similar wrapper-relative line mappings across utils files.
| const clientAuth = await applyTokenEndpointAuth(oauth2Config); | ||
| Object.assign(requestConfig.headers, clientAuth.headers); | ||
| Object.assign(data, clientAuth.bodyParams); |
There was a problem hiding this comment.
Redact generated client-auth material before the request debug log.
After these assignments, the existing request logging path will print raw Basic credentials, client_secret, and JWT client_assertion values verbatim. That is sensitive auth material and should not end up in logs.
Also applies to: 244-246
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/bruno-requests/src/auth/oauth2-helper.ts` around lines 159 - 161,
The code assigns sensitive client auth material from applyTokenEndpointAuth into
requestConfig.headers and data (via clientAuth, requestConfig.headers, and data)
which later gets logged; fix by avoiding logging raw secrets: before any debug
log path, create sanitized clones (e.g., shallow copy of requestConfig.headers
and data) and replace sensitive keys like Authorization (Basic), client_secret,
client_assertion, client_assertion_type with a redacted placeholder (e.g.,
"[REDACTED]") and use those clones for logging; keep the original requestConfig
and data unchanged so the real request still sends the credentials, and apply
the same change for the other similar block around lines 244-246.
There was a problem hiding this comment.
Same situation as the matching comment on bruno-electron/src/utils/oauth2.js — the request-logging path that ends up holding these headers/body params already received unredacted Authorization: Basic … and client_secret=… for the pre-existing client_secret_basic / client_secret_post methods. This PR widens the set of material flowing through that path (now also signed client_assertion JWTs) but doesn't introduce the leak itself.
A redaction helper applied where the debug log is built would fix all methods at once, old and new. I'd prefer to handle that in a follow-up rather than bundle the refactor into this PR.
There was a problem hiding this comment.
@jattsson — fair point, the debug log path was already leaking Authorization: Basic … and client_secret before this PR; this change just adds client_assertion JWTs to the mix. A single redaction helper at the log site would cover all methods cleanly.
Would you like me to open a follow-up GitHub issue to track adding that redaction helper? I can link back to this PR and comment for context.
| const claims: Record<string, unknown> = { | ||
| iss: opts.clientId, | ||
| sub: opts.clientId, | ||
| aud: opts.audience && opts.audience.trim() !== '' ? opts.audience : opts.accessTokenUrl, | ||
| iat: now, | ||
| exp: now + lifetime, | ||
| jti: crypto.randomUUID() | ||
| }; | ||
|
|
||
| for (const claim of opts.additionalClaims || []) { | ||
| if (claim?.enabled && claim?.name) { | ||
| claims[claim.name] = claim.value ?? ''; | ||
| } | ||
| } |
There was a problem hiding this comment.
Block reserved JWT claims from additionalClaims.
additionalClaims are merged after iss, sub, aud, iat, exp, and jti, so a user can currently replace the core assertion claims and generate an invalid or weaker client assertion. Reject or ignore those reserved names before merging.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/bruno-requests/src/auth/tokenEndpointAuth.ts` around lines 92 - 105,
The merge of opts.additionalClaims into the JWT claims in tokenEndpointAuth.ts
currently allows overriding reserved claims (iss, sub, aud, iat, exp, jti);
update the logic in the code that builds the claims object (the claims variable
and the loop over opts.additionalClaims) to ignore or reject any additionalClaim
whose name matches the reserved set ['iss','sub','aud','iat','exp','jti'] before
assigning to claims (e.g., filter out those names or throw an error), and ensure
you still honor claim.enabled and claim.value handling for non-reserved names.
There was a problem hiding this comment.
This is intentional — additionalClaims is the escape hatch for OP-specific quirks, and a blanket filter would remove its primary use case.
Concrete example: some OPs (Microsoft Entra in certain tenant configurations, parts of FAPI 1.0 Advanced) require aud to be an array, or tenant-scoped, or the issuer URL rather than the token endpoint — none of which the dedicated audience string field can express. additionalClaims is the supported override.
The other reserved claims fall into the same bucket to varying degrees: exp is harmless to re-set (redundant with assertionLifetime), and iss/sub differ on a small number of non-compliant OPs or development setups that users still need to talk to. iat and jti have no legitimate override I'm aware of, but the OP will reject a stale iat or replayed jti immediately — a misconfiguration there fails fast rather than silently weakening the assertion, so it doesn't justify removing the escape hatch.
There was a problem hiding this comment.
@jattsson, that's a solid rationale — the aud-as-array / tenant-scoped aud case for Entra and FAPI 1.0 Advanced is a clear gap that the dedicated audience string can't cover, and the fail-fast argument for iat/jti holds. Acknowledged, withdrawing the suggestion.
It would be worth adding a brief comment in the code near the merge loop noting that reserved claim overrides are intentional, so future reviewers don't flag this again.
✏️ Learnings added
Learnt from: jattsson
Repo: usebruno/bruno PR: 8051
File: packages/bruno-requests/src/auth/tokenEndpointAuth.ts:92-105
Timestamp: 2026-05-21T05:37:24.077Z
Learning: In `packages/bruno-requests/src/auth/tokenEndpointAuth.ts`, the `additionalClaims` loop intentionally allows overriding reserved JWT claims (`iss`, `sub`, `aud`, `iat`, `exp`, `jti`). This is a deliberate escape hatch for OP-specific quirks: e.g., Microsoft Entra and FAPI 1.0 Advanced require `aud` as an array or tenant-scoped URL that the dedicated `audience` string field cannot express; `iss`/`sub` overrides support non-compliant OPs; `iat`/`jti` overrides fail fast at the OP rather than silently weakening the assertion. Do not flag this as a security issue.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| tokenEndpointAuthMethod: Yup.string().when('grantType', { | ||
| is: (val) => ['client_credentials', 'password', 'authorization_code'].includes(val), | ||
| then: Yup.string().oneOf(tokenEndpointAuthMethodValues).nullable(), | ||
| otherwise: Yup.string().nullable().strip() | ||
| }), | ||
| tokenEndpointAuthSigningAlg: Yup.string().when('grantType', { | ||
| is: (val) => ['client_credentials', 'password', 'authorization_code'].includes(val), | ||
| then: Yup.string().oneOf(tokenEndpointAuthSigningAlgValues).nullable(), | ||
| otherwise: Yup.string().nullable().strip() | ||
| }), | ||
| privateKey: Yup.string().when('grantType', { | ||
| is: (val) => ['client_credentials', 'password', 'authorization_code'].includes(val), | ||
| then: Yup.string().nullable(), | ||
| otherwise: Yup.string().nullable().strip() | ||
| }), | ||
| privateKeyType: Yup.string().when('grantType', { | ||
| is: (val) => ['client_credentials', 'password', 'authorization_code'].includes(val), | ||
| then: Yup.string().oneOf(['text', 'file']).nullable(), | ||
| otherwise: Yup.string().nullable().strip() | ||
| }), | ||
| privateKeyFormat: Yup.string().when('grantType', { | ||
| is: (val) => ['client_credentials', 'password', 'authorization_code'].includes(val), | ||
| then: Yup.string().oneOf(['pem', 'jwk']).nullable(), | ||
| otherwise: Yup.string().nullable().strip() | ||
| }), | ||
| keyId: Yup.string().when('grantType', { | ||
| is: (val) => ['client_credentials', 'password', 'authorization_code'].includes(val), | ||
| then: Yup.string().nullable(), | ||
| otherwise: Yup.string().nullable().strip() | ||
| }), | ||
| audience: Yup.string().when('grantType', { | ||
| is: (val) => ['client_credentials', 'password', 'authorization_code'].includes(val), | ||
| then: Yup.string().nullable(), | ||
| otherwise: Yup.string().nullable().strip() | ||
| }), | ||
| assertionLifetime: Yup.number().when('grantType', { | ||
| is: (val) => ['client_credentials', 'password', 'authorization_code'].includes(val), | ||
| then: Yup.number().integer().positive().nullable(), | ||
| otherwise: Yup.number().nullable().strip() | ||
| }), | ||
| additionalClaims: Yup.array().when('grantType', { | ||
| is: (val) => ['client_credentials', 'password', 'authorization_code'].includes(val), | ||
| then: Yup.array().of(oauth2AdditionalClaimSchema).nullable(), | ||
| otherwise: Yup.array().nullable().strip() |
There was a problem hiding this comment.
Make the OAuth2 validation method-aware.
These branches still validate only by grantType, so the schema will happily persist configs that can never work at runtime: private_key_jwt without key material, client_secret_jwt without a secret, or HS*/RSA*/ES* algs on the wrong auth method. Tighten the when() logic around tokenEndpointAuthMethod so invalid combinations are rejected before save time.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/bruno-schema/src/collections/index.js` around lines 373 - 416, The
current Yup conditions only check grantType; update the relevant schemas
(tokenEndpointAuthSigningAlg, privateKey, privateKeyType, privateKeyFormat,
keyId, audience, assertionLifetime, additionalClaims and any client secret
field) to validate based on tokenEndpointAuthMethod as well (use
when(['grantType','tokenEndpointAuthMethod'], ...) or a when on
tokenEndpointAuthMethod) so that: private_key_jwt requires
privateKey/privateKeyType/privateKeyFormat/keyId and only allows RSA/EC algs
(RS*/ES*), client_secret_jwt requires client secret and only allows HMAC algs
(HS*), and client_secret_basic/ post allow no signing alg; tighten
tokenEndpointAuthSigningAlg oneOf checks to depend on tokenEndpointAuthMethod
(HS* vs RS*/ES*) and make other branches strip() when method isn’t applicable to
prevent saving invalid combinations.
There was a problem hiding this comment.
Bruno's schema convention is permissive save + strict runtime — fields gate on grantType only, with no cross-field when(['…', 'tokenEndpointAuthMethod'], …) validators elsewhere in the file. Tightening one area here would make this PR an outlier without catching anything the existing layers don't already catch:
316b3562cfilters JWT-only fields fromx-bruno-oauth2when the active method isn'tclient_secret_jwtorprivate_key_jwt, so stale signing-alg/keyId/audience never reach yml.103b90333makesapplyTokenEndpointAuthfail fast with"<method> requires a client_id", and the existing guards cover emptyclientSecret/privateKeyfor the JWT methods.- The form's
algorithmOptions(method)already restricts the alg dropdown to the right HS*/RS*/ES* family per method.
Adding save-time strictness would also fight users mid-edit: switching private_key_jwt → client_secret_basic with a still-populated keyId would now throw on save, putting unsaved config at risk. I'd rather keep this layer permissive.
There was a problem hiding this comment.
@jattsson, that's a well-reasoned defense — the layered approach (filestore filter, runtime guards, UI restriction) provides the necessary correctness guarantees without burdening the schema layer with cross-field logic that would break mid-edit workflows. Happy to drop this suggestion.
✏️ Learnings added
Learnt from: jattsson
Repo: usebruno/bruno PR: 8051
File: packages/bruno-schema/src/collections/index.js:373-416
Timestamp: 2026-05-21T06:29:42.245Z
Learning: In the Bruno codebase (`packages/bruno-schema/src/collections/index.js`), the schema convention is deliberately permissive at save time: OAuth2 fields (including new token endpoint auth fields like `tokenEndpointAuthMethod`, `privateKey`, `tokenEndpointAuthSigningAlg`, etc.) are gated only on `grantType`, never cross-validated against `tokenEndpointAuthMethod`. Correctness is enforced in downstream layers: filestore filters stale JWT-only fields, `applyTokenEndpointAuth` fails fast at runtime, and the UI restricts algorithm dropdowns per method. Do not suggest adding cross-field `when(['grantType','tokenEndpointAuthMethod'], …)` Yup validators to this schema — it would be an outlier and would break mid-edit transitions (e.g., switching from `private_key_jwt` to `client_secret_basic` with residual fields still populated would throw on save).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.
…ctions OpenCollection doesn't model token-endpoint auth methods beyond client_secret_basic/post, so saving as yml dropped private_key_jwt, client_secret_jwt, tls_client_auth, self_signed_tls_client_auth, plus the JWT-bearer assertion config (privateKey, keyId, audience, etc.). Carry the unrepresentable fields in an x-bruno-oauth2 extension block; older Bruno builds without these features just ignore the extension.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bruno-filestore/src/formats/yml/common/auth-oauth2.ts`:
- Around line 625-629: The current loop copies every property from (oauth as
any)[BRUNO_OAUTH2_EXTENSION_KEY] onto brunoOAuth, allowing hand-edited
collections to overwrite canonical fields; change the logic in the block that
reads ext (where BRUNO_OAUTH2_EXTENSION_KEY is used and brunoOAuth is mutated)
to only copy a fixed whitelist of allowed Bruno extension keys (e.g., the known
Bruno-specific fields used elsewhere in the codebase) instead of
Object.entries(ext) — check and only assign those specific keys onto brunoOAuth,
leaving canonical oauth fields (like grantType/URLs) untouched.
- Around line 314-325: The serializer oauth2ExtensionFromBruno is leaking
JWT/private-key fields; update it so JWT-specific fields (e.g., privateKey,
audience, assertionLifetime, additionalClaims) are only emitted when the
selected tokenEndpointAuthMethod is a JWT/private-key placement. In
oauth2ExtensionFromBruno, before assigning ext[k], add a guard: if k is one of
the JWT_ONLY_FIELDS and isPlacementRepresentable(oauth.tokenEndpointAuthMethod)
=== false (i.e., the current method is not a JWT/private-key method),
continue/skip that field. Use the existing OAUTH2_EXTENSION_FIELDS and
isPlacementRepresentable(oauth.tokenEndpointAuthMethod) checks and reference
BrunoOAuth2/oauth.tokenEndpointAuthMethod to implement this conditional
filtering so inactive JWT material is not serialized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 713b11dd-d168-4540-aead-b72a87ce0b86
📒 Files selected for processing (1)
packages/bruno-filestore/src/formats/yml/common/auth-oauth2.ts
…ethods client_secret_basic/post/jwt and private_key_jwt now fail fast with a clear error when client_id is empty, matching the existing mTLS guards. Previously an empty client_id produced a malformed Basic header, client_id= body param, or a JWT with iss=sub="" — all rejected by the OP with opaque "invalid_client" errors.
OpenCollection only models client_secret_basic / client_secret_post via its `placement` field. Previously we collapsed client_secret_jwt and private_key_jwt onto `placement: body`, which an external reader (without Bruno's x-bruno-oauth2 extension) would interpret as client_secret_post — silently rewriting JWT methods and exposing the secret as a body parameter. Leave `placement` unset for JWT methods; the actual method is carried in the extension.
Skip JWT-only fields (privateKey/keyId/audience/assertionLifetime/etc.) when the active method isn't client_secret_jwt or private_key_jwt, so stale material from a previous method doesn't linger in yml. On restore, whitelist the carried fields against OAUTH2_EXTENSION_FIELDS so a hand-edited extension can't override canonical fields like grantType or the endpoint URLs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bruno-filestore/src/formats/yml/common/auth-oauth2.ts`:
- Around line 319-336: The current guard treats any JWT auth method the same,
letting private-key fields (privateKey, privateKeyType, privateKeyFormat, keyId)
be emitted even for client_secret_jwt; update oauth2ExtensionFromBruno to only
serialize private-key material when the tokenEndpointAuthMethod is exactly
'private_key_jwt' (not just any JWT). Concretely, split the
JWT_ONLY_EXTENSION_FIELDS into two sets (or add a PRIVATE_KEY_ONLY_FIELDS set)
and check both: use isJwtMethod(oauth.tokenEndpointAuthMethod) for JWT-only
fields like audience/assertionLifetime/additionalClaims, and a strict equality
check (oauth.tokenEndpointAuthMethod === 'private_key_jwt') for private-key
fields so they are omitted for client_secret_jwt; adjust the loop in
oauth2ExtensionFromBruno to use these two guards accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 68671654-308c-4e0c-bbb1-b66e24948d53
📒 Files selected for processing (1)
packages/bruno-filestore/src/formats/yml/common/auth-oauth2.ts
The to-JSON helper's password, authorization_code, and client_credentials branches copied all the other new JWT client-auth fields but dropped additionalClaims, so custom JWT claims didn't survive an export-import round-trip. Implicit flow is unaffected — it has no token endpoint.
A switch from private_key_jwt to client_secret_jwt was still serialising privateKey/privateKeyType/privateKeyFormat into the x-bruno-oauth2 extension because the previous JWT_ONLY bucket treated both JWT methods the same. client_secret_jwt signs with the client secret; the PEM/JWK material is only meaningful for private_key_jwt. keyId stays JWT-wide since RFC 7515 §4.1.4 `kid` applies to both signing modes.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…inheritance
The per-grant-type switches in prepare-request.js and interpolate-vars.js
predate this PR and hand-rolled the property list for each grant type.
The JWT client-auth fields added by this PR (tokenEndpointAuthMethod,
privateKey, audience, additionalClaims, etc.) were never added to those
lists, so collection- or folder-level OAuth2 config was silently dropped
before auto-fetch fired — and additionalClaims values never got
{{var}}-interpolated. Replace the switches with a single canonical field
list (oauth2-fields.js) and a known-grant-types whitelist.
The POST→token-endpoint exchange (apply client auth, merge additional token-stage params, form-encode, axios, push debugInfo frame, persist credentials) was duplicated inline at the tail of every token fetcher. Extract the common path as performTokenExchange and migrate getOAuth2TokenUsingAuthorizationCode to use it. The other three fetchers (client_credentials, password, OIDC follow-up) can adopt the helper in subsequent commits.
1 participant
Summary
Replace the binary Add Credentials to: Request Body | Basic Auth header dropdown with a proper Client Authentication method selector that supports every method standardised by RFC 7591 §2 / OpenID Connect Core 1.0 §9 plus the mTLS variants from RFC 8705:
client_secret_basic—Authorization: Basicheader (RFC 6749 §2.3.1)client_secret_post—client_id+client_secretin form body (RFC 6749 §2.3.1)client_secret_jwt— HMAC-signed JWT assertion (OIDC Core §9 / RFC 7523)private_key_jwt— RSA/EC/EdDSA-signed JWT assertion, PEM or JWK input (RFC 7523 §2.2)tls_client_auth/self_signed_tls_client_auth— mTLS at the transport layer (RFC 8705); reuses Bruno's existing per-domain Client Certificate confignone— public client (RFC 7591 §2)The legacy
credentials_placement: body | basic_auth_headerfield is read for backward compatibility and migrated totoken_endpoint_auth_methodon next save — following the same "read with graceful default, always write the new key" shape used whentokenSourcewas added (commit4797abbef).Applies to all three grant types that hit the token endpoint:
client_credentials,password,authorization_code(plus the refresh-token follow-up).What's in here
bruno-schema,bruno-schema-types): new OAuth2 fields (tokenEndpointAuthMethod,tokenEndpointAuthSigningAlg,privateKey,privateKeyType,privateKeyFormat,keyId,audience,assertionLifetime,additionalClaims);credentialsPlacementkept as@deprecated..brugrammar + serializer (bruno-lang): parses the new keys; serializes only the new keys; migrates legacycredentials_placementon read.bruno-filestore): bridgestokenEndpointAuthMethod↔credentials.placementso yml collections round-trip correctly.bruno-requests/src/auth/tokenEndpointAuth.ts): singleapplyTokenEndpointAuth()that returns headers + body params per method. Usesjosefor JWT signing. Private key can be supplied as PEM or JWK / JWK Set (kid-based selection in a JWK Set; the JWK's ownkidpropagates to the JWT header when no override is set). File-path source loads via the existing OAuth1readKeyFile()cache, extracted into a sharedkeyMaterial.ts.bruno-requests,bruno-electron):client_credentials,password,authorization_code, andrefresh_tokenall delegate to the shared helper. CLI inherits the change via@usebruno/requests.bruno-app): sharedClientAuthMethodsubcomponent renders the method dropdown + Client Secret + JWT-bearer fields (signing alg, key id, audience, lifetime, additional claims) + Private Key (inline textarea with "Select file…" button modelled on the OAuth1 private-key UX). For mTLS methods the auth panel surfaces a warning when no client certificate is configured for the token endpoint's hostname (host-pattern matcher mirrorscert-utils.js).bruno-converters): Postman, OpenCollection, OpenAPI, and Swagger2 all updated.bruno-requestscovering all 7 methods, JWT claim correctness againstjose.jwtVerify, JWK Set kid-selection, file-path key source, and the legacy-migration fallback. All workspace tests pass (`bruno-schema` 22, `bruno-lang` 306, `bruno-converters` 998, `bruno-requests` 420, `bruno-electron` 298, `bruno-filestore` 11).Backward compatibility
Collections saved with
credentials_placement: body|basic_auth_headercontinue to load — `bruToJson.js` mapsbody → client_secret_postandbasic_auth_header → client_secret_basicwhen the new key is absent. Re-saving such a collection in this build writes onlytoken_endpoint_auth_method(the legacy field is dropped on write). A collection saved by this build and then opened in an older Bruno will appear to lose its client-auth setting — the same compatibility characteristic as prior field renames in Bruno (e.g.body:binary-file→body:file).Default
New OAuth2 blocks default to
client_secret_post. RFC 7591 §2's nominal default isclient_secret_basic, but `client_secret_post` preserves Bruno's current behaviour (the previous default was `credentialsPlacement: 'body'`).Test plan
Notes for reviewers
🤖 Generated with Claude Code
Closes #5205
Summary by CodeRabbit
New Features
Bug Fixes
Tests