Expose the handle constructors and fix multiline response parsing - #5
Draft
onslaughtq wants to merge 5 commits into
Draft
Expose the handle constructors and fix multiline response parsing#5onslaughtq wants to merge 5 commits into
onslaughtq wants to merge 5 commits into
Conversation
Adds the cases that distinguish the new terminator check from the old one. The multiline test that has been here since 277d621 passes either way, so it never covered this: its continuation line does not repeat the code. Requiring the code followed by a space is what RFC 959 specifies, but it regressed servers whose final line is the bare code with no trailing space. Those terminated under the old prefix comparison and would now loop until the socket closed. Accept both forms.
The three newly exported functions are an additive API change, so PVP asks for a minor bump. This stays inside the ftp-client == 0.5.* bound that ftp-client-conduit depends on, which therefore needs no bump of its own. The package has never carried a changelog. Seed one covering this release and point at git history for anything earlier.
There was a problem hiding this comment.
Pull request overview
Exposes lower-level connection constructors and corrects FTP multiline response termination.
Changes:
- Exports handle and TLS connection constructors.
- Implements RFC 959-compatible multiline parsing with regression tests.
- Bumps the package version and updates the changelog.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/Network/FTP/Client.hs |
Exports constructors and fixes multiline parsing. |
test/test.hs |
Adds multiline response regression tests. |
package.yaml |
Bumps version to 0.5.2.0. |
ftp-client.cabal |
Synchronizes the generated package version. |
CHANGELOG.md |
Documents the release changes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+45
to
+46
| createTLSConnection, | ||
| connectTLS, |
| FTPException(..), | ||
| -- * System Handle Creation | ||
| createSIOHandle, | ||
| createTLSConnection, |
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
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.
@pucsdian's three commits from #2, cherry-picked so the authorship stays theirs, plus tests and one correction.
The diagnosis in #2 is right. RFC 959 ends a multiline reply with the code followed by a space and continues it with the code followed by a hyphen, so the old three byte comparison stopped at the first continuation line that happened to repeat the code.
220-First/220-Second/220 Thirdcame back as two lines.Two things needed adding. The suite has had a multiline test since 2019, but its continuation line doesn't repeat the code, so it passes under both the old and the new logic and never covered this. And requiring the code followed by a space regresses servers whose final line is the bare code with no trailing text — those terminated under the old comparison but would now loop until the socket closed. Both forms are accepted.
11 examples. The RFC case fails against the old check, and the bare code case fails against the unhardened fix.
Two notes for whoever merges this:
Please use a merge commit or a rebase rather than a squash, or the three commits collapse into one authored by whoever pressed the button.
This supersedes #2, but GitHub's closing keywords only work on issues, so #2 needs closing by hand afterwards.
Stacked:
add-ci-multi-ghc→fix-response-line-partiality→pr-2-multiline-and-handle-exports→add-ci-flag→add-henforcer-fourmolu→fix-audit-findings. Merge bottom-up.