Skip to content

Refuse document ids the read route cannot serve - #56

Merged
Frenzie merged 1 commit into
koreader:masterfrom
pid1:fix/write-only-document-ids
Sep 21, 2026
Merged

Frenzie merged 1 commit into
koreader:masterfrom
pid1:fix/write-only-document-ids

Conversation

@pid1

@pid1 pid1 commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Closes #53. Two commits, and the second one is optional: if you would rather not touch gin, drop de12acc and the first commit stands on its own. I have written them so that works cleanly.

ab61a39 refuses the write. update_progress now 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 left is_valid_key_field alone deliberately: usernames go through it too, and tightening it there would lock out existing accounts.

de12acc makes the already-written positions readable. gin substitutes a fixed [A-Za-z0-9_]+ for every named parameter in build_named_parameters and 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 through gin.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.patch is 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.patch already 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 Reviewable

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>
@Frenzie

Frenzie commented Sep 21, 2026

Copy link
Copy Markdown
Member

Yeah, best to drop the second commit.

@pid1
pid1 force-pushed the fix/write-only-document-ids branch from de12acc to ab61a39 Compare September 21, 2026 18:07
@pid1

pid1 commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

Dropped, thanks. #56 is just the guard now, and gin.patch is untouched.

@pid1 pid1 changed the title Refuse document ids the read route cannot serve, and widen the route Refuse document ids the read route cannot serve Sep 21, 2026
@Frenzie
Frenzie merged commit d941a63 into koreader:master Sep 21, 2026
1 check passed
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Document ids containing a hyphen or a dot are written but can never be read back

2 participants