feat: Добавление ТокенДоступа (AccessToken) #1571
Conversation
WalkthroughThis PR adds JWT token support to OneScript's HTTP library, introducing an AccessTokenContext class for creating and signing tokens with multiple algorithms. HTTP methods are updated to accept an IBslProcess parameter and integrate token handling, with new test coverage validating token creation, signing, and HTTP transmission. Changes
Sequence DiagramsequenceDiagram
participant Client as Client Script
participant HTTP as HttpConnectionContext
participant Token as AccessTokenContext
participant Request as HttpRequestContext
participant Handler as JwtSecurityTokenHandler
participant WebReq as HttpWebRequest
Client->>Token: Constructor()
Token-->>Client: AccessTokenContext instance
Client->>Token: Add Headers/Payload
Note over Token: Store metadata (LifeTime, Issuer, Recipients)
Client->>Token: Sign(process, algorithm, secretKey)
activate Token
Token->>Token: BuildToken()
Note over Token: Populate claims<br/>(iss, iat, nbf, exp, aud)
Token->>Token: ResolveSigningCredentials(algorithm, key)
Note over Token: Load PEM key<br/>Map to SecurityKey
Token->>Handler: CreateToken(descriptor)
Handler-->>Token: JwtSecurityToken
Token->>Handler: WriteToken(jwtToken)
Handler-->>Token: Signed token string
Token->>Token: _isSigned = true
deactivate Token
Client->>Request: AddAccessToken(token)
Request->>Request: AccessToken = token
Client->>HTTP: Get(process, request)
activate HTTP
HTTP->>Request: (extract AccessToken)
HTTP->>Token: ToString(process)
Token-->>HTTP: Signed token string
HTTP->>HTTP: SetRequestHeaders(request, webRequest, process)
Note over HTTP: Set Authorization header<br/>Bearer {token}
HTTP->>WebReq: Add headers & send
deactivate HTTP
HTTP-->>Client: HttpResponseContext
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 4
🧹 Nitpick comments (9)
tests/http.os (1)
374-389: Solid integration test; consider tightening assertion and improving failure diagnostics
- You assert substring presence for Authorization; consider asserting exact equality to catch subtle formatting issues.
- If the header is absent, indexing will throw; adding a pre-check can yield a clearer test failure message.
You can optionally refine the assertions like this:
- юТест.ПроверитьВхождение(JsonВОбъект(ТелоОтвета)["headers"]["Authorization"], "Bearer " + Токен); + Заг = JsonВОбъект(ТелоОтвета)["headers"]; + юТест.ПроверитьИстину(Заг.Свойство("Authorization"), "Заголовок Authorization отсутствует в ответе"); + юТест.ПроверитьРавенство("Bearer " + Токен, Заг["Authorization"]);src/OneScript.StandardLibrary/Http/HttpRequestContext.cs (2)
48-49: AccessToken property: consider exposing read-only to scripts (optional)Currently the token is only settable via the context method. If you want scripts to inspect the attached token, expose it as a read-only ContextProperty.
Example:
- public AccessTokenContext AccessToken { get; private set; } + [ContextProperty("ТокенДоступа", "AccessToken")] + public AccessTokenContext AccessToken { get; private set; }
121-129: Docstring is misleading: header is applied later (in connection), not hereThe method only stores the token; the Authorization header is (per PR) applied by HttpConnectionContext during request execution. Adjust the XML comment to avoid confusion.
- /// Добавляет токен доступа к HTTP-запросу. Устанавливает заголовок Authorization. + /// Добавляет токен доступа к HTTP-запросу. + /// Заголовок Authorization (Bearer ...) будет установлен при отправке запроса.src/OneScript.StandardLibrary/Security/Tokens/AccessTokenContext.cs (3)
148-153: Set temporal claims via SecurityTokenDescriptor to ensure correct typing and avoid duplicationRelying on descriptor fields produces proper numeric claims (iat/nbf/exp) and avoids duplication with manual claims. You can still keep AddStandardClaims for other fields.
Apply within the tokenDescriptor initialization:
var tokenDescriptor = new SecurityTokenDescriptor { Subject = new ClaimsIdentity(claims), AdditionalInnerHeaderClaims = GetHeaderClaims(process), - SigningCredentials = GetSigningCredentials(algorithm, secretKey) + SigningCredentials = GetSigningCredentials(algorithm, secretKey), + IssuedAt = CreationTime != 0 + ? DateTimeOffset.FromUnixTimeSeconds(CreationTime).UtcDateTime + : (DateTime?)null, + NotBefore = CreationTime != 0 + ? DateTimeOffset.FromUnixTimeSeconds(CreationTime).UtcDateTime + : (DateTime?)null, + Expires = (CreationTime != 0 || LifeTime != 0) + ? DateTimeOffset.FromUnixTimeSeconds((long)CreationTime + LifeTime).UtcDateTime + : (DateTime?)null };As a follow-up, remove the manual iat/nbf/exp additions in AddStandardClaims (see below).
166-188: Temporal claims: ensure numeric type and avoid empty-string claims
- Use long arithmetic for Unix time to avoid overflow.
- Avoid adding claims when the value is an empty string to reduce noisy tokens.
- if (CreationTime != 0) - { - claims.Add(new Claim(JwtRegisteredClaimNames.Iat, CreationTime.ToString(), ClaimValueTypes.Integer64)); - claims.Add(new Claim(JwtRegisteredClaimNames.Nbf, CreationTime.ToString(), ClaimValueTypes.Integer64)); - } - - if (CreationTime != 0 || LifeTime != 0) - { - int expires = CreationTime + LifeTime; - claims.Add(new Claim(JwtRegisteredClaimNames.Exp, expires.ToString(), ClaimValueTypes.Integer64)); - } + if (CreationTime != 0) + { + var iat = ((long)CreationTime).ToString(); + claims.Add(new Claim(JwtRegisteredClaimNames.Iat, iat, ClaimValueTypes.Integer64)); + claims.Add(new Claim(JwtRegisteredClaimNames.Nbf, iat, ClaimValueTypes.Integer64)); + } + if (CreationTime != 0 || LifeTime != 0) + { + long exp = (long)CreationTime + LifeTime; + claims.Add(new Claim(JwtRegisteredClaimNames.Exp, exp.ToString(), ClaimValueTypes.Integer64)); + } - if(_tokenId != null) + if(!string.IsNullOrEmpty(_tokenId)) claims.Add(new Claim(JwtRegisteredClaimNames.Jti, _tokenId)); - if(_userMatchingKey != null) + if(!string.IsNullOrEmpty(_userMatchingKey)) claims.Add(new Claim(JwtRegisteredClaimNames.Sub, _userMatchingKey));If you adopt the descriptor-based approach (previous comment), you can remove the iat/nbf/exp additions entirely.
201-214: Payload values are coerced to strings; consider preserving types (optional)Currently all payload values are stringified. If callers pass numbers/booleans, those become string claims, which may break downstream validators expecting numeric/boolean types.
- Detect OneScript numeric/boolean types and set ClaimValueTypes.Integer64/Boolean accordingly.
- Alternatively, allow passing a pre-serialized payload JSON and inject as raw claims (documented contract).
I can draft a type-preserving implementation if you confirm desired behavior.tests/access-token.os (1)
53-71: ES384/ES512 require P-384/P-521 keys; HS should use a symmetric secret, not an RSA PEM*
- ES384/ES512 with a P-256 (secp256r1) key is not spec-compliant and may be accepted/ignored depending on the underlying implementation. Prefer supplying curve-matching keys or drop those cases.
- HS256/384/512 are HMAC-based and should use a symmetric secret (raw bytes), not an RSA private key PEM. Using an RSA PEM string as the HMAC secret is confusing and fragile.
Proposed changes for HS* to use a clear symmetric secret:
- ТестовыеСлучаи = Новый Соответствие(); - ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.ES256, ПриватныйКлючECDSA); - ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.ES384, ПриватныйКлючECDSA); - ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.ES512, ПриватныйКлючECDSA); - ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.HS256, ПриватныйКлючRSA); - ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.HS384, ПриватныйКлючRSA); - ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.HS512, ПриватныйКлючRSA); + ТестовыеСлучаи = Новый Соответствие(); + // ECDSA: supply curve-matching keys (see note below for ES384/ES512) + ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.ES256, ПриватныйКлючECDSA); + // Consider providing P-384 and P-521 keys for the next two lines, or temporarily skip them. + // ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.ES384, ПриватныйКлючECDSA384); + // ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.ES512, ПриватныйКлючECDSA521); + // HMAC: use a symmetric secret + СекретHS = "tests-only-symmetric-secret"; + ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.HS256, СекретHS); + ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.HS384, СекретHS); + ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.HS512, СекретHS);If you prefer to keep ES384/ES512 now, please add curve-appropriate private keys (P‑384 and P‑521). Otherwise, comment out those cases to avoid false positives when switching token implementations.
src/OneScript.StandardLibrary/Http/HttpConnectionContext.cs (2)
202-206: New CallHTTPMethod is useful; consider minimal validationCurrently any method string is forwarded. Optionally guard against null/empty to surface a clearer error and normalize:
- public HttpResponseContext CallHTTPMethod(string method, HttpRequestContext request, string output = null, IBslProcess process = null) + public HttpResponseContext CallHTTPMethod(string method, HttpRequestContext request, string output = null, IBslProcess process = null) { - return GetResponse(request, method, output, process); + if (string.IsNullOrWhiteSpace(method)) + throw RuntimeException.InvalidArgumentValue(); + return GetResponse(request, method.Trim(), output, process); }
389-501: Two tweaks: User-Agent typo and Authorization precedence
- UA value contains an unintended '$' producing e.g. "1Script v$1.3.0". Remove the extra '$'.
- Authorization: AccessToken header is always set last, overriding any explicit Authorization header set by the caller. Consider setting Bearer only if Authorization is not already set.
Proposed diff:
- if (webRequest.UserAgent == default) - { - webRequest.UserAgent = $"1Script v${Assembly.GetExecutingAssembly().GetName().Version}"; - } + if (webRequest.UserAgent == default) + { + webRequest.UserAgent = $"1Script v{Assembly.GetExecutingAssembly().GetName().Version}"; + } - if (request.AccessToken != null) - { - string token = request.AccessToken.ToString(process); - webRequest.Headers.Set(HttpRequestHeader.Authorization, $"Bearer {token}"); - } + if (request.AccessToken != null) + { + // If caller already set Authorization, don't override unless you explicitly prefer token precedence. + var existingAuth = webRequest.Headers[HttpRequestHeader.Authorization]; + if (string.IsNullOrEmpty(existingAuth)) + { + string token = request.AccessToken.ToString(process); + webRequest.Headers.Set(HttpRequestHeader.Authorization, $"Bearer {token}"); + } + }If the intended behavior is "AccessToken always wins over Basic/explicit Authorization," keep the override but please document this precedence in HttpRequestContext.AddAccessToken docs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
src/OneScript.StandardLibrary/Http/HttpConnectionContext.cs(11 hunks)src/OneScript.StandardLibrary/Http/HttpRequestContext.cs(3 hunks)src/OneScript.StandardLibrary/OneScript.StandardLibrary.csproj(1 hunks)src/OneScript.StandardLibrary/Security/Tokens/AccessTokenContext.cs(1 hunks)src/OneScript.StandardLibrary/Security/Tokens/AccessTokenSignAlgorithmEnum.cs(1 hunks)tests/access-token.os(1 hunks)tests/http.os(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/OneScript.StandardLibrary/Http/HttpRequestContext.cs (1)
src/OneScript.StandardLibrary/Security/Tokens/AccessTokenContext.cs (2)
AccessTokenContext(357-362)AccessTokenContext(364-368)
src/OneScript.StandardLibrary/Http/HttpConnectionContext.cs (2)
src/OneScript.StandardLibrary/Security/Tokens/AccessTokenContext.cs (1)
ToString(347-355)src/OneScript.Core/Values/BslValue.cs (1)
ToString(25-28)
🪛 Gitleaks (8.27.2)
tests/access-token.os
172-199: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
205-209: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🔇 Additional comments (13)
src/OneScript.StandardLibrary/OneScript.StandardLibrary.csproj (1)
29-29: Dependency TFM and version conflicts verifiedThe new System.IdentityModel.Tokens.Jwt 8.14.0 package is compatible with the repository’s resolved
net8.0TFM (from src/oscommon.targets), and no other IdentityModel/Microsoft.IdentityModel.* references exist in any.csproj. No further action required.
- All projects using
$(TargetFrameworkVersion)resolve tonet8.0(defined in src/oscommon.targets).- No other
IdentityModel*orMicrosoft.IdentityModel.*package references found.tests/http.os (1)
40-40: Test suite update looks goodIncluding the new token-transmission test in the list is correct, and keeps coverage aligned with the new API.
src/OneScript.StandardLibrary/Http/HttpRequestContext.cs (1)
14-14: Import addition is appropriateReferencing OneScript.StandardLibrary.Security.Tokens is expected for the new AccessTokenContext integration.
src/OneScript.StandardLibrary/Security/Tokens/AccessTokenSignAlgorithmEnum.cs (1)
15-56: Enum coverage is comprehensive and scripting-friendlyGood set of algorithms; the EnumerationType/EnumValue mapping will expose clear names to 1Script. No changes required.
src/OneScript.StandardLibrary/Security/Tokens/AccessTokenContext.cs (1)
190-199: Audience handling: confirm that multiple recipients serialize as a single 'aud' arrayAdding multiple 'aud' claims usually results in an array in the payload, but behavior can differ between library versions. Ensure tests cover multi-recipient output shape.
Would you add a test case with two recipients and assert that the resulting payload has "aud" as an array of both values?
tests/access-token.os (5)
3-15: Test list aggregation looks goodEntry point wiring and test names are clear and conventional.
17-31: Default constructor assertions are thoroughCovers all expected fields including collections; good baseline.
32-51: Constructor with headers/payload test is solidValidates both counts and specific key/value entries; good coverage.
90-102: Unsigned token test is appropriateValidates alg=none and empty signature. Good negative-path coverage.
104-112: Error-on-missing-key test is correctClear assertion of error code/message for signing without a key.
src/OneScript.StandardLibrary/Http/HttpConnectionContext.cs (3)
16-16: Importing OneScript.Execution is correctRequired for IBslProcess usage throughout the methods.
134-193: Process-aware HTTP methods: LGTMAdding an optional IBslProcess and threading it through is consistent and backwards-compatible for scripts. Good coverage across Get/Put/Post/Delete/Patch/Head.
339-345: Forwarding process into header construction: LGTMSignature change is consistent; body handling and response creation remain unaffected.
src/OneScript.StandardLibrary/Security/Tokens/AccessTokenContext.cs
Outdated
Show resolved
Hide resolved
tests/access-token.os
Outdated
| Функция НормализоватьBase64(Значение) | ||
| Длина = СтрДлина(Значение); | ||
| Остаток = Длина % 4; | ||
| Паддинг = ""; | ||
| Если Остаток = 2 Тогда | ||
| Паддинг = "=="; | ||
| ИначеЕсли Остаток = 3 Тогда | ||
| Паддинг = "="; | ||
| КонецЕсли; | ||
| Возврат Значение + Паддинг; | ||
| КонецФункции |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize Base64URL by translating characters before padding
Current implementation only pads; it should also translate '-'/'_' to '+'/'/'.
-Функция НормализоватьBase64(Значение)
+Функция НормализоватьBase64URL(Значение)
Длина = СтрДлина(Значение);
Остаток = Длина % 4;
Паддинг = "";
+ // Translate Base64URL to Base64
+ Значение = СтрЗаменить(Значение, "-", "+");
+ Значение = СтрЗаменить(Значение, "_", "/");
Если Остаток = 2 Тогда
Паддинг = "==";
ИначеЕсли Остаток = 3 Тогда
Паддинг = "=";
КонецЕсли;
Возврат Значение + Паддинг;
КонецФункции📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Функция НормализоватьBase64(Значение) | |
| Длина = СтрДлина(Значение); | |
| Остаток = Длина % 4; | |
| Паддинг = ""; | |
| Если Остаток = 2 Тогда | |
| Паддинг = "=="; | |
| ИначеЕсли Остаток = 3 Тогда | |
| Паддинг = "="; | |
| КонецЕсли; | |
| Возврат Значение + Паддинг; | |
| КонецФункции | |
| Функция НормализоватьBase64URL(Значение) | |
| Длина = СтрДлина(Значение); | |
| Остаток = Длина % 4; | |
| Паддинг = ""; | |
| // Translate Base64URL to Base64 | |
| Значение = СтрЗаменить(Значение, "-", "+"); | |
| Значение = СтрЗаменить(Значение, "_", "/"); | |
| Если Остаток = 2 Тогда | |
| Паддинг = "=="; | |
| ИначеЕсли Остаток = 3 Тогда | |
| Паддинг = "="; | |
| КонецЕсли; | |
| Возврат Значение + Паддинг; | |
| КонецФункции |
🤖 Prompt for AI Agents
In tests/access-token.os around lines 157 to 167, the НормализоватьBase64
function currently only appends padding but does not convert Base64URL
characters; update it to first replace all '-' with '+' and '_' with '/' in
Значение, then compute Остаток = СтрДлина(Значение) % 4 and append the proper
"=" padding ("" for 0, "==" for 2, "=" for 3) to return a standard Base64 string
ready for decoding.
tests/access-token.os
Outdated
| // 2048 bits, PKCS#8 | ||
| Функция ПриватныйКлючRSA() Экспорт | ||
| Возврат " | ||
| |-----BEGIN PRIVATE KEY----- | ||
| |MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQCT4Ws/kRkM92gs | ||
| |rljP85+ZaBoE/aJa5yBoWOJI5notYGDwAU66WofBunLIlUNn/YuETvs12mCCClBX | ||
| |ODurlwi+wLs5HyXf1gqNzVP4JN5QFUpqd2RQrUZLqBoONjZAok9P3oAw4JuJDnBm | ||
| |JODFLQjbCUOvorxR33iCPDfflvSa8kIxkQ3hA6zB7vu0j5s8HGCXczRI0pSblAH3 | ||
| |GEPSPp8D4IBmVKi90lQjCVVlCMY/hVkecqjc2hhfsh2cF/R2Q3NptNSLtw+lEMOU | ||
| |uUQtgVnULhTYDRaRJ9HripxQvQ2Se/BnozFVPfKokouh6f0ti9b4vWSDN3EjuMEu | ||
| |wf+ttUPhAgMBAAECggEAZRT3lzraMbfHmIsQIAz6MgUri+/HE3Xa/BiNjKEmMHAp | ||
| |ssRiCRWqqCyHGz6UFbgErsrCAhykTR85rXS9rNuPWbwp/bCh1e/bxPPuCrdA0uLK | ||
| |vB2iD1dxrnNYZBCAYwvPRNvlKgPNrRCWmetpTL4syHP7tUl6ikhDelopIwdvaZ1W | ||
| |cQeDq6QqkzOcJFdXHIHxZ2qbnjJoinyfPzmA98Ijdi8ynjIqKxj0f+0r567nxe/q | ||
| |Z4FwbPi7bRSmfMsTCM0SagwMuQoqvB6iktrZEYYUJmcN7V/gjN1viqVkjudk/CC5 | ||
| |RgMBp7yhrt3SP2oErAaAlTWldRVJ8qquJSpXkBhTxQKBgQDM8S9WIZgl2HwWsK9c | ||
| |C2JTC0peB8W1B3rM5IYvrCN0uB/P/KZ/Igwkla3wsOhy2SDPBYqzP/4OPzz4atOL | ||
| |PFDdBGrIQgseSajPp7nuD91IjI9KFXUT1EUBbKhWmATZwQqCOlHtSoi4zl+dJWgr | ||
| |dVzQxykh3ME85mIU3cxWNxk+swKBgQC4uPSsE9aJ5JK5LCfpcsVYeeatko72rA2t | ||
| |N/aPActklWwkR/tvJ43jG5RwKwbY4N2V6a5Tz5xQIe550vvBd56ku6tmEJDLL+jc | ||
| |QLh5lajV3JJq+tazdyZkztaSwOucuZve9R+eYAPM3uqYO8gAJSrJQQwlAoJkrWk6 | ||
| |zoISqvk9GwKBgFJQsVWkCqtwx26JqvWKcQSv7T/VWVi711wCkc8GEfuolMaCURGR | ||
| |SFVNdV7Of57ZjS75p9sVYeKxDbktlyg+orATPRyQQkz5Av/c+3YeEyA3rLnx4mOD | ||
| |h+/ph7e4OYKI4cKq0AtCO6YW5hqFUtDZw9zrkZ7TPx5J3q1I5PDgEpi3AoGAYokO | ||
| |wwfmBjJubeZ299loGdUUzvwB1OVcek0C8a/kXigywnu/TMDNuBrLKLZa9a+lo8U+ | ||
| |X6i8WiZvfn6kGsSDVJ5jRJOLmUdaMLs5bGn/4OzDEdvyUVM6oMWQwkG0rSp5Ugpx | ||
| |rlrLTCqQ+1nQSvuIgkh3gPqAsdGq81qbGfgn8YcCgYEAjodWERs0eCSMNGIshss4 | ||
| |/GN50lbOiI0+ke99160kT/hpep1WCEpXBfCeXLBamcG36Uk8c34DrxJ2Uj8LC895 | ||
| |/tB4HFgbQ52VxQdQCNGcs9hOxPuLlXlSdKNuplvfAkg2JhMrES7KtAxOBAcK77kx | ||
| |JzEREQEFN8CmwDvSpw8Mz9k= | ||
| |-----END PRIVATE KEY-----"; | ||
| КонецФункции | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Hardcoded private keys in repo are flagged by secret scanners
Gitleaks flagged these RSA/ECDSA private keys. Even for tests, checked-in private keys trigger alerts.
Options:
- Prefer generating ephemeral keys at test runtime, if feasible in the test environment.
- Or move PEMs to dedicated testdata and add a scoped allowlist in a gitleaks config to silence false positives only for these files.
If you choose allowlisting, introduce a .gitleaks.toml like:
title = "gitleaks config"
[allowlist]
description = "Allow test-only keys in OneScript tests"
paths = [
'''^tests/access-token\.os$'''
]
regexTarget = "file"Alternatively, split the PEMs into separate files under tests/testdata/ and point the allowlist to that path. I can open a follow-up PR to add the config if you prefer.
Also applies to: 203-210
Remove Hardcoded Private Keys from Tests
Tests in tests/access-token.os (currently lines 169–201 and 203–210) embed a 2048-bit RSA private key, which will be flagged by secret scanners (e.g., Gitleaks) and risks accidental exposure. Please address this by choosing one of the following:
• Generate keys at test runtime (for example, via openssl or a OneScript crypto API) so no PEM is checked in.
• Move the PEM blocks into files under tests/testdata/, then configure a scoped allowlist in .gitleaks.toml to suppress alerts only for those paths.
Example .gitleaks.toml to allowlist test-only keys:
title = "Gitleaks configuration"
[allowlist]
description = "Allow embedded RSA keys in OneScript tests"
paths = ["^tests/access-token\.os$"]
regexTarget = "file"Either approach will prevent CI failures and keep secret scanning strict for all other code.
🧰 Tools
🪛 Gitleaks (8.27.2)
172-199: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🤖 Prompt for AI Agents
In tests/access-token.os around lines 169–201 (and also referenced 203–210), a
2048-bit RSA private key PEM is hardcoded and triggers secret scanners; remove
the embedded private key by either (A) generating the RSA key at test runtime
(e.g., call openssl or a OneScript crypto API in test setup and load the
generated PEM) so no secret is checked into the repo, or (B) move the PEM blocks
into files under tests/testdata/ and update repository secret-scan configuration
by adding a scoped allowlist entry in .gitleaks.toml that only permits those
test paths; implement one of these approaches, update tests to read the
runtime/generated or testdata file, and remove the inline PEM from
tests/access-token.os.
| [ContextClass("ТокенДоступа", "AccessToken")] | ||
| public class AccessTokenContext : AutoContext<AccessTokenContext>, IDisposable | ||
| { | ||
| private int? _lifeTime; |
There was a problem hiding this comment.
Мне кажется, что должен существовать какой-то стандартный класс дотнета для JWT-токенов и их запчастей где-то внутри System.IdentityModel.Tokens.Jwt
|
|
||
| if (request.AccessToken != null) | ||
| { | ||
| string token = request.AccessToken.ToString(process); |
There was a problem hiding this comment.
Упадет, если null. (везде в методах он необязательный со значением null по умолчанию)
There was a problem hiding this comment.
Сделал обязательным process
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/OneScript.StandardLibrary/Http/HttpConnectionContext.cs (1)
496-500: Don’t override an existing Authorization header and guard null process when token is presentTwo issues:
- If the user or earlier code already set Authorization (e.g., Basic in CreateRequest or a custom header in request.Headers), the AccessToken code will overwrite it. That’s surprising; explicit headers should win.
- If process is null and the token isn’t pre-signed, AccessTokenContext.ToString(process) can throw (was raised in the previous review and still applicable).
Recommend:
- Only set Bearer if Authorization isn’t already present.
- Fail fast with a clear error if process is required but missing.
Proposed change:
- if (request.AccessToken != null) - { - string token = request.AccessToken.ToString(process); - webRequest.Headers.Set(HttpRequestHeader.Authorization, $"Bearer {token}"); - } + if (request.AccessToken != null && string.IsNullOrEmpty(webRequest.Headers[HttpRequestHeader.Authorization])) + { + if (process == null) + throw new RuntimeException("Для использования ТокенаДоступа требуется контекст процесса (IBslProcess)."); + + var token = request.AccessToken.ToString(process); + webRequest.Headers.Set(HttpRequestHeader.Authorization, $"Bearer {token}"); + }Optionally, add a test to cover:
- Explicit Authorization header remains untouched when AccessToken is present.
- AccessToken with null process yields a clear RuntimeException (or confirm that IBslProcess is always injected and cannot be null).
I can add/update tests for these cases if you’d like.
🧹 Nitpick comments (1)
src/OneScript.StandardLibrary/Http/HttpConnectionContext.cs (1)
491-494: User-Agent default string has a stray '$' and fragile null checkThe header currently becomes "1Script v$". Also, comparing to default is unusual for strings here.
Apply this minimal fix:
- if (webRequest.UserAgent == default) + if (webRequest.UserAgent == null) { - webRequest.UserAgent = $"1Script v${Assembly.GetExecutingAssembly().GetName().Version}"; + webRequest.UserAgent = $"1Script v{Assembly.GetExecutingAssembly().GetName().Version}"; }If you prefer, we can also guard against empty/whitespace:
- if (webRequest.UserAgent == default) + if (string.IsNullOrWhiteSpace(webRequest.UserAgent))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/OneScript.StandardLibrary/Http/HttpConnectionContext.cs(11 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/OneScript.StandardLibrary/Http/HttpConnectionContext.cs (2)
src/OneScript.StandardLibrary/Security/Tokens/AccessTokenContext.cs (1)
ToString(347-355)src/OneScript.Core/Values/BslValue.cs (1)
ToString(25-28)
🔇 Additional comments (3)
src/OneScript.StandardLibrary/Http/HttpConnectionContext.cs (3)
16-16: Import of OneScript.Execution is correct and necessaryRequired for IBslProcess usage. No issues.
134-137: API now threads IBslProcess through all HTTP methods — verify binder behavior and null semanticsGood move keeping the signatures consistent across methods and delegating to GetResponse. Please double-check:
- OneScript binder injects IBslProcess automatically so script-facing signatures remain unchanged.
- At runtime IBslProcess is never null for these [ContextMethod]s; downstream token code assumes non-null (see lines 496-500).
Would you like me to scan the test suite/usages and confirm that no script-visible signature changes slipped through?
Also applies to: 145-148, 157-160, 168-171, 179-182, 190-193, 203-206
339-345: Process-aware GetResponse is finePassing process down and calling SetRequestHeaders(process) is the right place to hook token/header logic. No functional concerns here.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/access-token.os (4)
208-210: Name the normalizer for Base64URL and use it in the parserDecoding logic now correctly translates '-'/'_' and applies padding. For clarity and future maintenance, rename to НормализоватьBase64URL and update call sites in РазобратьJWT.
Apply:
- Результат.Заголовки = ПолучитьСтрокуИзДвоичныхДанных(Base64Значение(НормализоватьBase64(Части[0]))); - Результат.ПолезнаяНагрузка = ПолучитьСтрокуИзДвоичныхДанных(Base64Значение(НормализоватьBase64(Части[1]))); + Результат.Заголовки = ПолучитьСтрокуИзДвоичныхДанных(Base64Значение(НормализоватьBase64URL(Части[0]))); + Результат.ПолезнаяНагрузка = ПолучитьСтрокуИзДвоичныхДанных(Base64Значение(НормализоватьBase64URL(Части[1])));-Функция НормализоватьBase64(Знач Значение) +Функция НормализоватьBase64URL(Знач Значение)Also applies to: 216-233
58-73: Avoid shadowing functions with identically named variables; also use a proper symmetric secret for HS algorithms*
- Local vars ПриватныйКлючECDSA/ПриватныйКлючRSA shadow function names, hurting readability.
- HS* algorithms are HMAC; using an RSA PEM as a symmetric secret is confusing. Use a simple test secret.
Apply:
- ПриватныйКлючECDSA = ПриватныйКлючECDSA(); - ПриватныйКлючRSA = ПриватныйКлючRSA(); + КлючECDSA = ПриватныйКлючECDSA(); + КлючRSA = ПриватныйКлючRSA(); ТестовыеСлучаи = Новый Соответствие(); - ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.ES256, ПриватныйКлючECDSA); - ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.ES384, ПриватныйКлючECDSA); - ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.ES512, ПриватныйКлючECDSA); - ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.HS256, ПриватныйКлючRSA); - ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.HS384, ПриватныйКлючRSA); - ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.HS512, ПриватныйКлючRSA); - ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.PS256, ПриватныйКлючRSA); - ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.PS384, ПриватныйКлючRSA); - ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.PS512, ПриватныйКлючRSA); - ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.RS256, ПриватныйКлючRSA); - ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.RS384, ПриватныйКлючRSA); - ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.RS512, ПриватныйКлючRSA); + СекретHMAC = "test-secret"; + ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.ES256, КлючECDSA); + ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.ES384, КлючECDSA); + ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.ES512, КлючECDSA); + ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.HS256, СекретHMAC); + ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.HS384, СекретHMAC); + ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.HS512, СекретHMAC); + ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.PS256, КлючRSA); + ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.PS384, КлючRSA); + ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.PS512, КлючRSA); + ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.RS256, КлючRSA); + ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.RS384, КлючRSA); + ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.RS512, КлючRSA);
113-114: Remove unused variables in recipient testsОжидаемыеЧастиТокена is assigned but never used in these tests.
Apply:
- ОжидаемыеЧастиТокена = ОжидаемыеЧастиТестовогоТокена(); - +- ОжидаемыеЧастиТокена = ОжидаемыеЧастиТестовогоТокена(); - +Also applies to: 126-127
15-15: Fix Latin 'C' vs Cyrillic 'С' in test nameThe name uses Latin 'C' in one place, which is easy to miss and harms consistency. Replace with Cyrillic 'С'.
Apply:
- ВсеТесты.Добавить("ТестДолжен_ПроверитьГенерациюТокенаCРазличнымиТипамиЗначений"); + ВсеТесты.Добавить("ТестДолжен_ПроверитьГенерациюТокенаСРазличнымиТипамиЗначений");-Процедура ТестДолжен_ПроверитьГенерациюТокенаCРазличнымиТипамиЗначений() Экспорт +Процедура ТестДолжен_ПроверитьГенерациюТокенаСРазличнымиТипамиЗначений() ЭкспортAlso applies to: 132-132
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/OneScript.StandardLibrary/Security/Tokens/AccessTokenContext.cs(1 hunks)tests/access-token.os(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/OneScript.StandardLibrary/Security/Tokens/AccessTokenContext.cs
🧰 Additional context used
🪛 Gitleaks (8.27.2)
tests/access-token.os
237-264: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
270-274: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🔇 Additional comments (1)
tests/access-token.os (1)
202-215: Parser looks good and asserts structure before decodingJWT is split into three parts and base64url-decoded safely. Once the normalizer is renamed as suggested, this helper is solid for tests.
| ОжидаемоеЗначение = "{""Число"":1000.10,""Булево"":true,""Дата"":""2001-01-01T17:20:20.0000000Z"",""Нуль"":"""",""Неопределено"":"""",""Массив"":[100,{""Ключ1"":""Значение1""},{""Ключ2"":""Значение2"",""Структура"":{""Ключ1"":""Значение1""}}],""Структура"":{""Ключ1"":""Значение1""},""Соответствие"":{""Ключ2"":""Значение2"",""Структура"":{""Ключ1"":""Значение1""}}}"; | ||
|
|
||
| Структура = Новый Структура("Ключ1", "Значение1"); | ||
|
|
||
| Соответствие = Новый Соответствие(); | ||
| Соответствие.Вставить("Ключ2", "Значение2"); | ||
| Соответствие.Вставить("Структура", Структура); | ||
|
|
||
| Массив = Новый Массив(); | ||
| Массив.Добавить(100); | ||
| Массив.Добавить(Структура); | ||
| Массив.Добавить(Соответствие); | ||
|
|
||
| Токен = Новый ТокенДоступа(); | ||
| Токен.ПолезнаяНагрузка.Вставить("Число", 1000.10); | ||
| Токен.ПолезнаяНагрузка.Вставить("Булево", Истина); | ||
| Токен.ПолезнаяНагрузка.Вставить("Дата", Дата(2001, 1, 1, 20, 20, 20)); | ||
| Токен.ПолезнаяНагрузка.Вставить("Нуль", null); | ||
| Токен.ПолезнаяНагрузка.Вставить("Неопределено", Неопределено); | ||
| Токен.ПолезнаяНагрузка.Вставить("Массив", Массив); | ||
| Токен.ПолезнаяНагрузка.Вставить("Структура", Структура); | ||
| Токен.ПолезнаяНагрузка.Вставить("Соответствие", Соответствие); | ||
|
|
||
| РазобранныйТокен = РазобратьJWT(Токен); | ||
|
|
||
| юТест.ПроверитьРавенство(РазобранныйТокен.ПолезнаяНагрузка, ОжидаемоеЗначение); | ||
|
|
||
| КонецПроцедуры |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Date serialization assertion is timezone-dependent and brittle
The expected payload hardcodes "2001-01-01T17:20:20.0000000Z" while the inserted date is 2001-01-01 20:20:20 (local). This assumes a +03:00 local timezone (e.g., Europe/Moscow). On runners with different TZ (most CIs use UTC), the test will fail.
Suggested fixes (choose one):
- Make the assertion timezone-agnostic: parse payload JSON and assert the "Дата" field is ISO 8601 with trailing Z, while keeping exact assertions for other fields.
- Or compute the expected "Дата" dynamically from the local TZ at runtime rather than hardcoding it.
- Or pin CI timezone for this test run (least desirable).
Do you want me to adjust the test to parse the payload and assert fields individually (including a tolerant check for "Дата")?
Зависимость от часового пояса в тесте
Тест жёстко сравнивает весь JSON-пейлоад строкой, ожидая
"Дата":"2001-01-01T17:20:20.0000000Z"
хотя в коде передаётся Дата(2001,1,1,20,20,20) локального времени. При запуске в UTC (например на CI) фактическое значение будет 2001-01-01T20:20:20.0000000Z и тест упадёт.
Рекомендуемые варианты исправления:
- Вместо строковой проверки распарсить JSON и поэлементно проверить поля, включая «Дата» как ISO 8601-строку с «Z», без жёсткой привязки к смещению.
- Либо динамически вычислять ожидаемую строку даты исходя из текущего часового пояса.
- Либо (менее желательно) фиксировать TZ среды выполнения для этого теста.
Готов помочь с переработкой проверки на разбор JSON и валидацию ключей, включая гибкую проверку поля «Дата».
🤖 Prompt for AI Agents
В файле tests/access-token.os вокруг строк 134-161 тест жёстко сравнивает весь
JSON-пейлоад строкой, из‑за чего зависимость от часового пояса ломает тест
(ожидается "2001-01-01T17:20:20.0000000Z" при создании Дата(2001,1,1,20,20,20)).
Исправьте тест, распарсив ОжидаемоеЗначение и РазобранныйТокен.ПолезнаяНагрузка
в объекты (JSON) и поэлементно сравнив поля: числовые и булевы значения через
равенство, коллекции и структуры рекурсивно, а поле "Дата" проверяйте как
ISO8601 строку в UTC без жёсткого контроля конкретного смещения (например,
сконвертируйте дату из полезной нагрузки в UTC и сравните по моменту времени или
проверяйте startsWith("2001-01-01T20:20:20")/endsWith("Z") эквивалентным
способом); альтернативно вычислите ожидаемую дату динамически с учётом
локального смещения перед формированием ожидаемого JSON.
tests/access-token.os
Outdated
| Функция ПриватныйКлючRSA() Экспорт | ||
| Возврат " | ||
| |-----BEGIN PRIVATE KEY----- | ||
| |MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQCT4Ws/kRkM92gs | ||
| |rljP85+ZaBoE/aJa5yBoWOJI5notYGDwAU66WofBunLIlUNn/YuETvs12mCCClBX | ||
| |ODurlwi+wLs5HyXf1gqNzVP4JN5QFUpqd2RQrUZLqBoONjZAok9P3oAw4JuJDnBm | ||
| |JODFLQjbCUOvorxR33iCPDfflvSa8kIxkQ3hA6zB7vu0j5s8HGCXczRI0pSblAH3 | ||
| |GEPSPp8D4IBmVKi90lQjCVVlCMY/hVkecqjc2hhfsh2cF/R2Q3NptNSLtw+lEMOU | ||
| |uUQtgVnULhTYDRaRJ9HripxQvQ2Se/BnozFVPfKokouh6f0ti9b4vWSDN3EjuMEu | ||
| |wf+ttUPhAgMBAAECggEAZRT3lzraMbfHmIsQIAz6MgUri+/HE3Xa/BiNjKEmMHAp | ||
| |ssRiCRWqqCyHGz6UFbgErsrCAhykTR85rXS9rNuPWbwp/bCh1e/bxPPuCrdA0uLK | ||
| |vB2iD1dxrnNYZBCAYwvPRNvlKgPNrRCWmetpTL4syHP7tUl6ikhDelopIwdvaZ1W | ||
| |cQeDq6QqkzOcJFdXHIHxZ2qbnjJoinyfPzmA98Ijdi8ynjIqKxj0f+0r567nxe/q | ||
| |Z4FwbPi7bRSmfMsTCM0SagwMuQoqvB6iktrZEYYUJmcN7V/gjN1viqVkjudk/CC5 | ||
| |RgMBp7yhrt3SP2oErAaAlTWldRVJ8qquJSpXkBhTxQKBgQDM8S9WIZgl2HwWsK9c | ||
| |C2JTC0peB8W1B3rM5IYvrCN0uB/P/KZ/Igwkla3wsOhy2SDPBYqzP/4OPzz4atOL | ||
| |PFDdBGrIQgseSajPp7nuD91IjI9KFXUT1EUBbKhWmATZwQqCOlHtSoi4zl+dJWgr | ||
| |dVzQxykh3ME85mIU3cxWNxk+swKBgQC4uPSsE9aJ5JK5LCfpcsVYeeatko72rA2t | ||
| |N/aPActklWwkR/tvJ43jG5RwKwbY4N2V6a5Tz5xQIe550vvBd56ku6tmEJDLL+jc | ||
| |QLh5lajV3JJq+tazdyZkztaSwOucuZve9R+eYAPM3uqYO8gAJSrJQQwlAoJkrWk6 | ||
| |zoISqvk9GwKBgFJQsVWkCqtwx26JqvWKcQSv7T/VWVi711wCkc8GEfuolMaCURGR | ||
| |SFVNdV7Of57ZjS75p9sVYeKxDbktlyg+orATPRyQQkz5Av/c+3YeEyA3rLnx4mOD | ||
| |h+/ph7e4OYKI4cKq0AtCO6YW5hqFUtDZw9zrkZ7TPx5J3q1I5PDgEpi3AoGAYokO | ||
| |wwfmBjJubeZ299loGdUUzvwB1OVcek0C8a/kXigywnu/TMDNuBrLKLZa9a+lo8U+ | ||
| |X6i8WiZvfn6kGsSDVJ5jRJOLmUdaMLs5bGn/4OzDEdvyUVM6oMWQwkG0rSp5Ugpx | ||
| |rlrLTCqQ+1nQSvuIgkh3gPqAsdGq81qbGfgn8YcCgYEAjodWERs0eCSMNGIshss4 | ||
| |/GN50lbOiI0+ke99160kT/hpep1WCEpXBfCeXLBamcG36Uk8c34DrxJ2Uj8LC895 | ||
| |/tB4HFgbQ52VxQdQCNGcs9hOxPuLlXlSdKNuplvfAkg2JhMrES7KtAxOBAcK77kx | ||
| |JzEREQEFN8CmwDvSpw8Mz9k= | ||
| |-----END PRIVATE KEY-----"; | ||
| КонецФункции |
There was a problem hiding this comment.
Stop committing private keys; move to testdata or generate at runtime
Hardcoded RSA/ECDSA private keys will trip secret scanners (see Gitleaks hints) and are a real risk, even in tests.
Recommended options:
- Preferred: move PEMs to tests/testdata/ and load them at runtime in tests. Add a tight allowlist in .gitleaks.toml limited to those testdata paths.
- Alternative: generate ephemeral keys at test runtime (only if your CI image reliably provides the needed crypto tooling/APIs).
Example .gitleaks.toml allowlist (scoped):
title = "gitleaks config"
[allowlist]
description = "Allow test-only keys for AccessToken tests"
paths = [
'''^tests/testdata/.*\.(pem|key)$'''
]
regexTarget = "file"If you want, I can push a follow-up patch that:
- moves these PEMs to tests/testdata/,
- updates the two functions to read from files,
- adds the .gitleaks.toml entry.
Also applies to: 268-275
🧰 Tools
🪛 Gitleaks (8.27.2)
237-264: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
|
|
||
| КонецПроцедуры | ||
|
|
||
| Процедура ТестДолженПроверитьПередачуТокенаЧерезДобавитьТокенДоступа() Экспорт |
There was a problem hiding this comment.
Кажется нужны еще тесты на Claims и на то, как отрабатывает метод ConvertToClrObject внутри AccessTokenContext, мне пока неясно, что за API получается у класса и как им пользоваться.
|
|
||
| foreach (var payloadItem in Payload) | ||
| { | ||
| var key = payloadItem.Key?.AsString(process); |
There was a problem hiding this comment.
Может лучше вместо сложностей с bsl-процессом и обработкой любых значений здесь запретим мапы с ключами, которые не строка? Прямо исключение "Неверный тип аргумента" тут выбрасывать, если тип ключа не Строка? Явно же контракт подразумевает строковые ключи, а не произвольные
| var recipientsStrings = new List<string>(); | ||
| foreach (var recipient in Recipients) | ||
| { | ||
| recipientsStrings.Add(recipient.AsString(process)); |
There was a problem hiding this comment.
Тут аналогично. Я бы ругался на нестроковый элемент вместо сложностей с приведением пользовательских скриптов к строке. Кто передаст сюда свой Сценарий, тот явно нарушает контракт, т.к. в спеке свойства Recipients явно сказано "массив строк"
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/OneScript.StandardLibrary/Security/Tokens/AccessTokenContext.cs (3)
386-393: Enforce string keys for nested maps when converting to JSON-compatible objectsDo not auto-stringify non-string keys inside nested maps; JSON requires string keys and the contract expects strings. Throw on non-string keys to surface script errors early.
Apply this diff:
- var dict = new Dictionary<string, object>(); + var dict = new Dictionary<string, object>(); foreach (var item in (IEnumerable<KeyAndValueImpl>)value) { - var key = item.Key.AsString(process); - dict[key] = ConvertToClrObject(process, item.Value); + if (item.Key.SystemType != BasicTypes.String) + throw RuntimeException.InvalidArgumentType(); + var key = item.Key.ToString(); + dict[key] = ConvertToClrObject(process, item.Value); } return dict;
8-14: Using System.IdentityModel.Tokens.Jwt is appropriateThis addresses the earlier concern about leveraging standard .NET JWT facilities.
173-186: Block user override of JWT "alg" header to prevent header confusionDo not allow user-provided Headers to set/override "alg". It can conflict with SigningCredentials (or "none") and is a known class of JWT vulnerabilities.
Apply this diff:
if (Headers != null) { foreach (var headerItem in Headers) { if (headerItem.Key.SystemType != BasicTypes.String) throw RuntimeException.InvalidArgumentType(); var key = headerItem.Key.ToString(); var value = headerItem.Value.AsString(process); - if(!String.IsNullOrEmpty(key)) - header[key] = value; + if (String.IsNullOrEmpty(key)) + continue; + // Do not allow overriding the algorithm set by SigningCredentials + if (string.Equals(key, "alg", StringComparison.OrdinalIgnoreCase)) + continue; + header[key] = value; } }
🧹 Nitpick comments (8)
src/OneScript.StandardLibrary/Security/Tokens/AccessTokenContext.cs (8)
255-265: Avoid overriding registered claims via Payload or clarify precedenceCurrently, user-provided Payload can override iss/sub/exp/nbf/iat/jti/aud set from properties, which is surprising and may bypass LifeTime/Issuer settings. Either block registered claim keys in Payload or document/guarantee precedence.
Apply this diff to block overrides:
var key = payloadItem.Key.ToString(); var value = ConvertToClrObject(process, payloadItem.Value); - if(!String.IsNullOrEmpty(key) && value != null) - payload[key] = value; + if (String.IsNullOrEmpty(key) || value == null) + continue; + // Prevent overriding registered claims set via properties + if (IsRegisteredClaim(key)) + continue; + payload[key] = value;Add this helper in the class (outside this range):
private static bool IsRegisteredClaim(string key) { // Registered claim names per RFC 7519 section 4.1 return string.Equals(key, JwtRegisteredClaimNames.Iss, StringComparison.Ordinal) || string.Equals(key, JwtRegisteredClaimNames.Sub, StringComparison.Ordinal) || string.Equals(key, JwtRegisteredClaimNames.Aud, StringComparison.Ordinal) || string.Equals(key, JwtRegisteredClaimNames.Exp, StringComparison.Ordinal) || string.Equals(key, JwtRegisteredClaimNames.Nbf, StringComparison.Ordinal) || string.Equals(key, JwtRegisteredClaimNames.Iat, StringComparison.Ordinal) || string.Equals(key, JwtRegisteredClaimNames.Jti, StringComparison.Ordinal); }
395-397: Rename typo and avoid double stringificationMinor nit: rename unwarpedValue -> unwrappedValue. Also prefer using the unwrapped CLR value when it’s already a string reference type.
Apply this diff:
- var unwarpedValue = value.UnwrapToClrObject() ?? ""; - return unwarpedValue.GetType().IsValueType ? unwarpedValue : value.AsString(process); + var unwrappedValue = value.UnwrapToClrObject() ?? ""; + return unwrappedValue.GetType().IsValueType ? unwrappedValue : unwrappedValue;
126-131: Treat Sign(None) as unsigned; avoid marking object as "signed"Currently _isSigned = true even for algorithm None. Consider leaving it false so ToString keeps reflecting current state, or explicitly treat "none" as a finalized unsigned token depending on intended semantics.
Apply this diff:
public void Sign(IBslProcess process, AccessTokenSignAlgorithmEnum algorithm, string secretKey = "") { CreateToken(process, algorithm, secretKey); - _isSigned = true; + _isSigned = algorithm != AccessTokenSignAlgorithmEnum.None; }
412-420: Defaulting ToString() to produce an unsigned alg=none token may be riskyUnsigned JWTs with alg=none are widely discouraged. Consider either requiring explicit Sign() before ToString(), or require an explicit flag/property to allow unsigned tokens. At minimum, document this behavior clearly.
If you decide to require explicit signing:
public override string ToString(IBslProcess process) { - if (_isSigned) - return _token; - - CreateUnsignedToken(process); - - return _token; + if (!_isSigned) + throw new SecurityTokenException("Токен не подписан. Вызовите Подписать()."); + return _token; }
162-171: Set 'typ' header to 'JWT' when missing (cosmetic, improves interoperability)Some consumers expect typ=JWT. Safe to set if not provided.
Apply this diff:
else { var signingCredentials = GetSigningCredentials(algorithm, secretKey); header = new JwtHeader(signingCredentials); + if (!header.ContainsKey("typ")) + header["typ"] = "JWT"; }
284-301: Enforce minimum HMAC key length by algorithm (security hardening)Weak HMAC secrets degrade token security. Validate byte length: HS256 ≥ 32 bytes, HS384 ≥ 48, HS512 ≥ 64.
Apply this refactor (helper + usage):
// Add helper in class private static void ValidateHmacKeyLength(AccessTokenSignAlgorithmEnum alg, int byteLen) { int min = alg switch { AccessTokenSignAlgorithmEnum.HS256 => 32, AccessTokenSignAlgorithmEnum.HS384 => 48, AccessTokenSignAlgorithmEnum.HS512 => 64, _ => 0 }; if (min > 0 && byteLen < min) throw new ArgumentException($"Секрет слишком короткий для {alg}. Минимальная длина: {min} байт."); }- AccessTokenSignAlgorithmEnum.HS256 => new SymmetricSecurityKey(Encoding.UTF8.GetBytes(secretKey)), - AccessTokenSignAlgorithmEnum.HS384 => new SymmetricSecurityKey(Encoding.UTF8.GetBytes(secretKey)), - AccessTokenSignAlgorithmEnum.HS512 => new SymmetricSecurityKey(Encoding.UTF8.GetBytes(secretKey)), + AccessTokenSignAlgorithmEnum.HS256 => CreateHmacKey(AccessTokenSignAlgorithmEnum.HS256, secretKey), + AccessTokenSignAlgorithmEnum.HS384 => CreateHmacKey(AccessTokenSignAlgorithmEnum.HS384, secretKey), + AccessTokenSignAlgorithmEnum.HS512 => CreateHmacKey(AccessTokenSignAlgorithmEnum.HS512, secretKey),Add the factory:
private static SecurityKey CreateHmacKey(AccessTokenSignAlgorithmEnum alg, string secretKey) { var bytes = Encoding.UTF8.GetBytes(secretKey); ValidateHmacKeyLength(alg, bytes.Length); return new SymmetricSecurityKey(bytes); }
119-131: Unit tests to add (coverage for edge cases and security behaviors)Recommend tests for:
- LifeTime only set (CreationTime omitted) → exp computed from now.
- Negative LifeTime / CreationTime → throws.
- Headers cannot override 'alg'.
- Payload reserved claims do not override property-set claims (if you adopt that change).
- HS256 with too-short secret → throws (if you adopt key length validation).
I can draft these tests against tests/access-token.os if you confirm the preferred behaviors.
202-224: Optional: 2038-safe timestampsCurrent int properties will overflow in 2038. Consider moving CreationTime/LifeTime-derived claims to long and, if possible, changing property types to long in a future PR to be Y2038-safe.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/OneScript.StandardLibrary/Security/Tokens/AccessTokenContext.cs(1 hunks)tests/access-token.os(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/access-token.os
🧰 Additional context used
🧬 Code graph analysis (1)
src/OneScript.StandardLibrary/Security/Tokens/AccessTokenContext.cs (4)
src/OneScript.StandardLibrary/Http/HttpRequestContext.cs (8)
ContextMethod(66-70)ContextMethod(72-76)ContextMethod(82-87)ContextMethod(89-94)ContextMethod(102-106)ContextMethod(108-112)ContextMethod(114-119)ContextMethod(125-129)src/OneScript.StandardLibrary/Http/HttpConnectionContext.cs (7)
ContextMethod(133-137)ContextMethod(144-148)ContextMethod(156-160)ContextMethod(167-171)ContextMethod(178-182)ContextMethod(189-193)ContextMethod(202-206)src/OneScript.StandardLibrary/Collections/KeyAndValueImpl.cs (1)
KeyAndValueImpl(21-28)src/ScriptEngine/Machine/ValueAdoptionExtensions.cs (1)
UnwrapToClrObject(60-63)
🔇 Additional comments (1)
src/OneScript.StandardLibrary/Security/Tokens/AccessTokenContext.cs (1)
231-247: Recipients validation looks correctYou enforce string-only elements for 'aud' and output either a single string or string array per JWT spec. Good.
| public int LifeTime | ||
| { | ||
| get => _lifeTime ?? 0; | ||
| set => _lifeTime = value; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate LifeTime is non-negative
Negative LifeTime yields nonsensical exp and can create tokens invalid at issuance.
Apply this diff:
public int LifeTime
{
get => _lifeTime ?? 0;
- set => _lifeTime = value;
+ set
+ {
+ if (value < 0)
+ throw new ArgumentOutOfRangeException(nameof(LifeTime), "ВремяЖизни не может быть отрицательным");
+ _lifeTime = value;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public int LifeTime | |
| { | |
| get => _lifeTime ?? 0; | |
| set => _lifeTime = value; | |
| } | |
| public int LifeTime | |
| { | |
| get => _lifeTime ?? 0; | |
| set | |
| { | |
| if (value < 0) | |
| throw new ArgumentOutOfRangeException(nameof(LifeTime), "ВремяЖизни не может быть отрицательным"); | |
| _lifeTime = value; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/OneScript.StandardLibrary/Security/Tokens/AccessTokenContext.cs around
lines 47 to 51, the LifeTime setter currently allows negative values which
produce invalid token expirations; change the setter to validate value >= 0 and
reject negatives (e.g., throw an ArgumentOutOfRangeException with a clear
message) before assigning to _lifeTime so negative lifetimes cannot be set.
| public int CreationTime | ||
| { | ||
| get => _creationTime ?? 0; | ||
| set => _creationTime = value; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate CreationTime is non-negative
Reject negative Unix timestamps early.
Apply this diff:
public int CreationTime
{
get => _creationTime ?? 0;
- set => _creationTime = value;
+ set
+ {
+ if (value < 0)
+ throw new ArgumentOutOfRangeException(nameof(CreationTime), "ВремяСоздания не может быть отрицательным");
+ _creationTime = value;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public int CreationTime | |
| { | |
| get => _creationTime ?? 0; | |
| set => _creationTime = value; | |
| } | |
| public int CreationTime | |
| { | |
| get => _creationTime ?? 0; | |
| set | |
| { | |
| if (value < 0) | |
| throw new ArgumentOutOfRangeException(nameof(CreationTime), "ВремяСоздания не может быть отрицательным"); | |
| _creationTime = value; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/OneScript.StandardLibrary/Security/Tokens/AccessTokenContext.cs around
lines 59 to 63, the CreationTime setter currently accepts negative values;
update the setter to validate that value is non-negative and reject negatives
early by throwing an ArgumentOutOfRangeException (include the parameter name and
a clear message like "CreationTime must be non-negative"), leaving the getter
unchanged.
| private void AddStandardClaimsToPayload(JwtPayload payload) | ||
| { | ||
| if (_issuer != null) | ||
| payload[JwtRegisteredClaimNames.Iss] = _issuer; | ||
|
|
||
| if (CreationTime != 0) | ||
| { | ||
| payload[JwtRegisteredClaimNames.Iat] = CreationTime; | ||
| payload[JwtRegisteredClaimNames.Nbf] = CreationTime; | ||
| } | ||
|
|
||
| if (CreationTime != 0 || LifeTime != 0) | ||
| { | ||
| int expires = CreationTime + LifeTime; | ||
| payload[JwtRegisteredClaimNames.Exp] = expires; | ||
| } | ||
|
|
||
| if (_tokenId != null) | ||
| payload[JwtRegisteredClaimNames.Jti] = _tokenId; | ||
|
|
||
| if(_userMatchingKey != null) | ||
| payload[JwtRegisteredClaimNames.Sub] = _userMatchingKey; | ||
| } |
There was a problem hiding this comment.
Fix exp calculation when CreationTime is not set (prevents already-expired tokens)
With LifeTime > 0 and CreationTime = 0, exp becomes LifeTime (seconds since 1970), yielding an already-expired token. Use current time when CreationTime is not provided, and only emit exp when LifeTime > 0.
Apply this diff:
private void AddStandardClaimsToPayload(JwtPayload payload)
{
if (_issuer != null)
payload[JwtRegisteredClaimNames.Iss] = _issuer;
- if (CreationTime != 0)
- {
- payload[JwtRegisteredClaimNames.Iat] = CreationTime;
- payload[JwtRegisteredClaimNames.Nbf] = CreationTime;
- }
+ // Determine creation time: use explicit value if provided; otherwise default to current time only for expiration math
+ long now = DateTimeOffset.UtcNow.ToUnixTimeSeconds();
+ bool creationProvided = _creationTime.HasValue && _creationTime.Value != 0;
+ long creation = creationProvided ? _creationTime.Value : now;
+ if (creationProvided)
+ {
+ payload[JwtRegisteredClaimNames.Iat] = creation;
+ payload[JwtRegisteredClaimNames.Nbf] = creation;
+ }
- if (CreationTime != 0 || LifeTime != 0)
- {
- int expires = CreationTime + LifeTime;
- payload[JwtRegisteredClaimNames.Exp] = expires;
- }
+ if (LifeTime > 0)
+ {
+ payload[JwtRegisteredClaimNames.Exp] = creation + LifeTime;
+ }
if (_tokenId != null)
payload[JwtRegisteredClaimNames.Jti] = _tokenId;
if(_userMatchingKey != null)
payload[JwtRegisteredClaimNames.Sub] = _userMatchingKey;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private void AddStandardClaimsToPayload(JwtPayload payload) | |
| { | |
| if (_issuer != null) | |
| payload[JwtRegisteredClaimNames.Iss] = _issuer; | |
| if (CreationTime != 0) | |
| { | |
| payload[JwtRegisteredClaimNames.Iat] = CreationTime; | |
| payload[JwtRegisteredClaimNames.Nbf] = CreationTime; | |
| } | |
| if (CreationTime != 0 || LifeTime != 0) | |
| { | |
| int expires = CreationTime + LifeTime; | |
| payload[JwtRegisteredClaimNames.Exp] = expires; | |
| } | |
| if (_tokenId != null) | |
| payload[JwtRegisteredClaimNames.Jti] = _tokenId; | |
| if(_userMatchingKey != null) | |
| payload[JwtRegisteredClaimNames.Sub] = _userMatchingKey; | |
| } | |
| private void AddStandardClaimsToPayload(JwtPayload payload) | |
| { | |
| if (_issuer != null) | |
| payload[JwtRegisteredClaimNames.Iss] = _issuer; | |
| // Determine creation time: use explicit value if provided; otherwise default to current time only for expiration math | |
| long now = DateTimeOffset.UtcNow.ToUnixTimeSeconds(); | |
| bool creationProvided = _creationTime.HasValue && _creationTime.Value != 0; | |
| long creation = creationProvided ? _creationTime.Value : now; | |
| if (creationProvided) | |
| { | |
| payload[JwtRegisteredClaimNames.Iat] = creation; | |
| payload[JwtRegisteredClaimNames.Nbf] = creation; | |
| } | |
| if (LifeTime > 0) | |
| { | |
| payload[JwtRegisteredClaimNames.Exp] = creation + LifeTime; | |
| } | |
| if (_tokenId != null) | |
| payload[JwtRegisteredClaimNames.Jti] = _tokenId; | |
| if(_userMatchingKey != null) | |
| payload[JwtRegisteredClaimNames.Sub] = _userMatchingKey; | |
| } |
🤖 Prompt for AI Agents
In src/OneScript.StandardLibrary/Security/Tokens/AccessTokenContext.cs around
lines 202-224, the Exp (JwtRegisteredClaimNames.Exp) is calculated as
CreationTime + LifeTime which, when CreationTime==0, yields an already-expired
timestamp; change the logic to compute an effectiveCreation = (CreationTime != 0
? CreationTime : current Unix time in seconds) and use that for the Exp
calculation; also only emit Exp when LifeTime > 0; leave Iat/Nbf/Jti/Sub
behavior unchanged (or keep Iat/Nbf set only when CreationTime != 0) so you only
alter how Exp is derived and ensure it uses the current time when CreationTime
is not provided.
EvilBeaver
left a comment
EvilBeaver
left a comment
There was a problem hiding this comment.
Тесты на claims надо бы сделать, иначе непонятно API
@EvilBeaver Добавил |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/http.os (1)
375-394: Усилить проверку значения заголовка Authorization и отвязать её отToStringтокенаИнтеграционный сценарий в целом хороший: формируется
ТокенДоступа, он подписывается и черезДобавитьТокенДоступауходит вAuthorizationкак Bearer‑токен.Есть два момента, которые стоит доработать именно как тест:
- Сейчас проверка такая:
юТест.ПроверитьВхождение( JsonВОбъект(ТелоОтвета)["headers"]["Authorization"], "Bearer " + ТокенДоступа );Она:
- использует
ПроверитьВхождениевместо строгого равенства, что допускает лишние данные в заголовке;- опирается на неявное преобразование
ТокенДоступак строке. Если реализацияToStringу объекта когда‑нибудь изменится, и тот жеToStringначнёт использоваться при формировании заголовка, тест продолжит проходить, даже если реальный формат JWT в API «поедет».Рекомендации:
- заменить проверку на
ПроверитьРавенствос ожидаемым значением заголовка;- если у
ТокенДоступаесть явное API получения строкового представления (свойство или метод с JWT‑строкой), использовать его в ожидаемом значении вместо неявной конкатенации с объектом.Это сделает тест более жёстким и устойчивым к будущим изменениям API токена.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/http.os(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-01T15:13:19.064Z
Learnt from: CR
Repo: EvilBeaver/OneScript PR: 0
File: .cursor/rules/runbsltests.mdc:0-0
Timestamp: 2025-12-01T15:13:19.064Z
Learning: Ignore errors from http.os test file and the test named 'Тест_ДолженПроверитьЧтоПеремещениеФайлаРаботаетПоHTTP' when reviewing BSL test results
Applied to files:
tests/http.os
🪛 Gitleaks (8.30.0)
tests/http.os
[high] 414-414: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🔇 Additional comments (1)
tests/http.os (1)
40-42: Регистрация нового теста в списке выполнена корректноИмя теста в
ВсеТесты.Добавить("ТестДолженПроверитьПередачуТокенаЧерезДобавитьТокенДоступа")совпадает с именем процедуры, тест будет подхватываться раннером.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/OneScript.StandardLibrary/Http/HttpRequestContext.cs (1)
121-129: Уточнить XML‑комментарий кAddAccessTokenСейчас метод лишь сохраняет
AccessTokenв контексте, а установка заголовка Authorization, судя по остальному коду PR, происходит позже при формировании HTTP‑запроса. Это логично, но комментарий может ввести в заблуждение сопровождающих. Можно слегка переформулировать:- /// Добавляет токен доступа к HTTP-запросу. Устанавливает заголовок Authorization. + /// Добавляет токен доступа к HTTP-запросу. Заголовок Authorization будет установлен при выполнении запроса.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/OneScript.StandardLibrary/Http/HttpRequestContext.cs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.cursor/rules/langversion.mdc)
Do not use C# language features that are not available in C# 8 when generating code for projects using .NET 8.0 with LangVersion 8
Files:
src/OneScript.StandardLibrary/Http/HttpRequestContext.cs
🧬 Code graph analysis (1)
src/OneScript.StandardLibrary/Http/HttpRequestContext.cs (1)
src/OneScript.StandardLibrary/Security/Tokens/AccessTokenContext.cs (2)
AccessTokenContext(422-427)AccessTokenContext(429-433)
🔇 Additional comments (2)
src/OneScript.StandardLibrary/Http/HttpRequestContext.cs (2)
14-14: Подключение пространства имен для токенов выглядит корректноКласс использует
AccessTokenContext, поэтомуusing OneScript.StandardLibrary.Security.Tokens;уместен и не создает лишних зависимостей.
48-49: СвойствоAccessTokenкорректно инкапсулирует токен запроса
public+private setпозволяет управлять токеном только из внутреннего кода (черезAddAccessToken), при этом свойство не экспортируется в скриптовый контекст (нет[ContextProperty]), что выглядит согласованным с текущим HTTP API.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/security/access-token.os (1)
162-191: Timezone-dependent test will fail in different environments.Line 180 creates
Дата(2001, 1, 1, 20, 20, 20)in local time, but line 164 expects it to serialize as"2001-01-01T17:20:20.0000000Z"(UTC). This assumes a UTC+3 timezone offset. The test will fail in CI environments running in UTC or other timezones.As previously suggested, consider one of these fixes:
Parse and compare JSON fields individually with timezone-tolerant date validation:
РазобранныйJSON = JsonВОбъект(РазобранныйТокен.ПолезнаяНагрузка); юТест.ПроверитьРавенство(РазобранныйJSON["Число"], 1000.10); юТест.ПроверитьРавенство(РазобранныйJSON["Булево"], Истина); // Validate date is ISO 8601 with Z suffix, without hardcoding timezone юТест.ПроверитьИстину(СтрНачинаетсяС(РазобранныйJSON["Дата"], "2001-01-01T")); юТест.ПроверитьИстину(СтрЗаканчиваетсяНа(РазобранныйJSON["Дата"], "Z"));Use UTC date directly:
- Токен.ПолезнаяНагрузка.Вставить("Дата", Дата(2001, 1, 1, 20, 20, 20)); + ДатаUTC = ТекущаяУниверсальнаяДата() - (ТекущаяДата() - Дата(2001, 1, 1, 20, 20, 20)); + Токен.ПолезнаяНагрузка.Вставить("Дата", ДатаUTC);Based on learnings, this issue was previously flagged and remains unresolved.
🧹 Nitpick comments (1)
tests/security/access-token.os (1)
232-269: LGTM with optional improvement.The Base64URL normalization (lines 246-263) correctly handles the conversion by replacing URL-safe characters and adding padding, properly addressing the previous review concern.
Consider adding error handling to
ПрочитатьТекстовыйФайлfor better diagnostics:Функция ПрочитатьТекстовыйФайл(ОтносительныйПуть) ПутьКФайлу = ОбъединитьПути(ТекущийСценарий().Каталог, ОтносительныйПуть); + Если Не Новый Файл(ПутьКФайлу).Существует() Тогда + ВызватьИсключение "Файл не найден: " + ПутьКФайлу; + КонецЕсли; ЧтениеТекста = Новый ЧтениеТекста(ПутьКФайлу); Возврат ЧтениеТекста.Прочитать(); КонецФункцииBased on learnings, previous concerns about Base64URL handling have been resolved.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tests/security/test_ecdsa_key.pemis excluded by!**/*.pemtests/security/test_rsa_key.pemis excluded by!**/*.pem
📒 Files selected for processing (2)
tests/http.os(3 hunks)tests/security/access-token.os(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-01T15:13:19.064Z
Learnt from: CR
Repo: EvilBeaver/OneScript PR: 0
File: .cursor/rules/runbsltests.mdc:0-0
Timestamp: 2025-12-01T15:13:19.064Z
Learning: Ignore errors from http.os test file and the test named 'Тест_ДолженПроверитьЧтоПеремещениеФайлаРаботаетПоHTTP' when reviewing BSL test results
Applied to files:
tests/http.os
🔇 Additional comments (6)
tests/http.os (2)
40-40: LGTM!Test properly added to the exported test list.
381-400: Verify algorithm selection: HS256 with RSA key may be incorrect.Line 388 signs the token with
АлгоритмПодписиТокенаДоступа.HS256usingПрочитатьПриватныйКлючRSA(). According to JWT standards, HS256 is a symmetric algorithm (HMAC-SHA256) that requires a shared secret string, while RS256 is the asymmetric algorithm designed for RSA keypairs. Confirm whether this combination is intentional or if the algorithm should be changed to RS256 to match the RSA key, or vice versa with a symmetric secret for HS256.tests/security/access-token.os (4)
1-19: LGTM!Test suite structure is well-organized with clear test names covering various token creation and signing scenarios.
21-86: LGTM!The tests properly validate default construction, parameterized construction, and token generation with comprehensive property assertions.
125-161: LGTM!Tests correctly validate unsigned tokens (alg=none) and proper serialization of single vs. multiple recipients in the JWT payload.
193-230: LGTM!Error handling test properly verifies that signing with a cryptographic algorithm without providing a key raises an appropriate error. Helper functions provide clean test data construction.
| Функция ПрочитатьПриватныйКлючRSA() | ||
| ПутьКФайлу = ОбъединитьПути(ТекущийСценарий().Каталог, "./security/test_rsa_key.pem"); | ||
| ЧтениеТекста = Новый ЧтениеТекста(ПутьКФайлу); | ||
| Возврат ЧтениеТекста.Прочитать(); | ||
| КонецФункции |
There was a problem hiding this comment.
Misleading function name when used with HMAC algorithms.
The function name ПрочитатьПриватныйКлючRSA suggests it reads an RSA private key, but it's being used with HS256 (HMAC) in line 388, which requires a symmetric secret, not an RSA key. This creates confusion about what the file actually contains.
Consider one of these approaches:
-
If the file contains a symmetric secret for HMAC testing, rename the function:
-Функция ПрочитатьПриватныйКлючRSA() +Функция ПрочитатьТестовыйСекрет() ПутьКФайлу = ОбъединитьПути(ТекущийСценарий().Каталог, "./security/test_rsa_key.pem");
-
If the file contains an RSA key, create a separate function for reading symmetric secrets:
+Функция ПрочитатьТестовыйСекретHS256() + ПутьКФайлу = ОбъединитьПути(ТекущийСценарий().Каталог, "./security/test_hs_secret.txt"); + ЧтениеТекста = Новый ЧтениеТекста(ПутьКФайлу); + Возврат ЧтениеТекста.Прочитать(); +КонецФункции
Also consider adding error handling:
Функция ПрочитатьПриватныйКлючRSA()
ПутьКФайлу = ОбъединитьПути(ТекущийСценарий().Каталог, "./security/test_rsa_key.pem");
+ Если Не Новый Файл(ПутьКФайлу).Существует() Тогда
+ ВызватьИсключение "Файл ключа не найден: " + ПутьКФайлу;
+ КонецЕсли;
ЧтениеТекста = Новый ЧтениеТекста(ПутьКФайлу);
Возврат ЧтениеТекста.Прочитать();
КонецФункции📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Функция ПрочитатьПриватныйКлючRSA() | |
| ПутьКФайлу = ОбъединитьПути(ТекущийСценарий().Каталог, "./security/test_rsa_key.pem"); | |
| ЧтениеТекста = Новый ЧтениеТекста(ПутьКФайлу); | |
| Возврат ЧтениеТекста.Прочитать(); | |
| КонецФункции | |
| Функция ПрочитатьПриватныйКлючRSA() | |
| ПутьКФайлу = ОбъединитьПути(ТекущийСценарий().Каталог, "./security/test_rsa_key.pem"); | |
| Если Не Новый Файл(ПутьКФайлу).Существует() Тогда | |
| ВызватьИсключение "Файл ключа не найден: " + ПутьКФайлу; | |
| КонецЕсли; | |
| ЧтениеТекста = Новый ЧтениеТекста(ПутьКФайлу); | |
| Возврат ЧтениеТекста.Прочитать(); | |
| КонецФункции |
| Процедура ТестДолжен_ПроверитьГенерациюТокенаСКлючом() Экспорт | ||
|
|
||
| ПриватныйКлючRSA = ПрочитатьТекстовыйФайл("./test_rsa_key.pem"); | ||
| ПриватныйКлючECDSA = ПрочитатьТекстовыйФайл("./test_ecdsa_key.pem"); | ||
|
|
||
| ТестовыеСлучаи = Новый Соответствие(); | ||
| ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.ES256, ПриватныйКлючECDSA); | ||
| ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.ES384, ПриватныйКлючECDSA); | ||
| ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.ES512, ПриватныйКлючECDSA); | ||
| ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.HS256, ПриватныйКлючRSA); | ||
| ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.HS384, ПриватныйКлючRSA); | ||
| ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.HS512, ПриватныйКлючRSA); | ||
| ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.PS256, ПриватныйКлючRSA); | ||
| ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.PS384, ПриватныйКлючRSA); | ||
| ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.PS512, ПриватныйКлючRSA); | ||
| ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.RS256, ПриватныйКлючRSA); | ||
| ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.RS384, ПриватныйКлючRSA); | ||
| ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.RS512, ПриватныйКлючRSA); | ||
|
|
||
| Токен = ТестовыйТокенДоступа(); | ||
| ОжидаемыеЧастиТокена = ОжидаемыеЧастиТестовогоТокена(); | ||
|
|
||
| Для Каждого ТестовыйСлучай Из ТестовыеСлучаи Цикл | ||
| Алгоритм = ТестовыйСлучай.Ключ; | ||
| ПриватныйКлюч = ТестовыйСлучай.Значение; | ||
|
|
||
| Токен.Подписать(Алгоритм, ПриватныйКлюч); | ||
|
|
||
| РазобранныйТокен = РазобратьJWT(Токен); | ||
|
|
||
| юТест.ПроверитьРавенство(РазобранныйТокен.Заголовки, СтрШаблон(ОжидаемыеЧастиТокена.Заголовки, Алгоритм), "Заголовки для " + Алгоритм); | ||
| юТест.ПроверитьРавенство(РазобранныйТокен.ПолезнаяНагрузка, ОжидаемыеЧастиТокена.ПолезнаяНагрузка, "Полезная нагрузка для " + Алгоритм); | ||
| юТест.ПроверитьЗаполненность(РазобранныйТокен.Сигнатура, "Сигнатура для " + Алгоритм); | ||
| КонецЦикла; | ||
|
|
||
| КонецПроцедуры |
There was a problem hiding this comment.
Critical algorithm/key type mismatch: HMAC algorithms require symmetric secrets, not RSA keys.
Lines 97-99 configure HS256, HS384, and HS512 (HMAC algorithms) to use ПриватныйКлючRSA. HMAC algorithms require symmetric secret strings, not asymmetric RSA private keys. This will either cause the signing operation to fail or produce invalid tokens.
Apply this diff to use appropriate keys for each algorithm family:
+ СекретHS = "test-secret-for-hmac-algorithms-do-not-use-in-production";
+
ТестовыеСлучаи = Новый Соответствие();
ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.ES256, ПриватныйКлючECDSA);
ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.ES384, ПриватныйКлючECDSA);
ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.ES512, ПриватныйКлючECDSA);
- ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.HS256, ПриватныйКлючRSA);
- ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.HS384, ПриватныйКлючRSA);
- ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.HS512, ПриватныйКлючRSA);
+ ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.HS256, СекретHS);
+ ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.HS384, СекретHS);
+ ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.HS512, СекретHS);
ТестовыеСлучаи.Вставить(АлгоритмПодписиТокенаДоступа.PS256, ПриватныйКлючRSA);2 participants
Реализован объект ТокенДоступа (AccessToken) и добавлен новый метод ДобавитьТокенДоступа в HTTPЗапрос
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.