Skip to content

Remove credential logging and dead OAuth code#24

Open
kvmgithub wants to merge 1 commit into
Promises:mainfrom
kvmgithub:fix/upstream-22-security-hardening
Open

Remove credential logging and dead OAuth code#24
kvmgithub wants to merge 1 commit into
Promises:mainfrom
kvmgithub:fix/upstream-22-security-hardening

Conversation

@kvmgithub

Copy link
Copy Markdown
Contributor

Context

This PR addresses the security findings from #22 that are fixable without
architectural changes. The two critical/high findings were credential
material being written to stderr, which is readable via adb logcat by any
app with debug access on the device.

Fixes #22

What changed

Credential logging removed (findings 1, 2)

  • auth.rs: device registration no longer logs the request body, which
    contained the OAuth authorization code and PKCE verifier.
  • license.rs: license decryption no longer logs the decrypted voucher
    JSON (the audiobook AES key/IV) or the key/IV lengths. The voucher parse
    error no longer embeds the decrypted JSON either.

Error messages no longer reflect API response bodies (finding 7)

  • client.rs: JSON parse errors report only line/column instead of an
    800-character response excerpt; the body stays in the non-displayed
    response_body debug field.
  • HTTP error responses are capped at 300 characters in the error message.

Dead OAuth component deleted (findings 5, 8)

src/components/OAuthWebView.tsx was an unused leftover: nothing imported
it. Deleting it removes both the mixedContentMode="always" setting and
the Math.random()-based serial/PKCE generators the issue flagged. The
live login flow in LoginScreen.tsx does not set mixedContentMode (the
WebView default blocks mixed content) and gets its OAuth material from the
Rust core.

Test fixture PII replaced (finding 6)

test_fixtures/registration_response.json now uses synthetic account ids,
device serials, and names; the registration and integration tests assert
the synthetic values.

Findings not addressed here, and why

  • State parameter (3): The login flow mirrors Libation's Amazon
    OpenID/oa2 flow, which does not send a state parameter, and the
    callback is only consumed from navigation events inside the app's own
    WebView — the app registers no deep link for the callback URL, so there
    is no external entry point to inject a forged callback. Code injection
    is additionally blocked by PKCE: a code minted for another session fails
    the verifier check at token exchange. Sending a state value Amazon may
    not echo risks breaking login across marketplaces for no measurable gain.
  • Encryption at rest (4) and certificate pinning (9): both are
    real hardening options but are feature-sized changes (Android Keystore
    integration, pinning rotation strategy) that deserve their own focused
    PRs rather than riding along here.

User-visible behavior

None. Log output is quieter and error alerts no longer contain raw API
response dumps.

Validation

  • npm run typecheck
  • cargo test -p rust-core (lib + integration suites, all green)
  • Android debug build installed; Audible login, library sync, and
    download verified manually on device

Registration requests logged the authorization code and PKCE verifier to
stderr, and license decryption logged the audiobook AES key; both are
readable via logcat on Android. Error messages no longer embed full API
response bodies, the unused OAuthWebView component (Math.random serial
generation, mixedContentMode=always) is deleted, and the registration
test fixture uses synthetic account data.

Refs Promises#22
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.

Security Analysis

1 participant