Merge upstream v2.17.1#7
Conversation
…s well as just LF
fix: when decoding pem files, handle lines being terminated by CRLF as well as just LF
…code CRLF support
📝 WalkthroughWalkthrough本次变更包含多个行为修正与测试更新：SSHPem.decode 现在接受 CRLF（\r\n）和 LF 行尾；SSHPem.encode 在 lineLength < 1 时抛出 ArgumentError，改为构建行列表并以单个 '\n' 连接且保证尾部换行；dynamic_forward_io 在启动连接前先将连接插入集合、调整 SOCKS 握手中 Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/src/ssh_pem_test.dart (1)
50-55:non-positive场景建议补上负数分支。当前只校验了
encode(0)；若测试名保持“non-positive”，建议再加encode(-1)断言，完整覆盖实现契约（lineLength < 1）。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/ssh_pem_test.dart` around lines 50 - 55, The test only asserts SSHPem('TEST', {}, Uint8List.fromList([1, 2, 3])).encode(0) throws an ArgumentError but the name says "non-positive"; add an additional assertion that calling SSHPem(...).encode(-1) also throwsA(isA<ArgumentError>()), so the test covers the negative branch of the contract (lineLength < 1) for SSHPem.encode.test/src/ssh_key_pair_test.dart (1)
12-30: 建议补一条SSHKeyPair.fromPem的 CRLF 集成用例。这里统一做了换行归一化，跨平台稳定性更好；但同时会让该文件不再覆盖“原始 CRLF PEM 输入”路径。建议额外加一条不归一化的 CRLF 用例，确保
SSHKeyPair.fromPem端到端行为不回退。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/ssh_key_pair_test.dart` around lines 12 - 30, The tests normalize newlines for most fixtures which hides original CRLF PEM behavior; add a new integration test case that uses SSHKeyPair.fromPem with a fixture that preserves CRLF (i.e., call fixture('...id_rsa_crlf', normalizeNewlines: false) or reuse an existing fixture path without normalizeNewlines) and assert the same parsing/round-trip expectations as other keys; update the test block that initializes rsaPrivate/rsaPrivateOpenSSH/rsaPrivateEncrypted (or add a new variable like rsaPrivateCrlf) and create a test that calls SSHKeyPair.fromPem on that raw-CRLF input to ensure end-to-end behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/src/ssh_key_pair_test.dart`:
- Around line 12-30: The tests normalize newlines for most fixtures which hides
original CRLF PEM behavior; add a new integration test case that uses
SSHKeyPair.fromPem with a fixture that preserves CRLF (i.e., call
fixture('...id_rsa_crlf', normalizeNewlines: false) or reuse an existing fixture
path without normalizeNewlines) and assert the same parsing/round-trip
expectations as other keys; update the test block that initializes
rsaPrivate/rsaPrivateOpenSSH/rsaPrivateEncrypted (or add a new variable like
rsaPrivateCrlf) and create a test that calls SSHKeyPair.fromPem on that raw-CRLF
input to ensure end-to-end behavior remains unchanged.
In `@test/src/ssh_pem_test.dart`:
- Around line 50-55: The test only asserts SSHPem('TEST', {},
Uint8List.fromList([1, 2, 3])).encode(0) throws an ArgumentError but the name
says "non-positive"; add an additional assertion that calling
SSHPem(...).encode(-1) also throwsA(isA<ArgumentError>()), so the test covers
the negative branch of the contract (lineLength < 1) for SSHPem.encode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee91d130-84a6-4ed9-af27-df4c59567aef
📒 Files selected for processing (9)
lib/src/dynamic_forward_io.dartlib/src/http/http_client.dartlib/src/http/http_date.dartlib/src/ssh_pem.darttest/src/http/http_date_test.darttest/src/ssh_client_test.darttest/src/ssh_key_pair_test.darttest/src/ssh_pem_test.darttest/test_utils.dart
✅ Files skipped from review due to trivial changes (1)
- test/src/http/http_date_test.dart
🚧 Files skipped from review as they are similar to previous changes (4)
- test/src/ssh_client_test.dart
- lib/src/http/http_client.dart
- test/test_utils.dart
- lib/src/ssh_pem.dart
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/src/ssh_pem_test.dart (2)
42-50: 可选：抽取共享SSHPem实例减少重复构造两个断言重复创建相同对象，提取局部变量能让用例更紧凑。
可选修改示例
test('SSHPem.encode rejects non-positive lineLength', () { + final pem = SSHPem('TEST', {}, Uint8List.fromList([1, 2, 3])); expect( - () => SSHPem('TEST', {}, Uint8List.fromList([1, 2, 3])).encode(0), + () => pem.encode(0), throwsA(isA<ArgumentError>()), ); expect( - () => SSHPem('TEST', {}, Uint8List.fromList([1, 2, 3])).encode(-1), + () => pem.encode(-1), throwsA(isA<ArgumentError>()), ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/ssh_pem_test.dart` around lines 42 - 50, The two expect assertions recreate the same SSHPem instance; refactor the test by extracting a local SSHPem variable (e.g., final pem = SSHPem('TEST', {}, Uint8List.fromList([1,2,3]));) and call pem.encode(0) and pem.encode(-1) in the two expect lines so the test uses a single SSHPem object; keep the assertions throwing ArgumentError for SSHPem.encode.
17-23: 建议增强 CRLF 用例断言强度当前只校验
type，建议再校验content，避免未来出现“类型可解析但正文归一化异常”的回归。可选修改示例
test('SSHPem.decode works with crlf not just lf', () { final pem = SSHPem.decode( '${_openSshPrivateKeyPem.replaceAll('\n', '\r\n')}\r\n', ); expect(pem.type, 'OPENSSH PRIVATE KEY'); + expect(pem.content, hasLength(1)); + expect(pem.content[0], 0); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/src/ssh_pem_test.dart` around lines 17 - 23, The test for SSHPem.decode only asserts pem.type, so add an assertion that the decoded pem.content (or the field that holds the PEM body) equals the expected normalized content to prevent regressions in body normalization; locate the test named "SSHPem.decode works with crlf not just lf" and the call to SSHPem.decode (using _openSshPrivateKeyPem.replaceAll) and add a second expect comparing pem.content (or the actual property name for the PEM payload) to the original _openSshPrivateKeyPem trimmed/normalized value so the CRLF→LF normalization is validated as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/src/ssh_pem_test.dart`:
- Around line 42-50: The two expect assertions recreate the same SSHPem
instance; refactor the test by extracting a local SSHPem variable (e.g., final
pem = SSHPem('TEST', {}, Uint8List.fromList([1,2,3]));) and call pem.encode(0)
and pem.encode(-1) in the two expect lines so the test uses a single SSHPem
object; keep the assertions throwing ArgumentError for SSHPem.encode.
- Around line 17-23: The test for SSHPem.decode only asserts pem.type, so add an
assertion that the decoded pem.content (or the field that holds the PEM body)
equals the expected normalized content to prevent regressions in body
normalization; locate the test named "SSHPem.decode works with crlf not just lf"
and the call to SSHPem.decode (using _openSshPrivateKeyPem.replaceAll) and add a
second expect comparing pem.content (or the actual property name for the PEM
payload) to the original _openSshPrivateKeyPem trimmed/normalized value so the
CRLF→LF normalization is validated as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28d69e30-99b0-48a6-90cf-c466e35776a9
📒 Files selected for processing (3)
lib/src/ssh_transport.darttest/src/ssh_pem_test.darttest/src/ssh_transport_aead_test.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/src/ssh_transport.dart
- test/src/ssh_transport_aead_test.dart
| test('throws for unsupported transfer encoding', () async { | ||
| final socket = _FakeSocket([ | ||
| 'HTTP/1.1 200 OK\r\n', | ||
| 'transfer-encoding: chunked\r\n', | ||
| 'transfer-encoding: gzip\r\n', | ||
| 'content-length: 0\r\n', | ||
| '\r\n', | ||
| ]); |
There was a problem hiding this comment.
🚩 No test for successful chunked transfer-encoding response parsing
The test at test/src/http/http_client_test.dart:44 was updated to verify that unsupported encodings (like gzip) throw, but there is no test that verifies a chunked transfer-encoded response is actually parsed correctly end-to-end. The chunked parsing state machine in processLine (lines 428-478 of http_client.dart) has non-trivial logic for chunk sizes, chunk data, CRLF consumption, and trailers. A test with a multi-chunk response body would improve confidence in the new code path.
(Refers to lines 44-56)
Was this helpful? React with 👍 or 👎 to provide feedback.
3 participants
Summary by CodeRabbit
新增功能
修复与改进
identity或chunked，其他值将报错。测试
其他