fix(security): prevent content injection via URL query param flash messages#153
fix(security): prevent content injection via URL query param flash messages#153
Conversation
…ssages - Replace direct c.Query() pass-through with map-based lookups in login, authorization, and device handlers - Remove default case in login error handler that rendered arbitrary user-supplied text as an official error message - Add authzSuccessMessages and authzErrorMessages maps for authorization page - Remove unused error query param pass-through in device page
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR hardens handler “flash message” rendering by preventing user-controlled URL query parameters from being displayed verbatim as official-looking success/error banners, reducing phishing/content-injection risk across login, authorization, and device pages.
Changes:
- Replace raw
c.Query()pass-through for login/authorization flash messages with allowlist map lookups. - Remove the login handler’s
defaultswitch branch that previously echoed arbitraryerrorquery text. - Stop surfacing the (apparently unused)
errorquery param on the device page by always settingErrorto an empty string.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/handlers/auth.go | Introduces loginErrorMessages allowlist and removes raw query-param error echoing on the login page. |
| internal/handlers/authorization.go | Adds allowlisted success/error message maps and uses them when rendering the authorizations list page. |
| internal/handlers/device.go | Removes query-param error pass-through by always rendering an empty error message on the device page. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add auth_test.go with tests for loginErrorMessages known keys and unknown key rejection (content injection prevention) - Add authorization_test.go tests for authzSuccessMessages and authzErrorMessages known keys and unknown key rejection
- Verify all device code error types map to correct user-facing messages - Cover unknown error fallback to generic message
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Replace services.ErrDeviceCodeNotFound with errors.New() in the unknown error test case to properly exercise the default branch
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
c.Query()pass-through with map-based lookups in login, authorization, and device handlers to prevent content injection via crafted URLsdefault: errorMsg = errorParamin login handler that allowed arbitrary user-controlled text to be rendered as an official error message (e.g.,/login?error=Your+account+is+locked.+Call+1-800-SCAM)authzSuccessMessages/authzErrorMessagesmaps for authorization page, following the same safe pattern astoken_admin.go/device?error=...)Audit of all 7 query param flash message locations
handlers/client.gohandlers/token_admin.gohandlers/user_client.gohandlers/auth.godefaultcasehandlers/authorization.gohandlers/device.gohandlers/audit.goTest plan
make lintpasses with 0 issuesmake testpasses all tests/login?error=injected_textshows no error message/login?error=session_timeoutshows the expected expiry message/account/authorizations?success=revokedshows success message/account/authorizations?error=crafted_textshows no error message🤖 Generated with Claude Code