Repository navigation
Refuse document ids the read route cannot serve - #56
Merged
Merged
Conversation
PUT takes the document from the JSON body and accepts anything, while GET routes :document through gin's fixed [A-Za-z0-9_]+ binding, so an id with a hyphen or a dot is stored and then 404s at nginx before the controller runs. The position is written and can never be read back. Reject it on the way in instead, with a new error rather than an overloaded one, so a client is told immediately rather than silently losing progress. is_valid_key_field is left alone: usernames go through it too, and tightening it there would lock out existing accounts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
|
Yeah, best to drop the second commit. |
pid1
force-pushed
the
fix/write-only-document-ids
branch
from
September 21, 2026 18:07
de12acc to
ab61a39
Compare
Contributor
Author
|
Dropped, thanks. #56 is just the guard now, and |
pid1
added a commit
to pid1/koreader-sync-server
that referenced
this pull request
Sep 21, 2026
Version 2 validated the document with is_valid_key_field alone and declares the same /syncs/progress/:document route, so it stranded a write exactly as version 1 did before koreader#56. The helper is duplicated rather than shared, the way is_valid_key_field already is in this file, so nothing in version 1 has to change for version 2 to have it. Identifiers need no guard of their own: a record is always keyed by the first identifier, which has to equal the document. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pid1
added a commit
to pid1/koreader-sync-server
that referenced
this pull request
Sep 21, 2026
Version 2 validated the document with is_valid_key_field alone and declares the same /syncs/progress/:document route, so it stranded a write exactly as version 1 did before koreader#56. The helper is duplicated rather than shared, the way is_valid_key_field already is in this file, so nothing in version 1 has to change for version 2 to have it. Identifiers need no guard of their own: a record is always keyed by the first identifier, which has to equal the document. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Closes #53. Two commits, and the second one is optional: if you would rather not touch gin, drop
de12accand the first commit stands on its own. I have written them so that works cleanly.ab61a39refuses the write.update_progressnow rejects a document id the read route cannot serve, with a new error rather than an overloaded one, so a client finds out immediately instead of losing progress silently. I leftis_valid_key_fieldalone deliberately: usernames go through it too, and tightening it there would lock out existing accounts.de12accmakes the already-written positions readable. gin substitutes a fixed[A-Za-z0-9_]+for every named parameter inbuild_named_parametersand has no per-route override, so the read route cannot be fixed from this repository alone. The commit widens that to a single path segment throughgin.patch, and relaxes the guard from the first commit in step so the two agree on what is servable. A slash is still refused, since it would change which route matches.The reason to treat the second one as optional is that
gin.patchis applied by the Dockerfile, so it reaches the documented Docker install and not a manual one. With only the first commit, new writes fail loudly and nothing is silently stranded, but positions already sitting in people's databases under a hyphen or a dot stay unreadable. With both, they come back with no client change and no migration.28 tests pass, 26 of them the existing suite unchanged. The round trip test fails against an unpatched gin and passes with it, so the second commit is doing what it claims rather than being asserted.
Two notes while I was in there. The status nginx returns depends on the character: 404 for a hyphen or a dot, 400 for a space, so the test asserts only that the read is never served rather than pinning a code. And
gin.patchalready has a hunk whose line counts do not match its body, which GNU patch inside the Debian image tolerates and BSD patch on macOS rejects; I left it as it is, but it is worth knowing if anyone tries to apply the patch outside Docker.This change is