[abdulmajidcse]

Pull Request Checklist

Q	A
Branch?	4.x
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	-
License	MIT

Description

This fixes four security issues.

• Trailing-slash redirect: RouteLauncher redirected to the raw REQUEST_URI, so a URI starting with // or /\ could send the browser to another site. The target now always starts with exactly one slash. The check runs on the raw path, the query string is kept unchanged, and a URI with control characters is never redirected, because browsers drop tabs and newlines from a Location.
• Error views: the 401, 403, 404, 429 and 500 views echoed $message unescaped, and the 404 message includes the request path. They now escape it, as the 400 view already does.
• JSON errors: JsonErrorRenderer returned file, line and trace in every environment. These now appear only when APP_DEBUG is on. With debugging off, the exception's own message is shown only for an HttpException with a 4xx status and for the CSRF mismatch; everything else says Something went wrong.
• APP_DEBUG as a string: WebErrorHandler treated the string "false" as true, for example from a server environment variable, and showed the debug page. A new app_debug() helper parses the value as a boolean, and both error paths use it.

Behavior changes

• With debugging off, JSON errors return "errors": [] instead of file, line and trace.
• With debugging off, JSON errors from anything other than a 4xx HttpException or a CSRF mismatch say Something went wrong, unless the caller passes explicit details.
• HTML in an error message now displays as plain text in the bundled views.
• /users/?next=/ now redirects to /users?next=/, and /users?next=/ no longer redirects.

Testing

• Added regression tests for all four fixes.
• The full suite passes (5922 tests, no failures, 1121 skipped), and so does PHPStan.
• Not tested in a full application behind Nginx or Apache.

Checklist

• Tests have been added or updated
• Documentation has been updated if necessary
• Code follows the project coding standards
• All tests pass locally

[techmahedy]

Thanks for this @abdulmajidcse . The error-view escaping, the // and /\ redirect fix and the APP_DEBUG string parsing look good. A few things need changing before merge.

1. Keep remember-me working for 2FA users (Authenticate::user())

Please revert the return null in the remember-cookie path. A user with a valid remember cookie should stay signed in, including with 2FA enabled. That is intended behavior, and it is how industry works too.

Please also remove this line from Behavior changes in the description: "A user with 2FA enabled now has to sign in again…"

2. JSON errors can still expose internal messages (JsonErrorRenderer)

With debugging off, the message is shown whenever $statusCode < 500. But for non-HTTP exceptions, JsonErrorHandler takes the status from the exception's code:

$statusCode = (int) $exception->getCode() ?: 500;

So throw new RuntimeException('Cannot connect to 10.0.3.7 as root', 2) becomes status 2, and the message is returned to the client. Please decide by exception type, not by number: show the message only for an HttpException with a 4xx status, and show Something went wrong for everything else. Please add a test with a plain RuntimeException that has a small non-zero code.

3. The trailing-slash redirect test locks in a bug (RouteLauncher)

'query string is kept' => ['/users?next=/', '/users?next='],

The path /users has no trailing slash, yet this returns a 301 and the query value is lost. The check runs against the whole REQUEST_URI instead of the path. Please:

• check the path: str_ends_with($path, '/');
• trim the raw (not URL-decoded) path the same safe way you already do;
• add ?query back unchanged.

Expected results:

• /users/?next=/ → /users?next=/
• /users?next=/ → no redirect

4. Nit: the debug check is written out twice

filter_var(env('APP_DEBUG', false), FILTER_VALIDATE_BOOLEAN) is now in both WebErrorHandler and JsonErrorRenderer. Please move it into one shared helper, so the two can't drift apart again (that drift is what this PR fixes).

[abdulmajidcse]

@techmahedy Thanks for the review. All four are addressed in c82bc12:

1. Reverted the 2FA change, so a valid remember cookie keeps 2FA users signed in, and removed that line from the description.
2. JsonErrorRenderer now decides by exception type: with debugging off, the message shows only for a 4xx HttpException, and everything else says Something went wrong. Tests cover a plain RuntimeException with a small non-zero code. I also applied this to the 422-style branch, which leaked the same way, and kept the fixed CSRF message visible.
3. The redirect now checks the raw path and keeps the query unchanged: /users/?next=/ goes to /users?next=/, and /users?next=/ no longer redirects.
4. Moved the debug check into a shared app_debug() helper.

The full suite and PHPStan pass.

[techmahedy]

Thanks @abdulmajidcse, all four points are addressed in c82bc12:

1. The 2FA change is reverted, and the description no longer mentions it.
2. JSON errors are decided by exception type, and every JsonErrorRenderer caller goes through publicMessage(), including abort() and the fallback paths.
3. The trailing-slash redirect checks the raw path and keeps the query, and the tests pin the expected results.
4. app_debug() is now the only place that reads APP_DEBUG.

One thing before merge: please update the PR title. It still says "2FA", and a squash merge would put that into the history.

Optional: also reject control characters in trailingSlashRedirect(). Browsers drop tabs and newlines, so /\t/evil.com/ would become //evil.com. Nginx and Apache already reject these, so this is only defence in depth.

[abdulmajidcse]

@techmahedy Both are done in 2da65d9:

1. The PR title no longer mentions 2FA.
2. trailingSlashRedirect() no longer redirects a URI with control characters, so /\t/evil.com/ can't become //evil.com. An encoded %09 still redirects within the site, because browsers don't strip it.

The full suite and PHPStan pass locally.

[techmahedy]

Thanks @abdulmajidcse, 2da65d9 covers it