Sitelet https://github.com/nextcloud/talk-android/pull/6807
Skip to content

fix(login): fall back to http with a correct status URL - #6807

Open
ToteMeiSter wants to merge 3 commits into
nextcloud:masterfrom
ToteMeiSter:fix/server-url-http-fallback
Open

fix(login): fall back to http with a correct status URL#6807
ToteMeiSter wants to merge 3 commits into
nextcloud:masterfrom
ToteMeiSter:fix/server-url-http-fallback

Conversation

@ToteMeiSter

@ToteMeiSter ToteMeiSter commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

Fixes #3899

When a server address is entered without a scheme, the app tries https:// first and falls back to http://.
The fallback passed the already complete status URL to checkServer(), which appends /status.php again.
The request went to .../status.php/status.php and a misleading error was shown.
The fallback now passes the base URL.

How to test

Enter the address of a server that serves only http (for example 10.0.2.2/nc from an emulator) without a scheme.
Expected: login continues over http.

🖼️ Screenshots

n/a, no UI change

🏁 Checklist

  • ⛑️ Tests (unit and/or integration) are included or not needed (one-line change in an Activity)
  • 🔖 Capability is checked or not needed
  • 🔙 Backport requests are created or not needed: /backport to stable-xx.x — maintainers to decide
  • 📅 Milestone is set
  • 🌸 PR title is meaningful (if it should be in the changelog: is it meaningful to users?)

🤖 AI (if applicable)

  • The content of this PR was partly or fully generated using AI

The change was prepared with Claude Code (claude-sonnet-5-5) and reviewed by the author; the commit carries an Assisted-by trailer.
It was not tested on a device. detekt and ktlintCheck pass.

🤖 Generated with Claude Code

@ToteMeiSter
fix(login): fall back to http with a correct status URL
cfb88f2 
When the server address is entered without a scheme, https is tried first
and http is the fallback. The fallback passed the already complete status
URL to checkServer(), which appends /status.php again. The request went to
.../status.php/status.php and the user saw a misleading error.

Pass the base URL instead, so the http fallback queries .../status.php.

Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru>
Assisted-by: Claude-Code:claude-sonnet-5-5
@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •
edited
Loading

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: ce7abb4d-9af6-452a-b799-507c90b03ae9
📥 Commits

Reviewing files that changed from the base of the PR and between cfb88f2 and f510e81.

📒 Files selected for processing (2)
  • app/src/main/java/com/nextcloud/talk/account/ServerSelectionActivity.kt
  • app/src/main/res/values/strings.xml

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

📝 Walkthrough

Walkthrough

When a forced-HTTPS check fails for a reason other than a certificate or TLS peer-verification failure, the activity offers an HTTP retry. Confirmation retries with the URL scheme changed to HTTP. Dismissal displays the original HTTPS error. The activity does not show the prompt if it is finishing or destroyed.

Priority: ➖ Normal

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to f510e

No concrete issue remains that would prevent merging after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to f510e

The fallback now requires consent and rejects recognized certificate and peer-verification failures. Confirmed HTTP connections remain exposed to network tampering. The change strengthens downgrade controls, but complete end-to-end transport and cancellation guarantees were not established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The supported exposure is the affected device's selected server and login attempt. A network-positioned attacker can tamper with approved HTTP status, capability, and login-bootstrap traffic. Credential exposure additionally depends on the resulting login and poll destinations; no broader tenant-wide or infrastructure authority expansion is established.

Security Findings and Attack Paths

  • inferred — Plaintext fallback is an existing capability, not an established introduced vulnerability. The base already issued HTTP requests automatically, and an attacker controlling responses could answer its malformed endpoint. Head fixes legitimate-server reachability while adding consent and recognized trust-failure exclusions; that comparison does not support an active PR regression concern.

Trust Boundaries and Controls

  • observed — The fallback captures the failed HTTPS destination rather than rereading input. Confirmation changes its scheme and disables further fallback; cancellation preserves the HTTPS error. CertificateException and SSLPeerUnverifiedException anywhere in the cause chain block the prompt, with cycle protection.

Resilience and Maintainability Implications

  • inferred — Attempt ownership remains incomplete: disposal covers the status request, not its capability subscription. An earlier capability callback can therefore launch login after a newer attempt begins or destruction occurs. This ownership gap exists in the inspected base and head, including explicit HTTP attempts; no material worsening by this PR was established.

Hardening Proposals

  • proposed — As separate hardening, bind status checks, confirmation, capability discovery, and login launch to one attempt identity and cancellation owner so obsolete work cannot complete a superseded authentication transition.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 1 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the HTTP fallback and status URL fix, which is the main change.
Description check ✅ Passed The description explains the issue, the fix, and how to test it. It includes the screenshots and checklist sections. The TODO section from the template is missing, and the milestone is not set, but th…
Linked Issues check ✅ Passed Issue #3899 describes a scheme-less address, an HTTPS-first check, and compares the failure with an explicit http:// address. The issue's phrase “fall back to https” conflicts with that context; thi…
Out of Scope Changes check ✅ Passed The dialog strings, fallback handling, TLS trust-failure check, and extracted error-display method all support the HTTP fallback required by issue #3899. The whole-PR diff contains no unrelated change…
Full details: Docstring Coverage

Explanation

Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 1 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

coderabbitai[bot]
coderabbitai Bot reviewed Oct 3, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 04d84013-973c-4861-99ab-862631f94d36
📥 Commits

Reviewing files that changed from the base of the PR and between baaca43 and cfb88f2.

📒 Files selected for processing (1)
  • app/src/main/java/com/nextcloud/talk/account/ServerSelectionActivity.kt

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

Comment thread app/src/main/java/com/nextcloud/talk/account/ServerSelectionActivity.kt Outdated
@ToteMeiSter
fix(login): ask before falling back to http
2f0414d 
When the HTTPS status request fails for an address typed without a
scheme, the client silently switched to HTTP and started the login
flow. An on-path attacker could block HTTPS to force that downgrade
(CWE-319). Now the user must confirm the insecure connection; declining
or dismissing the dialog shows the regular HTTPS error.

Manually typed http:// addresses are unchanged and get no dialog.

Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru>
Assisted-by: Claude-Code:claude-opus-5-5
@ToteMeiSter
fix(login): do not offer http after a certificate error
f510e81 
Skip the HTTP fallback dialog when the HTTPS failure has a
CertificateException or SSLPeerUnverifiedException in its cause chain,
so a declined certificate or host name mismatch is never downgraded.
The dialog message now says "Could not connect" instead of "does not
respond", which is wrong for 404/5xx.

Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru>
Assisted-by: Claude-Code:claude-opus-5-5
@mahibi

mahibi commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator

Hi @ToteMeiSter thank you for your contributions.
We will review them soon

@ToteMeiSter

ToteMeiSter commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

This is a bug fix and all CodeRabbit review threads are resolved. Once it is approved and merged, would it be possible to backport it to stable-25.0.x (e.g. for 25.0.3)? The bug is present in v25.0.2.

Suggested command for a maintainer after merge: /backport to stable-25.0.x

This comment was drafted with AI assistance (Claude Code).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments
@rapterjet2004 rapterjet2004 rapterjet2004 approved these changes

Assignees

No one assigned

Labels

3. to review Waiting for reviews AI assisted

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Server not found if specified without http

4 participants

@ToteMeiSter @mahibi @rapterjet2004 @AndyScherzinger