feat(controller): validate sync credentials to close rsync injection (S1)#9
Merged
luisguzman-adfa merged 1 commit intoJun 18, 2026
Conversation
…(S1) Start Phase 1 (security) with a refactor-by-feature slice. The peer-to-peer sync handshake transports host/port/user/pass inside a scanned QR code; those untrusted values were interpolated into rsyncd.conf (server) and a rsync:// URL (client) with no validation, allowing config-directive and URL injection (tech-debt item S1). - domain: new pure, unit-tested SyncCredentialValidator (org.iiab.controller.sync.domain) — the single rule for a "safe" sync credential: strict user/host charsets, port range, control-char-free password, and isSafeConfigValue() for rsyncd.conf lines. No Android deps. - seam: SyncHandshakeHelper.parsePayload() validates at the QR parse boundary (returns null -> existing "invalid QR" toast); RsyncManager re-checks defensively before writing the daemon config and before building the client URL, failing closed with a new rsync_error_invalid_credentials string (en+es). - tests: SyncCredentialValidatorTest (charset/range/injection cases, and that generated passwords are always accepted) + three malicious-payload cases in SyncHandshakeHelperTest. - docs: CLAUDE.md design map + controller/docs/TECH_DEBT_PLAN.md progress log (S1 done; Phase 1 now in progress). Validator is the reusable contract for the remaining injection fixes (S4, D2).
luisguzman-adfa
force-pushed
the
feat/phase1-security-sync-credential-validation
branch
from
6538b67 to
bf8b621
Compare
luisguzman-adfa
deleted the
feat/phase1-security-sync-credential-validation
branch
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
1 participant
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.
Summary
Starts Phase 1 (security hardening) with a small, layered slice. The peer-to-peer sync handshake transports host / port / user / password inside a scanned QR code. Those untrusted values were interpolated with no validation into the server-side
rsyncd.confand the client-sidersync://user@host:port/...URL, allowing config-directive / URL injection — tech-debt item S1.Changes
sync/domain/SyncCredentialValidator(new, pure JVM — no Android deps): the single rule for a "safe" sync credential — strict charsets for username/host, TCP port range, control-char-free password, andisSafeConfigValue()forrsyncd.conflines. Reusable contract for the remaining injection fixes (S4, D2).SyncHandshakeHelper.parsePayload(): validates at the QR parse boundary and returnsnullon invalid (reuses the existing "invalid QR" toast — no new UI wiring).RsyncManager: defence in depth — refuses to write the daemon config or build the client URL when credentials/share path are unsafe, failing closed with a newrsync_error_invalid_credentialsstring (en + es).SyncCredentialValidatorTest(charset/range/injection cases + a property check that app-generated passwords are always accepted) and three malicious-payload cases added toSyncHandshakeHelperTest.CLAUDE.mddesign map +controller/docs/TECH_DEBT_PLAN.mdprogress log (S1 done; Phase 1 now in progress).Testing
./gradlew :app:testDebugUnitTest— new JVM unit tests (Phase 0 CI gate already blocks on this). All logic is pure JVM; no emulator needed. ExistingSyncHandshakeHelperTestround-trip/legacy cases still pass.Notes
Follows
CLAUDE.md: new code is a feature package split by layer (sync/domain), domain stays pure JVM, legacy classes touched only through a narrow additive seam. Independent of the other in-flight branches.