Skip to content

web security hw#663

Open
RostyslavOnysh wants to merge 13 commits into
mate-academy:masterfrom
RostyslavOnysh:secutiryWebBranch
Open

web security hw#663
RostyslavOnysh wants to merge 13 commits into
mate-academy:masterfrom
RostyslavOnysh:secutiryWebBranch

Conversation

@RostyslavOnysh
Copy link
Copy Markdown

No description provided.

Comment thread src/main/java/taxi/model/Driver.java Outdated
+ ", name='" + name + '\''
+ ", licenseNumber='" + licenseNumber + '\''
+ ", login='" + login + '\''
+ ", password='" + password + '\''
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Never include sensitive data like user's password in the toString() method. This can lead to unintended exposure of sensitive data.

protected void doGet(HttpServletRequest req, HttpServletResponse resp)
throws ServletException, IOException {
req.getSession().invalidate();
req.getRequestDispatcher("/WEB-INF/views/login.jsp").forward(req,resp);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

After invalidating the session, you should redirect the user to the login page instead of forwarding. Forwarding keeps the original request URL in the browser address bar, which could lead to unexpected behavior if the user refreshes the page.

String name = resultSet.getString("name");
String licenseNumber = resultSet.getString("license_number");
String login = resultSet.getString("login");
String password = resultSet.getString("password");
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You are trying to get the 'password' field from the result set, but 'password' field is not included in SQL query. You need to add 'password' in your SQL query.


@Override
public Driver login(String login, String password) throws AuthenticationException {
Optional<Driver> driver = Optional.ofNullable(driverService.findByLogin(login));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The 'Optional.ofNullable' call is unnecessary here because the 'driverService.findByLogin(login)' method should already return an Optional. Instead of using 'Optional.ofNullable', you should expect 'driverService.findByLogin(login)' to return an Optional.

Comment thread src/main/java/taxi/controller/LoginController.java Outdated
Driver driver = authenticationService.login(login,password);
HttpSession session = req.getSession();
session.setAttribute("driver_id", driver.getId());
resp.sendRedirect("/index");
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Checklist item:
If you use sendRedirect() method in your controllers, please pass request.getContextPath() + "/your-endpoint" as a parameter. Currently, the context path is empty, but if it is not, your code still should work.

Please recheck all occurrences

Comment thread src/main/java/taxi/model/Driver.java Outdated
Comment on lines +85 to +90
&& Objects.equals(password, driver.password);
}

@Override
public int hashCode() {
return Objects.hash(id, name, licenseNumber);
return Objects.hash(id, name, licenseNumber, login, password);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do not include password into equals/hashCode methods. Further password will not be stored as plain text, it will be hashed and this may cause unpredictable behaviour

@RostyslavOnysh RostyslavOnysh requested a review from olekskov July 16, 2023 14:42
Copy link
Copy Markdown

@Ivan95kos Ivan95kos left a comment

Choose a reason for hiding this comment

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

Great job

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.

3 participants