Closed
Conversation
|
Konstantin Kukharev seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds Sber and Sber IFT OIDC provider implementations, related Sber-specific claim normalization and trait-mapping utilities, extensive token-exchange logging with optional mTLS, certificate resources for mTLS, tests and a mock server, and integration of Sber flows into existing OIDC strategy logic and diagnostics. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Strategy as OIDC Strategy
participant OAuth2 as oauth2.Config/Client
participant Logger as sberTokenLoggingTransport
participant MTLS as mTLS Loader
participant Sber as Sber Token Endpoint
Client->>Strategy: exchangeCode(code, opts)
Strategy->>OAuth2: build OAuth2 client with custom HTTP client
Strategy->>Logger: instantiate sberTokenLoggingTransport (rquid)
Logger->>MTLS: attempt attach mTLS certs (embedded or files)
OAuth2->>Logger: perform POST /token (cloned request)
Logger->>Logger: set Accept and rquid headers, dump request
Logger->>Sber: POST /token (with rquid and optional cert)
Sber-->>Logger: response (status, body)
Logger->>Logger: read/preserve response, build curl trace
Logger-->>OAuth2: return response to oauth2 client
OAuth2-->>Strategy: token (id_token, access_token)
Strategy->>Strategy: validate ID token, fetch userinfo if needed
Strategy-->>Client: return tokens/claims or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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: 12
🧹 Nitpick comments (12)
selfservice/strategy/oidc/state.go (1)
73-82: Downgrade this Sber diagnostic from warning level.This runs on every successful Sber flow start, so
Warnwill flood operational logs and make real incidents harder to spot.Debug/Traceor a config-gated diagnostic path is a better fit here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@selfservice/strategy/oidc/state.go` around lines 73 - 82, The Sber PKCE diagnostic currently logs at Warn in the isSberProviderID branch (s.d.Logger()...Warn("Sber PKCE diagnostics during state generation")), which floods logs on every successful flow start; change the call from Warn to Debug (or Trace) so it becomes non-alerting (e.g., s.d.Logger()....Debug("Sber PKCE diagnostics during state generation")), and keep all the same fields (flow.GetID, p.Config().ID, p.Config().PKCE, state.GetPkceVerifier(), len(pkce)) so the message is still available for troubleshooting; alternatively, wrap the log emission behind a config flag (e.g., an existing diagnostics toggle) if config-gated behavior is preferred.selfservice/strategy/oidc/provider_private_net_test.go (1)
85-85: Add a real Sber case here instead of only the comment.This suite still never constructs a Sber provider, so we do not actually verify that its fixed token/userinfo URLs obey the same private-network restrictions described by the new comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@selfservice/strategy/oidc/provider_private_net_test.go` at line 85, Add a real Sber case to the provider_private_net_test suite by constructing a Sber provider instance (using the same constructor used elsewhere in the file—e.g., the OpenID provider factory/constructor function used by other tests) with the Sber config so its fixed token and userinfo URLs are actually created, then run the same private-network assertions used for other providers on those fixed token/userinfo endpoints (i.e., assert that the fixed token URL and userinfo URL for "Sber" are subject to the same private-network restrictions as other providers). Include a subtest named like "sber fixed token/userinfo URLs" so it’s clear and consistent with the existing test patterns.selfservice/strategy/oidc/sber_claims_normalize_test.go (1)
22-26: Consider expanding phone normalization test coverage.The phone normalization test only covers a single happy-path case. Based on the implementation in
sber_phone_format.go, the normalizer handles multiple input patterns (11-digit with 8/7 prefix, 10-digit). Consider adding tests for:
89991234567→+7999123456779991234567→+799912345679991234567→+79991234567- Invalid inputs →
""- Empty string →
""📝 Suggested additional test cases
func TestNormalizeRussianMobilePlus79(t *testing.T) { t.Parallel() require.Equal(t, "+79991234567", normalizeRussianMobilePlus79("+7 (999) 123 45 67")) + require.Equal(t, "+79991234567", normalizeRussianMobilePlus79("89991234567")) + require.Equal(t, "+79991234567", normalizeRussianMobilePlus79("79991234567")) + require.Equal(t, "+79991234567", normalizeRussianMobilePlus79("9991234567")) + require.Equal(t, "", normalizeRussianMobilePlus79("")) + require.Equal(t, "", normalizeRussianMobilePlus79("12345")) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@selfservice/strategy/oidc/sber_claims_normalize_test.go` around lines 22 - 26, Add more unit tests for normalizeRussianMobilePlus79 in sber_claims_normalize_test.go to cover all input patterns handled by the implementation: assert that "89991234567" and "79991234567" and "9991234567" normalize to "+79991234567", and that invalid inputs and an empty string produce "". Use the same test style as TestNormalizeRussianMobilePlus79 (t.Parallel(), require.Equal) and add separate subtests or table-driven cases to clearly name each scenario while referencing the normalizeRussianMobilePlus79 function.selfservice/strategy/oidc/provider_sber_test.go (1)
36-44: Consider adding more specific assertions for AuthCodeURLOptions.The test only verifies that options are non-empty. For better coverage, consider asserting specific expected parameters (e.g., nonce, client_type) are present in the returned options.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@selfservice/strategy/oidc/provider_sber_test.go` around lines 36 - 44, The test TestProviderSber_AuthCodeURLOptions only checks that AuthCodeURLOptions returns something non-empty; update it to assert specific expected parameters are present by calling p.(oidc.OAuth2Provider).AuthCodeURLOptions(nil) and checking the returned options contain the expected keys/values (e.g., a "nonce" parameter is included and non-empty, and "client_type" equals the expected value for ProviderSber), and optionally verify any other required OAuth2 params; modify assertions to inspect the options map/slice returned rather than only using assert.NotEmpty.certs/cacerts.cer (1)
1-80: Clarify production certificate configuration for Sber mTLS.These are test certificates by design. The code implements a fallback chain: it first checks environment variables
SBER_MTLS_CERT_PATHandSBER_MTLS_KEY_PATH, then looks for local certificate files (certs/client_cert.crt,certs/file.crt.pem), and finally falls back to embedded test certificates. For production deployments using actual Sber endpoints, ensure production certificates are provided via environment variables or by replacing the certificate files, as the embedded test certificates are intended only for local development and testing against Sber's test environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@certs/cacerts.cer` around lines 1 - 80, The repo currently contains embedded Sber test certificates (the contents of cacerts.cer) and the code falls back from SBER_MTLS_CERT_PATH / SBER_MTLS_KEY_PATH to local files (certs/client_cert.crt, certs/file.crt.pem) and then to these embedded test certs; update the TLS loading logic so that when running in production (e.g., NODE_ENV=production or a new SBER_MTLS_REQUIRED flag) the process must require valid production certs from SBER_MTLS_CERT_PATH and SBER_MTLS_KEY_PATH and exit with an error if they are missing, stop silently falling back to embedded test certs in functions/modules that reference SBER_MTLS_CERT_PATH, SBER_MTLS_KEY_PATH and the local paths, and move or remove the embedded test certificates (cacerts.cer) from the production branch or guard their use behind an explicit development-only flag and document the required env vars for production.selfservice/strategy/oidc/strategy_registration.go (1)
477-486: Edge case: consecutive underscores produce empty parts.
toUpperCamel("given__name")would produce"GivenName"(correct) but the loop processes empty strings. While thecontinuehandles this safely, consider whether input sanitization is needed upstream if claim names with unusual patterns are possible.♻️ Optional: filter empty parts
func toUpperCamel(in string) string { parts := strings.Split(in, "_") + result := make([]string, 0, len(parts)) for i, p := range parts { if p == "" { continue } - parts[i] = strings.ToUpper(p[:1]) + p[1:] + result = append(result, strings.ToUpper(p[:1]) + p[1:]) } - return strings.Join(parts, "") + return strings.Join(result, "") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@selfservice/strategy/oidc/strategy_registration.go` around lines 477 - 486, The toUpperCamel function currently splits on "_" and skips empty parts inside the loop, which still iterates over empty entries; update toUpperCamel to filter out empty parts immediately (e.g., build a new slice of non-empty parts or use strings.FieldsFunc) before capitalizing to avoid processing empty strings and make intent explicit; keep the capitalization logic (strings.ToUpper(p[:1]) + p[1:]) and return strings.Join(filteredParts, "") to preserve current output like "given__name" -> "GivenName".selfservice/strategy/oidc/provider_sber.go (1)
53-72: OAuth2 config uses hardcoded URLs unlike provider_sber_ift.go.Unlike
ProviderSberIftwhich allowsAuthURLandTokenURLoverrides from configuration (lines 40-48 of provider_sber_ift.go),ProviderSberhardcodes these values. This inconsistency may cause issues if URL overrides are needed for testing or environment-specific deployments.Consider allowing configuration overrides here as well:
♻️ Proposed fix
func (g *ProviderSber) OAuth2(ctx context.Context) (*oauth2.Config, error) { + authURL := "https://id.sber.ru/CSAFront/oidc/sberbank_id/authorize.do" + if g.config.AuthURL != "" { + authURL = g.config.AuthURL + } + tokenURL := "https://oauth.sber.ru/ru/prod/tokens/v2/oidc" + if g.config.TokenURL != "" { + tokenURL = g.config.TokenURL + } + oauthConfig := &oauth2.Config{ ClientID: g.config.ClientID, ClientSecret: g.config.ClientSecret, Endpoint: oauth2.Endpoint{ - AuthURL: "https://id.sber.ru/CSAFront/oidc/sberbank_id/authorize.do", - TokenURL: "https://oauth.sber.ru/ru/prod/tokens/v2/oidc", + AuthURL: authURL, + TokenURL: tokenURL, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@selfservice/strategy/oidc/provider_sber.go` around lines 53 - 72, The OAuth2 method in ProviderSber currently hardcodes AuthURL and TokenURL; modify ProviderSber.OAuth2 to use configurable overrides like ProviderSberIft does by checking g.config.AuthURL and g.config.TokenURL (or equivalent fields on the provider config) and falling back to the existing hardcoded URLs only when those config values are empty; update the oauth2.Endpoint construction to use the chosen values and adjust the log fields (e.g., include "auth_url" as well as "token_url") so the effective endpoints are visible in the debug output.selfservice/strategy/oidc/sber_mapper_traits.go (2)
115-146: Potential issue: phone claim is checked differently than other claims.For phone number (line 127-132), the check
isPhoneTraitKey(lk)comes first in thecasecondition, butclaims.PhoneNumberis only checked implicitly vianormalizeRussianMobilePlus79. All other claim types check if the claim value is non-empty first (e.g.,claims.GivenName != "").This means if
PhoneNumberis empty,normalizeRussianMobilePlus79will be called and return empty string, which is handled correctly by theif n == ""check. However, this is inconsistent with the pattern used for other claims and performs unnecessary work.♻️ Suggested fix for consistency
- case isPhoneTraitKey(lk): + case claims.PhoneNumber != "" && isPhoneTraitKey(lk): n := normalizeRussianMobilePlus79(claims.PhoneNumber) if n == "" { return "", false } return n, true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@selfservice/strategy/oidc/sber_mapper_traits.go` around lines 115 - 146, In claimForTraitKey, the phone branch calls normalizeRussianMobilePlus79 even when claims.PhoneNumber is empty; change the case to first check claims.PhoneNumber != "" && isPhoneTraitKey(lk) so it follows the same pattern as other claims, and only call normalizeRussianMobilePlus79(claims.PhoneNumber) inside that branch, returning the normalized value if non-empty; references: claimForTraitKey, claims.PhoneNumber, isPhoneTraitKey, normalizeRussianMobilePlus79.
14-16: Consider translating comments to English.The comments are in Russian. For consistency and maintainability in an open-source project, consider translating these to English so all contributors can understand the code documentation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@selfservice/strategy/oidc/sber_mapper_traits.go` around lines 14 - 16, Translate the Russian comments above the function applySberClaimsToMapperTraitsOutput in sber_mapper_traits.go into clear English (preserving technical meaning) so they explain that the function copies Sber userinfo strings into identity.traits recursively for nested objects to preserve casing and provider values (including after Jsonnet and when traits schema has nested keys); replace or append the English text in place of the existing Russian comments and keep any code-specific terms (Jsonnet, identity.traits, userinfo) unchanged for clarity.selfservice/strategy/oidc/provider_sber_ift.go (1)
91-287: Significant code duplication with provider_sber.go.The
Claimsmethod is nearly identical toProviderSber.Claims(provider_sber.go:74-269). Both:
- Set up timeout and OAuth2 config
- Attach mTLS transport
- Make userinfo request with same headers
- Parse and normalize claims identically
Consider extracting shared logic into a helper function in
provider_sber_common.goto reduce maintenance burden.// Example shared helper signature: func sberFetchUserinfoClaims( ctx context.Context, reg Dependencies, config *Configuration, exchange *oauth2.Token, userinfoURL string, issuer string, ) (*Claims, error)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@selfservice/strategy/oidc/provider_sber_ift.go` around lines 91 - 287, The Claims method in ProviderSberIft duplicates logic found in ProviderSber.Claims (timeout/OAuth2 setup, mTLS attachment, userinfo HTTP request, response handling, JSON decode and claim normalization); extract the shared behavior into a new helper (e.g. sberFetchUserinfoClaims(ctx, reg, config, exchange, userinfoURL, issuer)) implemented in provider_sber_common.go and have both ProviderSber.Claims and ProviderSberIft.Claims call it; move common pieces such as OAuth2 retrieval (OAuth2()), mTLS setup (withSberMTLS/baseRoundTripper usage), request creation/headers, response status handling, JSON decoding into the helper and return a *Claims so provider-specific caller only passes userinfoURL and issuer and performs any tiny provider-specific tweaks.selfservice/strategy/oidc/strategy_sber_ift_api_flow_test.go (1)
131-137: Consider adding explicit test case naming.The test checks for either
200 OKor303 See Otherresponses (line 131), but the meaning differs significantly between API and browser flows. Consider adding comments or using subtests to clarify which path is being tested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@selfservice/strategy/oidc/strategy_sber_ift_api_flow_test.go` around lines 131 - 137, The test branch handling res.StatusCode (using require.Contains on http.StatusSeeOther and http.StatusOK with variables res and callbackBody) conflates two distinct flows; split into explicit subtests or add clear comments to name/describe each path — e.g., create subtests "browser flow - redirect 303" and "api flow - success 200" that assert the expected status and payload (for the 200 path assert presence of "session_token" and "continue_with"), or at minimum add inline comments above the if/res.StatusCode checks to document which branch corresponds to API vs browser behavior so the intent is unambiguous.selfservice/strategy/oidc/provider_sber_token_logging.go (1)
123-219: Logging potentially sensitive data in request/response bodies.The
RoundTripmethod logs:
- Full request headers (line 155)
- Raw request body (line 156, 163)
- Full response body (line 203)
While this is valuable for debugging, token exchange requests/responses contain sensitive data (client secrets, authorization codes, tokens). Consider:
- Using a higher log level (Trace) for full body dumps
- Redacting sensitive fields in production
- Ensuring log retention policies align with security requirements
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@selfservice/strategy/oidc/provider_sber_token_logging.go` around lines 123 - 219, The RoundTrip function on sberTokenLoggingTransport currently logs full headers and raw bodies (requestBodyRaw, responseBody, requestForm) at Debug which can leak secrets; modify RoundTrip so that full body/header dumps (request_body_raw, response_raw, request_headers, response_headers, request_form) are logged only at Trace level (use t.trace presence or logger.Trace) and/or replaced with redacted versions for Debug/Error paths, and implement and call helper redaction routines (e.g., redactSensitiveForm(requestForm), redactSensitiveHeaders(clone.Header), redactSensitiveBody(string(responseBody))) before passing values to reqLog/respLog; keep non-sensitive metadata (http_method, http_status, latency_ms, mtls info) at Debug/Error as currently done and attach original error objects to logs but never include unredacted secret fields in non-Trace logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@certs/client_cert.crt`:
- Around line 1-50: The commit exposes a private key and client certificate
(files certs/client_cert.crt and certs/private.key—note localKeyID
E2:25:74:6C:41:B7:A1:77:0E:1A:FA:A3:E9:1B:1F:C5:0C:D9:1F:83 and friendlyName
ede958f8-ea14-4287-ad8b-db0dfb10194e) and must be removed and rotated: remove
the files from the repo history using a history-rewriting tool (git filter-repo
or git filter-branch) and force-push, add certs/private.key and
certs/client_cert.crt to .gitignore, notify the CA (SberCA Test1 Ext) to
revoke/rotate the certificate/key pair, and update code to load credentials from
a secure source (environment/secrets manager or mounted volume) instead of
committing them.
In `@certs/private.key`:
- Around line 1-32: The repository contains a real private key block (-----BEGIN
PRIVATE KEY----- ... -----END PRIVATE KEY-----) with identifiers like
friendlyName "ede958f8-ea14-4287-ad8b-db0dfb10194e" and localKeyID that must be
removed; delete the committed key file from the tree, remove it from history
(git filter-repo or BFG) or replace the commit with a history-rewriting purge,
add the file pattern to .gitignore to prevent re-commits, rotate/revoke any
certificates/keys that used this private key, and replace the file in tests with
a generated runtime fixture or a clearly dummy key (not the real material) so no
sensitive key material remains in the repo or its history.
In `@selfservice/strategy/oidc/error.go`:
- Around line 61-76: logUpstreamError currently logs the raw upstream response
body and can leak secrets; update logUpstreamError to pass the response body
through safeBodyForLog (the same sanitizer used by extractOAuth2RetrieveError)
before logging, so sensitiveFragmentRE masking and newline/CR cleanup are
applied; locate logUpstreamError and replace direct body logging with
safeBodyForLog(body, 0) (or an explicit limit) and ensure the log message uses
the sanitized string.
In `@selfservice/strategy/oidc/provider_config.go`:
- Around line 237-238: The test table in TestProviderPrivateIP
(provider_private_net_test.go) is missing entries for the new providers
referenced by NewProviderSber and NewProviderSberIft in provider_config.go; add
two test cases to that table for "sber" and "sber-ift" (matching the comment
about Sber's fixed token URL behavior) so the test covers their expected
private-net/token URL behavior, ensuring the test names/keys match the provider
IDs used by the provider factory and that any expected token URL or private-net
flag values are set appropriately in each row.
In `@selfservice/strategy/oidc/provider_sber_common.go`:
- Around line 43-46: The current early-return in validateSberIDToken when flowID
== uuid.Nil silently skips ID token validation; change this to surface the issue
by returning a descriptive error (e.g., "missing flowID: cannot validate ID
token") and emit a warning log so mis-propagated/nil flow IDs are visible;
update the validateSberIDToken function to remove the nil-return, log a warning
referencing flowID, providerID and clientID, and return the error so callers
cannot accidentally accept unvalidated tokens.
- Around line 95-112: The current decodeJWTClaims function only base64-decodes
the payload and must not be trusted without cryptographic verification; replace
or augment it so id_token signature verification is performed using Sber's JWKS
before returning claims—fetch and cache Sber's JWKS, select the key by token
header.kid, and verify the JWT signature and alg (mirror the approach used for
NetID/Generic providers) inside validateSberIDToken (or create a new
verifySberIDToken function) so that decodeJWTClaims only runs after
verification; ensure you check issuer/audience/exp/nonce as already done and
surface verification errors instead of returning unverified claims from
decodeJWTClaims.
In `@selfservice/strategy/oidc/provider_sber_token_logging.go`:
- Around line 33-106: The code currently hardcodes the test certificate and
private key in embeddedSberClientCertPEM and embeddedSberClientKeyPEM which is a
security risk; remove the embedded private key and change the code that
references embeddedSberClientCertPEM/embeddedSberClientKeyPEM to load the cert
and key at runtime (preferably from a secrets manager, otherwise from
environment variables or file paths) and fail fast with a clear error if not
available; ensure consumers (functions that build TLS config or call NewXxx
using those symbols) accept cert/key bytes or paths rather than relying on
package-level constants, and add a fallback to load test data from a git-ignored
test-only file or dev-only env var for local testing.
- Around line 351-370: The cloned TLS configs created when wrapping the original
transport (the `clone` variable derived from `t.Clone()` and the `mtlsTransport`
created from `http.DefaultTransport.(*http.Transport).Clone()`) do not set a
minimum TLS version; update both `clone.TLSClientConfig` and
`mtlsTransport.TLSClientConfig` to include `MinVersion: tls.VersionTLS12` (or
`tls.VersionTLS13` if Sber supports it) when initializing or cloning the
`tls.Config` before appending `Certificates` so the transport enforces a minimum
TLS version.
In `@selfservice/strategy/oidc/provider_sber.go`:
- Around line 130-139: The current userinfo failure log in provider_sber.go uses
Debug level; change the logger call on the error path (the block that checks "if
err != nil") to use Error instead of Debug so failures are visible in production
logs; keep the same context fields (g.config.ID as "oidc_provider", requestID,
userinfoURL, latency computed from stageStart) and the same WithError(err)
chain, but call Error("OIDC userinfo request failed") rather than Debug(...)
inside the error handling in the function handling the OIDC userinfo stage.
In `@selfservice/strategy/oidc/sber_mock_server_test.go`:
- Around line 37-125: Handlers for routes "/authorize", "/token", "/userinfo"
and "/auth/completed" currently call require.* (e.g., require.Equal,
require.NotEmpty, require.NoError) inside server goroutines which can call
FailNow; instead modify those handlers in sber_mock_server_test.go to avoid
require.*: validate inputs and on error write appropriate HTTP error responses
(e.g., 400/500) or record failures into a thread-safe field on the mock (e.g.,
add an errors []error or lastError protected by m.mu) and return normal
responses when valid; then update the test code that makes requests to check
response status/body or inspect m.errors/m.lastError after the request to assert
failures; update references in handlers that touch m.nonceByCode, m.subByCode,
makeUnsignedJWT and m.authDone to continue using m.mu for concurrency while not
calling require inside the handlers.
- Around line 65-97: The /token mock handler currently issues tokens for any
POST with code and client_id; tighten it by validating PKCE and required fields:
in the router.HandleFunc("/token") handler verify grant_type ==
"authorization_code", require non-empty redirect_uri and code_verifier (use
r.ParseForm() and r.Form.Get), and if the mock stores a code challenge (e.g.,
m.codeChallengeByCode[code]) validate the code_verifier against that challenge
(support S256 transform -> base64url) before issuing the token (you can still
use m.nonceByCode and m.subByCode to look up nonce/sub and return id_token via
makeUnsignedJWT only after these checks pass).
In `@selfservice/strategy/oidc/strategy.go`:
- Around line 1017-1055: In confirmSberAuthorizationCompleted, add a short
comment immediately above the if mtlsErr != nil block clarifying that
withSberMTLS is best-effort and will return the original transport on error (so
mtlsErr does not block the request); mention that cl.Transport is set to the
returned transport whether or not certificate loading succeeded and that the
code intentionally continues to allow the request to proceed without mTLS if
attachment fails.
---
Nitpick comments:
In `@certs/cacerts.cer`:
- Around line 1-80: The repo currently contains embedded Sber test certificates
(the contents of cacerts.cer) and the code falls back from SBER_MTLS_CERT_PATH /
SBER_MTLS_KEY_PATH to local files (certs/client_cert.crt, certs/file.crt.pem)
and then to these embedded test certs; update the TLS loading logic so that when
running in production (e.g., NODE_ENV=production or a new SBER_MTLS_REQUIRED
flag) the process must require valid production certs from SBER_MTLS_CERT_PATH
and SBER_MTLS_KEY_PATH and exit with an error if they are missing, stop silently
falling back to embedded test certs in functions/modules that reference
SBER_MTLS_CERT_PATH, SBER_MTLS_KEY_PATH and the local paths, and move or remove
the embedded test certificates (cacerts.cer) from the production branch or guard
their use behind an explicit development-only flag and document the required env
vars for production.
In `@selfservice/strategy/oidc/provider_private_net_test.go`:
- Line 85: Add a real Sber case to the provider_private_net_test suite by
constructing a Sber provider instance (using the same constructor used elsewhere
in the file—e.g., the OpenID provider factory/constructor function used by other
tests) with the Sber config so its fixed token and userinfo URLs are actually
created, then run the same private-network assertions used for other providers
on those fixed token/userinfo endpoints (i.e., assert that the fixed token URL
and userinfo URL for "Sber" are subject to the same private-network restrictions
as other providers). Include a subtest named like "sber fixed token/userinfo
URLs" so it’s clear and consistent with the existing test patterns.
In `@selfservice/strategy/oidc/provider_sber_ift.go`:
- Around line 91-287: The Claims method in ProviderSberIft duplicates logic
found in ProviderSber.Claims (timeout/OAuth2 setup, mTLS attachment, userinfo
HTTP request, response handling, JSON decode and claim normalization); extract
the shared behavior into a new helper (e.g. sberFetchUserinfoClaims(ctx, reg,
config, exchange, userinfoURL, issuer)) implemented in provider_sber_common.go
and have both ProviderSber.Claims and ProviderSberIft.Claims call it; move
common pieces such as OAuth2 retrieval (OAuth2()), mTLS setup
(withSberMTLS/baseRoundTripper usage), request creation/headers, response status
handling, JSON decoding into the helper and return a *Claims so
provider-specific caller only passes userinfoURL and issuer and performs any
tiny provider-specific tweaks.
In `@selfservice/strategy/oidc/provider_sber_test.go`:
- Around line 36-44: The test TestProviderSber_AuthCodeURLOptions only checks
that AuthCodeURLOptions returns something non-empty; update it to assert
specific expected parameters are present by calling
p.(oidc.OAuth2Provider).AuthCodeURLOptions(nil) and checking the returned
options contain the expected keys/values (e.g., a "nonce" parameter is included
and non-empty, and "client_type" equals the expected value for ProviderSber),
and optionally verify any other required OAuth2 params; modify assertions to
inspect the options map/slice returned rather than only using assert.NotEmpty.
In `@selfservice/strategy/oidc/provider_sber_token_logging.go`:
- Around line 123-219: The RoundTrip function on sberTokenLoggingTransport
currently logs full headers and raw bodies (requestBodyRaw, responseBody,
requestForm) at Debug which can leak secrets; modify RoundTrip so that full
body/header dumps (request_body_raw, response_raw, request_headers,
response_headers, request_form) are logged only at Trace level (use t.trace
presence or logger.Trace) and/or replaced with redacted versions for Debug/Error
paths, and implement and call helper redaction routines (e.g.,
redactSensitiveForm(requestForm), redactSensitiveHeaders(clone.Header),
redactSensitiveBody(string(responseBody))) before passing values to
reqLog/respLog; keep non-sensitive metadata (http_method, http_status,
latency_ms, mtls info) at Debug/Error as currently done and attach original
error objects to logs but never include unredacted secret fields in non-Trace
logs.
In `@selfservice/strategy/oidc/provider_sber.go`:
- Around line 53-72: The OAuth2 method in ProviderSber currently hardcodes
AuthURL and TokenURL; modify ProviderSber.OAuth2 to use configurable overrides
like ProviderSberIft does by checking g.config.AuthURL and g.config.TokenURL (or
equivalent fields on the provider config) and falling back to the existing
hardcoded URLs only when those config values are empty; update the
oauth2.Endpoint construction to use the chosen values and adjust the log fields
(e.g., include "auth_url" as well as "token_url") so the effective endpoints are
visible in the debug output.
In `@selfservice/strategy/oidc/sber_claims_normalize_test.go`:
- Around line 22-26: Add more unit tests for normalizeRussianMobilePlus79 in
sber_claims_normalize_test.go to cover all input patterns handled by the
implementation: assert that "89991234567" and "79991234567" and "9991234567"
normalize to "+79991234567", and that invalid inputs and an empty string produce
"". Use the same test style as TestNormalizeRussianMobilePlus79 (t.Parallel(),
require.Equal) and add separate subtests or table-driven cases to clearly name
each scenario while referencing the normalizeRussianMobilePlus79 function.
In `@selfservice/strategy/oidc/sber_mapper_traits.go`:
- Around line 115-146: In claimForTraitKey, the phone branch calls
normalizeRussianMobilePlus79 even when claims.PhoneNumber is empty; change the
case to first check claims.PhoneNumber != "" && isPhoneTraitKey(lk) so it
follows the same pattern as other claims, and only call
normalizeRussianMobilePlus79(claims.PhoneNumber) inside that branch, returning
the normalized value if non-empty; references: claimForTraitKey,
claims.PhoneNumber, isPhoneTraitKey, normalizeRussianMobilePlus79.
- Around line 14-16: Translate the Russian comments above the function
applySberClaimsToMapperTraitsOutput in sber_mapper_traits.go into clear English
(preserving technical meaning) so they explain that the function copies Sber
userinfo strings into identity.traits recursively for nested objects to preserve
casing and provider values (including after Jsonnet and when traits schema has
nested keys); replace or append the English text in place of the existing
Russian comments and keep any code-specific terms (Jsonnet, identity.traits,
userinfo) unchanged for clarity.
In `@selfservice/strategy/oidc/state.go`:
- Around line 73-82: The Sber PKCE diagnostic currently logs at Warn in the
isSberProviderID branch (s.d.Logger()...Warn("Sber PKCE diagnostics during state
generation")), which floods logs on every successful flow start; change the call
from Warn to Debug (or Trace) so it becomes non-alerting (e.g.,
s.d.Logger()....Debug("Sber PKCE diagnostics during state generation")), and
keep all the same fields (flow.GetID, p.Config().ID, p.Config().PKCE,
state.GetPkceVerifier(), len(pkce)) so the message is still available for
troubleshooting; alternatively, wrap the log emission behind a config flag
(e.g., an existing diagnostics toggle) if config-gated behavior is preferred.
In `@selfservice/strategy/oidc/strategy_registration.go`:
- Around line 477-486: The toUpperCamel function currently splits on "_" and
skips empty parts inside the loop, which still iterates over empty entries;
update toUpperCamel to filter out empty parts immediately (e.g., build a new
slice of non-empty parts or use strings.FieldsFunc) before capitalizing to avoid
processing empty strings and make intent explicit; keep the capitalization logic
(strings.ToUpper(p[:1]) + p[1:]) and return strings.Join(filteredParts, "") to
preserve current output like "given__name" -> "GivenName".
In `@selfservice/strategy/oidc/strategy_sber_ift_api_flow_test.go`:
- Around line 131-137: The test branch handling res.StatusCode (using
require.Contains on http.StatusSeeOther and http.StatusOK with variables res and
callbackBody) conflates two distinct flows; split into explicit subtests or add
clear comments to name/describe each path — e.g., create subtests "browser flow
- redirect 303" and "api flow - success 200" that assert the expected status and
payload (for the 200 path assert presence of "session_token" and
"continue_with"), or at minimum add inline comments above the if/res.StatusCode
checks to document which branch corresponds to API vs browser behavior so the
intent is unambiguous.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 46df2335-cc5a-41cc-b94d-58f76f9377ae
⛔ Files ignored due to path filters (2)
certs/file.crt.pemis excluded by!**/*.pemcerts/file.key.pemis excluded by!**/*.pem
📒 Files selected for processing (30)
certs/ca.crtcerts/cacerts.cercerts/client_cert.crtcerts/private.keycerts/russian_trusted_root_ca_pem.crtcerts/russian_trusted_sub_ca_pem.crtselfservice/strategy/oidc/error.goselfservice/strategy/oidc/provider.goselfservice/strategy/oidc/provider_config.goselfservice/strategy/oidc/provider_private_net_test.goselfservice/strategy/oidc/provider_sber.goselfservice/strategy/oidc/provider_sber_common.goselfservice/strategy/oidc/provider_sber_ift.goselfservice/strategy/oidc/provider_sber_ift_test.goselfservice/strategy/oidc/provider_sber_test.goselfservice/strategy/oidc/provider_sber_token_logging.goselfservice/strategy/oidc/sber_case_debug.goselfservice/strategy/oidc/sber_claims_normalize.goselfservice/strategy/oidc/sber_claims_normalize_test.goselfservice/strategy/oidc/sber_mapper_traits.goselfservice/strategy/oidc/sber_mapper_traits_test.goselfservice/strategy/oidc/sber_mock_server_test.goselfservice/strategy/oidc/sber_phone_format.goselfservice/strategy/oidc/sber_phone_format_test.goselfservice/strategy/oidc/state.goselfservice/strategy/oidc/strategy.goselfservice/strategy/oidc/strategy_login.goselfservice/strategy/oidc/strategy_registration.goselfservice/strategy/oidc/strategy_sber_ift_api_flow_test.goselfservice/strategy/oidc/strategy_settings.go
Comment on lines
+1
to
+50
| Bag Attributes | ||
| localKeyID: E2 25 74 6C 41 B7 A1 77 0E 1A FA A3 E9 1B 1F C5 0C D9 1F 83 | ||
| friendlyName: ede958f8-ea14-4287-ad8b-db0dfb10194e | ||
| subject=C=RU, ST=G.MOSKVA, L=G.MOSKVA, O=SFUTSK BLAGODARYA, OU=7736372057, OU=CI02440297, OU=sberid-client-1y, OU=sberid, CN=ede958f8-ea14-4287-ad8b-db0dfb10194e | ||
| issuer=C=RU, O=Sberbank of Russia, CN=SberCA Test1 Ext | ||
| -----BEGIN CERTIFICATE----- | ||
| MIIH7jCCBdagAwIBAgIUaZQLP+xefNUT4sICYhqx7ylo+LAwDQYJKoZIhvcNAQEL | ||
| BQAwRTELMAkGA1UEBhMCUlUxGzAZBgNVBAoMElNiZXJiYW5rIG9mIFJ1c3NpYTEZ | ||
| MBcGA1UEAwwQU2JlckNBIFRlc3QxIEV4dDAeFw0yNjA0MDIwNjIyNDRaFw0yNzA0 | ||
| MDIwNjI3NDRaMIHUMQswCQYDVQQGEwJSVTERMA8GA1UECBMIRy5NT1NLVkExETAP | ||
| BgNVBAcTCEcuTU9TS1ZBMRowGAYDVQQKExFTRlVUU0sgQkxBR09EQVJZQTETMBEG | ||
| A1UECxMKNzczNjM3MjA1NzETMBEGA1UECxMKQ0kwMjQ0MDI5NzEZMBcGA1UECxMQ | ||
| c2JlcmlkLWNsaWVudC0xeTEPMA0GA1UECxMGc2JlcmlkMS0wKwYDVQQDEyRlZGU5 | ||
| NThmOC1lYTE0LTQyODctYWQ4Yi1kYjBkZmIxMDE5NGUwggEiMA0GCSqGSIb3DQEB | ||
| AQUAA4IBDwAwggEKAoIBAQDEvgomMfzHpBhFfTKWdgxF/gclWEBqDgB/GyavNeay | ||
| CdaB4Wv9269HhQM9kaGWO2H7qUNjvwaX0qrA+IBAAOwsRwiAFkhdjPJLsCpmAOT7 | ||
| uB8euSKyDAIGoUg69oexBrhTMzeAwhE4GT+y3FfB/idYk07bB2xOV/29ztuxIqJA | ||
| eONc0XhTI8PJoDJSOURSHlUXprQvGJpLVW5D90aXBRl+5VYWWtWQH4ptsi2yPHfJ | ||
| Vjx3m+1YtvaS6H1N6hft0nYRVUwt7cQmvDOBRtEPceswiSh9p9/5xUibz0dMF9gG | ||
| 6kFx/aH6jE1cqeWT4rf7QHjwcb63C76kHA20PffWnXDxAgMBAAGjggNEMIIDQDAJ | ||
| BgNVHRMEAjAAMB0GA1UdDgQWBBTiJXRsQbehdw4a+qPpGx/FDNkfgzCBggYDVR0j | ||
| BHsweYAU4MVo9rlGFWWVNT0/YvUPGdxTPomhTqRMMEoxCzAJBgNVBAYTAlJVMRsw | ||
| GQYDVQQKDBJTYmVyYmFuayBvZiBSdXNzaWExHjAcBgNVBAMMFVNiZXJDQSBUZXN0 | ||
| MSBSb290IEV4dIIRAMIMwGCgJtPpMU5YMhwleeMwggGDBggrBgEFBQcBAQSCAXUw | ||
| ggFxMFEGCCsGAQUFBzAChkVodHRwOi8vc2JlcmNhLXByb3h5LWlmdC5kZWx0YS5z | ||
| YnJmLnJ1L3NiZXJjYS9haWEvc2JlcmNhLXRlc3QxLWV4dC5jcnQwSwYIKwYBBQUH | ||
| MAKGP2h0dHA6Ly9zYmVyY2EtcHJveHktaWZ0LnNiZXIucnUvc2JlcmNhL2FpYS9z | ||
| YmVyY2EtdGVzdDEtZXh0LmNydDBNBggrBgEFBQcwAoZBaHR0cDovL2hhcHJveHkt | ||
| ZWR6MS5zaWdtYS5zYnJmLnJ1L3NiZXJjYS9haWEvc2JlcmNhLXRlc3QxLWV4dC5j | ||
| cnQwQgYIKwYBBQUHMAGGNmh0dHA6Ly9zYmVyY2EtcHJveHktaWZ0LmRlbHRhLnNi | ||
| cmYucnUvc2JlcmNhLXRlc3QxLWV4dDA8BggrBgEFBQcwAYYwaHR0cDovL3NiZXJj | ||
| YS1wcm94eS1pZnQuc2Jlci5ydS9zYmVyY2EtdGVzdDEtZXh0MA4GA1UdDwEB/wQE | ||
| AwIHgDAWBgNVHSUBAf8EDDAKBggrBgEFBQcDAjCB3wYDVR0fBIHXMIHUMIHRoIHO | ||
| oIHLhkVodHRwOi8vc2JlcmNhLXByb3h5LWlmdC5kZWx0YS5zYnJmLnJ1L3NiZXJj | ||
| YS9jZHAvc2JlcmNhLXRlc3QxLWV4dC5jcmyGP2h0dHA6Ly9zYmVyY2EtcHJveHkt | ||
| aWZ0LnNiZXIucnUvc2JlcmNhL2NkcC9zYmVyY2EtdGVzdDEtZXh0LmNybIZBaHR0 | ||
| cDovL2hhcHJveHktZWR6MS5zaWdtYS5zYnJmLnJ1L3NiZXJjYS9jZHAvc2JlcmNh | ||
| LXRlc3QxLWV4dC5jcmwwDQYJKoZIhvcNAQELBQADggIBAErVeWOsvYZFObiRYYK/ | ||
| IyZ0+tZsPS8ThFxHoCIgwU1aCffX0kvzDRjakbHOKdLhYElTCbzTmoVPCwQv3aEu | ||
| 4PP6WD/ZtXnaepi5wAOvTyl5U5WW4W4p8kUof/G+lEBuJp+I+E0ZqZb/UhQj5R+g | ||
| hmKU88rTPV5xdEmTizrfeS2FeUewrrZWpekPJ7QktdljI755ZFC91nojtQW8W8Nm | ||
| ThgyVnazzKRkAeBgXXgceUleRHP//bAepX+7yiHVFdICkESPybEPP1LptW0oRDNI | ||
| oIrd7CGcewYSFupY79Q4hhRS5Ho/esxKuMHhzmzCloIFH6d1ywP9CzyiX4E4wu5w | ||
| 1fjHk+iFYll/zgC33pOx+iT97n+uVa7750H4Ab6MQGoMAQ7tKRFGuKFT27s4tVt/ | ||
| 40ls3OAgbBUD9tmkgR6iU6a6Tk6VCZp8rTNqtkwUXYLs7WEZyAExsFQMnOzgS6fD | ||
| IvPQyonhJCzXjKw/X+4Pe5ULKLkj9fNWhQi4wmmF8/Bwb0V++z0ZXfTATuhut/XO | ||
| t/+MgMsUv8YWB3jWAZhaF45JpLn6u91gg5HcnhtOlFCaQ68bKC0ilFxzpkbFsFqW | ||
| cQsVKhHgHIdeHW9iWlPLiC2DmqmuhBleOKlMQUwgjzgfO0Kdb2NnfHGlWlBq1tA8 | ||
| KRIi8ZNErdhx2xnyT9BVoK2Q | ||
| -----END CERTIFICATE----- |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if a private key file is committed to the repository
fd -e key -e pem . certs/
rg -l 'PRIVATE KEY' certs/
cat certs/private.key 2>/dev/null | head -5 || echo "File not found or not readable"Repository: ory/kratos
Length of output: 353
Critical: Client certificate AND private key committed to repository.
The repository contains both the client certificate and its corresponding private key in plain text:
certs/client_cert.crt(the certificate)certs/private.key(the private key with identifying localKeyID)
This is a critical security vulnerability. These credentials must be removed from version control immediately:
- Force-push to remove these files from all commits using
git filter-branchor similar tools - Rotate/revoke the certificate and key pair with the issuing CA (SberCA Test1 Ext)
- Never commit certificates or private keys to source control
Load credentials from environment variables, secrets management systems, or mounted volumes instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@certs/client_cert.crt` around lines 1 - 50, The commit exposes a private key
and client certificate (files certs/client_cert.crt and certs/private.key—note
localKeyID E2:25:74:6C:41:B7:A1:77:0E:1A:FA:A3:E9:1B:1F:C5:0C:D9:1F:83 and
friendlyName ede958f8-ea14-4287-ad8b-db0dfb10194e) and must be removed and
rotated: remove the files from the repo history using a history-rewriting tool
(git filter-repo or git filter-branch) and force-push, add certs/private.key and
certs/client_cert.crt to .gitignore, notify the CA (SberCA Test1 Ext) to
revoke/rotate the certificate/key pair, and update code to load credentials from
a secure source (environment/secrets manager or mounted volume) instead of
committing them.
Comment on lines
+1
to
+32
| Bag Attributes | ||
| localKeyID: E2 25 74 6C 41 B7 A1 77 0E 1A FA A3 E9 1B 1F C5 0C D9 1F 83 | ||
| friendlyName: ede958f8-ea14-4287-ad8b-db0dfb10194e | ||
| Key Attributes: <No Attributes> | ||
| -----BEGIN PRIVATE KEY----- | ||
| MIIEvwIBADANBgkqhkiG9w0BAQEFAASCBKkwggSlAgEAAoIBAQDEvgomMfzHpBhF | ||
| fTKWdgxF/gclWEBqDgB/GyavNeayCdaB4Wv9269HhQM9kaGWO2H7qUNjvwaX0qrA | ||
| +IBAAOwsRwiAFkhdjPJLsCpmAOT7uB8euSKyDAIGoUg69oexBrhTMzeAwhE4GT+y | ||
| 3FfB/idYk07bB2xOV/29ztuxIqJAeONc0XhTI8PJoDJSOURSHlUXprQvGJpLVW5D | ||
| 90aXBRl+5VYWWtWQH4ptsi2yPHfJVjx3m+1YtvaS6H1N6hft0nYRVUwt7cQmvDOB | ||
| RtEPceswiSh9p9/5xUibz0dMF9gG6kFx/aH6jE1cqeWT4rf7QHjwcb63C76kHA20 | ||
| PffWnXDxAgMBAAECggEBAIR7P9RWhtRcoGdylfURitQ66c7w7Xc89IKi4trLHgy3 | ||
| aTXOeOzZ2N79J6B3B2tlk2ZmpVVuld74YjlNXPc8Z8ytDIFL//DW73WeK/7CDW+f | ||
| nX0Px9hDE74pLr1dsyO21bpY28AdboDrJ6SmkYW1QgN4NnpxNjJPODNvLyrJmp50 | ||
| WpwBR5tbsjgaMRyKVyTmtp0DjFvfP8IZK9QmoP9cV8GEZ0oAbW0mxF3AYvcgFsFr | ||
| yobiyjwCb/8dVSpsQyQdN3J3M+sLCQK7vtITBrlV2m4LmpkiFY1KdmUDHOdeUC37 | ||
| VtFNPV/OOjLp7q0E9Rjgmhite8rBr3gwxLyCUVPbzF0CgYEA6K9wlsYicSdzXjtP | ||
| eg8PqTMP4EMgb4SWFo71bSTooqaXWpnGzSr8HAkFt9/COVMuoYabgwPUjG+mXZ8E | ||
| /Ey5h6hJnqPOBWSJYK4wkvv/MHlJzNpRXBOIbT/mD2Vi9tDngzQeTXuemZtBuV6I | ||
| g4gv9aLgYzh2JD9qeHl9t6B7t6MCgYEA2HSic3UZDQwEIkOeRRVObSDvY167KTs2 | ||
| dXywd7HVDG2UGtsJP9NBh3Y3kVa2+u+cayHWcHPkZ36/lBRy8tLQnqNXS9zFFRhK | ||
| kD4kwe23N13KnGOrQzePhBBtNniscqTXBFwAgeHCze2qx/gM6y3bB1X+P9cTE1Nd | ||
| b3JRTbWKzlsCgYEAp8IlOG8tUcuRn/S+/k9xiRmpbpS3A+/hje4QAFrF5s6Y/Nc1 | ||
| v6IoFcZjewg2LcJNMmOsJy9RxNaSaZlGrOhcMvQf7+JFnRm4+h1cI/zPJZGspacZ | ||
| VXs3txyEr8D3Mt+2qp+e4VopJLINFqqTXdGIUl7VzHNeqg+Wobll7EgmKmUCgYEA | ||
| gmNP8GjbXEaevt0om8jH42jxi1RnPeETXxZrXs7a3Y+spbjIC5CAas9FjeFEfEiW | ||
| WtqZSEgnkEiDsvnWfHuNe+I9Fc+5UIm/cMBeeAtwUIPJJwfLBMSVSSJ0B1oN10mA | ||
| 1HlvPM34AQBn3emILqsCw5qDe4VdUkjngdjFLSBsqv0CgYBz5wKEeikHMrdSfMUN | ||
| CRvR/ivt+VIp2nVEupmUo4WZFjzDrvQVVW/yobKkSCYxothETjDahoKo6wQ5xYe+ | ||
| Fk/ScnfcTMdbl9FUHnw7SK3kZ9IbzFZD2PTh7g/ZIc1nnsuOye3s7r+52SLtmuJq | ||
| y2/etSfNii1ilJseT+mMcbiP3g== | ||
| -----END PRIVATE KEY----- |
There was a problem hiding this comment.
Remove this committed private key before merge.
This is real private key material, not a placeholder. Once merged, it becomes long-lived secret material in repository history and can be copied into downstream environments or tooling by accident. Please remove it from the tree, rotate/revoke anything derived from it, and generate test fixtures at runtime or keep clearly dummy fixtures under dedicated test data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@certs/private.key` around lines 1 - 32, The repository contains a real
private key block (-----BEGIN PRIVATE KEY----- ... -----END PRIVATE KEY-----)
with identifiers like friendlyName "ede958f8-ea14-4287-ad8b-db0dfb10194e" and
localKeyID that must be removed; delete the committed key file from the tree,
remove it from history (git filter-repo or BFG) or replace the commit with a
history-rewriting purge, add the file pattern to .gitignore to prevent
re-commits, rotate/revoke any certificates/keys that used this private key, and
replace the file in tests with a generated runtime fixture or a clearly dummy
key (not the real material) so no sensitive key material remains in the repo or
its history.
Comment on lines
+61
to
+76
| func safeBodyForLog(body []byte, limit int) string { | ||
| if len(body) == 0 { | ||
| return "" | ||
| } | ||
|
|
||
| if limit <= 0 { | ||
| limit = maxUpstreamBodyLogBytes | ||
| } | ||
| if len(body) > limit { | ||
| body = body[:limit] | ||
| } | ||
|
|
||
| safe := strings.ReplaceAll(string(body), "\n", " ") | ||
| safe = strings.ReplaceAll(safe, "\r", " ") | ||
| safe = sensitiveFragmentRE.ReplaceAllString(safe, `$1$2$3***`) | ||
| return strings.TrimSpace(safe) |
There was a problem hiding this comment.
Apply the new sanitizer to the generic upstream logger too.
safeBodyForLog is only used by extractOAuth2RetrieveError; logUpstreamError at Lines 33-44 still logs the raw upstream body. Any non-OAuth2 caller that hits that path can still leak tokens or client secrets into logs.
🔒 Suggested fix
func logUpstreamError(l *logrusx.Logger, resp *http.Response) error {
if resp.StatusCode == http.StatusOK {
return nil
}
- body, err := io.ReadAll(io.LimitReader(resp.Body, 1024))
+ body, err := io.ReadAll(io.LimitReader(resp.Body, maxUpstreamBodyLogBytes))
if err != nil {
l = l.WithError(err)
}
- l.WithField("response_code", resp.StatusCode).WithField("response_body", string(body)).Error("The upstream OIDC provider returned a non 200 status code.")
+ l.WithField("response_code", resp.StatusCode).
+ WithField("response_body", safeBodyForLog(body, maxUpstreamBodyLogBytes)).
+ Error("The upstream OIDC provider returned a non 200 status code.")
return errors.WithStack(herodot.ErrUpstreamError.WithReasonf("OpenID Connect provider returned a %d status code but 200 is expected.", resp.StatusCode))
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@selfservice/strategy/oidc/error.go` around lines 61 - 76, logUpstreamError
currently logs the raw upstream response body and can leak secrets; update
logUpstreamError to pass the response body through safeBodyForLog (the same
sanitizer used by extractOAuth2RetrieveError) before logging, so
sensitiveFragmentRE masking and newline/CR cleanup are applied; locate
logUpstreamError and replace direct body logging with safeBodyForLog(body, 0)
(or an explicit limit) and ensure the log message uses the sanitized string.
Comment on lines
+237
to
+238
| "sber": NewProviderSber, | ||
| "sber-ift": NewProviderSberIft, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if provider_private_net_test.go exists and contains tests for sber providers
fd 'provider_private_net_test.go' --type f --exec cat {} | rg -i 'sber' || echo "No sber references found"Repository: ory/kratos
Length of output: 115
🏁 Script executed:
# Read the provider_config.go file around lines 205-208 to see the warning
head -n 250 selfservice/strategy/oidc/provider_config.go | tail -n 50Repository: ory/kratos
Length of output: 1833
🏁 Script executed:
# Search for test functions in provider_private_net_test.go
fd 'provider_private_net_test.go' --type f -exec grep -n "func Test" {} +Repository: ory/kratos
Length of output: 284
🏁 Script executed:
# Check the entire provider_private_net_test.go file to see if sber tests exist
fd 'provider_private_net_test.go' --type f -exec wc -l {} +Repository: ory/kratos
Length of output: 240
🏁 Script executed:
# Find and read the test file
fd 'provider_private_net_test.go' -type fRepository: ory/kratos
Length of output: 224
🏁 Script executed:
# Read the full test file to check for sber tests
fd 'provider_private_net_test.go' -type f -exec cat {} \;Repository: ory/kratos
Length of output: 224
🏁 Script executed:
# Find the test file with correct syntax
fd 'provider_private_net_test.go' --type fRepository: ory/kratos
Length of output: 108
🏁 Script executed:
# Read the test file content
fd 'provider_private_net_test.go' --type f --exec cat {} \;Repository: ory/kratos
Length of output: 5277
Add test cases for sber and sber-ift providers to provider_private_net_test.go.
The warning at lines 205-208 of provider_config.go requires tests for new providers. While provider_private_net_test.go contains a comment explaining Sber's fixed token URL behavior, the test case table in TestProviderPrivateIP lacks actual test entries for both sber and sber-ift providers. Add them alongside other provider test cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@selfservice/strategy/oidc/provider_config.go` around lines 237 - 238, The
test table in TestProviderPrivateIP (provider_private_net_test.go) is missing
entries for the new providers referenced by NewProviderSber and
NewProviderSberIft in provider_config.go; add two test cases to that table for
"sber" and "sber-ift" (matching the comment about Sber's fixed token URL
behavior) so the test covers their expected private-net/token URL behavior,
ensuring the test names/keys match the provider IDs used by the provider factory
and that any expected token URL or private-net flag values are set appropriately
in each row.
Comment on lines
+43
to
+46
| func validateSberIDToken(token *oauth2.Token, providerID, clientID string, flowID uuid.UUID) error { | ||
| if flowID == uuid.Nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Skipping validation when flowID is nil may mask configuration issues.
When flowID == uuid.Nil, the function returns nil (no error), completely bypassing ID token validation. If this state occurs unexpectedly (e.g., due to a bug in flow ID propagation), invalid or malicious tokens would be accepted silently.
Consider logging a warning when this happens, or returning an error for flows that should always have a valid flow ID.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@selfservice/strategy/oidc/provider_sber_common.go` around lines 43 - 46, The
current early-return in validateSberIDToken when flowID == uuid.Nil silently
skips ID token validation; change this to surface the issue by returning a
descriptive error (e.g., "missing flowID: cannot validate ID token") and emit a
warning log so mis-propagated/nil flow IDs are visible; update the
validateSberIDToken function to remove the nil-return, log a warning referencing
flowID, providerID and clientID, and return the error so callers cannot
accidentally accept unvalidated tokens.
Comment on lines
+351
to
+370
| if t != nil { | ||
| clone := t.Clone() | ||
| if clone.TLSClientConfig == nil { | ||
| clone.TLSClientConfig = &tls.Config{} | ||
| } else { | ||
| clone.TLSClientConfig = clone.TLSClientConfig.Clone() | ||
| } | ||
| clone.TLSClientConfig.Certificates = append(clone.TLSClientConfig.Certificates, clientCert) | ||
| return clone, certPath, keyPath, fmt.Sprintf("%T", rt), nil | ||
| } | ||
|
|
||
| // Base is not *http.Transport (wrappers/custom RT). Build dedicated mTLS transport to guarantee cert is sent. | ||
| mtlsTransport := http.DefaultTransport.(*http.Transport).Clone() | ||
| if mtlsTransport.TLSClientConfig == nil { | ||
| mtlsTransport.TLSClientConfig = &tls.Config{} | ||
| } else { | ||
| mtlsTransport.TLSClientConfig = mtlsTransport.TLSClientConfig.Clone() | ||
| } | ||
| mtlsTransport.TLSClientConfig.Certificates = append(mtlsTransport.TLSClientConfig.Certificates, clientCert) | ||
| return mtlsTransport, certPath, keyPath, fmt.Sprintf("%T", rt), nil |
There was a problem hiding this comment.
Set TLS MinVersion to prevent downgrade attacks.
Static analysis correctly flagged that tls.Config{} is created without specifying MinVersion. This defaults to TLS 1.0 for servers and TLS 1.2 for clients. For security best practices, explicitly set MinVersion: tls.VersionTLS12 (or tls.VersionTLS13 if the Sber endpoints support it).
🔒 Proposed fix
if t != nil {
clone := t.Clone()
if clone.TLSClientConfig == nil {
- clone.TLSClientConfig = &tls.Config{}
+ clone.TLSClientConfig = &tls.Config{MinVersion: tls.VersionTLS12}
} else {
clone.TLSClientConfig = clone.TLSClientConfig.Clone()
}
+ if clone.TLSClientConfig.MinVersion == 0 {
+ clone.TLSClientConfig.MinVersion = tls.VersionTLS12
+ }
clone.TLSClientConfig.Certificates = append(clone.TLSClientConfig.Certificates, clientCert)
return clone, certPath, keyPath, fmt.Sprintf("%T", rt), nil
}
// Base is not *http.Transport (wrappers/custom RT). Build dedicated mTLS transport to guarantee cert is sent.
mtlsTransport := http.DefaultTransport.(*http.Transport).Clone()
if mtlsTransport.TLSClientConfig == nil {
- mtlsTransport.TLSClientConfig = &tls.Config{}
+ mtlsTransport.TLSClientConfig = &tls.Config{MinVersion: tls.VersionTLS12}
} else {
mtlsTransport.TLSClientConfig = mtlsTransport.TLSClientConfig.Clone()
}
+ if mtlsTransport.TLSClientConfig.MinVersion == 0 {
+ mtlsTransport.TLSClientConfig.MinVersion = tls.VersionTLS12
+ }🧰 Tools
🪛 ast-grep (0.42.1)
[warning] 353-353: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
[warning] 364-364: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@selfservice/strategy/oidc/provider_sber_token_logging.go` around lines 351 -
370, The cloned TLS configs created when wrapping the original transport (the
`clone` variable derived from `t.Clone()` and the `mtlsTransport` created from
`http.DefaultTransport.(*http.Transport).Clone()`) do not set a minimum TLS
version; update both `clone.TLSClientConfig` and `mtlsTransport.TLSClientConfig`
to include `MinVersion: tls.VersionTLS12` (or `tls.VersionTLS13` if Sber
supports it) when initializing or cloning the `tls.Config` before appending
`Certificates` so the transport enforces a minimum TLS version.
Comment on lines
+130
to
+139
| if err != nil { | ||
| g.reg.Logger(). | ||
| WithError(err). | ||
| WithField("oidc_provider", g.config.ID). | ||
| WithField("oidc_stage", "userinfo_claims"). | ||
| WithField("request_id", requestID). | ||
| WithField("userinfo_url", userinfoURL). | ||
| WithField("latency_ms", time.Since(stageStart).Milliseconds()). | ||
| Debug("OIDC userinfo request failed") | ||
| return nil, errors.WithStack(herodot.ErrUpstreamError.WithWrap(err).WithReasonf("%s", err)) |
There was a problem hiding this comment.
Log level should be Error for failed requests, not Debug.
Line 138 logs userinfo request failures at Debug level, but this represents an actual error condition that should be visible in production logs.
🐛 Proposed fix
if err != nil {
g.reg.Logger().
WithError(err).
WithField("oidc_provider", g.config.ID).
WithField("oidc_stage", "userinfo_claims").
WithField("request_id", requestID).
WithField("userinfo_url", userinfoURL).
WithField("latency_ms", time.Since(stageStart).Milliseconds()).
- Debug("OIDC userinfo request failed")
+ Error("OIDC userinfo request failed")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@selfservice/strategy/oidc/provider_sber.go` around lines 130 - 139, The
current userinfo failure log in provider_sber.go uses Debug level; change the
logger call on the error path (the block that checks "if err != nil") to use
Error instead of Debug so failures are visible in production logs; keep the same
context fields (g.config.ID as "oidc_provider", requestID, userinfoURL, latency
computed from stageStart) and the same WithError(err) chain, but call
Error("OIDC userinfo request failed") rather than Debug(...) inside the error
handling in the function handling the OIDC userinfo stage.
Comment on lines
+37
to
+125
| router.HandleFunc("/authorize", func(w http.ResponseWriter, r *http.Request) { | ||
| require.Equal(t, http.MethodGet, r.Method) | ||
| q := r.URL.Query() | ||
| redirectURI := q.Get("redirect_uri") | ||
| state := q.Get("state") | ||
| nonce := q.Get("nonce") | ||
| require.NotEmpty(t, redirectURI) | ||
| require.NotEmpty(t, state) | ||
| require.NotEmpty(t, nonce) | ||
|
|
||
| code := fmt.Sprintf("code-%d", time.Now().UnixNano()) | ||
| sub := "sber-ift-user@example.org" | ||
|
|
||
| m.mu.Lock() | ||
| m.nonceByCode[code] = nonce | ||
| m.subByCode[code] = sub | ||
| m.mu.Unlock() | ||
|
|
||
| target, err := url.Parse(redirectURI) | ||
| require.NoError(t, err) | ||
| query := target.Query() | ||
| query.Set("code", code) | ||
| query.Set("state", state) | ||
| target.RawQuery = query.Encode() | ||
|
|
||
| http.Redirect(w, r, target.String(), http.StatusSeeOther) | ||
| }) | ||
|
|
||
| router.HandleFunc("/token", func(w http.ResponseWriter, r *http.Request) { | ||
| require.Equal(t, http.MethodPost, r.Method) | ||
| require.NoError(t, r.ParseForm()) | ||
|
|
||
| code := r.Form.Get("code") | ||
| clientID := r.Form.Get("client_id") | ||
| require.NotEmpty(t, code) | ||
| require.NotEmpty(t, clientID) | ||
|
|
||
| m.mu.Lock() | ||
| nonce := m.nonceByCode[code] | ||
| sub := m.subByCode[code] | ||
| m.mu.Unlock() | ||
| require.NotEmpty(t, nonce) | ||
| require.NotEmpty(t, sub) | ||
|
|
||
| now := time.Now().Unix() | ||
| idToken := makeUnsignedJWT(map[string]any{ | ||
| "aud": clientID, | ||
| "sub": sub, | ||
| "nonce": nonce, | ||
| "iat": now, | ||
| "exp": now + 300, | ||
| }) | ||
|
|
||
| w.Header().Set("Content-Type", "application/json") | ||
| require.NoError(t, json.NewEncoder(w).Encode(map[string]any{ | ||
| "access_token": "access-" + code, | ||
| "token_type": "bearer", | ||
| "expires_in": 300, | ||
| "id_token": idToken, | ||
| "refresh_token": "refresh-" + code, | ||
| })) | ||
| }) | ||
|
|
||
| router.HandleFunc("/userinfo", func(w http.ResponseWriter, r *http.Request) { | ||
| require.Equal(t, http.MethodGet, r.Method) | ||
| require.True(t, strings.HasPrefix(r.Header.Get("Authorization"), "Bearer access-")) | ||
| require.NotEmpty(t, r.Header.Get("x-introspect-rquid")) | ||
|
|
||
| w.Header().Set("Content-Type", "application/json") | ||
| require.NoError(t, json.NewEncoder(w).Encode(map[string]any{ | ||
| "sub": "sber-ift-user@example.org", | ||
| "email": "sber-ift-user@example.org", | ||
| "phone_number": "+7 (999) 123-45-67", | ||
| "given_name": "ИВАН", | ||
| "family_name": "ИВАНОВ", | ||
| "middle_name": "ИВАНОВИЧ", | ||
| "birthdate": "1990-01-02", | ||
| "gender": 2, | ||
| })) | ||
| }) | ||
|
|
||
| router.HandleFunc("/auth/completed", func(w http.ResponseWriter, r *http.Request) { | ||
| require.Equal(t, http.MethodGet, r.Method) | ||
| require.True(t, strings.HasPrefix(r.Header.Get("authorization"), "Bearer access-")) | ||
| require.NotEmpty(t, r.Header.Get("rquid")) | ||
| m.mu.Lock() | ||
| m.authDone++ | ||
| m.mu.Unlock() | ||
| w.WriteHeader(http.StatusNoContent) |
There was a problem hiding this comment.
Avoid require.* inside httptest handlers.
These handlers run on server goroutines, and require uses FailNow, which is only safe on the test goroutine. That can turn request validation failures into panics or partial test failures instead of deterministic assertions. Return HTTP errors from the handler and assert on the client side, or record failures and check them after the request completes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@selfservice/strategy/oidc/sber_mock_server_test.go` around lines 37 - 125,
Handlers for routes "/authorize", "/token", "/userinfo" and "/auth/completed"
currently call require.* (e.g., require.Equal, require.NotEmpty,
require.NoError) inside server goroutines which can call FailNow; instead modify
those handlers in sber_mock_server_test.go to avoid require.*: validate inputs
and on error write appropriate HTTP error responses (e.g., 400/500) or record
failures into a thread-safe field on the mock (e.g., add an errors []error or
lastError protected by m.mu) and return normal responses when valid; then update
the test code that makes requests to check response status/body or inspect
m.errors/m.lastError after the request to assert failures; update references in
handlers that touch m.nonceByCode, m.subByCode, makeUnsignedJWT and m.authDone
to continue using m.mu for concurrency while not calling require inside the
handlers.
Comment on lines
+65
to
+97
| router.HandleFunc("/token", func(w http.ResponseWriter, r *http.Request) { | ||
| require.Equal(t, http.MethodPost, r.Method) | ||
| require.NoError(t, r.ParseForm()) | ||
|
|
||
| code := r.Form.Get("code") | ||
| clientID := r.Form.Get("client_id") | ||
| require.NotEmpty(t, code) | ||
| require.NotEmpty(t, clientID) | ||
|
|
||
| m.mu.Lock() | ||
| nonce := m.nonceByCode[code] | ||
| sub := m.subByCode[code] | ||
| m.mu.Unlock() | ||
| require.NotEmpty(t, nonce) | ||
| require.NotEmpty(t, sub) | ||
|
|
||
| now := time.Now().Unix() | ||
| idToken := makeUnsignedJWT(map[string]any{ | ||
| "aud": clientID, | ||
| "sub": sub, | ||
| "nonce": nonce, | ||
| "iat": now, | ||
| "exp": now + 300, | ||
| }) | ||
|
|
||
| w.Header().Set("Content-Type", "application/json") | ||
| require.NoError(t, json.NewEncoder(w).Encode(map[string]any{ | ||
| "access_token": "access-" + code, | ||
| "token_type": "bearer", | ||
| "expires_in": 300, | ||
| "id_token": idToken, | ||
| "refresh_token": "refresh-" + code, | ||
| })) |
There was a problem hiding this comment.
Tighten the /token mock so PKCE regressions actually fail the suite.
Right now the token endpoint accepts any POST with code and client_id. A broken exchange that omits grant_type, redirect_uri, or code_verifier would still get a token here, so this test helper won't catch exactly the flow regressions this PR is introducing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@selfservice/strategy/oidc/sber_mock_server_test.go` around lines 65 - 97, The
/token mock handler currently issues tokens for any POST with code and
client_id; tighten it by validating PKCE and required fields: in the
router.HandleFunc("/token") handler verify grant_type == "authorization_code",
require non-empty redirect_uri and code_verifier (use r.ParseForm() and
r.Form.Get), and if the mock stores a code challenge (e.g.,
m.codeChallengeByCode[code]) validate the code_verifier against that challenge
(support S256 transform -> base64url) before issuing the token (you can still
use m.nonceByCode and m.subByCode to look up nonce/sub and return id_token via
makeUnsignedJWT only after these checks pass).
Comment on lines
+1017
to
+1055
| func (s *Strategy) confirmSberAuthorizationCompleted(ctx context.Context, providerID string, accessToken string) error { | ||
| if accessToken == "" { | ||
| return errors.WithStack(herodot.ErrUpstreamError.WithReasonf("sber auth completed failed: missing access token")) | ||
| } | ||
|
|
||
| endpoint := sberAuthCompletedURL(providerID, nil) | ||
| if provider, providerErr := s.Provider(ctx, providerID); providerErr == nil && provider != nil { | ||
| endpoint = sberAuthCompletedURL(providerID, provider.Config()) | ||
| } | ||
| requestID := randx.MustString(32, []rune("0123456789ABCDEF")) | ||
|
|
||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) | ||
| if err != nil { | ||
| return errors.WithStack(herodot.ErrInternalServerError.WithWrap(err).WithReasonf("%s", err)) | ||
| } | ||
| req.Header.Set("authorization", "Bearer "+accessToken) | ||
| req.Header.Set("rquid", requestID) | ||
|
|
||
| cl := s.d.HTTPClient(ctx).HTTPClient | ||
| if cl.Transport == nil { | ||
| cl.Transport = http.DefaultTransport | ||
| } | ||
| mtlsTransport, mtlsCertPath, mtlsKeyPath, mtlsBaseType, mtlsErr := withSberMTLS(cl.Transport) | ||
| cl.Transport = mtlsTransport | ||
| fields := s.d.Logger(). | ||
| WithField("oidc_provider", providerID). | ||
| WithField("oidc_stage", "auth_completed"). | ||
| WithField("sber_debug_version", sberTokenDebugVersion). | ||
| WithField("request_id", requestID). | ||
| WithField("auth_completed_url", endpoint). | ||
| WithField("mtls_cert_path", mtlsCertPath). | ||
| WithField("mtls_key_path", mtlsKeyPath). | ||
| WithField("mtls_base_transport_type", mtlsBaseType) | ||
| if mtlsErr != nil { | ||
| fields.WithError(mtlsErr).Error("Failed to attach mTLS certificate for Sber auth completed request") | ||
| } else { | ||
| fields.Debug("Attached mTLS certificate for Sber auth completed request") | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's documentation or comments about mTLS being optional vs required
rg -n -i "mtls|mTLS" --type=go -C3 selfservice/strategy/oidc/ | head -100Repository: ory/kratos
Length of output: 8699
🏁 Script executed:
# Find the withSberMTLS function definition
rg -n "func withSberMTLS" --type=goRepository: ory/kratos
Length of output: 207
🏁 Script executed:
# Search for any comments about Sber mTLS being optional or required
rg -n "mTLS.*optional|mTLS.*required|optional.*mTLS|required.*mTLS" --type=go -iRepository: ory/kratos
Length of output: 36
🏁 Script executed:
# Look for any early returns or error handling related to mTLS in the entire file
rg -n "withSberMTLS" --type=go -A5 -B5 selfservice/strategy/oidc/Repository: ory/kratos
Length of output: 5208
🏁 Script executed:
# Read the withSberMTLS function implementation
sed -n '329,380p' selfservice/strategy/oidc/provider_sber_token_logging.goRepository: ory/kratos
Length of output: 2137
🏁 Script executed:
# Check if there are any conditional returns based on mtlsErr in confirmSberAuthorizationCompleted
sed -n '1010,1070p' selfservice/strategy/oidc/strategy.goRepository: ory/kratos
Length of output: 2340
Add a comment clarifying that mTLS attachment is best-effort.
The code intentionally continues when withSberMTLS returns an error (line 1048), allowing the request to proceed with the original transport if mTLS fails. While the function design validates this is deliberate—it always returns a working transport (original if cert loading fails, modified if successful)—the intent is not obvious from reading the error log alone. Add a comment above the if mtlsErr != nil block clarifying that mTLS attachment is best-effort and failure does not block the request.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@selfservice/strategy/oidc/strategy.go` around lines 1017 - 1055, In
confirmSberAuthorizationCompleted, add a short comment immediately above the if
mtlsErr != nil block clarifying that withSberMTLS is best-effort and will return
the original transport on error (so mtlsErr does not block the request); mention
that cl.Transport is set to the returned transport whether or not certificate
loading succeeded and that the code intentionally continues to allow the request
to proceed without mTLS if attachment fails.
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
2 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.
Summary by CodeRabbit