Aias00 commented on PR #6409:
URL: https://github.com/apache/shenyu/pull/6409#issuecomment-5193464634
Good security improvement — credentials in the query string (CWE-598) leak
into access logs, browser history, Referer headers, and proxy logs. Moving to a
POST JSON body with `@Valid` + `@NotBlank` is the right call, and the test
coverage (valid POST + invalid-body fail-closed via the wired
`ExceptionHandlers` controller advice) is solid.
One thing worth flagging: the backward-compat `@GetMapping("/login")` is
kept `@Deprecated` but still fully functional, so the insecure path isn't
actually closed — old clients (or an attacker) can still send credentials in
the query string. That's a reasonable compat trade-off, but the vulnerability
surface only shrinks once GET is removed or restricted (rate-limited / logged /
disabled-by-config). Worth a note on when GET is planned for removal so this
doesn't live as deprecated-but-open indefinitely.
Also: the DTO comment says "password (encrypted on the front-end)", but the
POST test sends `"password":"123456"` (raw) and the GET path sends raw too. Is
the password expected to be encrypted by the client, or is that comment
aspirational? If encrypted, `dashboardUserService.login` presumably decrypts;
if not, the comment should be dropped to avoid confusion. Clarifying the
contract would help.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
