Skip to content

Remove the dead security cookie route - #136

Open
sadiqk2 wants to merge 2 commits into
NativePHP:mainfrom
sadiqk2:fix/security-cookie-config-namespace
Open

Remove the dead security cookie route#136
sadiqk2 wants to merge 2 commits into
NativePHP:mainfrom
sadiqk2:fix/security-cookie-config-namespace

Conversation

@sadiqk2

@sadiqk2 sadiqk2 commented Aug 14, 2026

Copy link
Copy Markdown

CreateSecurityCookieController reads config('native-php.secret'), but there is no native-php config namespace. The package ships config/nativephp.php and config/nativephp-internal.php, and grep does not find native-php anywhere else in the repo. So the guard passed only when no secret parameter was supplied, and the cookie was then issued with a null value, which PreventRegularBrowserAccess can never match.

As per review, instead of repairing it: the route is vestigial. Nothing calls _native/api/cookie at runtime. Electron sets _php_native itself through appendCookie() in server/index.ts at boot, and puts X-NativePHP-Secret on every request it makes. So this PR now deletes:

  • the route in routes/api.php
  • CreateSecurityCookieController
  • CreateSecurityCookieControllerTest
  • the _native/api/cookie exemption in PreventRegularBrowserAccess

Net diff is 46 deleted lines with no behaviour change. The deleted route could not issue a usable cookie in the first place, and removing the exemption closes a hole rather than taking away anything that was working.

config('native-php.secret') reads a namespace that does not exist; the config
files are nativephp.php and nativephp-internal.php. The guard therefore compared
user input against null, so it passed only when no secret was supplied, and the
cookie it issued had a null value.

This is also the one route PreventRegularBrowserAccess deliberately exempts, so
it is worth having work as intended.

@gwleuverink gwleuverink left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Confirmed this locally. config('native-php.secret') is null, so the guard only passes when no secret is sent at all, and the cookie it then sets is null, which the middleware can never match.

It does break CreateSecurityCookieControllerTest. That test sets native-php.secret and expects a 403 that only happens because of the bug, so it goes red. It needs pointing at nativephp-internal.secret with the secret actually passed.

Before fixing it though, is the route still doing anything? Nothing calls it at runtime. Electron sets _php_native itself and puts the secret header on every request. I'd rather delete the route, controller, test and the middleware exemption. If we keep it, it should 403 on a null secret instead of issuing an empty cookie.

Nothing calls _native/api/cookie: Electron sets _php_native itself via
appendCookie() at boot and sends X-NativePHP-Secret on every request, so
the route, its controller, its test and the middleware exemption all go.
@sadiqk2 sadiqk2 changed the title Fix config namespace in the security cookie route Remove the dead security cookie route Aug 19, 2026
@sadiqk2

sadiqk2 commented Aug 19, 2026

Copy link
Copy Markdown
Author

Agreed, deleted rather than fixed. a1ddd7a drops the route, CreateSecurityCookieController, CreateSecurityCookieControllerTest and the _native/api/cookie exemption in PreventRegularBrowserAccess. Net 46 deleted lines, nothing added.

That also takes care of the red test you hit. It went away along with the controller, so no need to repoint it at nativephp-internal.secret.

Before deleting I checked the same thing you did. The only writer of _php_native left is appendCookie() in server/index.ts, and notifyLaravel() sends X-NativePHP-Secret, so both middleware paths are still covered. Grep does not find any other reference to the route in this repo. Kindly do have a look at NativePHP/laravel also, in case something there still links to it. I have not checked that repo.

PR title and description are updated accordingly.

@gwleuverink

Copy link
Copy Markdown
Collaborator

The nativephp/laravel repo has been superceeded by this one. The desktop package combined the laravel and electron packages in a single repo, so since grep didnt turn anything up its safe to assume there are no more callers to this route 👌

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.

2 participants