Android: unset the request's header env vars after each dispatch - #337
Open
inoha-kudo wants to merge 1 commit into
Open
Android: unset the request's header env vars after each dispatch#337inoha-kudo wants to merge 1 commit into
inoha-kudo wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #324. Follow-up to #119.
Headers reach PHP as
HTTP_*environment variables, and the environment outlives a dispatch, so a request inherits every header of the one before it: a POST'sContent-Typeis still set when the next GET is dispatched, and so areX-Requested-With,Referer,Origin, and anything a JS library sends on only some requests.#119 addressed the visible symptom by stripping
HTTP_*andCONTENT_*from$_SERVERat the top of each dispatch, but the same eval re-imports the environment twenty lines later:so the stale keys return immediately. The Kotlin band-aid in
PHPBridge.ktcovers this forX-Inertiaonly, and by blanking rather than removing.iOS already does the right thing —
PersistentPHPRuntime.swiftrecords the keys it sets andunsetenvs them after the dispatch. This brings Android to the same behaviour.What changed
handleLaravelRequest()records every header env var it sets (headerEnvKeys), runs the dispatch inside atry, and unsets them infinally— so a failed dispatch, including the native early returns (php_embed_init()failure, "runtime not initialized"), cannot leave its headers behind either. The list is local to the request on the singlephpExecutorthread, so there is no shared state and nothing to lock. The C side gains one small JNI method,nativeUnsetEnv, sinceSystem.getenv()is a JVM snapshot and Kotlin otherwise has no way to remove a process variable.Track-and-remove rather than sweeping
environfor anything matchingHTTP_*, for three reasons: pre-existingHTTP_*variables whose names the current request does not set are left alone (and a request that does send aProxy:header has itsHTTP_PROXYremoved afterwards — the safer outcome either way); there is no name-length cutoff past which a header would escape removal; and it is exactly what iOS does.The five-name Inertia blanking block is gone — it was this fix, restricted to five names and blanking instead of removing.
One deliberate behaviour change: a failed
setenvused to go unnoticed and the request was dispatched with that header missing. It now aborts the request, andfinallystill removes whatever had been set up to that point.Notes:
HTTP_HOSTis re-set by the C side per dispatch (php_bridge.c:286,:487), which is untracked and unchanged — it is constant for the embedded server, so persisting is harmless.native_webview_php_request()is unaffected: it receives its cookies and content type as parameters rather than through the environment, so there is nothing to unset.setenv/unsetenvare not safe against a concurrentgetenv()from another thread. That is a pre-existing property of passing headers through the environment; this change does not add to it — all headersetenv/unsetenvcalls stay on the singlephpExecutorthread, as before.Test plan
Physical device (Pixel 4a 5G, Android 14), release build via
native:package android --build-type=release.Route::get('/leak', fn () => response($_SERVER['HTTP_X_FOO'] ?? '(absent)')); onefetchwithX-Foo: bar, then two without — before:bar,bar,bar; after:bar,(absent),(absent)POSTwithContent-Type: application/x-www-form-urlencodedstill parses into$_POSTafter the changenativeHandleRequest()sits inside the sametry/finallyso the path is covered identically, but I have only exercised the persistent default