Skip to content

chore: enhance AuthGuardFilter and JSP files for improved static asse… #73

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.

Sign up for GitHub

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

Merged
ArchILLtect merged 1 commit into from
Dec 18, 2025
Merged

chore: enhance AuthGuardFilter and JSP files for improved static asse… #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 30 additions & 2 deletions src/main/java/me/nickhanson/codeforge/web/AuthGuardFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import javax.servlet.http.HttpFilter;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpSession;
import java.io.IOException;
import java.util.Set;

Expand Down Expand Up @@ -37,11 +36,40 @@ protected void doFilter(HttpServletRequest req, HttpServletResponse resp, Filter
throws IOException, ServletException {

String contextPath = req.getContextPath();
String path = req.getRequestURI().substring(contextPath.length());
String uri = req.getRequestURI();
String path = uri.substring(contextPath.length());
Comment on lines +39 to +40
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

A new variable 'uri' is introduced on line 39 but it's only used once to derive 'path' on line 40. This adds an unnecessary intermediate variable. Consider removing the 'uri' variable and keeping the original single-line assignment: String path = req.getRequestURI().substring(contextPath.length());

Suggested change
String uri = req.getRequestURI();
String path = uri.substring(contextPath.length());
String path = req.getRequestURI().substring(contextPath.length());

Copilot uses AI. Check for mistakes.
String method = req.getMethod();

boolean needsAuth = false;

// 1) Always allow static assets
if (path.startsWith("/css/")
|| path.startsWith("/images/")
|| path.startsWith("/apidocs/")
|| path.equals("/favicon.ico")
|| path.startsWith("/favicon")
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The path "/favicon" (line 50) is redundant since the check on line 49 already covers "/favicon.ico" with an exact match, and any path starting with "/favicon" would include "/favicon.ico" as well. The startsWith check for "/favicon" would also match paths like "/favicon.png" or "/favicons/", which may be the intended behavior, but if "/favicon.ico" is the only favicon path that needs to be allowed, line 50 should be removed to avoid confusion.

Suggested change
|| path.startsWith("/favicon")

Copilot uses AI. Check for mistakes.
|| path.endsWith(".css")
|| path.endsWith(".js")
|| path.endsWith(".png")
|| path.endsWith(".jpg")
|| path.endsWith(".jpeg")
|| path.endsWith(".gif")
|| path.endsWith(".svg")
|| path.endsWith(".webp")) {
Comment on lines +46 to +58
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The static asset filtering has redundant checks. Lines 46-50 check for paths starting with "/css/", "/images/", etc., while lines 51-58 check for file extensions like ".css", ".js", ".png", etc. A file like "/css/styles.css" would be allowed by line 46 before reaching the extension check on line 51. Similarly, a CSS file served from a non-standard location (e.g., "/static/app.css") would be allowed by the ".css" extension check on line 51, even though "/static/" isn't in the path prefix list. Consider whether both approaches are needed or if one method would be more maintainable and clearer in intent.

Copilot uses AI. Check for mistakes.
chain.doFilter(req, resp);
return;
}

// 2) Allow your public pages (home/about/login/error)
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The comment on line 63 has inconsistent indentation. It should be aligned with the surrounding code at the same indentation level as the if statement below it, rather than being flush with the left margin.

Suggested change
// 2) Allow your public pages (home/about/login/error)
// 2) Allow your public pages (home/about/login/error)

Copilot uses AI. Check for mistakes.
if (path.equals("/") || path.equals("/home")
|| path.startsWith("/about")
|| path.startsWith("/logIn")
|| path.startsWith("/logout")
|| path.startsWith("/error")) {
chain.doFilter(req, resp);
return;
}

// Check if the request method is POST.
if ("POST".equalsIgnoreCase(method)) {
// Public practice submissions do NOT require auth
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/me/nickhanson/codeforge/web/HomeServlet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*
* @author Nick Hanson
*/
@WebServlet(urlPatterns = {"/", "/home"})
@WebServlet(urlPatterns = {"/home"})
public class HomeServlet extends HttpServlet {
private final QuoteService quotes = new QuoteService();

Expand Down
1 change: 1 addition & 0 deletions src/main/webapp/WEB-INF/jsp/challenges/detail.jsp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
<html lang="en">
<head>
<c:import url="/WEB-INF/jsp/head-meta.jsp" />
<link rel="stylesheet" href="${pageContext.request.contextPath}/css/layout.css" />
<link rel="stylesheet" href="${pageContext.request.contextPath}/css/home.css" />
</head>
<body>
Expand Down
1 change: 1 addition & 0 deletions src/main/webapp/WEB-INF/jsp/challenges/list.jsp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
<html lang="en">
<head>
<c:import url="/WEB-INF/jsp/head-meta.jsp" />
<link rel="stylesheet" href="${pageContext.request.contextPath}/css/layout.css" />
<link rel="stylesheet" href="${pageContext.request.contextPath}/css/home.css" />
</head>
<body>
Expand Down
1 change: 0 additions & 1 deletion src/main/webapp/WEB-INF/jsp/drill/solve.jsp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
<!DOCTYPE html>
<html lang="en">
<head>
<!-- removed explicit meta tags; head-meta.jspf provides them -->
<c:import url="/WEB-INF/jsp/head-meta.jsp" />
<link rel="stylesheet" href="${pageContext.request.contextPath}/css/home.css" />
<link rel="stylesheet" href="${pageContext.request.contextPath}/css/drill.css" />
Expand Down
22 changes: 4 additions & 18 deletions src/main/webapp/WEB-INF/jsp/header.jsp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,6 @@
<%@ page contentType="text/html; charset=UTF-8" pageEncoding="UTF-8" %>
<%@ taglib prefix="c" uri="http://java.sun.com/jsp/jstl/core" %>

<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title>Drill Queue</title>
<link rel="stylesheet" href="${pageContext.request.contextPath}/css/home.css" />
<link rel="stylesheet" href="${pageContext.request.contextPath}/css/layout.css" />
</head>
<body>

<header class="cf-header">
<div class="cf-header-inner">
<!-- Left: Logo + app name -->
Expand All @@ -38,22 +27,19 @@
<a href="${pageContext.request.contextPath}/challenges" class="cf-nav-link">Challenges</a>

<li class="cf-nav-item cf-nav-item-has-menu">
<a class="cf-nav-link"
href="#">
<a class="cf-nav-link" href="#">
Coding
<span class="cf-nav-caret">▾</span>
</a>

<ul class="cf-nav-dropdown">
<li>
<a href="${pageContext.request.contextPath}/practice"
class="cf-nav-dropdown-link">
<a href="${pageContext.request.contextPath}/practice" class="cf-nav-dropdown-link">
Practice
</a>
</li>
<li>
<a href="${pageContext.request.contextPath}/drill"
class="cf-nav-dropdown-link">
<a href="${pageContext.request.contextPath}/drill" class="cf-nav-dropdown-link">
Drill
</a>
</li>
Expand All @@ -66,7 +52,7 @@
</ul>
</div>

<!-- Right: User info / login -->
<!-- Right: User info / login -->
<div class="cf-user-area">
<c:choose>
<c:when test="${not empty sessionScope.user}">
Expand Down
3 changes: 1 addition & 2 deletions src/main/webapp/WEB-INF/jsp/home.jsp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,8 @@
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<c:import url="/WEB-INF/jsp/head-meta.jsp" />
<link rel="stylesheet" href="${pageContext.request.contextPath}/css/layout.css" />
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The layout.css stylesheet has been added to home.jsp and the challenges pages (list.jsp, detail.jsp), but not to drill pages (queue.jsp, solve.jsp) or the about.jsp page, which also use the same header component and page structure. This inconsistency could lead to different styling across pages that should have a consistent layout. Consider adding layout.css to all pages that use the header component for consistency.

Copilot uses AI. Check for mistakes.
<link rel="stylesheet" href="${pageContext.request.contextPath}/css/home.css" />
</head>
<body>
Expand Down
0 ...ain/webapp/WEB-INF/jsp/practice/index.jsp → src/main/webapp/index.jsp
View file
Open in desktop
File renamed without changes.