Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User search vulnerability #183

Merged
merged 36 commits into from
Jan 5, 2024
Merged

User search vulnerability #183

merged 36 commits into from
Jan 5, 2024

Conversation

kimura-developer
Copy link
Contributor

No description provided.

Copy link
Member

@restjohn restjohn left a comment

Choose a reason for hiding this comment

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

The web app is still sending the old search parameters to the new endpoint. This means the search does not correctly filter the results, returns too many items per page, and the summary of Active, Inactive, and Disabled users just shows the total number of users for each category. I believe the default page size on the old endpoint, or the parameter the web app specified, was 10. The new search endpoint's default page size is 250, so now the pages in the admin UI are showing too many users.

Copy link
Member

@restjohn restjohn left a comment

Choose a reason for hiding this comment

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

There just a few things to cleanup. We can revert UserReadService to its original state, and we shouldn't need to inject that into any of the components any more since UserService is using the new /api/next-users/search endpoint. Also, let's generally remove the debug console logging.

web-app/projects/core-lib/user/user-read.service.ts Outdated Show resolved Hide resolved
web-app/src/ng1/admin/users/users.component.js Outdated Show resolved Hide resolved
web-app/src/ng1/factories/user-paging.service.js Outdated Show resolved Hide resolved
web-app/src/ng1/factories/user-paging.service.js Outdated Show resolved Hide resolved
web-app/src/ng1/factories/user-paging.service.js Outdated Show resolved Hide resolved
web-app/src/ng1/factories/user-paging.service.js Outdated Show resolved Hide resolved
web-app/src/ng1/factories/user-paging.service.js Outdated Show resolved Hide resolved
Copy link
Member

@restjohn restjohn left a comment

Choose a reason for hiding this comment

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

After testing, I believe there is just one small issue remaining to resolve. We need to reset the page index of the search request when the search term changes. The following steps demonstrate the discrepant behavior.

  1. Go to the Users admin tab.
  2. Click the Next button to advance to the second page.
  3. Type a search term in the input that produces only one page matching user results. The app displays no results because the page index still points to the second page even though there is only one page of results matching the search term.

This behavior can mislead the user into thinking there are no results for the search, when in fact there could be one or more matching results on the first page. The easy fix is to reset the page index whenever the search term changes.

@restjohn restjohn force-pushed the user-search-vulnerability branch from cc040ef to 70eca32 Compare January 5, 2024 07:34
@restjohn restjohn merged commit 5633d78 into master Jan 5, 2024
5 checks passed
@restjohn restjohn deleted the user-search-vulnerability branch January 5, 2024 16:47
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