Skip to content

fix(settings-permissions): report Android permissions on evidence, not on an exit code - #690

Open
filip131311 wants to merge 1 commit into
mainfrom
filip/android-permission-verify
Open

fix(settings-permissions): report Android permissions on evidence, not on an exit code#690
filip131311 wants to merge 1 commit into
mainfrom
filip/android-permission-verify

Conversation

@filip131311

Copy link
Copy Markdown
Collaborator

Fixes #616.

Reproduced on an API 36 emulator

com.anonymous.myapp declares: SYSTEM_ALERT_WINDOW INTERNET READ_EXTERNAL_STORAGE
                              ACCESS_NETWORK_STATE WRITE_EXTERNAL_STORAGE VIBRATE

CAMERA rows before : 0
$ pm grant com.anonymous.myapp android.permission.CAMERA   → exit=0
CAMERA rows after  : 0

settings-permissions grant camera → { "applied": ["android.permission.CAMERA"] }

Nothing was granted, nothing recorded, exit 0 — and success was decided from that exit code (android.ts:240-244), so skipped was unreachable for the case the tool's own description says it describes.

The failure is per-permission, not per-call, and that shapes everything. One deny media-library returned:

applied: ["android.permission.READ_MEDIA_AUDIO", "android.permission.READ_EXTERNAL_STORAGE"]

dumpsys: READ_EXTERNAL_STORAGE: granted=false   ← real, declared, genuinely flipped
         READ_MEDIA_AUDIO                        ← absent entirely

So failing the whole call would have been as wrong as passing it.

The fix

Each permission is checked against the package manager's own state after the change — one read for the whole fan-out, not one per permission (7.4 KB / ~50 ms; skipped entirely when nothing claimed to land).

Reading the grant state alone would not have fixed this. For deny the target is "not granted", and a permission the app never declared is also not granted — so the reported case would still have passed. The declaration list settles that one; the grant state settles everything else. A runtime row outranks the declaration list, so a permission the platform splits into an app is judged on its real state.

The read only ever demotes. When the dump can't be parsed — older device, unfamiliar layout, failed read — the command's own verdict stands, because refusing to believe a success we can't check would break every device whose state we can't read. Those entries now appear in a new unverified list, so a caller can tell a confirmed change from one taken on trust. Without it the fix would have relocated the ambiguity rather than removed it.

Shared-user packages needed following. A package with android:sharedUserId keeps its runtime state in a separate top-level section keyed by the shared-user name, not the package name. On the reference device 39 of 249 packages are in that group — including Maps, Calendar and Settings. Without the fallback the fix would have silently done nothing for exactly the apps most likely to be targeted. (Found by an adversarial review that scanned every installed package; my first design would have shipped with this hole.)

deny/reset of an undeclared permission is now "already satisfied", not an error — the app cannot hold it, which is what the caller asked for. Erroring there would break the most ordinary setup call there is ("make sure this app has no camera access") on any app that doesn't use the hardware. grant still fails when nothing took effect, with wording that no longer claims anything was rejected.

Live verification

grant camera (undeclared)
  before: applied: [CAMERA]
  after : error — "no mapped runtime permission took effect … the app's manifest does not declare it"

deny media-library (mixed)
  before: applied: [READ_MEDIA_AUDIO, READ_EXTERNAL_STORAGE]
  after : applied: [READ_EXTERNAL_STORAGE]   skipped: [READ_MEDIA_AUDIO]

The parser was also validated against real dumps for three packages including a shared-user one — Calendar resolves 26 declared / 6 runtime rows through the fallback, and its READ_CALENDAR verifies as confirmed.

Tests

8 new cases: undeclared reported as skipped; the mixed case split correctly; a genuine change still applied with exactly one read for a 4-permission fan-out; unparseable state trusted and marked unverified; a dead read not failing a landed change; the grant-took-effect-nowhere error; deny of an undeclared permission satisfied; and a package-manager rejection keeping its own more actionable wording rather than being overwritten or promoted.

Mutation-verified: restoring exit-code trust fails 4.

Two existing tests needed updating — they assert exact command lists and now see the appended read. My plan had claimed "zero test churn"; that was wrong, and the review caught it.

Full tool-server suite green — 3094 passed.

Notes

  • iOS is untouched. Its permission commands fail loudly, so there's no equivalent silent no-op, and there's no cheap way to read TCC back. applied is therefore a stronger claim on Android than on iOS — the result type now says so rather than papering over it.
  • A risk I flagged for myself and the review resolved as not real: I suspected granting READ_MEDIA_VISUAL_USER_SELECTED might auto-revoke the full-access media permissions on API 34+, which would cause false demotions. Four packages on the device hold all three simultaneously, so the fan-out order is unchanged.
  • Not verifiable on this host: pre-API-36 devices, where the package manager still rejects outright. The design handles disagreement by letting the rejection win and keeping its wording, so that path is unchanged by construction — but a pass on an older image is worth adding to a merge checklist, along with one shared-user package.

…t on an exit code

Android 16 accepts a request to grant a permission an app never declared and
silently does nothing — the command exits 0, no state is written. Success was
decided from that exit code, so the permission came back in `applied` and the
`skipped` list the tool documents was unreachable for the case it exists to
describe.

Measured on an API 36 emulator: granting CAMERA to an app that doesn't declare it
exits 0 with no record of the permission before or after, and the tool reported
it applied.

The failure is per-permission, not per-call, which is what shapes the fix. One
`deny media-library` returned both READ_EXTERNAL_STORAGE, which genuinely flipped,
and READ_MEDIA_AUDIO, which the package doesn't have — so failing the whole call
would have been as wrong as passing it.

Each permission is now checked against the package manager's own state after the
change, with one read for the whole fan-out rather than one per permission.

Reading the grant state alone would not have fixed this. For `deny` the target is
"not granted", and a permission the app never declared is also not granted — so
the reported case would still have passed. The declaration list is what settles
it, and the grant state settles everything else.

The read only ever demotes. When it cannot be parsed — an older device, an
unfamiliar layout, a read that failed — the command's own verdict stands, because
refusing to believe a success we cannot check would break every device whose
state we cannot read. But those entries are now listed in `unverified`, so a
caller can tell a confirmed change from one taken on trust. Leaving that out
would have moved the ambiguity rather than removing it.

Packages that share a user id keep their runtime state in a separate section
keyed by the shared-user name, not the package name. Roughly one in six packages
on a stock image is one — Maps, Calendar and Settings among them — so without
following that link the fix would have silently done nothing for the apps most
likely to be targeted.

Denying a permission an app never declared is now treated as already satisfied
rather than an error: the app cannot hold it, which is what the caller asked for.
Erroring there would have broken the most ordinary setup call there is on any app
that simply doesn't use the hardware. Granting still fails when nothing took
effect, with wording that no longer claims anything was rejected.

iOS is untouched. Its permission commands fail loudly, so there is no equivalent
silent no-op, and there is no cheap way to read TCC back. `applied` is therefore
a stronger claim on Android than on iOS, which the result type now says.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

settings-permissions over-reports on Android 16 (API 36): pm grant exits 0 for undeclared permissions, skipped is never returned

1 participant