Skip to content

fix(kosync): serve the protocol endpoints at the paths KOReader requests - #1501

Open
thejdubb02 wants to merge 1 commit into
crocodilestick:mainfrom
thejdubb02:fix/kosync-protocol-root-routes
Open

fix(kosync): serve the protocol endpoints at the paths KOReader requests#1501
thejdubb02 wants to merge 1 commit into
crocodilestick:mainfrom
thejdubb02:fix/kosync-protocol-root-routes

Conversation

@thejdubb02

Copy link
Copy Markdown

Refs #1468.

What is actually wrong

/users/auth was never a missing route — it was being swallowed by the generic browse
rule in cps/web.py:

@web.route('/<data>/<sort_param>/', defaults={'page': 1, 'book_id': 1})
@login_required_if_no_ano
def books_list(data, sort_param, book_id, page):

/users/auth/ matches it with data="users", sort_param="auth", and the login
requirement turns the request into a 302 to /loginnext=%2Fusers%2Fauth%2F and all,
exactly as reported.

The KOSync handlers were only registered under /kosync/*, so a KOReader client
configured with the CWA base URL never reached them. The authentication code was not the
problem: authenticate_user() handles Basic Auth and the reverse-proxy header correctly.
It was simply never called.

The change

Three additional rules on the same three handlers, at the paths the protocol fixes.
strict_slashes=False matters here — KOReader sends the trailing slash, and without it
/users/auth/ is still won by the browse rule. The /kosync/* paths are untouched, so
anyone pointed at that prefix keeps working.

Does this shadow anything?

No. Static rules outrank converter rules in Werkzeug, so the aliases take only the exact
protocol paths. tests/unit/test_kosync_protocol_routes.py pins both halves: the
protocol paths reach KOSync, and /series/new, /authors/stored/, /category/new,
/publisher/stored still reach books_list. The five protocol assertions fail on
master and pass here; the browse assertions pass either way, which is the point of
having them.

Security

These handlers are @csrf.exempt, so it is fair to ask whether exposing them at the root
widens anything. It does not:

  • They were already publicly routable at /kosync/users/auth and
    /kosync/syncs/progress, already CSRF-exempt. This adds a path, not a capability.
  • authenticate_user() reads credentials only from request headers — Basic Auth, or the
    reverse-proxy header when that is enabled. There is no session or cookie fallback, so
    there are no ambient credentials for a cross-site request to ride. CSRF exemption is
    correct for this endpoint and unaffected by where it is mounted.
  • Access control is unchanged. The path previously answered with a login redirect; it now
    answers with an endpoint that requires valid credentials.

Scope

The x-auth-user / x-auth-key half of #1468 is deliberately not addressed. That scheme
authenticates with an MD5 of the password, and CWA stores werkzeug hashes — supporting it
means storing an additional credential per user. That is a schema and security decision
that belongs with you, not in a routing fix. Happy to open a separate issue if useful.

Provenance

Written with AI assistance, then verified rather than trusted:

  • tests/unit is unchanged in aggregate — 63 failed / 21 errors on both master and this
    branch (pre-existing, environment-dependent), with exactly +11 passing, which are the
    new tests.
  • The new tests were confirmed to fail without the source change and pass with it.
  • Routing precedence was verified against Werkzeug directly before writing the fix, not
    assumed.
  • Reviewed by three independent models; the one dissent raised the CSRF question answered
    above.
  • I run CWA, which is why I went looking at this one.

KOSync fixes its endpoints at the server root. A KOReader client configured with
the CWA base URL requests /users/auth, but the handlers were only registered under
/kosync/*, so that path fell through to web.books_list — whose
`/<data>/<sort_param>/` rule matches it with data="users", sort_param="auth" — and
its login requirement turned the request into a 302 to /login. That is the redirect
in crocodilestick#1468, `next=%2Fusers%2Fauth%2F` and all.

The authentication code was never the problem: authenticate_user() handles Basic
Auth (and the reverse-proxy header) correctly. It was simply never reached.

Registers the three protocol paths as additional rules on the same handlers, with
strict_slashes=False so both /users/auth and /users/auth/ match — KOReader sends
the trailing slash, and without this the generic browse rule still wins it. The
existing /kosync/* paths are untouched, so clients pointed at that prefix keep
working.

Static rules outrank converter rules in Werkzeug, so the aliases take only the
exact protocol paths; /series/new and /authors/stored/ still reach books_list.
tests/unit/test_kosync_protocol_routes.py pins both halves.

The x-auth-user/x-auth-key half of crocodilestick#1468 is deliberately not addressed here. That
scheme authenticates with an MD5 of the password, and CWA stores werkzeug hashes,
so supporting it means storing an additional credential per user — a schema and
security decision that belongs with the maintainers rather than in a routing fix.

Refs crocodilestick#1468
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.

1 participant