fix(reviewers): support localStorage again

[BrayanDSO]

After the migration to use a server that can handle the backend POST requests, AnkiDroid moved from having a static base URL to a dynamic one because the server port could change.

This fixes the issue by using a default port number. That should work in 99% of the cases.

If by chance another app uses the same port, a free one will be used instead. I haven't tested, but this may happen with parallel builds of AnkiDroid running at the same time. Still, the only issue that would surge is that localStorage wouldn't work on one of open builds until the other one is closed

Fixes

• Fixes [BUG]: localStorage does not seem to be persistent in version 2.17.5 #15911

Approach

In the commits

How Has This Been Tested?

Emulator 35 by using JS's localStorage between app sessions

Learning (optional, can help others)

I also tried to use a placeholder static URL, like https://foo.bar, then redirect the request to the server. That actually worked, but it was buggy and too hacky.

Checklist

Please, go through these checks before submitting the PR.

• You have a descriptive commit message with a short title (first line, max 50 chars).
• You have commented your code, particularly in hard-to-understand areas
• You have performed a self-review of your own code
• UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
• UI Changes: You have tested your change using the Google Accessibility Scanner

[Arthur-Milchior]

The unit tests fail with error, for example, "ReviewerNoParamTest > differentDeckPenColorDoesNotAffectCurrentDeck FAILED
java.net.BindException: Address already in use"

It seems related to your code and not a flake, so I won't review tonight

[BrayanDSO]

Issue with macos (or the macos emulator). Unfornutately, I don't have a mac, so I can't help with that.

[david-allison]

• port is consistently 40001
• setting gradleTestMaxParallelForks to 1 does not fix this

@BrayanDSO Seems fixed by the following, be sure to move the line out from Reviewer.kt/etc...

Index: AnkiDroid/src/main/java/com/ichi2/anki/AbstractFlashcardViewer.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/AbstractFlashcardViewer.kt b/AnkiDroid/src/main/java/com/ichi2/anki/AbstractFlashcardViewer.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/AbstractFlashcardViewer.kt	(revision 2a095f64b82de977e3eecdf36d77683e7b8a93da)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/AbstractFlashcardViewer.kt	(date 1748911426702)
@@ -664,6 +664,7 @@
 
     override fun onDestroy() {
         super.onDestroy()
+        server.stop()
         tts.releaseTts(this)
         // WebView.destroy() should be called after the end of use
         // http://developer.android.com/reference/android/webkit/WebView.html#destroy()

[david-allison]

(might be worthwhile moving this to one of the two issues)

• #15911
• #16230

For now, the port randomization offers some form of defence against access to the backend. We lose this 'obscurity' if we hardcode the port

Due to this, I'd be tempted to put it behind an advanced/developer option until the above issue is resolved.

Don't want to be a nag here, but it's security.

In addition: I don't believe the 'find another port' logic was working correctly on macOS; could this be explicitly tested on Android

[BrayanDSO]

I don't have the time nor the expertise to audit every detail and I'm quite in favor of making it a dev option for now

[BrayanDSO]

Maybe it is worth to randomize the port once, save the result in a SharedPreference then keep using it so we keep some of the "obscurity"

[david-allison]

Sure, that works for me, thanks!

[BrayanDSO]

Okay, now the port is "private" and under a developer option. It can be promoted to an Advanced setting or even the default behavior (with no setting) when someone makes sure that this is secure

[david-allison]

Minor concern that I believe getServerPort would not use 0 on macOS if the port is in use

❗Could do with a comment
❗Add a Timber on BindException so developers know a nonstandard port is used

Minor concern that we haven't defined "what happens to developer options if we disable the preference"

But that's out of scope and should be moved to a separate issue

Code is great, looks good to me

[mikehardy]

I like it - and I think it's fine as a developer setting now though I think even putting it as an advanced / non-developer setting now would be fine since it defaults to randomization and you have to opt-in to any possible extra security risk

one wording change (everyone proposes a wording change...) but it is intended to explain the setting which hopefully makes the idea more discoverable and the setting more useful

+1 though, approval stands regardless

[mikehardy]

a couple more words with intention to de-mystify the setting a bit, what do you think?

Suggested change

	android:title="Use fixed port in the Reviewer"
	android:title="Use fixed port to enable javascript localStorage in Reviewer"

[BrayanDSO]

that passes the maxLength="41" for preference titles. I ended up moving the preference to Advanced as below

[david-allison]

Our translators aren't gonna like this one

[BrayanDSO]

I agree. Completely open to simplify it, but I found hard doing that without losing context enough to explain the setting.

Honestly, I tend to prefer just making it the default. Even if you get access to the port,

1. CORS is rejected by default
2. All GET requests to the server are rejected

   Anki-Android/AnkiDroid/src/main/java/com/ichi2/anki/pages/AnkiServer.kt

   Lines 49 to 55 in 3ae9b9d

   	Method.GET -> {
   	Timber.d("Rejecting GET request to server %s", session.uri)
   	newFixedLengthResponse(Response.Status.NOT_FOUND, null, null)
   	}
   	else -> {
   	Timber.d("Ignored request of unhandled method %s, uri %s", session.method, session.uri)
   	newFixedLengthResponse(null)

3. The POST requests are somewhat restricted (idk about the JS API)

   Anki-Android/AnkiDroid/src/main/java/com/ichi2/anki/Reviewer.kt

   Line 1462 in 3ae9b9d

   override suspend fun handlePostRequest(

[david-allison]

I'll remove this from the merge queue if you're also unhappy. This was NOT a blocking comment, so feel free to re-add it. No 'hard' advice on this one, but I'd prefer it as a developer-only setting. We can expect developers to read.

I found hard doing that without losing context enough to explain the setting.

Good reason for a developer-only setting.

---

CORS is rejected by default

CORS is client-side security. A malicious app would skip the OPTIONS call.

Honestly, I tend to prefer just making it the default.

Hard no until we resolve

• [security] Audit: apps on-device can't access the backend server #16230

Especially with how this is currently in the tech news cycle: https://www.zeropartydata.es/p/localhost-tracking-explained-it-could

[BrayanDSO]

It's an advanced setting and under the Workarounds category. I think it matches any possible expectatives from the public. Some advanced notetypes from shared decks may use it, and I don't want people going to Developer options just to enable it.

Since I'm no expert in security, and it's way harder and time consuming to defend than to attack, I'll merge it as an Advanced setting.

[mikehardy]

I think this is a good compromise - advanced ppl will really want this, but it is opt in so don't increase surface area by default

[BrayanDSO]

Shoot. Should have synced strings

[mikehardy]

Shoot. Should have synced strings

I had just synced them - there just can't have been much in there, no problem with this one