diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 0d635babfe..a36d3d9d26 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -59,6 +59,10 @@ Feature code layers as **UI → ViewModel → Repository → data source**, with The optional file path is attacker-controllable (a URL segment), so it's resolved through `PathTraversal.resolveWithinDirectory`'s traversal/symlink guard rather than a bare `File` join, both when opening a file in the already-open project and when matching the requested project name to a directory under `Environment.PROJECTS_DIR` (`findValidProjectByName`). +**The same scheme is also written, and the writer verifies itself against the reader.** `DeepLinkRequest.buildUrl` (same file as `parse`, deliberately) is the inverse: `CreateLinkAction` — the "Create link" item on the editor's file-tab drop-down — turns the current project, file and cursor into a URL on the clipboard. Two invariants are load-bearing. **Line and column are one-based in the URL and zero-based in the editor**; `buildUrl` takes the one-based form and `EditorHandlerActivity.zeroBasedOrInvalid` converts back, so a caller holding a cursor passes `cursor.leftLine + 1`. And **`buildUrl` parses its own output back and compares it against the arguments that built it**, returning `null` on any mismatch — because `parse` peels `line`/`column` *positionally*, so a file path whose own trailing segments look like that metadata (`.../file/src/line/5` with no line of its own) would otherwise read back as a different file. That check is what makes “will not be misread” a guarantee rather than a list of enumerated cases — distinct from, and narrower than, “the reader will accept it”, which no writer-side check can promise (see `buildUrl`'s own KDoc on symlinked file paths, unicode normalization and `isValidProjectDirectory`); it is why the action always writes both coordinates, and why a path it cannot express unambiguously yields no link instead of a wrong one. + +Whether the open project *can* be named at all is `isDeepLinkTargetOfOpenProject` (`ProjectValidations.kt`): a link can only ever resolve to `/`, but a project can be opened from anywhere. Its `deepLinkTargetOfOpenProjectWithoutIo` half exists so a caller on the main thread can settle the common case — equal path strings name the same directory — without the `canonicalPath` calls the full rule needs. `CreateLinkAction` sends its residue to `Dispatchers.IO` rather than to the StrictMode whitelist (ADR 0007); the two older callers (`EditorHandlerActivity.onNewIntent`, `BaseEditorActivity.onCreate`) still fall through to `canonicalPath` on the main thread and have not been migrated, so the shortcut narrows that exposure rather than removing it. + ## Module Structure Strategy: **layer-and-subsystem based**, not feature-by-feature. The Gradle build has ~80 modules (`settings.gradle.kts`) plus three included composite builds. `app` is the integration point; the rest are libraries it composes. diff --git a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt new file mode 100644 index 0000000000..af06fb8cd0 --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt @@ -0,0 +1,371 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.actions.file + +import android.content.Context +import androidx.annotation.VisibleForTesting +import androidx.core.content.ContextCompat +import com.itsaky.androidide.R +import com.itsaky.androidide.actions.ActionData +import com.itsaky.androidide.actions.markInvisible +import com.itsaky.androidide.activities.editor.EditorHandlerActivity +import com.itsaky.androidide.activities.projectsRoot +import com.itsaky.androidide.models.DeepLinkRequest +import com.itsaky.androidide.projects.IProjectManager +import com.itsaky.androidide.utils.copyToClipboard +import com.itsaky.androidide.utils.deepLinkTargetOfOpenProjectOrNull +import com.itsaky.androidide.utils.deepLinkTargetOfOpenProjectWithoutIo +import com.itsaky.androidide.utils.flashError +import com.itsaky.androidide.utils.flashSuccess +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.launch +import org.slf4j.LoggerFactory +import java.io.File +import java.util.concurrent.ConcurrentHashMap + +/** + * Copies a deep link to the current file -- and to the cursor's line and column within it -- to the + * clipboard, so it can be pasted somewhere another person (or the same person on another device) can + * tap it. The read side of the same link is + * [DeepLinkActivity][com.itsaky.androidide.activities.DeepLinkActivity]. + * + * @author David Schachter + */ +class CreateLinkAction( + context: Context, + override val order: Int, +) : FileTabAction() { + override val id: String = ID + + companion object { + const val ID = "ide.editor.fileTab.createLink" + + private val log = LoggerFactory.getLogger(CreateLinkAction::class.java) + + /** + * Verdicts for "can a link name this project", keyed by project path. Keyed rather than a + * single slot so a slow answer for a project the user has left cannot be mistaken for the + * current one. Holds only strings and booleans, so it is safe in a companion. + * + * Deliberately NOT cleared in `destroy()`, which was tried and is wrong: + * `EditorActivityLifecyclerObserver.onPause` calls `EditorActivityActions.clearActions()`, + * which destroys every action in this location, and `onResume` registers fresh ones. So + * `destroy()` runs on every backgrounding, not at teardown -- clearing here threw the cache + * away on each background/foreground cycle, and clearing the in-flight set alongside it let a + * still-running job's completion release a claim the next cycle's job had already taken. + * + * Bounded instead by pruning to the open project on every lookup -- see [linkableProject]. + * Exactly one project is open at a time, so this holds one entry rather than one per project + * the session ever touched, and a path that stops being the open one is dropped rather than + * left answering a question about a directory that may since have been renamed. + */ + private val linkableProjects = ConcurrentHashMap() + + /** Paths whose canonicalisation is already running, so N menu opens launch one job, not N. */ + private val canonicalisationsInFlight: MutableSet = ConcurrentHashMap.newKeySet() + } + + /** + * Whether a link can name the open project, or `null` while that is still being decided off this + * thread. + * + * `prepare()` has to answer synchronously, and the full rule canonicalises both sides -- + * filesystem work that REVIEW.md section 3 and ADR 0007 require be moved rather than suppressed, + * since the StrictMode whitelist is for vendored code we cannot change and never for our own. So + * [deepLinkTargetOfOpenProjectWithoutIo] settles the common case with no disk call, and only the + * residue -- paths differing as text, where a symlink might still make them one directory -- goes + * off-thread. + * + * While that rare case is pending the item is absent and reappears on the next menu open. Showing + * it optimistically would mean a tap that fails, which reads worse. + */ + private fun linkableProject(projectPath: String): Boolean? { + // Only the open project's verdict is worth keeping, and exactly one project is open at a time. + // Pruning here bounds the map to a single entry rather than one per project the session ever + // touched, and it is also what retires a stale answer: renaming the project directory changes + // this key, so the old verdict is dropped instead of left answering a question about a + // directory that no longer exists. + if (linkableProjects.keys.any { it != projectPath }) { + linkableProjects.keys.retainAll(setOf(projectPath)) + } + + linkableProjects[projectPath]?.let { return it } + + // false, not null: null means "ask again shortly", and there is nothing to wait for if the + // projects root cannot be derived at all. Narrow, because runCatching here would swallow Error + // too -- the same reason the IO block below does not use it. + val root = + try { + projectsRoot() + } catch (e: Exception) { + log.warn("Cannot locate the projects directory, so nothing can be linked", e) + return false + } + + // The name half of the shared rule is trivially satisfied here -- the name is derived from the + // very path being compared -- so this call is really asking the parent question alone. + val projectName = File(projectPath).name + + deepLinkTargetOfOpenProjectWithoutIo(projectPath, projectName, root)?.let { + linkableProjects[projectPath] = it + return it + } + + // add() is the atomic claim: @Volatile would give visibility without atomicity, so a + // check-then-set here could still let two menu opens launch the same canonicalisation. + if (!canonicalisationsInFlight.add(projectPath)) return null + + // actionScope, not the activity's lifecycleScope: the base class builds this scope for exactly + // this kind of background work and cancels it in destroy(), so the action no longer has to be + // handed an Activity just to reach a scope. + val job = + actionScope.launch(Dispatchers.IO) { + // Cached only when the filesystem actually answered. deepLinkTargetOfOpenProjectOrNull + // returns null for "could not tell", which a momentary EACCES or EIO produces -- and + // caching that as "not linkable" would hide the item for the rest of the process over a + // condition that has since cleared. + val verdict = + try { + deepLinkTargetOfOpenProjectOrNull(projectPath, projectName, root) + } catch (e: CancellationException) { + // Rethrown, never logged as a failure: swallowing it would complete this job + // normally and break structured cancellation (REVIEW.md section 1). runCatching + // here would catch it, and every Error too. + throw e + } catch (e: Exception) { + // A backstop, not the retry path: that function reports failure by returning null + // rather than throwing. Present so an unforeseen throw cannot reach the crash + // wrapper from a launch with no handler. + log.warn("Could not determine whether the open project can be linked", e) + null + } + + if (verdict != null) { + linkableProjects[projectPath] = verdict + } + } + + // Released here and nowhere else. invokeOnCompletion fires for both outcomes that matter -- + // the block ran, and the block never ran because the scope was cancelled before the dispatcher + // picked it up. A release inside the body as well would let job A's completion clear a claim + // job B had already re-taken, which is the duplicate work the claim exists to prevent. + job.invokeOnCompletion { canonicalisationsInFlight.remove(projectPath) } + return null + } + + /** + * Shown in the clipboard preview on Android 13+, so it is user-facing and lives in resources. + * + * Composed from the app name rather than spelled out, because only half of it is translatable: + * "Code on the Go" is a product name and must not be localised, while the noun after it should + * be. Marking the whole string untranslatable, as an earlier round did, kept the noun out of + * translation too. + */ + private val clipLabel: String = context.getString(R.string.clip_label_deeplink, context.getString(R.string.app_name)) + + init { + label = context.getString(R.string.action_create_link) + icon = ContextCompat.getDrawable(context, R.drawable.ic_copy) + } + + override fun prepare(data: ActionData) { + super.prepare(data) + + if (!visible) { + return + } + + // No null branch: FileTabAction.prepare() (via super, above) already calls markInvisible() when + // there is no activity, and the !visible check just above returns in exactly that case. + val activity = data.getActivity() ?: return + + // Hidden rather than shown-and-failing. Most states that produce no link are properties of how + // the project was opened, which a second tap could not correct. One is a property of the FILE: + // a path long enough to push the decoded URL past parse()'s 512-character ceiling. That one + // makes the item come and go as the user switches between a deep and a shallow tab, with no + // explanation offered, because the tap that would explain is never available. Surfacing a + // reason means buildUrl reporting WHY it refused rather than just null -- worth doing, and + // deliberately not bundled into this change. + // + // Only the cheap predicates run here. prepare() is called synchronously, from the touch + // handler, for every action in this menu on every open, and building the URL just to compare + // it against null cost ~17 percent-encoding passes plus a full parse of the result -- all + // discarded, then paid again on the tap. + if (urlFor(activity) == null) { + markInvisible() + } + } + + override fun EditorHandlerActivity.doAction(data: ActionData): Boolean { + // Recomputed rather than cached from prepare(): the menu can outlive the state it was built + // from (the tab can be closed, the project re-synced) and a stale link is worse than none. + // prepare() hides this action for every state that is stably unlinkable, so getting here with + // nothing to copy means something moved underneath the open menu -- rare, but a tap that does + // nothing at all reads as a broken button, so say so. + val url = + urlFor(this) ?: run { + flashError(R.string.msg_deeplink_cannot_create) + return false + } + + copyToClipboard(url, label = clipLabel) + flashSuccess(R.string.msg_deeplink_copied) + return true + } + + /** + * The most recently built URL, with the target it was built from. + * + * `prepare()` has to know whether a URL can be built at all, not merely whether the pieces exist: + * [DeepLinkRequest.buildUrl] refuses paths the reader would reject, and those are permanent + * properties of the file, so an item shown without asking it fails on every tap forever. But + * building the URL and discarding it made `prepare()` -- which runs for every action on every + * menu open, in the touch handler -- pay for a full encode-and-reparse it threw away, then pay + * again on the tap. + * + * Keeping the last result settles both: `prepare()` builds, and the tap that follows reuses it, + * because the cursor cannot have moved in between. That pairing is the whole job -- one build per + * menu open instead of two. It deliberately does NOT survive the cursor moving between two menu + * opens, because the URL genuinely differs then and one build is the minimum either way. + * + * Read and written only from the main thread, because `ActionMenuUtils.showPopupWindow` prepares + * these actions synchronously from the touch handler that opens the menu -- not because of + * `requiresUIThread`, which governs `execAction` alone. + */ + private var lastBuilt: Pair? = null + + /** The URL for the current tab, built at most once per menu open, or `null` if there is none. */ + private fun urlFor(activity: EditorHandlerActivity): String? { + val target = linkTarget(activity) ?: return null + lastBuilt?.let { (built, url) -> if (built == target) return url } + + // The refusal is remembered too, not just the URL. A file buildUrl will never express -- a + // path over the 512-character ceiling, say -- is a permanent state, so leaving it uncached + // meant repeating the whole encode-and-reparse on every menu open and throwing it away, which + // is the cost this cache exists to remove. + val url = target.toUrl() + lastBuilt = target to url + return url + } + + /** + * Everything a link needs, gathered without building one. + */ + private data class LinkTarget( + val projectName: String, + val relativePath: String, + val line: Int, + val column: Int, + ) { + fun toUrl(): String? = + DeepLinkRequest.buildUrl( + projectName = projectName, + filePath = relativePath, + line = line, + column = column, + ) + } + + /** + * The link target for the file in [activity]'s currently selected tab, or `null` if that file has + * no link that would resolve anywhere. + */ + private fun linkTarget(activity: EditorHandlerActivity): LinkTarget? { + val editorView = activity.getCurrentEditor() ?: return null + + // The editor, then the file from it. CodeEditorView.file is itself `editor?.file`, so asking + // for the file first and then null-checking the editor separately would be asking the same + // question twice and dressing the second as a safeguard. + val editor = editorView.editor ?: return null + val file = editor.file ?: return null + + val projectPath = IProjectManager.getInstance().projectDirPath + if (projectPath.isBlank()) { + return null + } + val projectDir = File(projectPath) + + // The two free rejections first. Both are pure and settle the answer outright, so asking them + // before linkableProject -- which dispatches a canonicalisation as a side effect -- avoids + // launching filesystem work whose result could not change the verdict. + // + // buildUrl's own rule, called rather than restated: a direct child named ".foo" satisfies the + // containment check below (that one compares parents, not names) but is not a project a link + // can name. + if (!DeepLinkRequest.isLinkableProjectName(projectDir.name)) { + return null + } + + val relativePath = projectRelativePathOrNull(projectDir, file) ?: return null + + // A deep link can only ever name /, but a project can be opened from + // anywhere -- the file picker, Recents, a clone destination. For one of those there is no URL + // that resolves on this device, let alone on the recipient's, so there is nothing honest to + // put on the clipboard. + if (linkableProject(projectPath) != true) { + return null + } + + // Both coordinates are always written, never omitted at 1:1 -- see buildUrl, which refuses the + // coordinate-free and column-only shapes precisely because the reader mis-handles them. + val cursor = editor.cursor + val (line, column) = oneBasedCursorPosition(cursor.leftLine, cursor.leftColumn) + return LinkTarget( + projectName = projectDir.name, + relativePath = relativePath, + line = line, + column = column, + ) + } +} + +/** + * [file]'s path relative to [projectDir], always '/'-separated, or `null` when [file] does not lie + * inside [projectDir]. + * + * Split out of the action because it is the security-relevant half and the only part worth testing + * on its own: `relativeToOrNull` walks *up* with ".." when the file is outside rather than failing, + * so containment has to be checked on the result and not merely on the call succeeding. Without that + * check a link could name a file outside the project it claims. + */ +@VisibleForTesting +internal fun projectRelativePathOrNull( + projectDir: File, + file: File, +): String? { + val relative = file.relativeToOrNull(projectDir)?.invariantSeparatorsPath ?: return null + if (relative.isEmpty() || relative == ".." || relative.startsWith("../")) { + return null + } + return relative +} + +/** + * The one-based line and column a link carries, from the editor's zero-based cursor. + * + * Two characters of arithmetic, named and tested because dropping either `+ 1` fails silently: + * buildUrl refuses a line or column below 1, so a cursor at the very start of a file simply makes + * the menu item vanish -- indistinguishable from the ordinary hidden-when-unlinkable state -- and a + * cursor anywhere else yields a link that quietly points one line too high. + */ +@VisibleForTesting +internal fun oneBasedCursorPosition( + zeroBasedLine: Int, + zeroBasedColumn: Int, +): Pair = (zeroBasedLine + 1) to (zeroBasedColumn + 1) diff --git a/app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt b/app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt index c8aa83ab93..364a418500 100644 --- a/app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt +++ b/app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt @@ -120,6 +120,7 @@ import com.itsaky.androidide.utils.Environment import com.itsaky.androidide.utils.ImageUtils import com.itsaky.androidide.utils.IntentUtils.openImage import com.itsaky.androidide.utils.UniqueNameBuilder +import com.itsaky.androidide.utils.capHeightToSpaceBelow import com.itsaky.androidide.utils.flashError import com.itsaky.androidide.utils.flashSuccess import com.itsaky.androidide.utils.forEachViewRecursively @@ -2199,7 +2200,9 @@ open class EditorHandlerActivity : ).root closeItem.apply { - text = "Close Tab" + // A string resource, like the "Undock" item eleven lines below: this label sat + // untranslated next to a translated one in the same two-item popup. + text = getString(string.action_close_tab) setOnClickListener { val position = tab.position if (isPluginTab(position)) { @@ -2209,7 +2212,7 @@ open class EditorHandlerActivity : } } - binding.root.addView(closeItem) + binding.actionItems.addView(closeItem) val undockItem = FileActionPopupWindowItemBinding @@ -2238,8 +2241,10 @@ open class EditorHandlerActivity : popupWindow.dismiss() } } - binding.root.addView(undockItem) + binding.actionItems.addView(undockItem) + // Shares FileActionPopupWindowBinding with the file-tab popup, so it shares the sizing bug. + popupWindow.capHeightToSpaceBelow(anchorView, binding.root) popupWindow.showAsDropDown(anchorView, 0, 0) } @@ -2670,8 +2675,13 @@ open class EditorHandlerActivity : // branch reads the carried-forward extra itself -- they only apply whatever fileRequest THIS // intent carries, which is often none. Comparing the deep link's project name against the // currently-loading project's directory name (mirroring BaseEditorActivity.onCreate's own - // deepLinkTargetsAnotherProject check) is a synchronous, disk-free way to tell same from - // different without waiting on the deep-link path's own async resolve. + // deepLinkTargetsAnotherProject check) is a synchronous way to tell same from + // different without waiting on the deep-link path's own async resolve. It is not free, though: + // this comment used to claim "disk-free", and isDeepLinkTargetOfOpenProject falls through to + // two File.canonicalPath calls whenever the paths differ as text. Deep-link arrival is a rare, + // user-initiated event so the stall is bounded, but it is a main-thread read on this path and + // on BaseEditorActivity.onCreate's -- see deepLinkTargetOfOpenProjectWithoutIo for the half + // that needs no disk, which CreateLinkAction uses because it is called on every menu open. // EditorIntentExtras.EXTRA_PREVIOUS_PROJECT_PATH, when present, is what IProjectManager.projectDirPath held before // MainActivity.openProject's bookkeeping call overwrote it to the NEW path -- by the time this // intent arrives here, the global itself already reads as the new path regardless of whether diff --git a/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt b/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt index 926afbe085..53d312cf20 100644 --- a/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt +++ b/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt @@ -19,7 +19,9 @@ package com.itsaky.androidide.models import android.net.Uri import android.os.Parcelable +import com.itsaky.androidide.utils.ContainedPathResolver import kotlinx.parcelize.Parcelize +import java.nio.file.Paths /** * A request to open a file at an optional line/column, carried as part of a [DeepLinkRequest] or a @@ -56,10 +58,17 @@ data class DeepLinkRequest( private const val SCHEME = "https" + /** + * The host [buildUrl] writes. + * + * Both entries in [HOSTS] resolve identically, so this is only a choice about which one a + * generated link shows the person who receives it; the bare domain is the shorter. + */ + private const val CANONICAL_HOST = "appdevforall.org" + // Both hosts serve an identical, verified assetlinks.json (see AndroidManifest.xml's matching // pair of elements on DeepLinkActivity's intent-filter) -- kept in sync with that list. - private val HOSTS = setOf("www.appdevforall.org", "appdevforall.org") - private const val PATH_PREFIX = "/device/open/project/" + private val HOSTS = setOf("www.$CANONICAL_HOST", CANONICAL_HOST) /** * See [parse]: an upper bound on the whole path, since the parsed pieces get parcelled. @@ -79,6 +88,20 @@ data class DeepLinkRequest( private const val SEGMENT_LINE = "line" private const val SEGMENT_COLUMN = "column" + /** + * The fixed segments every link starts with, and the prefix [parse] matches, derived from them + * so the literal "project" is spelled once *here*. The old pairing had it in both [PATH_PREFIX] + * and [SEGMENT_PROJECT], which is the drift this is meant to prevent -- [parse] reads both. + * + * It is spelled once more outside Kotlin, and that copy is the one that decides whether a link + * ever reaches the app at all: `AndroidManifest.xml`'s two `android:pathPrefix` attributes on + * DeepLinkActivity's intent-filter. Changing this list without changing those leaves every + * generated link opening in a browser, with the whole unit suite still green -- so + * `DeepLinkManifestPrefixTest` fails on exactly that. + */ + private val PATH_SEGMENTS = listOf("device", "open", SEGMENT_PROJECT) + private val PATH_PREFIX = PATH_SEGMENTS.joinToString(separator = "/", prefix = "/", postfix = "/") + /** First index at or after [from] holding [segment], or -1. Unlike [List.indexOf], never * matches an already-consumed segment earlier in the path -- e.g. a project name that * happens to equal `"line"` can't be mistaken for the `line` keyword that follows it. */ @@ -225,6 +248,171 @@ data class DeepLinkRequest( return DeepLinkRequest(projectName = projectName, fileRequest = fileRequest) } + + /** + * Whether a link can name [projectName] at all. + * + * A project name is a single path segment naming a direct child of the projects root, so a + * name carrying a separator can never resolve -- lookupValidProjectByName rejects it before it + * ever touches the disk. A leading dot is rejected for the same reason one level down: + * isProjectCandidateDir() refuses any name starting with '.', so a hidden directory is never a + * project. That one check also covers "." and "..", which matter more than merely being + * unopenable -- a browser or messenger that normalizes dot segments rewrites + * "/device/open/project/../file/x" into an entirely different path before the app ever sees it. + * + * Public because [buildUrl] is not the only caller that needs it: a UI deciding whether to + * *offer* to make a link has to reach the same verdict, and cheaply. Its own copy of these + * four conditions is exactly the drift that put an offer in front of the user for a project + * this function refuses. + */ + fun isLinkableProjectName(projectName: String): Boolean = + projectName.isNotEmpty() && + !projectName.startsWith('.') && + !projectName.contains('/') && + !projectName.contains('\\') + + /** + * The inverse of [parse]: the canonical URL naming [projectName], optionally the + * project-relative [filePath] inside it, and optionally a [line] and [column] inside that file. + * + * [line] and [column] are ONE-BASED, matching the URL scheme rather than the editor. The + * editor's own cursor is zero-based and + * [EditorHandlerActivity][com.itsaky.androidide.activities.editor.EditorHandlerActivity] + * subtracts one again when it reads a link back, so a caller holding a cursor passes + * `cursor.leftLine + 1`. + * + * Returns `null` rather than a URL that [parse] would reject or read back as something other + * than what was asked for. Each rejection below mirrors one of the *static* checks in [parse] + * or in [lookupValidProjectByName][com.itsaky.androidide.utils.lookupValidProjectByName]: + * there is nothing to be gained by handing someone a link this same app refuses to open. + * + * Three of the reader's checks are deliberately NOT mirrored, because none of them can be + * answered without a filesystem call or a fact that outlives the link: + * [ContainedPathResolver][com.itsaky.androidide.utils.ContainedPathResolver]'s real-path + * containment (a file reached through a symlink inside the project relativises to a clean path + * here and resolves outside the project there), the reader's NFC/NFD candidate matching -- + * which it applies to the project NAME only, so an intermediary that normalizes the URL breaks + * the FILE half of a link even though the project half survives -- and that reader's + * [isValidProjectDirectory][com.itsaky.androidide.utils.isValidProjectDirectory] requirement, + * which is a fact about the project at *open* time -- one that stops looking like an Android + * project after the link is sent (its `app/build.gradle` renamed, say) invalidates an + * already-sent link no matter what was verified here. + * + * So a link can still be emitted that this app later declines. The guarantee this function + * does make is narrower and worth stating exactly: nothing it returns will be *misread* -- + * read back as naming a different project, file, line or column than the caller asked for. + */ + fun buildUrl( + projectName: String, + filePath: String? = null, + line: Int? = null, + column: Int? = null, + ): String? { + if (!isLinkableProjectName(projectName)) { + return null + } + + // parse() keeps line/column only when there is a file for them to apply to, and silently + // drops them otherwise. Refusing beats emitting a link that quietly loses them. + if (filePath == null && (line != null || column != null)) { + return null + } + + // And a column with no line is refused for exactly the same reason: zeroBasedOrInvalid(null) + // yields 0, so the reader would silently apply the column to line 1 -- a position the link + // never named, with no invalid-value message. parse() can still READ that shape (a + // hand-authored link), it is just not one worth writing. + if (column != null && line == null) { + return null + } + + // Zero and negative are exactly what zeroBasedOrInvalid() reports back to the user as an + // invalid line/column, so they must not be written down in the first place. + if ((line != null && line <= 0) || (column != null && column <= 0)) { + return null + } + + val builder = Uri.Builder().scheme(SCHEME).authority(CANONICAL_HOST) + + // The same list parse()'s own prefix is built from, so the two cannot disagree. + PATH_SEGMENTS.forEach { builder.appendPath(it) } + builder.appendPath(projectName) + + if (filePath != null) { + // The reader's own lexical rule, called rather than re-spelled: it splits on '\\' as + // well as '/' and refuses a leading one, which a guard looking only at '/' components + // misses -- a file legitimately named "a\\..\\b.kt" is one harmless-looking component + // here and a traversal there, so the link would copy with a success message and then be + // refused on open. + if (ContainedPathResolver.isLexicallyRejected(filePath)) { + return null + } + + val segments = filePath.split('/') + // Refused locally on top of that rule: an empty component would put "//" in the path, + // which parse() rejects outright, and a "." component normalizes away to the base + // directory, which resolveWithinDirectory refuses as "not a path inside". Other + // dot-prefixed names are fine -- unlike a project directory, a hidden FILE + // (.gitignore) is perfectly linkable. + // The decoded form is checked too: a directory literally named "%2e%2e" survives + // encoding as "%252e%252e" and round-trips cleanly here, but an intermediary that + // decodes once and then collapses dot segments rewrites the path in transit -- the same + // hazard the literal "."/".." rejection exists for. It fails closed either way, but a + // dead link is still the defect that check was written to prevent. + if (segments.any { it.isEmpty() || it == "." || Uri.decode(it) == "." || Uri.decode(it) == ".." }) { + return null + } + + // A character the reader's base.resolve() cannot accept -- a NUL, say -- percent-encodes + // and round-trips through parse() cleanly, and is then REJECTED there: lexicalResolve + // catches the InvalidPathException and returns null, so the link opens nothing and the + // user is told the file was not found. Refuse it here instead, with no filesystem call, + // rather than copying a link that cannot work. + if (runCatching { Paths.get(filePath) }.isFailure) { + return null + } + + builder.appendPath(SEGMENT_FILE) + + // One appendPath call PER COMPONENT. Uri.Builder.appendPath encodes its argument as a + // single segment, which means it percent-encodes '/' along with everything outside + // [A-Za-z0-9_-!.~'()*] -- so handing it the whole relative path in one call emits + // ".../file/src%2Fmain%2FMain.kt". parse() does read that back correctly (getPathSegments + // splits the ENCODED path, so an encoded slash never becomes a separator), but it is + // neither the shape this class documents nor a URL a human can read. Encoding is not + // optional either way: '#' and '?' are legal in a Linux filename and would otherwise + // truncate the path into a fragment or a query. + segments.forEach { builder.appendPath(it) } + + // Always written as a full keyword/value pair, even though parse() tolerates a bare + // trailing keyword. A real pair is also what keeps a file path whose own trailing + // segments look like "line"/"column" out of peelTrailingKeyword's reach -- see parse()'s + // notes on the shapes it cannot resolve. Hand-authored links stay exposed to that; + // links from here do not. + line?.let { builder.appendPath(SEGMENT_LINE).appendPath(it.toString()) } + column?.let { builder.appendPath(SEGMENT_COLUMN).appendPath(it.toString()) } + } + + val url = builder.build().toString() + + // Re-parsed from the STRING rather than checked against the builder's own Uri: that string + // is what goes on the clipboard and comes back through Uri.parse in DeepLinkActivity, and + // a builder-built Uri is a different implementation of the same interface. Verifying the + // object the reader never sees would leave the difference untested. + // + // There is deliberately no separate length check here: parse() applies + // MAX_LINK_PATH_LENGTH to this very path, so the round-trip below already enforces it. + val parsed = parse(Uri.parse(url)) ?: return null + if (parsed.projectName != projectName || + parsed.fileRequest?.filePath != filePath || + parsed.fileRequest?.lineRaw != line?.toString() || + parsed.fileRequest?.columnRaw != column?.toString() + ) { + return null + } + + return url + } } } diff --git a/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt b/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt index 1f301a2032..5a03d39e6c 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt @@ -18,6 +18,7 @@ package com.itsaky.androidide.utils import android.content.Context +import android.graphics.Rect import android.view.LayoutInflater import android.view.View import android.view.View.OnLongClickListener @@ -35,122 +36,184 @@ import com.itsaky.androidide.idetooltips.TooltipTag import com.itsaky.androidide.plugins.extensions.FileTabMenuItem object ActionMenuUtils { + fun showPopupWindow( + context: Context, + anchorView: View, + pluginMenuItems: List = emptyList(), + ) { + val registry = ActionsRegistry.getInstance() + val actionData = ActionData.create(context) - fun showPopupWindow( - context: Context, - anchorView: View, - pluginMenuItems: List = emptyList() - ) { - val registry = ActionsRegistry.getInstance() - val actionData = ActionData.create(context) - - val binding = - FileActionPopupWindowBinding.inflate(LayoutInflater.from(context), null, false) - - val popupWindow = PopupWindow( - binding.root, - LayoutParams.WRAP_CONTENT, - LayoutParams.WRAP_CONTENT, - ).apply { - elevation = 2f - isOutsideTouchable = true - } - - val tooltipListener = OnLongClickListener { view -> - TooltipManager.showIdeCategoryTooltip( - context = view.context, - anchorView = view, - tag = TooltipTag.DIALOG_FIND_IN_FILE_OPTIONS - ) - popupWindow.dismiss() - true - } - - binding.root.setOnLongClickListener(tooltipListener) - - val actions = registry.getActions(ActionItem.Location.EDITOR_FILE_TABS) - actions.forEach { action -> - action.value.prepare(actionData) - if (!action.value.visible || !action.value.enabled) return@forEach - - val itemView = - FileActionPopupWindowItemBinding.inflate( - LayoutInflater.from(context), - null, - false - ).root - itemView.apply { - text = action.value.label - setOnClickListener { - (registry as DefaultActionsRegistry).executeAction( - action.value, - actionData - ) - popupWindow.dismiss() - } - setOnLongClickListener { - TooltipManager.showIdeCategoryTooltip( - context = context, - anchorView = anchorView, - tag = TooltipTag.EDITOR_FILE_CLOSE_OPTIONS - ) - popupWindow.dismiss() - true - } - } - binding.root.addView(itemView) - } - - val visiblePluginItems = pluginMenuItems.filter { it.isEnabled && it.isVisible } - if (visiblePluginItems.isNotEmpty()) { - val divider = View(context).apply { - layoutParams = LinearLayout.LayoutParams( - LinearLayout.LayoutParams.MATCH_PARENT, 1 - ).apply { - topMargin = 8 - bottomMargin = 8 - } - val typedValue = android.util.TypedValue() - context.theme.resolveAttribute( - com.google.android.material.R.attr.colorOutline, typedValue, true - ) - setBackgroundColor(typedValue.data) - } - binding.root.addView(divider) - - visiblePluginItems.forEach { item -> - val itemView = - FileActionPopupWindowItemBinding.inflate( - LayoutInflater.from(context), - null, - false - ).root - itemView.apply { - text = item.title - setOnClickListener { - try { - item.action() - } catch (e: Exception) { - android.util.Log.e("ActionMenuUtils", "Plugin menu action failed", e) - } - popupWindow.dismiss() - } - item.tooltipTag?.let { tag -> - setOnLongClickListener { - TooltipManager.showIdeCategoryTooltip( - context = context, - anchorView = anchorView, - tag = tag - ) - popupWindow.dismiss() - true - } - } - } - binding.root.addView(itemView) - } - } - - popupWindow.showAsDropDown(anchorView, 0, 0) - } + val binding = + FileActionPopupWindowBinding.inflate(LayoutInflater.from(context), null, false) + + val popupWindow = + PopupWindow( + binding.root, + LayoutParams.WRAP_CONTENT, + LayoutParams.WRAP_CONTENT, + ).apply { + elevation = 2f + isOutsideTouchable = true + } + + val tooltipListener = + OnLongClickListener { view -> + TooltipManager.showIdeCategoryTooltip( + context = view.context, + anchorView = view, + tag = TooltipTag.DIALOG_FIND_IN_FILE_OPTIONS, + ) + popupWindow.dismiss() + true + } + + binding.actionItems.setOnLongClickListener(tooltipListener) + + val actions = registry.getActions(ActionItem.Location.EDITOR_FILE_TABS) + actions.forEach { action -> + action.value.prepare(actionData) + if (!action.value.visible || !action.value.enabled) return@forEach + + val itemView = + FileActionPopupWindowItemBinding + .inflate( + LayoutInflater.from(context), + null, + false, + ).root + itemView.apply { + text = action.value.label + setOnClickListener { + (registry as DefaultActionsRegistry).executeAction( + action.value, + actionData, + ) + popupWindow.dismiss() + } + setOnLongClickListener { + TooltipManager.showIdeCategoryTooltip( + context = context, + anchorView = anchorView, + tag = TooltipTag.EDITOR_FILE_CLOSE_OPTIONS, + ) + popupWindow.dismiss() + true + } + } + binding.actionItems.addView(itemView) + } + + val visiblePluginItems = pluginMenuItems.filter { it.isEnabled && it.isVisible } + if (visiblePluginItems.isNotEmpty()) { + val divider = + View(context).apply { + layoutParams = + LinearLayout + .LayoutParams( + LinearLayout.LayoutParams.MATCH_PARENT, + context.dpToPx(1f), + ).apply { + // dp, not raw pixels -- height included: a literal 1 is 0.33dp on a 3x device. + topMargin = context.dpToPx(8f) + bottomMargin = context.dpToPx(8f) + } + val typedValue = android.util.TypedValue() + context.theme.resolveAttribute( + com.google.android.material.R.attr.colorOutline, + typedValue, + true, + ) + setBackgroundColor(typedValue.data) + } + binding.actionItems.addView(divider) + + visiblePluginItems.forEach { item -> + val itemView = + FileActionPopupWindowItemBinding + .inflate( + LayoutInflater.from(context), + null, + false, + ).root + itemView.apply { + text = item.title + setOnClickListener { + try { + item.action() + } catch (e: Exception) { + android.util.Log.e("ActionMenuUtils", "Plugin menu action failed", e) + } + popupWindow.dismiss() + } + item.tooltipTag?.let { tag -> + setOnLongClickListener { + TooltipManager.showIdeCategoryTooltip( + context = context, + anchorView = anchorView, + tag = tag, + ) + popupWindow.dismiss() + true + } + } + } + binding.actionItems.addView(itemView) + } + } + + popupWindow.capHeightToSpaceBelow(anchorView, binding.root) + popupWindow.showAsDropDown(anchorView, 0, 0) + } +} + +/** + * Caps [this] at the space below [anchorView], measuring [content] at the width the window will + * actually be given. + * + * A PopupWindow built WRAP_CONTENT reports height -2 to `showAsDropDown`, whose fit check is + * `height <= spaceBelow` -- trivially true for -2. No resize is negotiated, so the content is + * measured against the whole display rather than the room under the anchor, the ScrollView concludes + * it fits and never scrolls, and the window either runs off the bottom or is shoved up over the tab + * strip and toolbar. At 2x font scale this menu can reach that size. + * + * Two things here are easy to get wrong, and the first version of this function got both: + * + * `PopupWindow.getMaxAvailableHeight` is NOT the space below. Every overload delegates to the + * three-argument form, which returns `Math.max(distanceToBottom, distanceToTop)` (AOSP android-36 + * `PopupWindow.java:2010`) -- so for an anchor low in the frame it yields the space ABOVE, and + * capping to that lets `showAsDropDown` flip the popup over the anchor at exactly the height that + * covers the tabs. The distance is computed directly instead, mirroring AOSP's own + * `distanceToBottom` for the non-`mOverlapAnchor` case. + * + * The width spec matters as much as the height. Measuring with UNSPECIFIED width lets every label + * lay out on one unbounded line, so a title that wraps in the real pass reports a fraction of its + * laid-out height and the cap is skipped in the case it exists for -- a long plugin title at 2x. The + * width is bounded by the visible frame; that is still slightly generous, since the popup background + * and the item container's 24dp padding narrow it further, but it errs toward capping rather than + * skipping. + * + * Left alone when the content already fits, so a bad measurement degrades to the previous behaviour + * rather than a clipped or zero-height popup. + */ +internal fun PopupWindow.capHeightToSpaceBelow( + anchorView: View, + content: View, +) { + val visibleFrame = Rect() + anchorView.getWindowVisibleDisplayFrame(visibleFrame) + + val anchorOnScreen = IntArray(2) + anchorView.getLocationOnScreen(anchorOnScreen) + val spaceBelow = visibleFrame.bottom - (anchorOnScreen[1] + anchorView.height) + if (spaceBelow <= 0) return + + content.measure( + View.MeasureSpec.makeMeasureSpec(visibleFrame.width(), View.MeasureSpec.AT_MOST), + View.MeasureSpec.makeMeasureSpec(0, View.MeasureSpec.UNSPECIFIED), + ) + + if (content.measuredHeight > spaceBelow) { + height = spaceBelow + } } diff --git a/app/src/main/java/com/itsaky/androidide/utils/EditorActivityActions.kt b/app/src/main/java/com/itsaky/androidide/utils/EditorActivityActions.kt index 64895fd1bd..d1866e0417 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/EditorActivityActions.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/EditorActivityActions.kt @@ -44,6 +44,7 @@ import com.itsaky.androidide.actions.etc.LaunchAppAction import com.itsaky.androidide.actions.file.CloseAllFilesAction import com.itsaky.androidide.actions.file.CloseFileAction import com.itsaky.androidide.actions.file.CloseOtherFilesAction +import com.itsaky.androidide.actions.file.CreateLinkAction import com.itsaky.androidide.actions.file.FormatCodeAction import com.itsaky.androidide.actions.file.InstallFileAction import com.itsaky.androidide.actions.file.SaveFileAction @@ -117,6 +118,7 @@ class EditorActivityActions { registry.registerAction(CloseOtherFilesAction(context, order++)) registry.registerAction(CloseAllFilesAction(context, order++)) registry.registerAction(InstallFileAction(context, order++)) + registry.registerAction(CreateLinkAction(context, order++)) // file tree actions registry.registerAction(CopyPathAction(context, ORDER_COPY_PATH)) diff --git a/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt b/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt index 10db33199a..eebbae8644 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt @@ -147,10 +147,77 @@ internal fun isDeepLinkTargetOfOpenProject( projectName: String, projectsRoot: File, ): Boolean { + deepLinkTargetOfOpenProjectWithoutIo(openProjectPath, projectName, projectsRoot)?.let { return it } + + // Deliberately NOT `deepLinkTargetOfOpenProjectOrNull(...) ?: false`, which was tried on the + // grounds that it could not change behaviour. It can: this compares per side, so when exactly one + // side's canonicalPath fails it still compares that side's absolutePath against the other's + // canonical path -- and those can match, e.g. a projectsRoot given as /sdcard/CodeOnTheGoProjects + // against an open project under /storage/emulated/0/CodeOnTheGoProjects whose parent has just + // become unreadable. The nullable variant returns null on the first failure, which collapses to + // false, and at this function's two callers a false means a same-project deep link is treated as + // a project switch: the project is closed and reopened instead of the file simply being shown. + // + // So the two functions differ on purpose. This one stays lenient for callers that must answer + // now; the nullable one is strict for a caller that will REMEMBER the answer, where a wrong false + // outlives the condition that caused it. + val parent = File(openProjectPath).parentFile ?: return false + return canonicalOrAbsolute(parent) == canonicalOrAbsolute(projectsRoot) +} + +/** + * [isDeepLinkTargetOfOpenProject] with "could not tell" kept apart from "no", for a caller that + * remembers the answer. + * + * The Boolean version cannot express the difference: [canonicalOrAbsolute] turns a failed + * `canonicalPath` into the plain `absolutePath` and the comparison then yields `false`, which is + * indistinguishable from a genuine mismatch. A caller that caches that `false` latches "not + * linkable" for the rest of the process over what may have been a momentary EACCES after a + * permission change, or an EIO on a flaky external volume. Same reasoning, and same shape, as + * [ProjectNameLookup.Unverifiable]. + */ +internal fun deepLinkTargetOfOpenProjectOrNull( + openProjectPath: String, + projectName: String, + projectsRoot: File, +): Boolean? { + deepLinkTargetOfOpenProjectWithoutIo(openProjectPath, projectName, projectsRoot)?.let { return it } + + // Non-null by construction: the shortcut returns false, not null, when there is no parent, so + // deferring to here already established one. + val parent = File(openProjectPath).parentFile ?: return false + val parentPath = runCatching { parent.canonicalPath }.getOrNull() ?: return null + val rootPath = runCatching { projectsRoot.canonicalPath }.getOrNull() ?: return null + return parentPath == rootPath +} + +/** + * The part of [isDeepLinkTargetOfOpenProject] decidable without any filesystem call, for callers + * that must answer on a thread where I/O is not allowed -- `null` means "only canonicalisation can + * tell", so the caller has to go off-thread for the rest. + * + * Lives here, beside the full rule, rather than being reimplemented at the call site: a writer with + * its own private copy of "is this project linkable" would keep answering the old question if this + * rule were ever tightened. + */ +internal fun deepLinkTargetOfOpenProjectWithoutIo( + openProjectPath: String, + projectName: String, + projectsRoot: File, +): Boolean? { if (openProjectPath.isBlank()) return false val open = File(openProjectPath) if (!projectNamesMatch(open.name, projectName)) return false - return canonicalOrAbsolute(open.parentFile ?: return false) == canonicalOrAbsolute(projectsRoot) + val parent = open.parentFile ?: return false + + // Equal path strings name the same directory, so canonicalising both sides could only agree. + // This is the case for every project reached through the projects list or a deep link, since + // both are resolved against projectsRoot to begin with. + if (parent.absolutePath == projectsRoot.absolutePath) return true + + // They differ as text, so a symlink on either side may still make them one directory -- and only + // canonicalPath can say, which is exactly the call this function exists to avoid. + return null } private fun canonicalOrAbsolute(file: File): String = runCatching { file.canonicalPath }.getOrElse { file.absolutePath } diff --git a/app/src/main/res/layout/file_action_popup_window.xml b/app/src/main/res/layout/file_action_popup_window.xml index da481701b1..64df3d1457 100644 --- a/app/src/main/res/layout/file_action_popup_window.xml +++ b/app/src/main/res/layout/file_action_popup_window.xml @@ -1,8 +1,21 @@ - - - + + + + + diff --git a/app/src/test/java/com/itsaky/androidide/actions/file/CreateLinkActionTest.kt b/app/src/test/java/com/itsaky/androidide/actions/file/CreateLinkActionTest.kt new file mode 100644 index 0000000000..757819bf13 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/actions/file/CreateLinkActionTest.kt @@ -0,0 +1,94 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.actions.file + +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.models.DeepLinkRequest +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import java.io.File + +/** + * The non-UI half of [CreateLinkAction]: deciding what path a link may name. A link gets handed to + * another person, so "outside the project" has to be a hard no rather than a best effort. + */ +@RunWith(RobolectricTestRunner::class) +class CreateLinkActionTest { + private val project = File("/storage/emulated/0/CodeOnTheGoProjects/MyApp") + + @Test + fun `a file inside the project relativises to a slash-separated path`() { + assertThat(projectRelativePathOrNull(project, File(project, "app/src/main/Main.kt"))) + .isEqualTo("app/src/main/Main.kt") + } + + @Test + fun `a file at the project root relativises to its bare name`() { + assertThat(projectRelativePathOrNull(project, File(project, "settings.gradle.kts"))) + .isEqualTo("settings.gradle.kts") + } + + @Test + fun `a file outside the project is refused rather than escaping with dot-dot`() { + // relativeToOrNull walks UP with ".." instead of failing, so a missing containment check here + // would emit a link naming a file the project does not contain. + assertThat(projectRelativePathOrNull(project, File("/storage/emulated/0/Download/secrets.txt"))).isNull() + + val sibling = File("/storage/emulated/0/CodeOnTheGoProjects/OtherApp/app/Main.kt") + assertThat(projectRelativePathOrNull(project, sibling)).isNull() + + // The project's own parent, which relativises to exactly "..". + assertThat(projectRelativePathOrNull(project, project.parentFile)).isNull() + } + + @Test + fun `the project directory itself has no relative path to name`() { + assertThat(projectRelativePathOrNull(project, project)).isNull() + } + + @Test + fun `a hidden file inside the project is allowed`() { + assertThat(projectRelativePathOrNull(project, File(project, ".gitignore"))).isEqualTo(".gitignore") + } + + @Test + fun `an unrelated absolute path elsewhere on the filesystem is refused`() { + // Not "a different volume": on Android both of these share the root "/", so relativeToOrNull + // succeeds and returns a "../.."-prefixed path. It is the containment check that refuses it, + // which is the branch worth covering anyway. + assertThat(projectRelativePathOrNull(project, File("/data/local/tmp/Main.kt"))).isNull() + } + + @Test + fun `the cursor is converted from zero-based to one-based`() { + assertThat(oneBasedCursorPosition(0, 0)).isEqualTo(1 to 1) + assertThat(oneBasedCursorPosition(9, 4)).isEqualTo(10 to 5) + } + + @Test + fun `a cursor at the very start of a file still yields a link`() { + // Why the conversion is load-bearing rather than cosmetic: buildUrl refuses a line or column + // below 1, so passing the raw zero-based cursor produces no link at all -- and the action + // hides the menu item, which looks exactly like the ordinary hidden-when-unlinkable state. + val (line, column) = oneBasedCursorPosition(0, 0) + assertThat(DeepLinkRequest.buildUrl("MyApp", "Main.kt", line, column)) + .endsWith("/file/Main.kt/line/1/column/1") + assertThat(DeepLinkRequest.buildUrl("MyApp", "Main.kt", 0, 0)).isNull() + } +} diff --git a/app/src/test/java/com/itsaky/androidide/models/DeepLinkBuildUrlTest.kt b/app/src/test/java/com/itsaky/androidide/models/DeepLinkBuildUrlTest.kt new file mode 100644 index 0000000000..a08c8c197d --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/models/DeepLinkBuildUrlTest.kt @@ -0,0 +1,251 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.models + +import android.net.Uri +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * [DeepLinkRequest.buildUrl] -- the write side of the deep-link scheme, checked mostly by + * round-tripping through [DeepLinkRequest.parse], which is the only consumer that matters. + */ +@RunWith(RobolectricTestRunner::class) +class DeepLinkBuildUrlTest { + private fun roundTrip(url: String?) = DeepLinkRequest.parse(Uri.parse(url!!)) + + @Test + fun `project only`() { + val url = DeepLinkRequest.buildUrl(projectName = "MyApp") + assertThat(url).isEqualTo("https://appdevforall.org/device/open/project/MyApp") + assertThat(roundTrip(url)).isEqualTo(DeepLinkRequest(projectName = "MyApp")) + } + + @Test + fun `file with line and column`() { + val url = DeepLinkRequest.buildUrl("MyApp", "src/main/Main.kt", line = 7, column = 3) + assertThat(url) + .isEqualTo("https://appdevforall.org/device/open/project/MyApp/file/src/main/Main.kt/line/7/column/3") + assertThat(roundTrip(url)) + .isEqualTo( + DeepLinkRequest( + projectName = "MyApp", + fileRequest = PendingFileRequest(filePath = "src/main/Main.kt", lineRaw = "7", columnRaw = "3"), + ), + ) + } + + @Test + fun `path separators stay separators, they are not percent-encoded away`() { + // Uri.Builder.appendPath encodes '/' inside a single segment, so the whole relative path must + // be appended one component at a time. Getting this wrong still round-trips -- it just emits + // an unreadable ".../file/src%2Fmain%2FMain.kt" -- so assert the emitted shape, not just parse. + val url = DeepLinkRequest.buildUrl("MyApp", "src/main/Main.kt") + assertThat(url).doesNotContain("%2F") + assertThat(url).endsWith("/file/src/main/Main.kt") + } + + @Test + fun `space in project name is encoded and survives the round trip`() { + val url = DeepLinkRequest.buildUrl("My App", "Main.kt", line = 1, column = 1) + assertThat(url).contains("/project/My%20App/") + assertThat(roundTrip(url)?.projectName).isEqualTo("My App") + } + + @Test + fun `hash in a filename is encoded rather than truncating the path into a fragment`() { + val url = DeepLinkRequest.buildUrl("MyApp", "notes/Draft#1.md", line = 4, column = 2) + assertThat(url).contains("Draft%231.md") + assertThat(roundTrip(url)?.fileRequest?.filePath).isEqualTo("notes/Draft#1.md") + } + + @Test + fun `question mark in a filename is encoded rather than starting a query`() { + val url = DeepLinkRequest.buildUrl("MyApp", "Why?.txt") + assertThat(url).contains("Why%3F.txt") + assertThat(roundTrip(url)?.fileRequest?.filePath).isEqualTo("Why?.txt") + } + + @Test + fun `decomposed accented name survives the round trip byte for byte`() { + // NFD: "Cafe" + COMBINING ACUTE ACCENT, the form a directory cloned from macOS carries. + val nfd = "Cafe\u0301" + val url = DeepLinkRequest.buildUrl(nfd, "Main.kt") + assertThat(url).contains("Cafe%CC%81") + assertThat(roundTrip(url)?.projectName).isEqualTo(nfd) + } + + @Test + fun `a directory named line does not swallow the file path when line and column are written`() { + // The shape parse() documents as unresolvable, defused: because a real trailing line/column + // pair always follows, peelTrailingKeyword consumes that pair and never reaches the "line" + // directory in the path itself. + val url = DeepLinkRequest.buildUrl("MyApp", "src/line/5", line = 7, column = 3) + assertThat(roundTrip(url)) + .isEqualTo( + DeepLinkRequest( + projectName = "MyApp", + fileRequest = PendingFileRequest(filePath = "src/line/5", lineRaw = "7", columnRaw = "3"), + ), + ) + } + + @Test + fun `refuses the same path with no line, because parse would read it as metadata`() { + // Without a trailing line/column pair to consume, parse() peels "line/5" off the path itself + // and reads this back as file "src" at line 5. buildUrl's round-trip check catches that and + // returns null rather than emitting a link that opens the wrong file. + assertThat(DeepLinkRequest.buildUrl("MyApp", "src/line/5")).isNull() + assertThat(DeepLinkRequest.buildUrl("MyApp", "src/column/5")).isNull() + + // Not over-refusing: the same path IS expressible once a real pair follows it. + assertThat(DeepLinkRequest.buildUrl("MyApp", "src/line/5", line = 7, column = 3)).isNotNull() + } + + @Test + fun `the shared project-name rule is exactly what buildUrl enforces`() { + // One rule, two callers: buildUrl and the UI deciding whether to offer a link. Pinned together + // because a private copy in the UI is what previously offered a link for a ".foo" project and + // then failed on the tap. + for (name in listOf("MyApp", "My App", "Cafe\u0301", "we ird-name_1")) { + assertThat(DeepLinkRequest.isLinkableProjectName(name)).isTrue() + assertThat(DeepLinkRequest.buildUrl(name)).isNotNull() + } + for (name in listOf("", ".", "..", ".hidden", "nested/MyApp", "nested\\MyApp")) { + assertThat(DeepLinkRequest.isLinkableProjectName(name)).isFalse() + assertThat(DeepLinkRequest.buildUrl(name)).isNull() + } + } + + @Test + fun `rejects dot segments the reader would refuse or an intermediary would rewrite`() { + // isProjectCandidateDir() refuses any name starting with '.', so none of these can name a + // project; "." and ".." are worse than unopenable, since a URL-normalizing browser or + // messenger rewrites them into a different path in transit. + assertThat(DeepLinkRequest.buildUrl(".")).isNull() + assertThat(DeepLinkRequest.buildUrl("..")).isNull() + assertThat(DeepLinkRequest.buildUrl(".hidden")).isNull() + + assertThat(DeepLinkRequest.buildUrl("MyApp", "../../etc/passwd")).isNull() + assertThat(DeepLinkRequest.buildUrl("MyApp", "src/./Main.kt")).isNull() + + // A hidden FILE is legitimate, unlike a hidden project directory -- resolveWithinDirectory + // rejects traversal, not dot-prefixed names. + assertThat(DeepLinkRequest.buildUrl("MyApp", ".gitignore", line = 1, column = 1)).isNotNull() + } + + @Test + fun `never returns a url that parses back as something else`() { + // The contract the round-trip check exists to hold. Anything buildUrl returns must read back + // as exactly the arguments it was given. + val cases = + listOf( + Triple("MyApp", "src/main/Main.kt", 7 to 3), + Triple("My App", "a b/c d.kt", 1 to 1), + Triple("Cafe\u0301", "notes/Draft#1.md", 12 to 40), + Triple("MyApp", "src/line/5", 7 to 3), + Triple("MyApp", "src/column/9/line/2", 4 to 6), + ) + for ((project, path, coords) in cases) { + val (line, column) = coords + val url = DeepLinkRequest.buildUrl(project, path, line, column) + assertThat(url).isNotNull() + assertThat(roundTrip(url)) + .isEqualTo( + DeepLinkRequest( + projectName = project, + fileRequest = + PendingFileRequest( + filePath = path, + lineRaw = line.toString(), + columnRaw = column.toString(), + ), + ), + ) + } + } + + @Test + fun `refuses a column with no line, which the reader would silently apply to line 1`() { + // parse() can read this shape, so the previous version of this test asserted it round-trips -- + // but zeroBasedOrInvalid(null) yields 0, so the reader puts the cursor at line 1 with the given + // column: a position the link never named, and no invalid-value message either. That is the + // same quiet wrongness a line with no file is refused for. + assertThat(DeepLinkRequest.buildUrl("MyApp", "Main.kt", line = null, column = 3)).isNull() + } + + @Test + fun `refuses paths the reader rejects lexically, not just the ones with slash components`() { + // The reader's isLexicallyRejected splits on '\\' too, so these are traversal to it while a + // guard looking only at '/' components sees one harmless filename. + assertThat(DeepLinkRequest.buildUrl("MyApp", "a\\..\\b.kt", line = 1, column = 1)).isNull() + assertThat(DeepLinkRequest.buildUrl("MyApp", "\\Main.kt", line = 1, column = 1)).isNull() + assertThat(DeepLinkRequest.buildUrl("MyApp", "/Main.kt", line = 1, column = 1)).isNull() + + // A character the reader's base.resolve() cannot accept: encodes and round-trips fine here, + // and is rejected there (lexicalResolve catches InvalidPathException), so the link opens + // nothing. + assertThat(DeepLinkRequest.buildUrl("MyApp", "a\u0000b.kt", line = 1, column = 1)).isNull() + + // A backslash that is NOT traversal stays linkable -- the guard mirrors the reader, it does + // not blanket-ban a legal filename character. + assertThat(DeepLinkRequest.buildUrl("MyApp", "we\\ird.kt", line = 1, column = 1)).isNotNull() + } + + @Test + fun `rejects a project name that cannot name a direct child of the projects root`() { + assertThat(DeepLinkRequest.buildUrl("")).isNull() + assertThat(DeepLinkRequest.buildUrl("nested/MyApp")).isNull() + assertThat(DeepLinkRequest.buildUrl("nested\\MyApp")).isNull() + } + + @Test + fun `rejects an empty path component, which would emit a rejected double slash`() { + assertThat(DeepLinkRequest.buildUrl("MyApp", "src//Main.kt")).isNull() + assertThat(DeepLinkRequest.buildUrl("MyApp", "/Main.kt")).isNull() + assertThat(DeepLinkRequest.buildUrl("MyApp", "")).isNull() + } + + @Test + fun `rejects a line or column that parse would report back as invalid`() { + assertThat(DeepLinkRequest.buildUrl("MyApp", "Main.kt", line = 0)).isNull() + assertThat(DeepLinkRequest.buildUrl("MyApp", "Main.kt", line = -1)).isNull() + assertThat(DeepLinkRequest.buildUrl("MyApp", "Main.kt", line = 1, column = 0)).isNull() + } + + @Test + fun `rejects a line with no file to apply it to`() { + assertThat(DeepLinkRequest.buildUrl("MyApp", filePath = null, line = 7)).isNull() + } + + @Test + fun `the length ceiling falls exactly on 512 decoded characters`() { + // Pinned either side of the boundary, not merely far from it: with only a 400 and a 600 case, + // flipping the guard to >= (or to > LIMIT + 1, which would emit a link parse then rejects) + // left the suite green. "/device/open/project/P/file/" is 28 characters, so a file path of + // (512 - 28) puts the decoded path at exactly the limit. + val prefixLength = "/device/open/project/P/file/".length + assertThat(prefixLength).isEqualTo(28) + + assertThat(DeepLinkRequest.buildUrl("P", "a".repeat(512 - prefixLength))).isNotNull() + assertThat(DeepLinkRequest.buildUrl("P", "a".repeat(513 - prefixLength))).isNull() + assertThat(DeepLinkRequest.buildUrl("P", "a".repeat(600))).isNull() + } +} diff --git a/app/src/test/java/com/itsaky/androidide/models/DeepLinkManifestPrefixTest.kt b/app/src/test/java/com/itsaky/androidide/models/DeepLinkManifestPrefixTest.kt new file mode 100644 index 0000000000..c05d49a9af --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/models/DeepLinkManifestPrefixTest.kt @@ -0,0 +1,76 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.models + +import android.net.Uri +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.utils.FileProvider +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * The deep-link path prefix is spelled in two places that cannot see each other: `DeepLinkRequest`, + * which builds and parses URLs, and `AndroidManifest.xml`, whose `android:pathPrefix` decides + * whether Android delivers the link to this app at all. + * + * Nothing else catches a disagreement. Change the Kotlin side alone and `buildUrl` and `parse` still + * agree with each other, every other test stays green, and every generated link quietly opens in a + * browser instead of the editor. + */ +@RunWith(RobolectricTestRunner::class) +class DeepLinkManifestPrefixTest { + @Test + fun `the manifest's deep-link filter matches the scheme, host and path buildUrl emits`() { + // Located from the repo's own root sentinel rather than by guessing relative paths: the + // previous two-guess list failed as "manifest missing" under any working directory that was + // neither the module nor the repo root, which is the misdiagnosis it was meant to avoid. + val manifest = FileProvider.projectRoot().resolve("app/src/main/AndroidManifest.xml").toFile() + assertThat(manifest.isFile).isTrue() + + // Taken from a URL the builder actually produces, so every assertion below is against the + // emitted shape rather than a second copy of a constant. + val emitted = Uri.parse(DeepLinkRequest.buildUrl("MyApp", "Main.kt", line = 1, column = 1)!!) + + // The elements are selected by the host the builder emits, not by a hard-coded domain. A + // literal here would be a third copy of CANONICAL_HOST -- and renaming the domain correctly + // in both Kotlin and the manifest would then fail this test for a change with no drift at all. + val elements = + Regex("""]*>""", RegexOption.DOT_MATCHES_ALL) + .findAll(manifest.readText()) + .map { it.value } + .filter { attribute(it, "android:host") == emitted.host } + .toList() + + // Not a vacuous pass: if no element names the emitted host, delivery is broken and that + // is the drift this test exists to catch. + assertThat(elements).isNotEmpty() + + // All three, because an intent-filter matches on all three. A path-only assertion stayed green + // through a changed CANONICAL_HOST or SCHEME, which breaks delivery just as surely. + for (element in elements) { + assertThat(emitted.path).startsWith(attribute(element, "android:pathPrefix")) + assertThat(attribute(element, "android:scheme")).isEqualTo(emitted.scheme) + } + } + + private fun attribute( + element: String, + name: String, + ): String? = Regex("""$name\s*=\s*"([^"]*)"""").find(element)?.groupValues?.get(1) +} diff --git a/app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt b/app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt index 02ce033d3b..bd003a54cf 100644 --- a/app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt +++ b/app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt @@ -147,4 +147,94 @@ class ProjectValidationsTest { assertThat(findValidProjectByName(root, name)).isEqualTo(expected) } } + + @Test + fun `the no-IO shortcut answers the same-parent case and defers the rest`() { + val root = tempFolder.newFolder("projects") + + // Same parent by path: decidable with no filesystem call at all. + assertThat(deepLinkTargetOfOpenProjectWithoutIo(File(root, "MyApp").path, "MyApp", root)).isTrue() + + // Definitively not this project, also decidable without I/O. + assertThat(deepLinkTargetOfOpenProjectWithoutIo(File(root, "MyApp").path, "Other", root)).isFalse() + assertThat(deepLinkTargetOfOpenProjectWithoutIo("", "MyApp", root)).isFalse() + + // Parent differs as text, so only canonicalisation can settle it -- the caller must go + // off-thread rather than treat this as a "no". + val elsewhere = tempFolder.newFolder("elsewhere") + assertThat(deepLinkTargetOfOpenProjectWithoutIo(File(elsewhere, "MyApp").path, "MyApp", root)).isNull() + } + + @Test + fun `the shortcut agrees with an independent canonical comparison wherever it commits`() { + val root = tempFolder.newFolder("projects2") + + // Compared against a separately computed answer, not against isDeepLinkTargetOfOpenProject: + // that function now returns the shortcut's value verbatim in this range, so asserting the two + // agree would hold even if the shortcut were inverted to always return false. + fun independentlyLinkable( + path: String, + name: String, + ): Boolean { + if (path.isBlank()) return false + val open = File(path) + // Normalised on both sides, matching projectNamesMatch. Comparing with plain == would + // leave the rule's NFD tolerance unpinned: dropping it from the shortcut would still pass. + if (Normalizer.normalize(open.name, Normalizer.Form.NFC) != Normalizer.normalize(name, Normalizer.Form.NFC)) { + return false + } + val parent = open.parentFile ?: return false + return parent.canonicalPath == root.canonicalPath + } + + val nfd = "Cafe\u0301" + val nfc = Normalizer.normalize(nfd, Normalizer.Form.NFC) + val cases = + listOf( + File(root, "MyApp").path to "MyApp", + File(root, "MyApp").path to "Other", + "" to "MyApp", + // A project on disk in decomposed form, named in the link in composed form -- the + // macOS-clone case lookupValidProjectByName tries both forms for. + File(root, nfd).path to nfc, + File(root, nfc).path to nfd, + ) + for ((path, name) in cases) { + val shortcut = deepLinkTargetOfOpenProjectWithoutIo(path, name, root) + assertThat(shortcut).isNotNull() + assertThat(shortcut).isEqualTo(independentlyLinkable(path, name)) + } + } + + @Test + fun `the nullable variant resolves a symlinked parent that differs as text`() { + val root = tempFolder.newFolder("real-projects") + val project = File(root, "MyApp") + assertThat(project.mkdirs()).isTrue() + + // An alias to the same directory, the shape /sdcard -> /storage/self/primary has on a device. + val alias = File(tempFolder.root, "alias") + java.nio.file.Files + .createSymbolicLink(alias.toPath(), root.toPath()) + + val viaAlias = File(alias, "MyApp").path + // The no-IO shortcut cannot settle this -- the parents differ as text. + assertThat(deepLinkTargetOfOpenProjectWithoutIo(viaAlias, "MyApp", root)).isNull() + // Canonicalising does, and that is the branch the nullable variant exists for. + assertThat(deepLinkTargetOfOpenProjectOrNull(viaAlias, "MyApp", root)).isTrue() + } + + @Test + fun `the nullable variant reports null, not false, when a path cannot be canonicalised`() { + val root = tempFolder.newFolder("projects3") + + // A NUL makes getCanonicalPath throw IOException. "Could not tell" must stay distinct from + // "no": a caller that caches false here latches "not linkable" for the whole process over a + // condition that may be momentary. + val unresolvable = File("/nonexistent\u0000dir/MyApp").path + assertThat(deepLinkTargetOfOpenProjectOrNull(unresolvable, "MyApp", root)).isNull() + + // And the Boolean form collapses that to false for callers that only want a Boolean. + assertThat(isDeepLinkTargetOfOpenProject(unresolvable, "MyApp", root)).isFalse() + } } diff --git a/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt index e3c2c8adcb..1c25e364b9 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt @@ -250,8 +250,13 @@ class ContainedPathResolver( * with a more lenient fallback for paths [resolve] refused ([ZipUtils]' skip of an existing * symlink) can apply the same reject first: an entry that fails here is a bad archive * however the filesystem looks, never fallback material. + * + * Public so a *writer* of these paths can refuse what this reader would -- the deep-link URL + * builder in the app module is one, and would otherwise re-spell a subset of this rule and + * drift from it. Named in prose rather than linked: `common` does not depend on `app`, and + * must not, so a KDoc link that way would never resolve. */ - internal fun isLexicallyRejected(relativePath: String): Boolean = + fun isLexicallyRejected(relativePath: String): Boolean = // Split on both separators: '\' is not a path separator on Android, but a caller handing // over a Windows-style path should not have it treated as one long filename. relativePath.isEmpty() || diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index 9d24ea5d36..f3a8ed20e7 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -146,6 +146,9 @@ \"%s\" is not a valid column number. (no value given) Could not scan projects for this link. + Link copied to the clipboard. + Could not create a link for this file. + %1$s link A project close is already in progress. Try again in a moment. Create new project Open a saved project @@ -281,6 +284,8 @@ Install Close others Close this + Close tab + Create link All files saved Failed to save files Destination: %s/